Re: [PATCH 3/5] fetch-pack: introduce `fetch_pack_options`

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2024, 01:46 UTC
Message-ID: <xmqqjzcwca74.fsf@gitster.g>
In-Reply-To: <20241121204119.1440773-4-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 10 quoted lines

> When `fetch_pack_config()` is invoked, fetch-pack configuration is
> parsed from the config. As part of this operation, fsck message severity
> configuration is assigned to the `fsck_msg_types` global variable. This
> is optionally used to configure the downstream git-index-pack(1) when
> the `--strict` option is specified.
>
> In a subsequent commit, the same fetch-pack fsck message configuration
> needs to be reused. To facilitate this, introduce `fetch_pack_options`
> which gets written to during the `fetch_pack_config_cb()` instead of
> directly modifying the `fsck_msg_types` global.

It is unclear how it facilitates to replace one global with another global that has the data that was previously global as one of its members. With the above I was somehow expecting that the option struct instance is allocated on the stack of a function common to both callers of the configuration reader (i.e. fetch_pack_config()) as well as the configuration user (i.e. get_pack()). If we were to allow the latter to keep accessing the global (which is perfectly fine), wouldn't it be sufficient for the purpose of this series (which I am imagining wants to call fetch_pack_config() from the sideways and grab what came from the configuration) to just pass the fsck_msg_types strbuf through the call chain of the reaading side?

That is,

 - fetch_pack_config()'s current callers pass the address of
   fsck_msg_types to a new parameter.

 - fetch_pack_config() passes that new parameter when calling
   git_config();

 - fetch_pack_config_cb() uses the cb parameter and stuff its
   findings there;

 - a third-party caller calls fetch_pack_config() with its own
   fsck_msg_types instance (presumably in this series, it would be
   the opts.fsck_msg_types member introduced earlier in the bundle
   code).

or something like that?

So, the reason for existence of the shell around the fsck_msg_types needs to be explained. It is perfectly fine to say "we'll add THIS THING in a later step", if that were the case, but a reviewer tends to start reading from the front, so the presentation order matters.

Leaving many questions unsolved tangling may be a good way to keep readers engaged when writing a mystery novel, but not a patch series. Having to keep too many things in head, especially when many of them are not explained well (hence raises "why should I keep these in my head?" question), is another distraction and discourages the reviewers from reading further on.

Assuming that the shell structure is necessary around it, the code changes in this patch looks sensible to me.

Thanks.