Re: [PATCH 4/5] fetch-pack: expose `fetch_pack_config_cb()`

> Justin Tobler <jltobler@gmail.com> 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.