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

* "git symbolic-ref" doesn't do a very good job
@ 2022-07-30 19:53 Linus Torvalds
  2022-07-30 20:21 ` Linus Torvalds
  0 siblings, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2022-07-30 19:53 UTC (permalink / raw)
  To: Junio Hamano C; +Cc: Git List Mailing

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 620 bytes --]

 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:

^ permalink raw reply related	[flat|nested] 17+ messages in thread