Re: [PATCH v2 0/7] Introduce fetch.followRemoteHEAD config variable

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

Matt Hunter <m@lfurio.us> writes:

> Changes in v2:
>   - Don't die() if the value of fetch.followRemoteHEAD is unrecognized.
>   - Use case-sensitive matching for fetch.followRemoteHEAD values.
>   - Avoid the phrase "configuration option".
>   - Minor documentation wording changes.
>   - Link to v1: https://patch.msgid.link/20260612055947.1499497-1-m@lfurio.us

>     @@ builtin/fetch.c: static int git_fetch_config(const char *k, const char *v,
>      +	if (!strcmp(k, "fetch.followremotehead")) {
>      +		if (!v)
>      +			return config_error_nonbool(k);
>     -+		else if (!strcasecmp(v, "never"))
>     ++		else if (!strcmp(v, "never"))
>      +			fetch_config->follow_remote_head = FOLLOW_REMOTE_NEVER;
>     -+		else if (!strcasecmp(v, "create"))
>     ++		else if (!strcmp(v, "create"))
>      +			fetch_config->follow_remote_head = FOLLOW_REMOTE_CREATE;
>     -+		else if (!strcasecmp(v, "warn"))
>     ++		else if (!strcmp(v, "warn"))
>      +			fetch_config->follow_remote_head = FOLLOW_REMOTE_WARN;
>     -+		else if (!strcasecmp(v, "always"))
>     ++		else if (!strcmp(v, "always"))
>      +			fetch_config->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
>     -+		else
>     -+			die(_("invalid value for '%s': '%s'"),
>     -+				"fetch.followRemoteHEAD", v);
>      +	}

Not dying on an unrecognised value is certainly better than dying,
but shouldn't we at least clear fetch_config->follow_remote_head
to some "unspecified" or "default" value?  What does the existing
parser routine for remote.*.followremotehead do?

Ideally,

 (1) If the "fetch" operation ends up with not needing to consult
     the value of fetch.followRemoteHEAD at all (e.g., it is a
     one-shot fetch that updates no remote-tracking hierarchy, or it
     has a more specific per-remote setting that this variable is
     meant to serve as a mere fallback), any bogus or unknown value
     will not get any warning.

 (2) If fetch.followRemoteHEAD ends up being _used_, and if it has
     an unknown value, we should at least warn "we do not understand
     what you wrote, 'awlays', and we ignore it", or die "we do not
     understand 'reset', perhaps it is from a future version of Git?".

I do not think customization based on git_config() callback like the
above can easily implement such an ideal semantics.

And I suspect that the existing per-remote configuration that this
variable is meant to serve as a fallback definition would not work
in such an ideal way (i.e., even if we are doing one-shot fetch that
does not touch any remote-tracking hierarchies, "git fetch" may warn
if the value is not understood, and when we do need the value, the
code would only warn and does not die), so in that sense this new
code is not making things _worse_, even though it may be spreading
the same badness more widely X-<.

Thanks.