From: Colin Hinton <colinlewishinton@gmail.com>
Date: Fri, 25 Sep 2026 18:48:38 GMT
Subject: Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
Message-ID: <CAHeTm9M9c7D71QUA-Dy9o_YD2PGbNYRa9LnNWKrYSaHEoUtpSQ@mail.gmail.com>
In-Reply-To: <DLNDRU6GIB30.1F5G8Z3JIR67W@lfurio.us>

On Thu, Sep 24, 2026 at 12:50 AM Matt Hunter <m@lfurio.us> wrote:
>
> 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.

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