Re: [PATCH v4] fetch.c: defer fetch.followRemoteHEAD validation

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2026, 17:42 UTC
Message-ID: <xmqqh5j1s6q0.fsf@gitster.g>
In-Reply-To: <CAHeTm9PK=sc4ajmf53rhurd532OST0qYfEaS-Kc5kpGZf1Zw2A@mail.gmail.com>

Colin Hinton <colinlewishinton@gmail.com> writes:

Show 14 quoted lines

> My rationale for this recent change came from when I was evaluating
> what calls get_follow_remote_head() in my patch.
>
> With the current design, get_follow_remote_head() is only called in
> do_fetch() in this conditional else if
> (config->follow_remote_head_raw).
>
> If followRemoteHEAD is now NULL because we set it as such in
> fetch_config->follow_remote_head_raw = xstrdup_or_null(v); Then this
> conditional is skipped, and we will never call the die(), and alert
> the user that their value is blank.
>
> To fully fix based on your suggestion, I suppose the design question
> is, should empty string warn or die?

I think we should behave the same when we see "nvere". We do not understand what they wanted us to do in either case, so we should behave the same way, be it warn-and-ignore or complain-and-die.

Given that we now check the validity of the value only after we determine that we need it, I think it is OK to tighten the rules to die() instead of warn(). The historical behavior of not dying, and instead warning and ignoring, was a weak excuse for leaving configuration parsing broken and checking the validity of the value in the wrong place.

This patch rectifies the situation, which is a very good step toward doing the right thing. It is perfectly fine to tighten the rules as a separate topic after this patch lands and things stabilize, but this patch lays the groundwork for us to move in that direction.

Show 6 quoted lines

> If empty string should warn, I likely will need to add some value in
> the fetch_config struct such as follow_remote_head_seen, and use this
> as our conditional in do_fetch() rather than the
> follow_remote_head_raw, to account for when followRemoteHEAD was set
> to anything. Then when the check in get_follow_remote_head() occurs,
> we know to die or warn based on NULL, or bogus.

... because? Ah, because then you lose distinction between "the configuration variable not set at all" and "the configuration variable is set to the valueless true"?

If so, you'd need to be able to tell _three_ cases. The empty string you use as a stand in for "valueless true" should be distinguishable from the empty string the user set (by mistake).

Show 12 quoted lines

> Visually, it would look something like this.
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 2cb0bcca8b..af22f63954 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -104,6 +104,7 @@ static struct string_list negotiation_include =
> STRING_LIST_INIT_NODUP;
>  struct fetch_config {
>         enum display_format display_format;
>         char *follow_remote_head_raw;
> +       int follow_remote_head_seen;

That would certainly work, and might be easier to work with than what I would have done, which is not to bother with this extra variable and instead to have a

	static const char *valueless_true = "true";

in the file scope.  Then use that ...

Show 12 quoted lines

>         int all;
>         int prune;
>         int prune_tags;
> @@ -177,10 +178,8 @@ static int git_fetch_config(const char *k, const char *v,
>
>         if (!strcmp(k, "fetch.followremotehead")) {
>                 free(fetch_config->follow_remote_head_raw);
> -               if (!v)
> -                       fetch_config->follow_remote_head_raw = xstrdup("");
> -               else
> -                       fetch_config->follow_remote_head_raw = xstrdup(v);
> +               fetch_config->follow_remote_head_raw = xstrdup_or_null(v);

... here like so:

		if (fetch_config->follow_remote_head_raw != valueless_true)
			free(fetch_config->follow_remote_head_raw);
		if (!v)
			fetch_config->follow_remote_head_raw = valueless_true;
		else
			...

Show 11 quoted lines

> @@ -189,7 +188,7 @@ static int git_fetch_config(const char *k, const char *v,
>
>  static enum follow_remote_head_settings get_follow_remote_head(const
> char *setting)
>  {
> -       if (!setting || !*setting)
> +       if (!setting) /*!*setting would return true on "" removing to
> warn instead*/
>                 die(_("missing value for 'fetch.followRemoteHEAD'"));
>         else if (!strcmp(setting, "never"))
>                 return FOLLOW_REMOTE_NEVER;

... and deal with the setting that is equal to valueless_true here.

I think either way would work, and the way you outlined would be better.