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

So in subsurface, we had trouble with a very annoying bug introduced in libgit2-1.2:

  https://github.com/libgit2/libgit2/issues/6368

which made "git_clone()" fail horribly if the remote repository doesn't have a default branch.

Subsurface uses git for the cloud storage back-end, and the cloud repositories are just bare repositories with a single branch, and they have no HEAD at all (ok, technically in the bare repo it points to the non-existent default branch, but as far as the client is concerned that's the same thing, because it won't show up in the remote listing).

That's all perfectly valid git behavior, and the real git client has no issues with this at all. It's a libgit2 bug, plain and simple.

I think the fix for libgit2 is probably a oneliner:

    https://github.com/libgit2/libgit2/pull/6369

but that doesn't really help subsurface, because the buggy version of libgit2 has already spread enough that it just ends up being a fact of life.

So what the subsurface cloud side will end up doing is to just force that pointless HEAD thing, to work around the bug. And I told Dirk to use

   git symbolic-ref HEAD refs/heads/<branch-name>

to do it, because it was "safer" than doing it by hand with a mindless

   echo "ref: refs/heads/<branch-name>" > HEAD

Which brings me to this email.

After I told Dirk that that was the "proper" way to do it, I actually tried it out.

And "git symbolic-ref" is perfectly happy to take complete garbage input. There seems to be no advantage over using that silly "echo" model.

You can do things like

    git symbolic-ref HEAD refs/heads/not..ok

and after that all the git commands that want to use HEAD will die with a fatal error

    fatal: your current branch appears to be broken

which kind of makes it pointless to try to use the git plumbing for this. The *only* verification that "git symbolic-ref" does is basically

                    starts_with(argv[1], "refs/")

and even that minimal test is only done for HEAD.

Does anybody care? Probably not. But it does seem to be a bit sloppy. We do have that 'check_refname_format()' function to actually check that it's a valid refname,.

Maybe create_symref() could do this, but if we do it in builtin/symbolic-ref.c we could give better error messages, perhaps?

Not a big deal, but I thought I'd at least send out this silly patch for comments, since I looked at this.

                   Linus

 builtin/symbolic-ref.c | 2 ++
 1 file changed, 2 insertions(+)

On Sat, Jul 30, 2022 at 12:53 PM Linus Torvalds <torvalds@linux-foundation.org> wrote:

Side note: it looks like that patch may break a test.

And that test most definitely *should* be broken.

t3200-branch.sh does

      git symbolic-ref refs/heads/dangling-symref nowhere

which really depends on that whole "git symbolic-ref does no sanity checking at all".

In fact, it seems to depend particularly on the fact that for non-HEAD refs, it does even *less* sanity checking, and doesn't even check that the ref starts with a "refs/"

There is also t4202-log, which wants to test that "git log" reacts well to a bad ref. But now that "git symbolic-ref" refuses to create such a bad ref, that test fails.

Anyway, here's a slightly updated patch that just fixes that test that depended on not just a dangling symref, but an *invalid* dangling symref. And it changes t4202-log to use "echo" to create the bad ref instead. Which is what the previous test did too, to create the bogus hash.

Again - this is such a low-level plumbing thing that maybe nobody cares, but it just struck me as a bad idea to have these kinds of maintenance commands that can be used to just mess up your repository. And if you have a bare repo, this really does look like the command that *should* be used to change HEAD, since it's not about "git checkout"

                  Linus

 builtin/symbolic-ref.c | 2 ++
 t/t3200-branch.sh      | 4 ++--
 t/t4202-log.sh         | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

Linus Torvalds <torvalds@linux-foundation.org> writes:

Yeah, once in the past, I thought

    git symbolic-ref refs/heads/main master

might be a way to adjust to the new world order without having me to change my workflow. I can always update master, and main follows it without me being aware of it even being there.

But it did not work.  Even worse, after doing so, running

    git update-ref refs/heads/main master

created ".git/master", which was a disaster.  Of course

    git symbolic-ref refs/heads/main refs/heads/master

would have worked.

I am not sure what workflow the "nowhere" thing is supposed to help. Of course, with s/nowhere/HEAD/, it is a perfectly sane repository immediately after "git init -b nowhere", so whatever tightening we do, we should make sure that

    git symbolic-ref refs/heads/main HEAD

keeps working.

On Sat, Jul 30, 2022 at 01:21:50PM -0700, Linus Torvalds wrote:

I think it is probably worth addressing. I'm sure it has bitten me before[0], and the HEAD logic from afe5d3d516 (symbolic ref: refuse non-ref targets in HEAD, 2009-01-29) was a cowardly attempt to fix the most egregious cases without breaking anything.

This forbids syntactically-invalid refnames, which is good.

