Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
- From
Matt Hunter <m@lfurio.us>
- Date
- Sep 24, 2026, 07:50 UTC
- Message-ID
- <DLNDRT2L2DYK.3LCFVZ6URY4G6@lfurio.us>
- In-Reply-To
- <xmqqwlsdhmvk.fsf@gitster.g>
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/