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

From: Matt Hunter <m@lfurio.us>
Date: Oct 5, 2026, 09:05 UTC
Message-ID: <DLWS9R3ZM2Z0.MVT71MWE19LN@lfurio.us>
In-Reply-To: <20261004201428.5210-1-colinlewishinton@gmail.com>

Hi Colin,

Just a couple comments and 'thinking aloud' on the design of the patch from me. Unfortunately I don't have time at the moment to test this new revision.

thanks

On Sun Oct 4, 2026 at 4:14 PM EDT, Colin Hinton wrote:

Show 14 quoted lines

> The value of the fetch.followRemoteHEAD configuration variable is
> validated while the configuration file is being parsed, which
> produces a warning even when this particular "git fetch" invocation
> will never consult it.
>
> Store the raw config string instead, and resolve/validate it lazily
> at the one place in do_fetch() that actually uses it, so a mistyped
> value only warns, and a missing value only dies, when this fetch
> would have consulted it.
>
> remote.c's handle_config() has the same problem for
> remote.<name>.followRemoteHEAD, but is left unaddressed here since
> it touches shared remote-parsing infrastructure used well beyond
> fetch. Leave NEEDSWORK comments at both the now unresolved call site

The new comment in fetch.c doesn't actually have the NEEDSWORK label. I would probably suggest placing one there, instead of adjusting this sentence.

Show 31 quoted lines

> @@ -103,7 +103,8 @@ static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
>  
>  struct fetch_config {
>  	enum display_format display_format;
> -	enum follow_remote_head_settings follow_remote_head;
> +	char *follow_remote_head_raw;
> +	int follow_remote_head_seen;
>  	int all;
>  	int prune;
>  	int prune_tags;
> @@ -176,24 +177,31 @@ static int git_fetch_config(const char *k, const char *v,
>  	}
>  
>  	if (!strcmp(k, "fetch.followremotehead")) {
> -		if (!v)
> -			return config_error_nonbool(k);
> -		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_or_null(v);
> +		fetch_config->follow_remote_head_seen = 1;
>  		return 0;
>  	}

We now have 'follow_remote_head_seen', which tells us whether to trust the 'raw' string ptr or if the variable hasn't been set by the user.

Therefore, when 'seen' is true, the raw string is:

    - NULL when a valueless true was specified
    - "" when an actual empty string was specified
    - any other string for a normal value

Because of this ...

Show 8 quoted lines

>  
>  	return git_default_config(k, v, ctx, cb);
>  }
>  
> +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> +{
> +	if (!setting)
> +		die(_("missing value for 'fetch.followRemoteHEAD'"));

... this case is now meaningful, as we couldn't previously reach this if 'setting' was NULL.

Show 9 quoted lines

> +	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 (!strcmp(setting, "always"))
> +		return FOLLOW_REMOTE_ALWAYS;
> +	warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting);

And the empty string case is handled here, which makes sense.

I believe Junio had mentioned tightening this warning to a die. I'm not sure if it was _just_ this case or something else. Personally, I think the warning is still the better call for some "f@KeValue". But it _could_ make sense to die on "", since that is more obviously mis-configured.

Having typed the above out, I now realize that is actually how the previous v4 behaved (but in slightly less code). So, we're getting into opinionated details here... Though, as-is I think this v5 implementation is reasonable.

Show 7 quoted lines

> @@ -2509,7 +2508,8 @@ int cmd_fetch(int argc,
>  {
>  	struct fetch_config config = {
>  		.display_format = DISPLAY_FORMAT_FULL,
> -		.follow_remote_head = FOLLOW_REMOTE_UNCONFIGURED,
> +		.follow_remote_head_raw = NULL,
> +		.follow_remote_head_seen = 0,

and the 'seen' variable is initialized to false, good.

Show 21 quoted lines

>  		.prune = -1,
>  		.prune_tags = -1,
>  		.show_forced_updates = 1,
> diff --git a/remote.c b/remote.c
> index fe62068463..58f3436222 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -582,6 +582,13 @@ static int handle_config(const char *key, const char *value,
>  					      &remote->negotiation_include);
>  	} else if (!strcmp(subkey, "followremotehead")) {
>  		const char *no_warn_branch;
> +		/*
> +		 * NEEDSWORK: this is validated/warned about here, during config
> +		 * parsing, regardless of whether the fetch that triggered this
> +		 * parse will ever consult it for this particular remote. See
> +		 * fetch.c's deferred handling of fetch.followRemoteHEAD for the
> +		 * pattern this should likely follow.
> +		 */
>  		if (!strcmp(value, "never"))
>  			remote->follow_remote_head = FOLLOW_REMOTE_NEVER;
>  		else if (!strcmp(value, "create"))