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

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

On Wed Sep 23, 2026 at 1:03 AM EDT, Colin Hinton wrote:
>> > @@ -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,

I'm not exactly sure why the config parsing in remote.c doesn't end with
a fallback 'return git_default_config(...)', though the followremotehead
case piggybacking the common 'return 0' at the end should be no problem.

> and it at most only throws a warning if no valid value is present.

which _was_ the case for fetch.followRemoteHEAD as well.  So, we should
keep the two in sync right?

> 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.

I spent some time drafting up what changes to remote.c could look like,
based on your work so far.  This follow-up patch also has extra changes
to builtin/fetch.c to accommodate the same allowed functionality as
before.  There are two awkward bits to this patch as-is, though:

builtin/remote.c::set_head()

012bc566bad7 (remote set-head: set followRemoteHEAD to "warn" if "always")
added this behavior to overrule a remote's "always" setting if the user
ever modified their HEAD manually.  So, this file needs to know about the
followRemoteHEAD values, but parsing into the enums is currently confined
to fetch.c.  This just adds another bit of string parsing.

builtin/fetch.c::get_follow_remote_head()

is updated to serve double-duty for both the fetch and remote configs,
and needs a better warning message if a bad value is detected.  Perhaps
add another parameter to the function?

With this patch below, it's arguable whether the enum definition for the
followRemoteHEAD values now better fits in fetch.c instead of remote.h.


Signed-off-by: Matt Hunter <m@lfurio.us>
---
 builtin/fetch.c  | 54 ++++++++++++++++++++++++++++++++----------------
 builtin/remote.c |  3 ++-
 remote.c         | 19 ++---------------
 remote.h         |  3 +--
 4 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 83074c48150b..5a4c9fb9309c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -187,18 +187,34 @@ 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)
+/* TODO might be worth considering a better name for this */
+struct follow_remote_head_target {
+	enum follow_remote_head_settings mode;
+	const char *no_warn_branch;
+};
+
+static struct follow_remote_head_target get_follow_remote_head(const char *setting,
+		int allow_warn_if_not_branch)
 {
+	struct follow_remote_head_target frh = { 0 };
+
 	if (!strcmp(setting, "never"))
-		return FOLLOW_REMOTE_NEVER;
+		frh.mode = FOLLOW_REMOTE_NEVER;
 	else if (!strcmp(setting, "create"))
-		return FOLLOW_REMOTE_CREATE;
+		frh.mode = FOLLOW_REMOTE_CREATE;
 	else if (!strcmp(setting, "warn"))
-		return FOLLOW_REMOTE_WARN;
+		frh.mode = FOLLOW_REMOTE_WARN;
+	else if (skip_prefix(setting, "warn-if-not-", &frh.no_warn_branch)
+			&& allow_warn_if_not_branch)
+		frh.mode = FOLLOW_REMOTE_WARN;
 	else if (!strcmp(setting, "always"))
-		return FOLLOW_REMOTE_ALWAYS;
-	warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting);
-	return FOLLOW_REMOTE_UNCONFIGURED;
+		frh.mode = FOLLOW_REMOTE_ALWAYS;
+	else
+		warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting);
+		/* TODO this also parses remote.<name>.followRemoteHEAD,
+		 * but the warning string says fetch.followRemoteHEAD */
+
+	return frh;
 }
 
 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
