Re: [PATCH v4 3/6] commit-graph: turn on commit-graph by default
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 20, 2021, 12:53 UTC
- Message-ID
- <8841729f-9310-c393-caa9-4b209909ea5d@gmail.com>
- In-Reply-To
- <YUfjIjgk2IwcJNI8@camp.crustytoothpaste.net>
On 20/09/2021 02:25, brian m. carlson wrote:
Show 41 quoted lines
> 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
Show 5 quoted lines
> 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 >