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

[Junio C Hamano <gitster@pobox.com>]

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

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