From: Matt Hunter Date: Mon, 05 Oct 2026 09:05:51 GMT Subject: Re: [PATCH v5] fetch.c: defer fetch.followRemoteHEAD validation Message-ID: 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: > 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..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. > @@ -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 ... > > 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. > + 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. > @@ -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. > .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"))