[v4,3/6] commit-graph: turn on commit-graph by default

Commit Message

Derrick Stolee via GitGitGadget Aug. 13, 2019, 6:37 p.m. UTC

From: Derrick Stolee <[e-mail]>

The commit-graph feature has seen a lot of activity in the past
year or so since it was introduced. The feature is a critical
performance enhancement for medium- to large-sized repos, and
does not significantly hurt small repos.

Change the defaults for core.commitGraph and gc.writeCommitGraph
to true so users benefit from this feature by default.

There are several places in the test suite where the environment
variable GIT_TEST_COMMIT_GRAPH is disabled to avoid reading a
commit-graph, if it exists. The config option overrides the
environment, so swap these. Some GIT_TEST_COMMIT_GRAPH assignments
remain, and those are to avoid writing a commit-graph when a new
commit is created.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/config/core.txt       | 2 +-
 Documentation/config/gc.txt         | 2 +-
 repo-settings.c                     | 4 ++++
 t/t0410-partial-clone.sh            | 2 +-
 t/t5307-pack-missing-commit.sh      | 4 ++--
 t/t5324-split-commit-graph.sh       | 2 ++
 t/t6011-rev-list-with-bad-commit.sh | 2 +-
 7 files changed, 12 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Sept. 20, 2021, 12:42 a.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> diff --git a/repo-settings.c b/repo-settings.c
> index 309577f6bc..d00b675687 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -2,6 +2,8 @@
>  #include "config.h"
>  #include "repository.h"
>  
> +#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
> +
>  void prepare_repo_settings(struct repository *r)
>  {
>  	int value;
> @@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
>  		r->settings.core_commit_graph = value;
>  	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
>  		r->settings.gc_write_commit_graph = value;
> +	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
> +	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);


This is a "review comment" that is more than 2 years late X-<, but I
noticed that this is used to muck with a structure that was
initialized by filling it with \0377 bytes.

+           /* Defaults */
+           memset(&r->settings, -1, sizeof(r->settings));

but the structure is is full of "int" and "enum", so apparently this
works only on 2's complement architecture.

        struct repo_settings {
                int initialized;

                int core_commit_graph;
                int commit_graph_read_changed_paths;
                int gc_write_commit_graph;
                int fetch_write_commit_graph;

                int index_version;
                enum untracked_cache_setting core_untracked_cache;

                int pack_use_sparse;
                enum fetch_negotiation_setting fetch_negotiation_algorithm;

                int core_multi_pack_index;

                unsigned command_requires_full_index:1,
                         sparse_index:1;
        };

I see that the earliest iteration of this series [*1*] set the
default explicitly using assignments of the correct types, like
this:


