Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2024, 02:39 UTC
Message-ID: <xmqq34j45fzr.fsf@gitster.g>
In-Reply-To: <xmqqy10w9x0m.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 9 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
>
>> I think the current version of this patch series is fine as-is then. It
>> might be a good idea to adapt this in a follow-up series, unless there
>> is a good reason not to.
>
> Sounds good.  We may want to tighten the rules a bit to reject
> obvious misconfigurations, but that is outside the scope of this
> topic.

This reminds me of a slightly related tangent.

There are pathname-valued ones that we added deliberately as an optional configuration variable, IIRC, and fsck.skiplist may be one of them. In other words, they mean "In a repository that I want the configuration to affect, I'll have the configuration file at this path, but it is merely optional. If it is missing, pretend as if the configuration variable does not even exist".

In retrospect, it was a mistake to define that this variable is required and triggers an error when misconfigured, and that variable is optional and triggers only a warning when misconfigured. That is one more thing for the user to remember and does not scale.

We probably should have done something like

 - the value given to all pathname valued configuration variables
   and command name options MUST correctly name a path that the
   command can read, and a misconfigured configuration variable or
   an invalid command line option should trigger an error when the
   command needs to access it.

 - the value (not the variable) can say "I am optional--if the path
   does not appear on this system, or if it is unreadable, pretend
   as if this configuration variable or the command line option were
   not given".

The "when the command needs to" part is important. Ideally, "git log" should not fail when core.hookspath is misconfigured, because it does not trigger any hook, but it currently dies while parsing the configuration files in git_default_config():

 $ git -c core.hookspath log
 error: missing value for 'core.hookspath'
 fatal: unable to parse 'core.hookspath' from command-line config.

which we may want to fix. Unsurprisingly there are other variables that do behave correctly, like

 $ git -c core.pager --no-pager log -20 >/dev/null
 $ git -c core.pager log -20 >/dev/null
 error: missing value for 'core.pager'
 fatal: unable to parse command-line config

where a misconfigured core.pager does not cause any trouble when the pager is not in use.

Any of the above are not within the scope of this topic, obviously.

cf. <20241014204427.1712182-1-gitster@pobox.com>