Since you don't pass REFNAME_ALLOW_ONELEVEL, it also forbids nonsense names like "nowhere". But that also breaks some probably-stupid cases that are currently possible, like:

  git symbolic-ref refs/heads/foo FETCH_HEAD

I'm not really sure why anybody would want to do that, but it does work currently. 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.

This is a sensible change. With ALLOW_ONELEVEL, it wouldn't be necessary, but I think it is representing a more plausible real-world scenario.

After this change, I think this would need to be marked with a REFFILES prereq similar to the test right before it.

-Peff

[0] Curiously there was a very similar patch to yours posted a while
    ago:

      https://lore.kernel.org/git/4FDE3D7D.4090502@elegosoft.com/

    There was some discussion and a followup:

      https://lore.kernel.org/git/1342440781-18816-1-git-send-email-mschub@elegosoft.com/

    but nothing seems to have been applied. I don't see any arguments
    against it there; I think the author simply didn't push it forward
    enough.

    There's also some bits in the sub-thread about limiting HEAD to
    "refs/heads/", which we couldn't quite do at the time. That might be
    worth revisiting, but definitely shouldn't hold up your patch.

On Sat, Jul 30, 2022 at 08:18:48PM -0400, Jeff King wrote:

Hmph, maybe not. The sticking point was topgit, which points HEAD at refs/top-bases. There's a fork here:

  https://github.com/mackyle/topgit

which has been active in the last 12 months and which still uses that convention. So maybe people really are still using it.

(Again, neither here nor there for your patch).

-Peff

On Sat, Jul 30, 2022 at 5:24 PM Jeff King <peff@peff.net> wrote:

Well, it *is* relevant for my patch in the sense that I clearly didn't think of all the crazy things people might have been doing.

That

     git symbolic-ref refs/heads/foo FETCH_HEAD

that you mentioned in the other mail would obviously be entirely disallowed by my patch, and again, I didn't for a second imagine that somebody would do something that strange. Junio mentioned a similarly odd possible situation.

So while I think my patch is the right thing to do, I will also admit that it's perhaps a "we should always have done this, but we didn't" situation, and maybe those really odd cases need to be allowed.

Adding ALLOW_ONELEVEL would make those things presumably still work, and would at least improve things *somewhat* - it would protect people from syntactically invalid branches (ie bad characters in the branch name etc).

That would imply still having to fix up that t4202-log.sh testcase, and I didn't even know or realize about that REFFILES prerequisite, since obviously in all my use it has been true. I still use and test only on Linux..

You are also right that without the ALLOW_ONELEVEL, the special-case check for HEAD should just be removed. That patch started out as the minimal possible "let's just disallow invalid ref names" patch, so I didn't touch that odd special case code.

Put another way: I think my patch is likely the right thing to do (and I'd personally prefer the stricter check without the ALLOW_ONELEVEL flag), but you and Junio are right about it being a bigger change than I in my naivete thought it was.

So I won't really push for this, I suspect this needs very much to be a judgement call by you guys.

Thanks,

                   Linus

On Sat, Jul 30, 2022 at 05:44:25PM -0700, Linus Torvalds wrote:

Just to lay out the options, I think we have:

  1. Do nothing. This breaks nothing. ;)

  2. Your patch, but with ALLOW_ONELEVEL. This fixes nonsense like
     "foo..bar", but doesn't break "FETCH_HEAD". Requires fixing t4202's
     ".lock" example. Replaces the HEAD starts_with("refs/") check.

  3. Your patch as-is. Same as (2), but also breaks FETCH_HEAD.

  4. Your patch, plus any extra tightening of HEAD to refs/heads/. I
     think this is probably breaking too much (I put more details
     elsewhere in the thread).

I'd be in favor of (2), which is really just catching syntactically invalid crap, and shouldn't break anyone. Technically it's possible somebody could be using a symref pointing at arbitrary data for who-knows-what reason, and extracting it with "symbolic-ref", but that is getting beyond far-fetched, I think.

I'm also tempted by (3), but we should be prepared for obscure breakage reports.

-Peff

On Mon, Aug 01, 2022 at 01:43:06PM -0400, Jeff King wrote:

Er, sorry, the starts_with() thing is the other way around. It is redundant with your patch, but necessary to keep it for (2). Luckily t1401 catches the problem if you dare to try it. ;)

-Peff

On Mon, Aug 01, 2022 at 01:43:06PM -0400, Jeff King wrote:

Just to keep things moving forward, here it is with a commit message. I left you as the author, but if you're OK with it, please tell Junio he can forge your sign-off.

-- >8 --
From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: [PATCH] symbolic-ref: refuse to set syntactically invalid target

