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

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

Justin Tobler <jltobler@gmail.com> writes:

Show 5 quoted lines

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