+void prepare_repo_settings(struct repository *r)
+{
+       if (r->settings)
+          return;
+
+       r->settings = xmalloc(sizeof(*r->settings));
+
+       /* Defaults */
+       r->settings->core_commit_graph = -1;
+       r->settings->gc_write_commit_graph = -1;
+       r->settings->pack_use_sparse = -1;
+       r->settings->index_version = -1;
+ ...

which I think should be a reasonable starting point to fix the
current code.

Another thing I noticed is that while it may have been only for
setting the default value for a boolean variable initially, other
changes abuse the macro to set an arbitrary integer values to
integer members of the structure, e.g. c6cc4c5a (repo-settings:
create feature.manyFiles setting, 2019-08-13) sets 4 to the
index_version (naturally, the choice between 0 and 1 does not make
much sense for the member), and ad0fb659 (repo-settings: parse
core.untrackedCache, 2019-08-13) stuffs UNTRACKED_CACHE_* enum to
core_untracked_cache.  The UPDATE_DEFAULT_BOOL() macro should be
renamed to UPDATE_DEFAULT_INT() at least, I would think, to save
readers from confusion.

Thanks.


[Reference]

*1* https://lore.kernel.org/git/72f6[e-mail]/

Ævar Arnfjörð Bjarmason Sept. 20, 2021, 1:22 a.m. UTC | #2

On Sun, Sep 19 2021, Junio C Hamano wrote:

> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
>
>> diff --git a/repo-settings.c b/repo-settings.c
>> index 309577f6bc..d00b675687 100644
>> --- a/repo-settings.c
>> +++ b/repo-settings.c
>> @@ -2,6 +2,8 @@
>>  #include "config.h"
>>  #include "repository.h"
>>  
>> +#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
>> +
>>  void prepare_repo_settings(struct repository *r)
>>  {
>>  	int value;
>> @@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
>>  		r->settings.core_commit_graph = value;
>>  	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
>>  		r->settings.gc_write_commit_graph = value;
>> +	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
>> +	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>
>
> This is a "review comment" that is more than 2 years late X-<, but I
> noticed that this is used to muck with a structure that was
> initialized by filling it with \0377 bytes.
>
> +           /* Defaults */
> +           memset(&r->settings, -1, sizeof(r->settings));
>
> but the structure is is full of "int" and "enum", so apparently this
> works only on 2's complement architecture.
>
>         struct repo_settings {
>                 int initialized;
>
>                 int core_commit_graph;
>                 int commit_graph_read_changed_paths;
>                 int gc_write_commit_graph;
>                 int fetch_write_commit_graph;
>
>                 int index_version;
>                 enum untracked_cache_setting core_untracked_cache;
>
>                 int pack_use_sparse;
>                 enum fetch_negotiation_setting fetch_negotiation_algorithm;
>
>                 int core_multi_pack_index;
>
>                 unsigned command_requires_full_index:1,
>                          sparse_index:1;
>         };
>
> I see that the earliest iteration of this series [*1*] set the
> default explicitly using assignments of the correct types, like
> this:
>
>
> +void prepare_repo_settings(struct repository *r)
> +{
> +       if (r->settings)
> +          return;
> +
> +       r->settings = xmalloc(sizeof(*r->settings));
> +
> +       /* Defaults */
> +       r->settings->core_commit_graph = -1;
> +       r->settings->gc_write_commit_graph = -1;
> +       r->settings->pack_use_sparse = -1;
> +       r->settings->index_version = -1;
> + ...
>
> which I think should be a reasonable starting point to fix the
> current code.
>
> Another thing I noticed is that while it may have been only for
> setting the default value for a boolean variable initially, other
> changes abuse the macro to set an arbitrary integer values to
> integer members of the structure, e.g. c6cc4c5a (repo-settings:
> create feature.manyFiles setting, 2019-08-13) sets 4 to the
> index_version (naturally, the choice between 0 and 1 does not make
> much sense for the member), and ad0fb659 (repo-settings: parse
> core.untrackedCache, 2019-08-13) stuffs UNTRACKED_CACHE_* enum to
> core_untracked_cache.  The UPDATE_DEFAULT_BOOL() macro should be
> renamed to UPDATE_DEFAULT_INT() at least, I would think, to save
> readers from confusion.

Yes this is all a bit weird and/or broken, but I'm a bit perplexed at
this reply to a 2+ year old E-Mail given my outstanding series to fix
all these issues you've noted here[1] posted in the last few days, and
you having read (at least part of) it[1].

But then again, the last patch you left a comment on was 3/5. It's 4/5
that fixes all the issues you note above[2] :)

The macro is gone, so is the memset to -1 and other weird emergent
behavior. We can rely on repo_init() having zero'd the structure for us,
and we just proceed to set sensible defaults in a way that doesn't stomp
over the types in the struct.

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/

> [Reference]
>
> *1* https://lore.kernel.org/git/72f6[e-mail]/

brian m. carlson Sept. 20, 2021, 1:25 a.m. UTC | #3

On 2021-09-20 at 00:42:57, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
> 
> > diff --git a/repo-settings.c b/repo-settings.c
> > index 309577f6bc..d00b675687 100644
> > --- a/repo-settings.c
> > +++ b/repo-settings.c
> > @@ -2,6 +2,8 @@
> >  #include "config.h"
> >  #include "repository.h"
> >  
> > +#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
> > +
> >  void prepare_repo_settings(struct repository *r)
> >  {
> >  	int value;
> > @@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
> >  		r->settings.core_commit_graph = value;
> >  	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
> >  		r->settings.gc_write_commit_graph = value;
> > +	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
> > +	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
> 
> 
> This is a "review comment" that is more than 2 years late X-<, but I
> noticed that this is used to muck with a structure that was
> initialized by filling it with \0377 bytes.
> 
> +           /* Defaults */
> +           memset(&r->settings, -1, sizeof(r->settings));
> 
> but the structure is is full of "int" and "enum", so apparently this
> works only on 2's complement architecture.

This statement is true, but are there systems capable of running Git
which don't use two's complement?  Rust requires two's complement signed
integers, and there's a proposal[0] to the C++ working group to only
support two's complement because "[t]o the author’s knowledge no modern
machine uses both C++ and a signed integer representation other than
two’s complement".  That proposal goes on to note that none of MSVC,
GCC, or LLVM support other options.

Personally I am not aware of any modern processor which provides signed
integer types using other than two's complement.

[0] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html

Phillip Wood Sept. 20, 2021, 12:53 p.m. UTC | #4

On 20/09/2021 02:25, brian m. carlson wrote:
> On 2021-09-20 at 00:42:57, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
>>
>>> diff --git a/repo-settings.c b/repo-settings.c
>>> index 309577f6bc..d00b675687 100644
>>> --- a/repo-settings.c
>>> +++ b/repo-settings.c
>>> @@ -2,6 +2,8 @@
>>>   #include "config.h"
>>>   #include "repository.h"
>>>   
>>> +#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
>>> +
>>>   void prepare_repo_settings(struct repository *r)
>>>   {
>>>   	int value;
>>> @@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
>>>   		r->settings.core_commit_graph = value;
>>>   	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
>>>   		r->settings.gc_write_commit_graph = value;
>>> +	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
>>> +	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>>
>>
>> This is a "review comment" that is more than 2 years late X-<, but I
>> noticed that this is used to muck with a structure that was
>> initialized by filling it with \0377 bytes.
>>
>> +           /* Defaults */
>> +           memset(&r->settings, -1, sizeof(r->settings));
>>
>> but the structure is is full of "int" and "enum", so apparently this
>> works only on 2's complement architecture.
> 
> This statement is true, but are there systems capable of running Git
> which don't use two's complement?  Rust requires two's complement signed
> integers, and there's a proposal[0] to the C++ working group to only
> support two's complement because "[t]o the author’s knowledge no modern
> machine uses both C++ and a signed integer representation other than
> two’s complement".  That proposal goes on to note that none of MSVC,
> GCC, or LLVM support other options.

A similar proposal [1] is included in the draft of the next C standard 
[2]. As integer representation is implementation defined I believe this 
code has well defined behavior on 2's complement implementations. If an 
enum has no negative members then the compiler may choose an unsigned 
representation but even then the comparison to -1 is well defined. In 
this case I'm pretty sure the enums all have -1 as a member so are 
signed. Using memset() to initialize the struct eases future maintenance 
when members are added or removed and seems to me to be a sensible 
design choice.

Best Wishes

Phillip

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf

> Personally I am not aware of any modern processor which provides signed
> integer types using other than two's complement.
> 
> [0] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html
>

Ævar Arnfjörð Bjarmason Sept. 20, 2021, 1:30 p.m. UTC | #5

On Mon, Sep 20 2021, Phillip Wood wrote:

> On 20/09/2021 02:25, brian m. carlson wrote:
>> On 2021-09-20 at 00:42:57, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
>>>
>>>> diff --git a/repo-settings.c b/repo-settings.c
>>>> index 309577f6bc..d00b675687 100644
>>>> --- a/repo-settings.c
>>>> +++ b/repo-settings.c
>>>> @@ -2,6 +2,8 @@
>>>>   #include "config.h"
>>>>   #include "repository.h"
>>>>   +#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } }
>>>> while(0)
>>>> +
>>>>   void prepare_repo_settings(struct repository *r)
>>>>   {
>>>>   	int value;
>>>> @@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
>>>>   		r->settings.core_commit_graph = value;
>>>>   	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
>>>>   		r->settings.gc_write_commit_graph = value;
>>>> +	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
>>>> +	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>>>
>>>
>>> This is a "review comment" that is more than 2 years late X-<, but I
>>> noticed that this is used to muck with a structure that was
>>> initialized by filling it with \0377 bytes.
>>>
>>> +           /* Defaults */
>>> +           memset(&r->settings, -1, sizeof(r->settings));
>>>
>>> but the structure is is full of "int" and "enum", so apparently this
>>> works only on 2's complement architecture.
>> This statement is true, but are there systems capable of running Git
>> which don't use two's complement?  Rust requires two's complement signed
>> integers, and there's a proposal[0] to the C++ working group to only
>> support two's complement because "[t]o the author’s knowledge no modern
>> machine uses both C++ and a signed integer representation other than
>> two’s complement".  That proposal goes on to note that none of MSVC,
>> GCC, or LLVM support other options.
>
> A similar proposal [1] is included in the draft of the next C standard
> [2]. As integer representation is implementation defined I believe
> this code has well defined behavior on 2's complement
> implementations. If an enum has no negative members then the compiler
> may choose an unsigned representation but even then the comparison to
> -1 is well defined.

That's informative, thanks.

> In this case I'm pretty sure the enums all have -1
> as a member so are signed. Using memset() to initialize the struct
> eases future maintenance when members are added or removed and seems
> to me to be a sensible design choice.

It's really not sensible at all in this particular case, as I think my
[1] which gets rid of the pattern convincingly argues.

I.e. the only reason it had a memset() of -1 after we'd already memset
it to 0 was because the function was tripping over itself and setting
defaults in the wrong order for no good reason.

I.e. it was doing things like (pseudocode);

    memset(&data, -1, ...)
    if_config_is_true_set("x.y", data.x_y);
    if (data.x_y == -1)
        data.x_y = x_y_default();

When we can instead just do:

    data.x_y = x_y_default();
    set_if_cfg_key_exists("x.y", &data.x_y);

Which is how we e.g. handle options parsing, we have hardcoded defaults,
then read defaults from config, then set options, in that order.

We don't set options, then check if each value is still -1, if so read
config etc. Just read them in priority order, doing it any other way is
just make-work for something that's the equivalent of a simple
short-circuit || operation.

Anyway, there are other cases where we need to read something and
distinguish e.g. false/true from "unset", and there a -1,0,1 tri-state
serves us well.

But even in those cases what repo-settings.c was doing of memsetting the
entire struct to -1 (2's compliment aside) just makes for needlessly
hard to read code.

If we've got some members that need -1 defaults we should instead have
that in an *_INIT macro or equivalent. The pre-[1] repo-settings.c also
has code like this pseudocode:

    data.a_b = -1; /* default for a bi-state, not tri-state variable */
    set_if_cfg_key_exists("a.b", &data.a_b);
    if (data.a_b == -1)
        data.a_b = 1; /* on by default */

Which, urm, you can just do as:

    data.a_b = 1; /* on by default */
    set_if_cfg_key_exists("a.b", &data.a_b);

I.e. the setup for things that never wanted or cared about being set to
-1 was complicated by them needing to un-set themselves from a -1
default they never wanted.

Thus the anti-pattern, yes set defaults for some members to -1, but not
the entire struct. The only value we should memset a whole bag-of-stuff
config struct to is 0, as that's the only sensible default & plays well
with other C semantics.

1. https://lore.kernel.org/git/[e-mail]/

>
> Best Wishes
>
> Phillip
>
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf
> [2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf
>
>> Personally I am not aware of any modern processor which provides signed
>> integer types using other than two's complement.
>> [0]
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html
>>

Phillip Wood Sept. 20, 2021, 6 p.m. UTC | #6

On 20/09/2021 14:30, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 20 2021, Phillip Wood wrote:
> 
>> On 20/09/2021 02:25, brian m. carlson wrote:
>>> On 2021-09-20 at 00:42:57, Junio C Hamano wrote:
>>>> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
>>>>
>>>>> diff --git a/repo-settings.c b/repo-settings.c
>>>>> index 309577f6bc..d00b675687 100644
>>>>> --- a/repo-settings.c
>>>>> +++ b/repo-settings.c
>>>>> @@ -2,6 +2,8 @@
>>>>>    #include "config.h"
>>>>>    #include "repository.h"
>>>>>    +#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } }
>>>>> while(0)
>>>>> +
>>>>>    void prepare_repo_settings(struct repository *r)
>>>>>    {
>>>>>    	int value;
>>>>> @@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
>>>>>    		r->settings.core_commit_graph = value;
>>>>>    	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
>>>>>    		r->settings.gc_write_commit_graph = value;
>>>>> +	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
>>>>> +	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>>>>
>>>>
>>>> This is a "review comment" that is more than 2 years late X-<, but I
>>>> noticed that this is used to muck with a structure that was
>>>> initialized by filling it with \0377 bytes.
>>>>
>>>> +           /* Defaults */
>>>> +           memset(&r->settings, -1, sizeof(r->settings));
>>>>
>>>> but the structure is is full of "int" and "enum", so apparently this
>>>> works only on 2's complement architecture.
>>> This statement is true, but are there systems capable of running Git
>>> which don't use two's complement?  Rust requires two's complement signed
>>> integers, and there's a proposal[0] to the C++ working group to only
>>> support two's complement because "[t]o the author’s knowledge no modern
>>> machine uses both C++ and a signed integer representation other than
>>> two’s complement".  That proposal goes on to note that none of MSVC,
>>> GCC, or LLVM support other options.
>>
>> A similar proposal [1] is included in the draft of the next C standard
>> [2]. As integer representation is implementation defined I believe
>> this code has well defined behavior on 2's complement
>> implementations. If an enum has no negative members then the compiler
>> may choose an unsigned representation but even then the comparison to
>> -1 is well defined.
> 
> That's informative, thanks.
> 
>> In this case I'm pretty sure the enums all have -1
>> as a member so are signed. Using memset() to initialize the struct
>> eases future maintenance when members are added or removed and seems
>> to me to be a sensible design choice.
> 
> It's really not sensible at all in this particular case, as I think my
> [1] which gets rid of the pattern convincingly argues.

I meant that it was a sensible way to initialize all the struct members 
to -1, I did not mean to comment either way on whether such an 
initialization was sensible. If we can just store the default and then 
update from the user's config that sounds sensible.

Best Wishes

Phillip

> I.e. the only reason it had a memset() of -1 after we'd already memset
> it to 0 was because the function was tripping over itself and setting
> defaults in the wrong order for no good reason.
> 
> I.e. it was doing things like (pseudocode);
> 
>      memset(&data, -1, ...)
>      if_config_is_true_set("x.y", data.x_y);
>      if (data.x_y == -1)
>          data.x_y = x_y_default();
> 
> When we can instead just do:
> 
>      data.x_y = x_y_default();
>      set_if_cfg_key_exists("x.y", &data.x_y);
> 
> Which is how we e.g. handle options parsing, we have hardcoded defaults,
> then read defaults from config, then set options, in that order.
> 
> We don't set options, then check if each value is still -1, if so read
> config etc. Just read them in priority order, doing it any other way is
> just make-work for something that's the equivalent of a simple
> short-circuit || operation.
> 
> Anyway, there are other cases where we need to read something and
> distinguish e.g. false/true from "unset", and there a -1,0,1 tri-state
> serves us well.
> 
> But even in those cases what repo-settings.c was doing of memsetting the
> entire struct to -1 (2's compliment aside) just makes for needlessly
> hard to read code.
> 
> If we've got some members that need -1 defaults we should instead have
> that in an *_INIT macro or equivalent. The pre-[1] repo-settings.c also
> has code like this pseudocode:
> 
>      data.a_b = -1; /* default for a bi-state, not tri-state variable */
>      set_if_cfg_key_exists("a.b", &data.a_b);
>      if (data.a_b == -1)
>          data.a_b = 1; /* on by default */
> 
> Which, urm, you can just do as:
> 
>      data.a_b = 1; /* on by default */
>      set_if_cfg_key_exists("a.b", &data.a_b);
> 
> I.e. the setup for things that never wanted or cared about being set to
> -1 was complicated by them needing to un-set themselves from a -1
> default they never wanted.
> 
> Thus the anti-pattern, yes set defaults for some members to -1, but not
> the entire struct. The only value we should memset a whole bag-of-stuff
> config struct to is 0, as that's the only sensible default & plays well
> with other C semantics.
> 
> 1. https://lore.kernel.org/git/[e-mail]/
> 
>>
>> Best Wishes
>>
>> Phillip
>>
>> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf
>> [2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf
>>
>>> Personally I am not aware of any modern processor which provides signed
>>> integer types using other than two's complement.
>>> [0]
>>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html
>>>
>

Jeff King Sept. 20, 2021, 7:18 p.m. UTC | #7

On Mon, Sep 20, 2021 at 03:30:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Thus the anti-pattern, yes set defaults for some members to -1, but not
> the entire struct. The only value we should memset a whole bag-of-stuff
> config struct to is 0, as that's the only sensible default & plays well
> with other C semantics.

FWIW, I agree. I had to scratch my head for a moment at why a memset of
"-1" would work at all on multi-byte types. I think it's better avoided
in the name of readability and obviousness, not to mention the trap it
leaves for items that don't sensibly initialize with it (like, say,
pointers).

As an aside, memset to 0 is _also_ undefined for pointers, but we long
ago decided not to care, as no real-world systems have a problem with
this.

-Peff

Patch

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 75538d27e7..e66d79fd76 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -577,7 +577,7 @@  the `GIT_NOTES_REF` environment variable.  See linkgit:git-notes[1].
 
 core.commitGraph::
 	If true, then git will read the commit-graph file (if it exists)
-	to parse the graph structure of commits. Defaults to false. See
+	to parse the graph structure of commits. Defaults to true. See
 	linkgit:git-commit-graph[1] for more information.
 
 core.useReplaceRefs::
diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt
index 02b92b18b5..00ea0a678e 100644
--- a/Documentation/config/gc.txt
+++ b/Documentation/config/gc.txt
@@ -63,7 +63,7 @@  gc.writeCommitGraph::
 	If true, then gc will rewrite the commit-graph file when
 	linkgit:git-gc[1] is run. When using `git gc --auto`
 	the commit-graph will be updated if housekeeping is
-	required. Default is false. See linkgit:git-commit-graph[1]
+	required. Default is true. See linkgit:git-commit-graph[1]
 	for details.
 
 gc.logExpiry::
diff --git a/repo-settings.c b/repo-settings.c
index 309577f6bc..d00b675687 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -2,6 +2,8 @@ 
 #include "config.h"
 #include "repository.h"
 
+#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
+
 void prepare_repo_settings(struct repository *r)
 {
 	int value;
@@ -16,6 +18,8 @@  void prepare_repo_settings(struct repository *r)
 		r->settings.core_commit_graph = value;
 	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
 		r->settings.gc_write_commit_graph = value;
+	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
+	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
 
 	if (!repo_config_get_bool(r, "index.version", &value))
 		r->settings.index_version = value;
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 5bd892f2f7..181ffa44e9 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -234,7 +234,7 @@  test_expect_success 'rev-list stops traversal at missing and promised commit' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "arbitrary string" &&
-	GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
+	GIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out &&
 	grep $(git -C repo rev-parse bar) out &&
 	! grep $FOO out
 '
diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh
index dacb440b27..f4338abb78 100755
--- a/t/t5307-pack-missing-commit.sh
+++ b/t/t5307-pack-missing-commit.sh
@@ -24,11 +24,11 @@  test_expect_success 'check corruption' '
 '
 
 test_expect_success 'rev-list notices corruption (1)' '
-	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list HEAD
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false rev-list HEAD
 '
 
 test_expect_success 'rev-list notices corruption (2)' '
-	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --objects HEAD
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false rev-list --objects HEAD
 '
 
 test_expect_success 'pack-objects notices corruption' '
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 03f45a1ed9..19aa40de15 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -8,6 +8,7 @@  GIT_TEST_COMMIT_GRAPH=0
 test_expect_success 'setup repo' '
 	git init &&
 	git config core.commitGraph true &&
+	git config gc.writeCommitGraph false &&
 	infodir=".git/objects/info" &&
 	graphdir="$infodir/commit-graphs" &&
 	test_oid_init
@@ -332,6 +333,7 @@  test_expect_success 'split across alternate where alternate is not split' '
 	git clone --no-hardlinks . alt-split &&
 	(
 		cd alt-split &&
+		rm -f .git/objects/info/commit-graph &&
 		echo "$(pwd)"/../.git/objects >.git/objects/info/alternates &&
 		test_commit 18 &&
 		git commit-graph write --reachable --split &&
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index 545b461e51..bad02cf5b8 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -42,7 +42,7 @@  test_expect_success 'corrupt second commit object' \
    '
 
 test_expect_success 'rev-list should fail' '
-	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false rev-list --all > /dev/null
 '
 
 test_expect_success 'git repack _MUST_ fail' \