Re: "git symbolic-ref" doesn't do a very good job

From: Jeff King <peff@peff.net>
Date: Aug 1, 2022, 17:36 UTC
Message-ID: <YugPER9UsH1z6MZo@coredump.intra.peff.net>
In-Reply-To: <xmqqzggpyu7q.fsf@gitster.g>

On Sun, Jul 31, 2022 at 12:10:01PM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I'm tempted to say that the symref-reading code should
> > actually complain about following something outside of "refs/", but that
> > carries an even higher possibility of breaking somebody. But it seems
> > like we should be consistent between what we allow to be read, and what
> > we allow to be written.
> >
> > At any rate, with the code as you have it above, I think the "make sure
> > HEAD starts with refs/" code is now redundant.
> 
> Isn't the rule these days "HEAD must be either detached or point
> into refs/heads/"?  I thought "checkout" ensures that, and I am
> tempted to think that "symbolic-ref" that works on HEAD should be
> consistent with "checkout".  So "make sure HEAD is within refs/"
> would certainly be "not wrong per-se" but not sufficiently tight,
> I suspect.

No, sadly, that isn't the rule. See afe5d3d516 (symbolic ref: refuse non-ref targets in HEAD, 2009-01-29) which tightened it to "refs/heads" and then e9cc02f0e4 (symbolic-ref: allow refs/<whatever> in HEAD, 2009-02-13) which had to loosen it.

Likewise we seemed to touch the reading side at the same time, via b229d18a80 (validate_headref: tighten ref-matching to just branches, 2009-01-29), and then 222b167386 (Revert "validate_headref: tighten ref-matching to just branches", 2009-02-12).

In both cases it was refs/top-bases that triggered the revert. The relevant thread is:

  https://lore.kernel.org/git/cc723f590902120009w432f5f61xd6550409835cdbb7@mail.gmail.com/

There's some discussion there about how topgit could do things differently, but I don't see any plan for moving away. However, the latest changelog for it I could find has:

  [from https://mackyle.github.io/topgit/changelog.html]
  TopGit 0.19.4 (2017-02-14) introduced support for a new top-bases
  location under heads. This new location will become the default as of
  the TopGit 0.20.0 release. The current location under refs will
  continue to be supported in the future. See tg help migrate-bases for
  more details.

So it looks like there is some will there to switch. But the default hasn't flipped yet, and we'd be breaking any non-migrated installs. Not to mention that we don't know if any _other_ tools care. The topgit folks reported the original problem in 2009 within a few weeks, and it never made it to a release.

So yeah. We could certainly work out a deprecation/migration plan, but I'm not sure it's worth the effort. I do suspect there are other parts of Git that assume HEAD points at refs/heads/, especially the clone/fetch HEAD-selection code. But that code is getting the symref target from an untrusted remote, and is already careful about discarding nonsense and continuing.

Regarding "checkout" versus "symbolic-ref", I do think "checkout" probably does limit us to refs/heads/. But it is OK for porcelain to be opinionated and restrictive, but plumbing probably needs to support existing possibly-silly use cases.

-Peff