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.
