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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jul 30, 2022, 19:53 UTC
Message-ID: <CAHk-=wh9f0EmsNFgoxUa8BzVej06+7MbLr-MLBjDjtj_=Pf90A@mail.gmail.com>

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(+)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index e547a08d6c..5354cfb4f1 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		if (!strcmp(argv[0], "HEAD") &&
 		    !starts_with(argv[1], "refs/"))
 			die("Refusing to point HEAD outside of refs/");
+		if (check_refname_format(argv[1], 0) < 0)
+			die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]);
 		ret = !!create_symref(argv[0], argv[1], msg);
 		break;
 	default: