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

Colin - thanks for picking this up.

Just wanted to make note of some lingering thoughts of mine from when
this config was added.  I wouldn't consider these necessary for your
patch, though you might agree with the ideas.

On Tue Sep 22, 2026 at 1:32 AM EDT, Junio C Hamano wrote:

Colin Hinton [off-list ref] writes:

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.

There was an idea I raised in [1] that didn't really get discussed.
That being that we should effectively act like FOLLOW_REMOTE_NEVER is
set when the configured value is unrecognized.

The situation I envision is a user porting their .gitconfig file to a
system running an older git, that doesn't know about their preferred
setting.  Given that _something_ is configured, the user obviously
doesn't want the default behavior, but that's what they'll get when
FOLLOW_REMOTE_UNCONFIGURED is returned.

FOLLOW_REMOTE_NEVER seems like the least suprising action to take when
we don't understand the request.  And I think this reasoning could apply
to remote.foo.followRemoteHEAD as well, if you think it's worth doing
here.

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.

It occurred to me a while ago that there's another case in which we
might want to skip querying the remote for its HEAD - when
FOLLOW_REMOTE_CREATE is in effect, and the remote already has a local
HEAD symref.  Given that 'create' is the default mode for this setting,
it would probably be a valuable save on network and server overhead.

Of course, I think this should probably be its own topic, separate from
what this patch is addressing.  But if the condition of that 'if'
statement is to become more complicated, it might be a good reason to
not start duplicating it here.

Hmmm?

1: https://lore.kernel.org/git/DJBVYP58YNTU.LQ7VXFIQE84H@lfurio.us/ (local)