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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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