@@ -1758,12 +1774,11 @@ static void warn_set_head(const char *remote, const char *head_name,
 }
 
 static int set_head(const struct ref *remote_refs, struct remote *remote,
-			int follow_remote_head)
+			struct follow_remote_head_target follow_remote_head)
 {
 	int result = 0, create_only, baremirror, was_detached;
 	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
 		      b_local_head = STRBUF_INIT;
-	const char *no_warn_branch = remote->no_warn_branch;
 	char *head_name = NULL;
 	struct ref *ref, *matches;
 	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
@@ -1793,7 +1808,7 @@ static int set_head(const struct ref *remote_refs, struct remote *remote,
 	if (!head_name)
 		goto cleanup;
 	baremirror = is_bare_repository(the_repository) && remote->mirror;
-	create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror;
+	create_only = follow_remote_head.mode == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror;
 	if (baremirror) {
 		strbuf_addstr(&b_head, "HEAD");
 		strbuf_addf(&b_remote_head, "refs/heads/%s", head_name);
@@ -1813,8 +1828,9 @@ static int set_head(const struct ref *remote_refs, struct remote *remote,
 		goto cleanup;
 	}
 	if (verbosity >= 0 &&
-		follow_remote_head == FOLLOW_REMOTE_WARN &&
-		(!no_warn_branch || strcmp(no_warn_branch, head_name)))
+		follow_remote_head.mode == FOLLOW_REMOTE_WARN &&
+		(!follow_remote_head.no_warn_branch ||
+		 strcmp(follow_remote_head.no_warn_branch, head_name)))
 		warn_set_head(remote->name, head_name, &b_local_head, was_detached);
 
 cleanup:
@@ -1929,7 +1945,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 = 0;
+	struct follow_remote_head_target follow_remote_head = { 0 };
 
 	if (tags == TAGS_DEFAULT) {
 		if (transport->remote->fetch_tags == 2)
@@ -1954,14 +1970,16 @@ static int do_fetch(struct transport *transport,
 			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;
+			if (transport->remote->follow_remote_head_raw)
+				follow_remote_head = get_follow_remote_head(
+						transport->remote->follow_remote_head_raw, 1);
 			else if (config->follow_remote_head_raw)
-				follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
+				follow_remote_head = get_follow_remote_head(
+						config->follow_remote_head_raw, 0);
 			else
-				follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+				follow_remote_head.mode = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
 			
-			if (follow_remote_head != FOLLOW_REMOTE_NEVER)
+			if (follow_remote_head.mode != FOLLOW_REMOTE_NEVER)
 				do_set_head = 1;
 		}
 		if (branch && branch_has_merge_config(branch) &&
diff --git a/builtin/remote.c b/builtin/remote.c
index de989ea3ba96..89ac1f0daa82 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1606,7 +1606,8 @@ static int set_head(int argc, const char **argv, const char *prefix,
 	}
 	if (opt_a)
 		report_set_head_auto(argv[0], head_name, &b_local_head, was_detached);
-	if (remote->follow_remote_head == FOLLOW_REMOTE_ALWAYS) {
+	if (remote->follow_remote_head_raw &&
+			!strcmp(remote->follow_remote_head_raw, "always")) {
 		struct strbuf config_name = STRBUF_INIT;
 		strbuf_addf(&config_name,
 			"remote.%s.followremotehead", remote->name);
diff --git a/remote.c b/remote.c
index fe6206846356..5fdcadfbdbf0 100644
--- a/remote.c
+++ b/remote.c
@@ -581,23 +581,8 @@ static int handle_config(const char *key, const char *value,
 		return parse_transport_option(key, value,
 					      &remote->negotiation_include);
 	} else if (!strcmp(subkey, "followremotehead")) {
-		const char *no_warn_branch;
-		if (!strcmp(value, "never"))
-			remote->follow_remote_head = FOLLOW_REMOTE_NEVER;
-		else if (!strcmp(value, "create"))
-			remote->follow_remote_head = FOLLOW_REMOTE_CREATE;
-		else if (!strcmp(value, "warn")) {
-			remote->follow_remote_head = FOLLOW_REMOTE_WARN;
-			remote->no_warn_branch = NULL;
-		} else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) {
-			remote->follow_remote_head = FOLLOW_REMOTE_WARN;
-			remote->no_warn_branch = no_warn_branch;
-		} else if (!strcmp(value, "always")) {
-			remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
-		} else {
-			warning(_("unrecognized followRemoteHEAD value '%s' ignored"),
-				value);
-		}
+		free(remote->follow_remote_head_raw);
+		remote->follow_remote_head_raw = xstrdup(value);
 	}
 	return 0;
 }
diff --git a/remote.h b/remote.h
index cca02033b9d7..cd97df017454 100644
--- a/remote.h
+++ b/remote.h
@@ -122,8 +122,7 @@ struct remote {
 	struct string_list negotiation_restrict;
 	struct string_list negotiation_include;
 
-	enum follow_remote_head_settings follow_remote_head;
-	const char *no_warn_branch;
+	char *follow_remote_head_raw;
 };
 
 /**
-- 
2.55.0