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

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

On Sun, Sep 19 2021, 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.
>
>         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/cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v3-4.5-28286a61162-20210919T084703Z-avarab@gmail.com/

> [Reference]
>
> *1* https://lore.kernel.org/git/72f652b89c71526cc423e7812de66f41a079f181.1563818059.git.gitgitgadget@gmail.com/