You can feed absolute garbage to symbolic-ref as a target like:

  git symbolic-ref HEAD refs/heads/foo..bar

While this doesn't technically break the repo entirely (our "is it a git directory" detector looks only for "refs/" at the start), we would never resolve such a ref, as the ".." is invalid within a refname.

Let's flag these as invalid at creation time to help the caller realize that what they're asking for is bogus.

A few notes:

  - We use REFNAME_ALLOW_ONELEVEL here, which lets:

     git update-ref refs/heads/foo FETCH_HEAD

    continue to work. It's unclear whether anybody wants to do something
    so odd, but it does work now, so this is erring on the conservative
    side. There's a test to make sure we didn't accidentally break this,
    but don't take that test as an endorsement that it's a good idea, or
    something we might not change in the future.

  - The test in t4202-log.sh checks how we handle such an invalid ref on
    the reading side, so it has to be updated to touch the HEAD file
    directly.

  - We need to keep our HEAD-specific check for "does it start with
    refs/". The ALLOW_ONELEVEL flag means we won't be enforcing that for
    other refs, but HEAD is special here because of the checks in
    validate_headref().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/symbolic-ref.c  |  2 ++
 t/t1401-symbolic-ref.sh | 10 ++++++++++
 t/t4202-log.sh          |  4 ++--
 3 files changed, 14 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

OK. Even if it were HEAD, it does look like a funny thing to do to point at a shallower ref with a more concrete ref.

OK.

Makes sense. Rejecting syntactically invalid thing like double-dot is something we should have done from day one.

Good.

Strange, but OK.

OK.

On Mon, Aug 01, 2022 at 11:54:36AM -0700, Junio C Hamano wrote:

I'd be OK to drop this if you hate it too much, btw. Mostly I wanted to make sure that the various iterations behaved as I expected. But there is a test in t3200 (the one Linus found earlier) that incidentally does check that something like this works.

-Peff

Jeff King <peff@peff.net> writes:

Oh, no, I do not hate it (or like it) at all.

The "strange" was mostly referring to the order of the symbolic thing that refers to another thing that is being pointed at, which looked backwards, i.e. "git symbolic-ref HEAD refs/heads/main" is what we usually expect (i.e. "we use this short name HEAD to refer to the longer refs/heads/main ref"), but after staring the one in the test "git symbolic-ref refs/heads/top-level FETCH_HEAD" too long, your eyes trick your brain into thinking we use the short name FETCH_HEAD to refer to the top-level branch, which is the other way around.

We've been allowing the one-level thing and I think the discussion has established that we need to keep it supported. There is nothing to hate or like about it X-<.

Thanks.

On Mon, Aug 1, 2022 at 11:15 AM Jeff King <peff@peff.net> wrote:

I'm certainly ok with that.

That said, I'm also ok with not getting the authorship credit for this at all. Not because you wrote the commit log and updated the patch with REFNAME_ALLOW_ONELEVEL and added a test-case, but because clearly Michael Schubert already did basically the core of that patch ten years ago.

So Junio, feel free to add my

  Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

but feel equally free to give credit to Jeff or - belatedly - Michael.

                 Linus

Jeff King <peff@peff.net> writes:

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.

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

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

)(

On Mon, Aug 1, 2022 at 10:36 AM Jeff King <peff@peff.net> wrote:

But it seems that at least this issue won't affect check_refname_format().

Hmm. Looking at that again - even without ALLOW_ONELEVEL I don't actually think check_refname_format() requires "refs/" per se. So the HEAD check isn't actually made redundant.

I wonder what the intended semantic meaning of ALLOW_ONELEVEL really is supposed to be. It seems to really only require *one* slash - but it doesn't really end up checking that it's in the "refs/" hierarchy, it can be anywhere.

I guess the only thing it disallows is literally HEAD and MERGE_HEAD and the like. But it's a bit odd, because it would seem to possibly allow you to use "refs" that point to the objects/ directory or similar.

Maybe the refs/ protection comes in somewhere later, I didn't really go around to check.

                Linus

On Mon, Aug 01, 2022 at 10:49:26AM -0700, Linus Torvalds wrote:

I'm actually not that surprised. I think the history of that flag is...weird. I think once upon a time, there was "one-level" checking which was meant to disallow "refs/foo" versus "refs/heads/foo". But there were also spots that wanted to make sure we were in refs/, and not touching MERGE_HEAD, etc.

And because of the generic-ness of the flag name, those two cases got conflated. I think it's mostly been sorted out over the years, but I won't be surprised if there are weird corner cases.

I didn't check where, but I did confirm that the "symbolic-ref HEAD foo" case in t1401 continues to pass even if we remove the special HEAD code. So _something_ is doing it. ;)

-Peff