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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2026, 05:32 UTC
Message-ID: <xmqqwlsdhmvk.fsf@gitster.g>
In-Reply-To: <20260922040047.2567-1-colinlewishinton@gmail.com>

Colin Hinton <colinlewishinton@gmail.com> writes:

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

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

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.

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

Show 7 quoted lines

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

Show 12 quoted lines

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

Show 13 quoted lines

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

Show 25 quoted lines

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

Show 7 quoted lines

> @@ -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?

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

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