Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation
- From
Colin Hinton <colinlewishinton@gmail.com>
- Date
- Sep 25, 2026, 18:48 UTC
- 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:
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