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

On Mon, Sep 21, 2026 at 10:32 PM Junio C Hamano [off-list ref] wrote:

Colin Hinton [off-list ref] writes:

quoted

Previously, fetch.followRemoteHEAD was validated and any invalid
value was warned about unconditionally during config parsing.

Early paragraphs that make observation on how the current system
works should be written in present tense.  It is the status quo, so
we shouldn't say "previously" and we do not need to say "currently".

    The value of configuration variable "fetch.followRemoteHEAD is
    validated while the configuration file is being parsed, which
    lead to a warning, even when we do not need to know the value.

quoted

Now store the raw config string instead, and resolve/validate it lazily
at the one call in do_fetch(), so an irrelevant fetch no longer warns about an unrelated
config value it never needed.

Well written, except that "an irrelevant fetch" is a bit awkward.
"Irrelevant how, for whom, and why?" is a set of natural questions
that come to readers' minds.  I am guessing that you wanted to say
that "git fetch" does not always need to know the value of the
fetch.followRemoteHEAD configuration variable, perhaps because a
particular invocation of "git fetch" receives specific refspec.
You'd need to find a concise way to say that and replace the
"irrelevant" there.

Okay, for my next patch I will clear this up, and will change my
tenses to all be in present tense.

In any case, it is a very good discipline to avoid dying or making
noises while reading the configuration file and instead complain
only when we know we will use the bad value.

quoted

 struct fetch_config {
      enum display_format display_format;
-     enum follow_remote_head_settings follow_remote_head;
+     char *follow_remote_head_raw;

OK.  So this is the read the value and keep it as-is.

quoted

      int all;
      int prune;
      int prune_tags;

@@ -178,22 +178,29 @@ static int git_fetch_config(const char *k, const char *v,
      if (!strcmp(k, "fetch.followremotehead")) {
              if (!v)
                      return config_error_nonbool(k);

This error still triggers even when the configuration variable is
irrelevant (e.g, "git fetch origin master", i.e., rs->nr != 0).
Dealing with it is well within the scope of the topic, isn't it?
You may be ignoring

        [fetch]
                followremotehead = bogus

when the user runs "git fetch https://over.there/repo master" with
this patch, which may be an improvement, but if the user has a
valueless truth

        [fetch]
                followremotehead

then the same command would die while parsing the configuration
variable, which is not what you wanted to see, right?

I Agree on this, I will defer this check to the same point of use that
I have abstracted in get_follow_remote_head. It would make sense to
only report a valueless fetch.followRemoteHEAD when the value is
needed. I also assume that if followRemoteHEAD is valueless, that we
should call die(...) rather than config_error_nonbool() to match
current behavior, so I will implement this in my next patch unless
there is something additional I should consider.

quoted

-             else if (!strcmp(v, "never"))
-                     fetch_config->follow_remote_head = FOLLOW_REMOTE_NEVER;
-             else if (!strcmp(v, "create"))
-                     fetch_config->follow_remote_head = FOLLOW_REMOTE_CREATE;
-             else if (!strcmp(v, "warn"))
-                     fetch_config->follow_remote_head = FOLLOW_REMOTE_WARN;
-             else if (!strcmp(v, "always"))
-                     fetch_config->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
-             else
-                     warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), v);
+             free(fetch_config->follow_remote_head_raw);
+             fetch_config->follow_remote_head_raw = xstrdup(v);

Good to see that the code is prepared to see the same variable
defined multiple times in the configuration stream without leaking
earlier values.

quoted

+static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
+{
+     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 FOLLOW_REMOTE_UNCONFIGURED;
+}

OK.  So unrecognised are treated as unconfigured, just like before.

quoted

 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
 {
      BUG_ON_OPT_NEG(unset);

@@ -1922,7 +1929,7 @@ static int do_fetch(struct transport *transport,
      struct ref_update_display_info_array display_array = { 0 };
      struct strmap rejected_refs = STRMAP_INIT;
      int summary_width = 0;
-     int follow_remote_head;
+     int follow_remote_head = 0;

      if (tags == TAGS_DEFAULT) {
              if (transport->remote->fetch_tags == 2)

@@ -1938,22 +1945,6 @@ static int do_fetch(struct transport *transport,
                      goto cleanup;
      }

-     /*
-      * NEEDSWORK: By the time this function executes, we have already parsed
-      * all such followRemoteHEAD values from the external configuration,
-      * potentially emitting warning messages for bogus values.  Ideally, if
-      * this fetch ends up not needing to consult these values, then git would
-      * not ever output a value warning. (eg: when pulling from a URL directly -
-      * rather than a configured remote, or when a remote's followRemoteHEAD
-      * overrides the fallback fetch setting)
-      */

Good write-up.  We should be able to steal some in our own description.

quoted

@@ -1962,6 +1953,14 @@ static int do_fetch(struct transport *transport,
              if (transport->remote->fetch.nr) {
                      refspec_ref_prefixes(&transport->remote->fetch,
                                           &transport_ls_refs_options.ref_prefixes);
+
+                     if (transport->remote->follow_remote_head)
+                             follow_remote_head = transport->remote->follow_remote_head;

The code assumes that remote.*.followRemoteHEAD has been pre-parsed.
Doesn't the code to do so in remote.c::handle_config() share exactly
the same problem as you are fixing here?

I agree that the same problem that is being addressed here is present
in remote.c as well. The only difference being, that there is no
return call in the followremotehead block in remote.c, and it at most
only throws a warning if no valid value is present. I think this
should be addressed, but I am uncertain if this is within the scope of
this issue and should be resolved now, or if this requires its own
investigation and should be resolved in a future patch. Regardless I
am eager to work on it, but would like some guidance as to what is
most appropriate for a change in remote.c.

quoted

+                     else if (config->follow_remote_head_raw)
+                             follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
+                     else
+                             follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;

Make a mental note that do_set_head is flipped on ONLY here in this
function.

quoted

                      if (follow_remote_head != FOLLOW_REMOTE_NEVER)
                              do_set_head = 1;
              }

And later, do_set_head is referenced twice.  Once when preparing the
transport options to first discover what refs they have (ls-refs)

        if (do_set_head)
                strvec_push(&transport_ls_refs_options.ref_prefixes,
                            "HEAD");

and then once more to make a set-head call using follow_remote_head.

        if (do_set_head) {
                /*
                 * Way too many cases where this can go wrong so let's just
                 * ignore errors and fail silently for now.
                 */
                set_head(remote_refs, transport->remote, follow_remote_head);
        }

Incidentally, after that "lazily turn configuration string into
follow_remote_head variable" block is left, this is the only place
that follow_remote_head variable is referenced.

Which suggests to me that we can get rid of do_set_head variable, we
can initialize follow_remote_head variable to FOLLOW_REMOTE_NEVER,
and replace these two

        if (do_set_head)

with

        if (follow_remote_head != FOLLOW_REMOTE_NEVER)

and the resulting code may become a tad easier to follow.

Hmmm?

Certainly would be an improvement, and one I will be implementing in
my next patch.