On 24/11/22 10:57AM, Junio C Hamano wrote:
> Justin Tobler <jltobler@xxxxxxxxx> writes:
>
> > With `fetch_pack_config_cb()`, fsck configuration gets populated to a
> > `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate
> > formatted fsck message configuration generation. In a subsequent commit,
> > this is used to wire message configuration to `unbundle()` during bundle
> > fetches.
>
> This is generally going in the right direction, but this particular
> iteration is highly disappointing for two reasons.
>
> - The callback calls git_default_config() at end. Other callers
> may not want it happen. Think of the reason why a new caller may
> want to use this callback (see the next item).
>
> - fetch_pack_config_cb() was perfectly good name back when it was
> hidden inside fetch-pack.c, as a private internal implementation
> detail, EVEN THOUGH it did not give its callers everything that
> tries to configure the behaviour of fetch-pack. It ONLY is about
> how fsck behaviour is affected. It must be renamed so that any
> new caller can realize that it is configuring fsck checking
> machinery and not general fetch-pack features.
>
> So, I would suggest making at least two changes.
>
> - rename it to a more sensible name that includes "fsck" somewhere
> (as it is about "fetch.fsck.*" configuration variables, "fetch"
> should also stay in the name). Let's tentatively call it foo().
>
> - stop calling git_default_config() from foo(). Instead, return
> some special value foo() does not currently return, let's say -1
> to signal that the key was something foo() was not interested in,
> and write a thin replacement helper
>
> static int fetch_pack_config_cb(...)
> {
> int st = foo(...);
> if (st < 0)
> return git_default_config(var, value, ctx, cb);
> return st;
> }
>
> and call that from fetch_pack_config().
>
> No, "foo()" has neither "fetch" or "fsck" in it; I am not suggesting
> to use that as the final name ;-).
>
> Thanks.
Thanks for the suggestions. I'll factor out the fsck configuration bit
as suggested and name it something like "fetch_pack_fsck_config()". The
new name should be more clear and this change will also ensure only the
desired fsck configuration is being parsed which makes more sense. :)
-Justin
