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

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

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