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

> 
> 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" <gitgitgadget@gmail.com> 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/patch-v3-4.5-28286a61162-20210919T084703Z-avarab@gmail.com/
> 
>>
>> 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
>>>
>