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

[Phillip Wood <phillip.wood123@gmail.com>]

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. 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
>