Re: [PATCH v2] remote.c: defer remote.<name>.followRemoteHEAD validation

["Matt Hunter" <m@lfurio.us>]

On Fri Oct 30, 2026 at 11:12 AM EDT, Junio C Hamano wrote:
> "Matt Hunter" <m@lfurio.us> writes:
>> ...
>> I wonder if it makes sense to incorporate such a new helper function
>> into the existing 'get_follow_remote_head()', since it would be the only
>> place using it.
>
> I am not sure what being the only place has to do with avoiding to
> split a helper function out to clarify the main logic flow, though.
> And simultaneously, another topic wants to have this piece of logic
> twice.

I should explain myself better.  I don't mean Colin should avoid
splitting out logic from the main flow in do_fetch.  But the end result
can be reached without introducing a new function.  I find needing to
hold fewer functions in my head makes the code easier to reason about,
though that is a subjective point.

The idea in my head is below and is
Signed-off-by: Matt Hunter <m@lfurio.us>
Feel free to run with this or not.

diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb59e70865ff..844ff5105f91 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -196,23 +196,44 @@ static int git_fetch_config(const char *k, const char *v,
 	return git_default_config(k, v, ctx, cb);
 }
 
-static enum follow_remote_head_settings get_follow_remote_head(const char *setting,
-    const char *key, const char **no_warn_branch)
+static enum follow_remote_head_settings get_follow_remote_head(
+		const struct transport *transport, const struct fetch_config *config,
+		const char **no_warn_branch)
 {
-	if (!setting)
-		die(_("missing value for '%s'"), key);
-	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 (no_warn_branch && skip_prefix(setting, "warn-if-not-", no_warn_branch))
-		return FOLLOW_REMOTE_WARN;
-	else if (!strcmp(setting, "always"))
-		return FOLLOW_REMOTE_ALWAYS;
-	warning(_("unrecognized %s value '%s' ignored"), key, setting);
-	return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+	int ret = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+	char *key = NULL;
+	const char *setting = NULL;
+
+	if (transport->remote->follow_remote_head_seen) {
+		key = xstrfmt("remote.%s.followRemoteHEAD", transport->remote->name);
+		setting = transport->remote->follow_remote_head_raw;
+	} else if (config->follow_remote_head_seen) {
+		key = xstrdup("fetch.followRemoteHEAD");
+		setting = config->follow_remote_head_raw;
+		no_warn_branch = NULL;
+	}
+
+	if (key) {
+		if (!setting)
+			die(_("missing value for '%s'"), key);
+		else if (!strcmp(setting, "never"))
+			ret = FOLLOW_REMOTE_NEVER;
+		else if (!strcmp(setting, "create"))
+			ret = FOLLOW_REMOTE_CREATE;
+		else if (!strcmp(setting, "warn"))
+			ret = FOLLOW_REMOTE_WARN;
+		else if (no_warn_branch &&
+				skip_prefix(setting, "warn-if-not-", no_warn_branch))
+			ret = FOLLOW_REMOTE_WARN;
+		else if (!strcmp(setting, "always"))
+			ret = FOLLOW_REMOTE_ALWAYS;
+		else
+			warning(_("unrecognized %s value '%s' ignored"), key, setting);
+
+		free(key);
+	}
+
+	return ret;
 }
 
 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
@@ -1990,19 +2011,9 @@ 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_seen) {
-				char *key = xstrfmt("remote.%s.followRemoteHEAD", transport->remote->name);
-				follow_remote_head = get_follow_remote_head(
-					transport->remote->follow_remote_head_raw,
-					key, &no_warn_branch);
-				free(key);
-			} else if (config->follow_remote_head_seen) {
-				follow_remote_head = get_follow_remote_head(
-					config->follow_remote_head_raw,
-					"fetch.followRemoteHEAD", NULL);
-			} else {
-				follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-			}
+			follow_remote_head =
+				get_follow_remote_head(transport, config,
+						&no_warn_branch);
 		}
 		if (branch && branch_has_merge_config(branch) &&
 		    !strcmp(branch->remote_name, transport->remote->name)) {


Admittedly, I haven't been following Harald's hn/fetch-refmap topic
closely, but am more familiar with the affected areas now.