On Sun, Sep 19 2021, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx/
2. https://lore.kernel.org/git/patch-v3-4.5-28286a61162-20210919T084703Z-avarab@xxxxxxxxx/
> [Reference]
>
> *1* https://lore.kernel.org/git/72f652b89c71526cc423e7812de66f41a079f181.1563818059.git.gitgitgadget@xxxxxxxxx/
