Colin HintonSep 22, 2026, 04:00 UTC on lorePreviously, fetch.followRemoteHEAD was validated and any invalid value was warned about unconditionally during config parsing.
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.
Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
---
builtin/fetch.c | 57 ++++++++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 29 deletions(-)
Show changes to builtin/fetch.c +28 −29
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab7db2be06..64ad26f5d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -103,7 +103,7 @@ 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 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);
- 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);
+
return 0;
}
return git_default_config(k, v, ctx, cb);
}
+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;
+}
+
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)
- */
- if (transport->remote->follow_remote_head)
- follow_remote_head = transport->remote->follow_remote_head;
- else if (config->follow_remote_head)
- follow_remote_head = config->follow_remote_head;
- else
- follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -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;
+ 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;
+
if (follow_remote_head != FOLLOW_REMOTE_NEVER)
do_set_head = 1;
}
@@ -2509,7 +2508,7 @@ 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,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
--
2.55.0.windows.3
Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
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?
Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
On Mon, Sep 21, 2026 at 10:32 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
>
> 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.
Okay, for my next patch I will clear this up, and will change my tenses to all be in present tense.
Show 37 quoted lines
>
> 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.
>
> > 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?I Agree on this, I will defer this check to the same point of use that I have abstracted in get_follow_remote_head. It would make sense to only report a valueless fetch.followRemoteHEAD when the value is needed. I also assume that if followRemoteHEAD is valueless, that we should call die(...) rather than config_error_nonbool() to match current behavior, so I will implement this in my next patch unless there is something additional I should consider.
Show 74 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.
>
> > +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.
>
> > 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.
>
> > @@ -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, and it at most only throws a warning if no valid value is present. 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.
Show 46 quoted lines
> > + 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?Certainly would be an improvement, and one I will be implementing in my next patch.
Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
Colin - thanks for picking this up.
Just wanted to make note of some lingering thoughts of mine from when this config was added. I wouldn't consider these necessary for your patch, though you might agree with the ideas.
On Tue Sep 22, 2026 at 1:32 AM EDT, Junio C Hamano wrote:
Show 17 quoted lines
> Colin Hinton <colinlewishinton@gmail.com> writes:
>
>> +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.There was an idea I raised in [1] that didn't really get discussed. That being that we should effectively act like FOLLOW_REMOTE_NEVER is set when the configured value is unrecognized.
The situation I envision is a user porting their .gitconfig file to a system running an older git, that doesn't know about their preferred setting. Given that _something_ is configured, the user obviously doesn't want the default behavior, but that's what they'll get when FOLLOW_REMOTE_UNCONFIGURED is returned.
FOLLOW_REMOTE_NEVER seems like the least suprising action to take when we don't understand the request. And I think this reasoning could apply to remote.foo.followRemoteHEAD as well, if you think it's worth doing here.
Show 40 quoted lines
>
> 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.It occurred to me a while ago that there's another case in which we might want to skip querying the remote for its HEAD - when FOLLOW_REMOTE_CREATE is in effect, and the remote already has a local HEAD symref. Given that 'create' is the default mode for this setting, it would probably be a valuable save on network and server overhead.
Of course, I think this should probably be its own topic, separate from what this patch is addressing. But if the condition of that 'if' statement is to become more complicated, it might be a good reason to not start duplicating it here.
>
> Hmmm?
1: https://lore.kernel.org/git/DJBVYP58YNTU.LQ7VXFIQE84H@lfurio.us/
Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
On Wed Sep 23, 2026 at 1:03 AM EDT, Colin Hinton wrote:
Show 15 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?
>>
> 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?
Show 6 quoted lines
> 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(-)
Show changes to 4 files +41 −38
builtin/fetch.c, builtin/remote.c, remote.c, remote.h
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
Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
On Thu, Sep 24, 2026 at 12:50 AM Matt Hunter <m@lfurio.us> wrote:
Show 27 quoted lines
>
> 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?
>To respond to both of your emails, I agree that the two should be kept in sync, as Junio pointed out, a valueless followremotehead will currently result in a die, yet I agree with your point from your [1] that it would be more sensible to warn, and treat a valueless or bogus followremotehead as FOLLOW_REMOTE_NEVER. For this patch, I will keep the behavior similar, but for a follow-on patch and with some approval I agree with this change.
Show 29 quoted lines
> > 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.
>
Thank you very much for this. I think this is a great starting point for a follow up patch.
-Colin Hinton
[PATCH v2] fetch.c: defer fetch.followRemoteHEAD validation
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 stale or mistyped fetch.followRemoteHEAD value only produces a warning when this fetch would have consulted it.
Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
---
builtin/fetch.c | 67 +++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 36 deletions(-)
Show changes to builtin/fetch.c +31 −36
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab7db2be06..6a5254a9bc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -103,7 +103,7 @@ 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 all;
int prune;
int prune_tags;
@@ -176,24 +176,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(v);
+
return 0;
}
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'"));
+ 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);
+ return FOLLOW_REMOTE_UNCONFIGURED;
+}
+
static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -1918,11 +1925,10 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
struct fetch_head fetch_head = { 0 };
struct strbuf err = STRBUF_INIT;
- int do_set_head = 0;
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 = FOLLOW_REMOTE_NEVER;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1938,22 +1944,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)
- */
- if (transport->remote->follow_remote_head)
- follow_remote_head = transport->remote->follow_remote_head;
- else if (config->follow_remote_head)
- follow_remote_head = config->follow_remote_head;
- else
- follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -1962,8 +1952,13 @@ 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 (follow_remote_head != FOLLOW_REMOTE_NEVER)
- do_set_head = 1;
+
+ if (transport->remote->follow_remote_head)
+ follow_remote_head = transport->remote->follow_remote_head;
+ 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;
}
if (branch && branch_has_merge_config(branch) &&
!strcmp(branch->remote_name, transport->remote->name)) {
@@ -1987,7 +1982,7 @@ static int do_fetch(struct transport *transport,
strvec_push(&transport_ls_refs_options.ref_prefixes,
"refs/tags/");
- if (do_set_head)
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER)
strvec_push(&transport_ls_refs_options.ref_prefixes,
"HEAD");
@@ -2164,7 +2159,7 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
- if (do_set_head) {
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER) {
/*
* Way too many cases where this can go wrong so let's just
* ignore errors and fail silently for now.
@@ -2509,7 +2504,7 @@ 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,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
--
2.55.0.windows.3
Re: [PATCH v2] fetch.c: defer fetch.followRemoteHEAD validation
Colin Hinton <colinlewishinton@gmail.com> writes:
Show 25 quoted lines
> struct fetch_config {
> enum display_format display_format;
> - enum follow_remote_head_settings follow_remote_head;
> + char *follow_remote_head_raw;
> int all;
> int prune;
> int prune_tags;
> @@ -176,24 +176,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(v);This will segfault when !v, so
fetch_config->follow_remote_head_raw = xstrdup_or_null(v);
With that change,
Show 15 quoted lines
> +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> +{
> + if (!setting)
> + die(_("missing value for 'fetch.followRemoteHEAD'"));
> + 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);
> + return FOLLOW_REMOTE_UNCONFIGURED;
> +}This would do a reasonable job.
We should do something similar to what remote.c parses for consistency, but other than that, it seems this topic is moving in the right direction.
Thanks.
Re: [PATCH v2] fetch.c: defer fetch.followRemoteHEAD validation
On Fri, Sep 25, 2026 at 12:40 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
>
> Colin Hinton <colinlewishinton@gmail.com> writes:
>
> > struct fetch_config {
> > enum display_format display_format;
> > - enum follow_remote_head_settings follow_remote_head;
> > + char *follow_remote_head_raw;
> > int all;
> > int prune;
> > int prune_tags;
> > @@ -176,24 +176,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(v);
>
> This will segfault when !v, so
>
> fetch_config->follow_remote_head_raw = xstrdup_or_null(v);Show 26 quoted lines
>
> With that change,
>
> > +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> > +{
> > + if (!setting)
> > + die(_("missing value for 'fetch.followRemoteHEAD'"));
> > + 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);
> > + return FOLLOW_REMOTE_UNCONFIGURED;
> > +}
>
> This would do a reasonable job.
>
> We should do something similar to what remote.c parses for
> consistency, but other than that, it seems this topic is moving in
> the right direction.
>
> Thanks.The only critical difference I see in the configuration parse between remote.c and fetch.c is the case for "warn-if-not-$branch". From reading the git-config manpage, this is only a setting for a remote and not for fetch directly so I do not see a reason to check this in fetch.c. Perhaps I am missing something else to make this more consistent, or perhaps there is an argument to support the configuration for fetch to "warn-if-not-$branch" in which case can be added to this patch.
-Colin Hinton
[PATCH v3] fetch.c: defer fetch.followRemoteHEAD validation
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 stale or mistyped fetch.followRemoteHEAD value only produces a warning when this fetch would have consulted it.
Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
---
builtin/fetch.c | 67 +++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 36 deletions(-)
Show changes to builtin/fetch.c +31 −36
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab7db2be06..85e3d1ca4b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -103,7 +103,7 @@ 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 all;
int prune;
int prune_tags;
@@ -176,24 +176,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);
+
return 0;
}
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'"));
+ 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);
+ return FOLLOW_REMOTE_UNCONFIGURED;
+}
+
static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -1918,11 +1925,10 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
struct fetch_head fetch_head = { 0 };
struct strbuf err = STRBUF_INIT;
- int do_set_head = 0;
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 = FOLLOW_REMOTE_NEVER;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1938,22 +1944,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)
- */
- if (transport->remote->follow_remote_head)
- follow_remote_head = transport->remote->follow_remote_head;
- else if (config->follow_remote_head)
- follow_remote_head = config->follow_remote_head;
- else
- follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -1962,8 +1952,13 @@ 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 (follow_remote_head != FOLLOW_REMOTE_NEVER)
- do_set_head = 1;
+
+ if (transport->remote->follow_remote_head)
+ follow_remote_head = transport->remote->follow_remote_head;
+ 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;
}
if (branch && branch_has_merge_config(branch) &&
!strcmp(branch->remote_name, transport->remote->name)) {
@@ -1987,7 +1982,7 @@ static int do_fetch(struct transport *transport,
strvec_push(&transport_ls_refs_options.ref_prefixes,
"refs/tags/");
- if (do_set_head)
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER)
strvec_push(&transport_ls_refs_options.ref_prefixes,
"HEAD");
@@ -2164,7 +2159,7 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
- if (do_set_head) {
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER) {
/*
* Way too many cases where this can go wrong so let's just
* ignore errors and fail silently for now.
@@ -2509,7 +2504,7 @@ 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,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
--
2.55.0.windows.3
Re: [PATCH v2] fetch.c: defer fetch.followRemoteHEAD validation
On Fri Sep 25, 2026 at 4:30 PM EDT, Colin Hinton wrote:
Show 14 quoted lines
> On Fri, Sep 25, 2026 at 12:40 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> We should do something similar to what remote.c parses for
>> consistency, but other than that, it seems this topic is moving in
>> the right direction.
>>
>> Thanks.
>
> The only critical difference I see in the configuration parse between
> remote.c and fetch.c is the case for "warn-if-not-$branch". From
> reading the git-config manpage, this is only a setting for a remote
> and not for fetch directly so I do not see a reason to check this in
> fetch.c. Perhaps I am missing something else to make this more
> consistent,
I agree with this assessment. However, I wonder if Junio meant
We should (do something similar) to (what remote.c parses) ...
instead of
We should do (something similar to what remote.c parses) ...
as the issue in the NEEDSWORK _does_ apply to both sides.
Perhaps at a minimum, this patch should leave the comment intact (or reworded) if not yet addressing remote.c. v3 otherwise is looking good to me, and functionality seems to work.
[PATCH v4] fetch.c: defer fetch.followRemoteHEAD validation
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 in do_fetch() and at the actual defect in handle_config(), so the remaining scope is easy to find for a follow-up patch.
Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
---
builtin/fetch.c | 68 ++++++++++++++++++++++++-------------------------
remote.c | 7 +++++
2 files changed, 41 insertions(+), 34 deletions(-)
Show changes to 2 files +41 −34
builtin/fetch.c, remote.c
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 533fdfe7d8..2cb0bcca8b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -103,7 +103,7 @@ 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 all;
int prune;
int prune_tags;
@@ -176,24 +176,33 @@ static int git_fetch_config(const char *k, const char *v,
}
if (!strcmp(k, "fetch.followremotehead")) {
+ free(fetch_config->follow_remote_head_raw);
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;
+ fetch_config->follow_remote_head_raw = xstrdup("");
else
- warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), v);
+ fetch_config->follow_remote_head_raw = xstrdup(v);
return 0;
}
return git_default_config(k, v, ctx, cb);
}
+static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
+{
+ if (!setting || !*setting)
+ die(_("missing value for 'fetch.followRemoteHEAD'"));
+ 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);
+ return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+}
+
static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -1918,11 +1927,10 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
struct fetch_head fetch_head = { 0 };
struct strbuf err = STRBUF_INIT;
- int do_set_head = 0;
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 = FOLLOW_REMOTE_NEVER;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1938,22 +1946,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)
- */
- if (transport->remote->follow_remote_head)
- follow_remote_head = transport->remote->follow_remote_head;
- else if (config->follow_remote_head)
- follow_remote_head = config->follow_remote_head;
- else
- follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -1962,8 +1954,16 @@ 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 (follow_remote_head != FOLLOW_REMOTE_NEVER)
- do_set_head = 1;
+ /*
+ * See remote.c's handling of remote.<name>.followRemoteHEAD
+ * for the analogous, still-unresolved case.
+ */
+ if (transport->remote->follow_remote_head)
+ follow_remote_head = transport->remote->follow_remote_head;
+ 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;
}
if (branch && branch_has_merge_config(branch) &&
!strcmp(branch->remote_name, transport->remote->name)) {
@@ -1987,7 +1987,7 @@ static int do_fetch(struct transport *transport,
strvec_push(&transport_ls_refs_options.ref_prefixes,
"refs/tags/");
- if (do_set_head)
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER)
strvec_push(&transport_ls_refs_options.ref_prefixes,
"HEAD");
@@ -2164,7 +2164,7 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
- if (do_set_head) {
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER) {
/*
* Way too many cases where this can go wrong so let's just
* ignore errors and fail silently for now.
@@ -2509,7 +2509,7 @@ 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,
.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"))
--
2.55.0.windows.3
Re: [PATCH v4] fetch.c: defer fetch.followRemoteHEAD validation
Colin Hinton <colinlewishinton@gmail.com> writes:
Show 6 quoted lines
> if (!strcmp(k, "fetch.followremotehead")) {
> + free(fetch_config->follow_remote_head_raw);
> if (!v)
> + fetch_config->follow_remote_head_raw = xstrdup("");
> else
> + fetch_config->follow_remote_head_raw = xstrdup(v);Hmph, this means that the code cannot distinguish between
[fetch] followremotehead
[fetch] followremotehead = ""
It would be less code and more expressive if you lost the conditional, i.e.,
if (!strcmp(k, "fetch.followremotehead"))
free(fetch_config->follow_remote_head_raw);
fetch_config->follow_remote_head_raw = xstrdup_or_null(v);
}
> +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> +{
> + if (!setting || !*setting)
> + die(_("missing value for 'fetch.followRemoteHEAD'"));Then you can differenciate
if (!setting)
... we got '[fetch] followRemoteHEAD' ...
die() as before, complaining that the this is not a Bool.
else if (!*setting)
... we got '[fetch] followRemoteHEAD = ""' ...
if we wanted to. It probably do not need to check for an empty string as it will fall through the "else if" cascade below and eventually end up with the warning + default.
Show 11 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);
> + return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
> +}Re: [PATCH v4] fetch.c: defer fetch.followRemoteHEAD validation
My rationale for this recent change came from when I was evaluating what calls get_follow_remote_head() in my patch.
With the current design, get_follow_remote_head() is only called in do_fetch() in this conditional else if (config->follow_remote_head_raw).
If followRemoteHEAD is now NULL because we set it as such in fetch_config->follow_remote_head_raw = xstrdup_or_null(v); Then this conditional is skipped, and we will never call the die(), and alert the user that their value is blank.
To fully fix based on your suggestion, I suppose the design question is, should empty string warn or die?
If empty string should warn, I likely will need to add some value in the fetch_config struct such as follow_remote_head_seen, and use this as our conditional in do_fetch() rather than the follow_remote_head_raw, to account for when followRemoteHEAD was set to anything. Then when the check in get_follow_remote_head() occurs, we know to die or warn based on NULL, or bogus.
Visually, it would look something like this.
Show changes to builtin/fetch.c +6 −7
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2cb0bcca8b..af22f63954 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -104,6 +104,7 @@ static struct string_list negotiation_include =
STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
char *follow_remote_head_raw;
+ int follow_remote_head_seen;
int all;
int prune;
int prune_tags;
@@ -177,10 +178,8 @@ static int git_fetch_config(const char *k, const char *v,
if (!strcmp(k, "fetch.followremotehead")) {
free(fetch_config->follow_remote_head_raw);
- if (!v)
- fetch_config->follow_remote_head_raw = xstrdup("");
- else
- fetch_config->follow_remote_head_raw = xstrdup(v);
+ fetch_config->follow_remote_head_raw = xstrdup_or_null(v);
+ follow_remote_head_seen = 1;
return 0;
}
@@ -189,7 +188,7 @@ static int git_fetch_config(const char *k, const char *v,
static enum follow_remote_head_settings get_follow_remote_head(const
char *setting)
{
- if (!setting || !*setting)
+ if (!setting) /*!*setting would return true on "" removing to
warn instead*/
die(_("missing value for 'fetch.followRemoteHEAD'"));
else if (!strcmp(setting, "never"))
return FOLLOW_REMOTE_NEVER;
@@ -1960,7 +1959,7 @@ static int do_fetch(struct transport *transport,
*/
if (transport->remote->follow_remote_head)
follow_remote_head =
transport->remote->follow_remote_head;
- else if (config->follow_remote_head_raw)
+ else if (config->follow_remote_head_seen)
follow_remote_head =
get_follow_remote_head(config->follow_remote_head_raw);
else
follow_remote_head =
BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
@@ -2510,6 +2509,7 @@ int cmd_fetch(int argc,
struct fetch_config config = {
.display_format = DISPLAY_FORMAT_FULL,
.follow_remote_head_raw = NULL,
+ .follow_remote_head_seen = 0,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
Let me know if this sounds right, and I will add this in for v5.
-Colin Hinton
On Sun, Oct 4, 2026 at 6:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Colin Hinton <colinlewishinton@gmail.com> writes:
>
> > if (!strcmp(k, "fetch.followremotehead")) {
> > + free(fetch_config->follow_remote_head_raw);
> > if (!v)
> > + fetch_config->follow_remote_head_raw = xstrdup("");
> > else
> > + fetch_config->follow_remote_head_raw = xstrdup(v);
>
> Hmph, this means that the code cannot distinguish between
>
> [fetch] followremotehead
>
> [fetch] followremotehead = ""
>
> It would be less code and more expressive if you lost the
> conditional, i.e.,
>
> if (!strcmp(k, "fetch.followremotehead"))
> free(fetch_config->follow_remote_head_raw);
> fetch_config->follow_remote_head_raw = xstrdup_or_null(v);
> }
>
> > +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> > +{
> > + if (!setting || !*setting)
> > + die(_("missing value for 'fetch.followRemoteHEAD'"));
>
> Then you can differenciate
>
> if (!setting)
> ... we got '[fetch] followRemoteHEAD' ...
> die() as before, complaining that the this is not a Bool.
> else if (!*setting)
> ... we got '[fetch] followRemoteHEAD = ""' ...
>
> if we wanted to. It probably do not need to check for an empty
> string as it will fall through the "else if" cascade below and
> eventually end up with the warning + default.
>
> > + 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);
> > + return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
> > +}
Re: [PATCH v4] fetch.c: defer fetch.followRemoteHEAD validation
Colin Hinton <colinlewishinton@gmail.com> writes:
Show 14 quoted lines
> My rationale for this recent change came from when I was evaluating
> what calls get_follow_remote_head() in my patch.
>
> With the current design, get_follow_remote_head() is only called in
> do_fetch() in this conditional else if
> (config->follow_remote_head_raw).
>
> If followRemoteHEAD is now NULL because we set it as such in
> fetch_config->follow_remote_head_raw = xstrdup_or_null(v); Then this
> conditional is skipped, and we will never call the die(), and alert
> the user that their value is blank.
>
> To fully fix based on your suggestion, I suppose the design question
> is, should empty string warn or die?
I think we should behave the same when we see "nvere". We do not understand what they wanted us to do in either case, so we should behave the same way, be it warn-and-ignore or complain-and-die.
Given that we now check the validity of the value only after we determine that we need it, I think it is OK to tighten the rules to die() instead of warn(). The historical behavior of not dying, and instead warning and ignoring, was a weak excuse for leaving configuration parsing broken and checking the validity of the value in the wrong place.
This patch rectifies the situation, which is a very good step toward doing the right thing. It is perfectly fine to tighten the rules as a separate topic after this patch lands and things stabilize, but this patch lays the groundwork for us to move in that direction.
Show 6 quoted lines
> If empty string should warn, I likely will need to add some value in
> the fetch_config struct such as follow_remote_head_seen, and use this
> as our conditional in do_fetch() rather than the
> follow_remote_head_raw, to account for when followRemoteHEAD was set
> to anything. Then when the check in get_follow_remote_head() occurs,
> we know to die or warn based on NULL, or bogus.
... because? Ah, because then you lose distinction between "the configuration variable not set at all" and "the configuration variable is set to the valueless true"?
If so, you'd need to be able to tell _three_ cases. The empty string you use as a stand in for "valueless true" should be distinguishable from the empty string the user set (by mistake).
Show 12 quoted lines
> Visually, it would look something like this.
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 2cb0bcca8b..af22f63954 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -104,6 +104,7 @@ static struct string_list negotiation_include =
> STRING_LIST_INIT_NODUP;
> struct fetch_config {
> enum display_format display_format;
> char *follow_remote_head_raw;
> + int follow_remote_head_seen;That would certainly work, and might be easier to work with than what I would have done, which is not to bother with this extra variable and instead to have a
static const char *valueless_true = "true";
in the file scope. Then use that ...
Show 12 quoted lines
> int all;
> int prune;
> int prune_tags;
> @@ -177,10 +178,8 @@ static int git_fetch_config(const char *k, const char *v,
>
> if (!strcmp(k, "fetch.followremotehead")) {
> free(fetch_config->follow_remote_head_raw);
> - if (!v)
> - fetch_config->follow_remote_head_raw = xstrdup("");
> - else
> - fetch_config->follow_remote_head_raw = xstrdup(v);
> + fetch_config->follow_remote_head_raw = xstrdup_or_null(v);... here like so:
if (fetch_config->follow_remote_head_raw != valueless_true)
free(fetch_config->follow_remote_head_raw);
if (!v)
fetch_config->follow_remote_head_raw = valueless_true;
else
...
Show 11 quoted lines
> @@ -189,7 +188,7 @@ static int git_fetch_config(const char *k, const char *v,
>
> static enum follow_remote_head_settings get_follow_remote_head(const
> char *setting)
> {
> - if (!setting || !*setting)
> + if (!setting) /*!*setting would return true on "" removing to
> warn instead*/
> die(_("missing value for 'fetch.followRemoteHEAD'"));
> else if (!strcmp(setting, "never"))
> return FOLLOW_REMOTE_NEVER;... and deal with the setting that is equal to valueless_true here.
I think either way would work, and the way you outlined would be better.
[PATCH v5] fetch.c: defer fetch.followRemoteHEAD validation
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 in do_fetch() and at the actual defect in handle_config(), so the remaining scope is easy to find for a follow-up patch.
Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
---
builtin/fetch.c | 72 ++++++++++++++++++++++++-------------------------
remote.c | 7 +++++
2 files changed, 43 insertions(+), 36 deletions(-)
Show changes to 2 files +43 −36
builtin/fetch.c, remote.c
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 533fdfe7d8..e0b4394fea 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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;
}
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'"));
+ 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);
+ return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+}
+
static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -1918,11 +1926,10 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
struct fetch_head fetch_head = { 0 };
struct strbuf err = STRBUF_INIT;
- int do_set_head = 0;
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 = FOLLOW_REMOTE_NEVER;
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)
- */
- if (transport->remote->follow_remote_head)
- follow_remote_head = transport->remote->follow_remote_head;
- else if (config->follow_remote_head)
- follow_remote_head = config->follow_remote_head;
- else
- follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -1962,8 +1953,16 @@ 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 (follow_remote_head != FOLLOW_REMOTE_NEVER)
- do_set_head = 1;
+ /*
+ * See remote.c's handling of remote.<name>.followRemoteHEAD
+ * for the analogous, still-unresolved case.
+ */
+ if (transport->remote->follow_remote_head)
+ follow_remote_head = transport->remote->follow_remote_head;
+ else if (config->follow_remote_head_seen)
+ follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
+ else
+ follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
}
if (branch && branch_has_merge_config(branch) &&
!strcmp(branch->remote_name, transport->remote->name)) {
@@ -1987,7 +1986,7 @@ static int do_fetch(struct transport *transport,
strvec_push(&transport_ls_refs_options.ref_prefixes,
"refs/tags/");
- if (do_set_head)
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER)
strvec_push(&transport_ls_refs_options.ref_prefixes,
"HEAD");
@@ -2164,7 +2163,7 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
- if (do_set_head) {
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER) {
/*
* Way too many cases where this can go wrong so let's just
* ignore errors and fail silently for now.
@@ -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,
.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"))
--
2.55.0.windows.3
Re: [PATCH v5] fetch.c: defer fetch.followRemoteHEAD validation
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 valueBecause 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"))Re: [PATCH v5] fetch.c: defer fetch.followRemoteHEAD validation
Colin Hinton <colinlewishinton@gmail.com> writes:
I see there are only two minor things remaining in this iteration.
> fetch. Leave NEEDSWORK comments at both the now unresolved call site
> in do_fetch() and at the actual defect in handle_config(), so the
> remaining scope is easy to find for a follow-up patch.
Here is one of the two. There is only one NEEDSWORK, not "at both".
Leave a NEEDSWORK comment at remote.c:handle_config() that
has a defect similar to what is fixed by this patch, so ...
should be sufficient.
Another is that
int cmd_fetch(int argc,
const char **argv,
const char *prefix,
struct repository *repo UNUSED)
{
struct fetch_config config = {
.display_format = DISPLAY_FORMAT_FULL,
.follow_remote_head_raw = NULL,
.follow_remote_head_seen = 0,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
.parallel = 1,
.submodule_fetch_jobs = -1,
};will hold onto a copy of config.follow_remote_head_seen that was read from the configuration and never frees it, so when cmd_fetch() leaves, it technically leaks a string.
Other than these two points, this round looks very good.
Thanks.
[PATCH v6] fetch.c: defer fetch.followRemoteHEAD validation
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 a NEEDSWORK comment at remote.c:handle_config() that has a defect similar to what is fixed by this patch, so the remaining scope is easy to find for a follow-up patch.
Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
---
builtin/fetch.c | 73 +++++++++++++++++++++++++------------------------
remote.c | 7 +++++
2 files changed, 44 insertions(+), 36 deletions(-)
Show changes to 2 files +44 −36
builtin/fetch.c, remote.c
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 533fdfe7d8..d800978c38 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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;
}
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'"));
+ 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);
+ return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+}
+
static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -1918,11 +1926,10 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
struct fetch_head fetch_head = { 0 };
struct strbuf err = STRBUF_INIT;
- int do_set_head = 0;
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 = FOLLOW_REMOTE_NEVER;
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)
- */
- if (transport->remote->follow_remote_head)
- follow_remote_head = transport->remote->follow_remote_head;
- else if (config->follow_remote_head)
- follow_remote_head = config->follow_remote_head;
- else
- follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -1962,8 +1953,16 @@ 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 (follow_remote_head != FOLLOW_REMOTE_NEVER)
- do_set_head = 1;
+ /*
+ * See remote.c's handling of remote.<name>.followRemoteHEAD
+ * for the analogous, still-unresolved case.
+ */
+ if (transport->remote->follow_remote_head)
+ follow_remote_head = transport->remote->follow_remote_head;
+ else if (config->follow_remote_head_seen)
+ follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
+ else
+ follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
}
if (branch && branch_has_merge_config(branch) &&
!strcmp(branch->remote_name, transport->remote->name)) {
@@ -1987,7 +1986,7 @@ static int do_fetch(struct transport *transport,
strvec_push(&transport_ls_refs_options.ref_prefixes,
"refs/tags/");
- if (do_set_head)
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER)
strvec_push(&transport_ls_refs_options.ref_prefixes,
"HEAD");
@@ -2164,7 +2163,7 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
- if (do_set_head) {
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER) {
/*
* Way too many cases where this can go wrong so let's just
* ignore errors and fail silently for now.
@@ -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,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
@@ -2929,5 +2929,6 @@ int cmd_fetch(int argc,
cleanup:
string_list_clear(&list, 0);
list_objects_filter_release(&filter_options);
+ free(config.follow_remote_head_raw);
return result;
}
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"))
--
2.55.0.windows.3