[0/2] limiting followRemoteHEAD being used

Message

Jeff King March 18, 2025, 5:39 a.m. UTC

On Mon, Mar 17, 2025 at 12:01:21PM -0700, Junio C Hamano wrote:

> > So I could see going in two different directions:
> >
> >   1. Only do the HEAD update when we are using the configured refspecs.
> >
> >   2. Do the HEAD update even when we are not fetching its destination,
> >      but do not otherwise trigger ls-refs to discover it (so basically,
> >      not on object-only fetches). This kicks in for more cases, but is
> >      hard to explain.
> >
> > Both are a user-visible divergence from how the feature behaves now (and
> > so I did not want to touch that in my series), but if we are all in
> > agreement, we can fix it on top. I do think option 1 (i.e., what you are
> > suggesting in your email) is how I would have done it if starting from
> > scratch. And the current rules are weird enough and the feature is new
> > enough that I think it is OK to change.
> 
> Yup, I do think #1 is the way to go.

It turned out to be pretty easy to do; the code ends up even shorter.
These patches apply on top of jk/fetch-ref-prefix-cleanup.

There is one interesting case we haven't discussed. What should:

  git fetch origin HEAD

do with respect to refs/remotes/origin/HEAD? Right now it does nothing
(at least with the v2 protocol). We ask about HEAD, but since we didn't
fetch the matching ref, set_head() won't accept it. And after my
patches, we would not even try to call set_head() at all (since we are
not using the full refspecs).

But it's also something I could see somebody doing to try to update
refs/remotes/origin/HEAD. I've left it alone for now, since my series
does not change the behavior either way. But it might be something we
could do on top (though it gets funny, because with the code as it is
now, we'd have to ask for all of refs/heads/ to see the pointed-to
branch advertised).

Anyway, here's what I think we should do now. The first one is the
change we discussed, and the second is a related optimization I noticed.

  [1/2]: fetch: only respect followRemoteHEAD with configured refspecs
  [2/2]: fetch: don't ask for remote HEAD if followRemoteHEAD is "never"

 Documentation/config/remote.adoc |  3 ++-
 builtin/fetch.c                  | 29 +++++++----------------------
 t/t5505-remote.sh                |  2 +-
 t/t5510-fetch.sh                 | 19 ++++++++++++++++++-
 4 files changed, 28 insertions(+), 25 deletions(-)

Comments

Junio C Hamano March 18, 2025, 7:18 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> There is one interesting case we haven't discussed. What should:
>
>   git fetch origin HEAD
>
> do with respect to refs/remotes/origin/HEAD? Right now it does nothing
> (at least with the v2 protocol). We ask about HEAD, but since we didn't
> fetch the matching ref, set_head() won't accept it. And after my
> patches, we would not even try to call set_head() at all (since we are
> not using the full refspecs).
>
> But it's also something I could see somebody doing to try to update
> refs/remotes/origin/HEAD. I've left it alone for now, since my series
> does not change the behavior either way. But it might be something we
> could do on top (though it gets funny, because with the code as it is
> now, we'd have to ask for all of refs/heads/ to see the pointed-to
> branch advertised).

With v2 protocol, we have direct knowledge of where their HEAD
points at (when we ask for it), so we shouldn't even have to know
about what they have under "refs/heads/".  I do not think it is
within our contract that we'd somehow make sure that HEAD in a
remote-tracking hierarchy is not dangling, or something.

In any case, I agree that it is sensible to leave it out of these
changes for now.

Thanks.

Taylor Blau March 18, 2025, 11:02 p.m. UTC | #2

On Tue, Mar 18, 2025 at 12:18:41PM -0700, Junio C Hamano wrote:
> Jeff King <[e-mail]> writes:
>
> > There is one interesting case we haven't discussed. What should:
> >
> >   git fetch origin HEAD
> >
> > do with respect to refs/remotes/origin/HEAD? Right now it does nothing
> > (at least with the v2 protocol). We ask about HEAD, but since we didn't
> > fetch the matching ref, set_head() won't accept it. And after my
> > patches, we would not even try to call set_head() at all (since we are
> > not using the full refspecs).
> >
> > But it's also something I could see somebody doing to try to update
> > refs/remotes/origin/HEAD. I've left it alone for now, since my series
> > does not change the behavior either way. But it might be something we
> > could do on top (though it gets funny, because with the code as it is
> > now, we'd have to ask for all of refs/heads/ to see the pointed-to
> > branch advertised).
>
> With v2 protocol, we have direct knowledge of where their HEAD
> points at (when we ask for it), so we shouldn't even have to know
> about what they have under "refs/heads/".  I do not think it is
> within our contract that we'd somehow make sure that HEAD in a
> remote-tracking hierarchy is not dangling, or something.
>
> In any case, I agree that it is sensible to leave it out of these
> changes for now.

Ditto, and I am quite happy with the end-state of this series.

> Thanks.

Thanks,
Taylor