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

From: Colin Hinton <colinlewishinton@gmail.com>
Date: Oct 4, 2026, 15:40 UTC
Message-ID: <CAHeTm9PK=sc4ajmf53rhurd532OST0qYfEaS-Kc5kpGZf1Zw2A@mail.gmail.com>
In-Reply-To: <xmqqa4otvbnk.fsf@gitster.g>

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?

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.

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;
        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);
+               follow_remote_head_seen = 1;
                return 0;
        }

@@ -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;
@@ -1960,7 +1959,7 @@ static int do_fetch(struct transport *transport,
                         */
                        if (transport->remote->follow_remote_head)
                                follow_remote_head =
transport->remote->follow_remote_head;
-                       else if (config->follow_remote_head_raw)
+                       else if (config->follow_remote_head_seen)
                                follow_remote_head =
get_follow_remote_head(config->follow_remote_head_raw);
                        else
                                follow_remote_head =
BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
@@ -2510,6 +2509,7 @@ int cmd_fetch(int argc,
        struct fetch_config config = {
                .display_format = DISPLAY_FORMAT_FULL,
                .follow_remote_head_raw = NULL,
+               .follow_remote_head_seen = 0,
                .prune = -1,
                .prune_tags = -1,
                .show_forced_updates = 1,

Let me know if this sounds right, and I will add this in for v5.
-Colin Hinton

On Sun, Oct 4, 2026 at 6:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Colin Hinton <colinlewishinton@gmail.com> writes:
>
> >       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);
>
> Hmph, this means that the code cannot distinguish between
>
>         [fetch] followremotehead
>
>         [fetch] followremotehead = ""
>
> It would be less code and more expressive if you lost the
> conditional, i.e.,
>
>         if (!strcmp(k, "fetch.followremotehead"))
>                 free(fetch_config->follow_remote_head_raw);
>                 fetch_config->follow_remote_head_raw = xstrdup_or_null(v);
>         }
>
> > +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> > +{
> > +     if (!setting || !*setting)
> > +             die(_("missing value for 'fetch.followRemoteHEAD'"));
>
> Then you can differenciate
>
>         if (!setting)
>                 ... we got '[fetch] followRemoteHEAD' ...
>                 die() as before, complaining that the this is not a Bool.
>         else if (!*setting)
>                 ... we got '[fetch] followRemoteHEAD = ""' ...
>
> if we wanted to.  It probably do not need to check for an empty
> string as it will fall through the "else if" cascade below and
> eventually end up with the warning + default.
>
> > +     else if (!strcmp(setting, "never"))
> > +             return FOLLOW_REMOTE_NEVER;
> > +     else if (!strcmp(setting, "create"))
> > +             return FOLLOW_REMOTE_CREATE;
> > +     else if (!strcmp(setting, "warn"))
> > +             return FOLLOW_REMOTE_WARN;
> > +     else if (!strcmp(setting, "always"))
> > +             return FOLLOW_REMOTE_ALWAYS;
> > +     warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting);
> > +     return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
> > +}