{"thread":{"id":"58247","subject":"\"git symbolic-ref\" doesn't do a very good job","startedAt":"2022-07-30T19:53:34Z","lastAt":"2022-08-02T00:57:41Z","messageCount":17,"participants":["Linus Torvalds","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"460310","messageId":"CAHk-=wh9f0EmsNFgoxUa8BzVej06+7MbLr-MLBjDjtj_=Pf90A@mail.gmail.com","threadId":"58247","inReplyTo":null,"subject":"\"git symbolic-ref\" doesn't do a very good job","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-07-30T19:53:09Z","receivedAt":"2022-07-30T19:53:34Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"So in subsurface, we had trouble with a very annoying bug introduced\nin libgit2-1.2:\n\n https://github.com/libgit2/libgit2/issues/6368\n\nwhich made \"git_clone()\" fail horribly if the remote repository\ndoesn't have a default branch.\n\nSubsurface uses git for the cloud storage back-end, and the cloud\nrepositories are just bare repositories with a single branch, and they\nhave no HEAD at all (ok, technically in the bare repo it points to the\nnon-existent default branch, but as far as the client is concerned\nthat's the same thing, because it won't show up in the remote\nlisting).\n\nThat's all perfectly valid git behavior, and the real git client has\nno issues with this at all. It's a libgit2 bug, plain and simple.\n\nI think the fix for libgit2 is probably a oneliner:\n\n https://github.com/libgit2/libgit2/pull/6369\n\nbut that doesn't really help subsurface, because the buggy version of\nlibgit2 has already spread enough that it just ends up being a fact of\nlife.\n\nSo what the subsurface cloud side will end up doing is to just force\nthat pointless HEAD thing, to work around the bug. And I told Dirk to\nuse\n\n git symbolic-ref HEAD refs/heads/\n\nto do it, because it was \"safer\" than doing it by hand with a mindless\n\n echo \"ref: refs/heads/\" > HEAD\n\nWhich brings me to this email.\n\nAfter I told Dirk that that was the \"proper\" way to do it, I actually\ntried it out.\n\nAnd \"git symbolic-ref\" is perfectly happy to take complete garbage\ninput. There seems to be no advantage over using that silly \"echo\"\nmodel.\n\nYou can do things like\n\n git symbolic-ref HEAD refs/heads/not..ok\n\nand after that all the git commands that want to use HEAD will die\nwith a fatal error\n\n fatal: your current branch appears to be broken\n\nwhich kind of makes it pointless to try to use the git plumbing for\nthis. The *only* verification that \"git symbolic-ref\" does is\nbasically\n\n starts_with(argv[1], \"refs/\")\n\nand even that minimal test is only done for HEAD.\n\nDoes anybody care? Probably not. But it does seem to be a bit sloppy.\nWe do have that 'check_refname_format()' function to actually check\nthat it's a valid refname,.\n\nMaybe create_symref() could do this, but if we do it in\nbuiltin/symbolic-ref.c we could give better error messages, perhaps?\n\nNot a big deal, but I thought I'd at least send out this silly patch\nfor comments, since I looked at this.\n\n Linus\n\n\n builtin/symbolic-ref.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex e547a08d6c..5354cfb4f1 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n \t\t !starts_with(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n+\t\tif (check_refname_format(argv[1], 0) < 0)\n+\t\t\tdie(\"Refusing to set '%s' to invalid ref '%s'\", argv[0], argv[1]);\n \t\tret = !!create_symref(argv[0], argv[1], msg);\n \t\tbreak;\n \tdefault:\n"},{"id":"460313","messageId":"CAHk-=wg9LaHeg0UmZ90gLOaBpO-5fhoaH22iNNm=1eror95pFg@mail.gmail.com","threadId":"58247","inReplyTo":"CAHk-=wh9f0EmsNFgoxUa8BzVej06+7MbLr-MLBjDjtj_=Pf90A@mail.gmail.com","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-07-30T20:21:50Z","receivedAt":"2022-07-30T20:22:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sat, Jul 30, 2022 at 12:53 PM Linus Torvalds\n wrote:\n>\n> And \"git symbolic-ref\" is perfectly happy to take complete garbage\n> input. There seems to be no advantage over using that silly \"echo\"\n> model.\n\nSide note: it looks like that patch may break a test.\n\nAnd that test most definitely *should* be broken.\n\nt3200-branch.sh does\n\n git symbolic-ref refs/heads/dangling-symref nowhere\n\nwhich really depends on that whole \"git symbolic-ref does no sanity\nchecking at all\".\n\nIn fact, it seems to depend particularly on the fact that for non-HEAD\nrefs, it does even *less* sanity checking, and doesn't even check that\nthe ref starts with a \"refs/\"\n\nThere is also t4202-log, which wants to test that \"git log\" reacts\nwell to a bad ref. But now that \"git symbolic-ref\" refuses to create\nsuch a bad ref, that test fails.\n\nAnyway, here's a slightly updated patch that just fixes that test that\ndepended on not just a dangling symref, but an *invalid* dangling\nsymref. And it changes t4202-log to use \"echo\" to create the bad ref\ninstead. Which is what the previous test did too, to create the bogus\nhash.\n\nAgain - this is such a low-level plumbing thing that maybe nobody\ncares, but it just struck me as a bad idea to have these kinds of\nmaintenance commands that can be used to just mess up your repository.\nAnd if you have a bare repo, this really does look like the command\nthat *should* be used to change HEAD, since it's not about \"git\ncheckout\"\n\n Linus\n\n\n builtin/symbolic-ref.c | 2 ++\n t/t3200-branch.sh | 4 ++--\n t/t4202-log.sh | 2 +-\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex e547a08d6c..5354cfb4f1 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n \t\t !starts_with(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n+\t\tif (check_refname_format(argv[1], 0) < 0)\n+\t\t\tdie(\"Refusing to set '%s' to invalid ref '%s'\", argv[0], argv[1]);\n \t\tret = !!create_symref(argv[0], argv[1], msg);\n \t\tbreak;\n \tdefault:\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 9723c2827c..b194c1b09b 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -723,9 +723,9 @@ test_expect_success 'deleting a symref' '\n '\n \n test_expect_success 'deleting a dangling symref' '\n-\tgit symbolic-ref refs/heads/dangling-symref nowhere &&\n+\tgit symbolic-ref refs/heads/dangling-symref refs/heads/nowhere &&\n \ttest_path_is_file .git/refs/heads/dangling-symref &&\n-\techo \"Deleted branch dangling-symref (was nowhere).\" >expect &&\n+\techo \"Deleted branch dangling-symref (was refs/heads/nowhere).\" >expect &&\n \tgit branch -d dangling-symref >actual &&\n \ttest_path_is_missing .git/refs/heads/dangling-symref &&\n \ttest_cmp expect actual\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 6e66352558..427b06442d 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -2114,7 +2114,7 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '\n \n test_expect_success 'log diagnoses bogus HEAD symref' '\n \tgit init empty &&\n-\tgit --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&\n+\techo \"ref: refs/heads/invalid.lock\" > empty/.git/HEAD &&\n \ttest_must_fail git -C empty log 2>stderr &&\n \ttest_i18ngrep broken stderr &&\n \ttest_must_fail git -C empty log --default totally-bogus 2>stderr &&\n"},{"id":"460314","messageId":"xmqqtu6y1gky.fsf@gitster.g","threadId":"58247","inReplyTo":"CAHk-=wg9LaHeg0UmZ90gLOaBpO-5fhoaH22iNNm=1eror95pFg@mail.gmail.com","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-30T20:38:37Z","receivedAt":"2022-07-30T20:39:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> t3200-branch.sh does\n>\n> git symbolic-ref refs/heads/dangling-symref nowhere\n>\n> which really depends on that whole \"git symbolic-ref does no sanity\n> checking at all\".\n\nYeah, once in the past, I thought\n\n git symbolic-ref refs/heads/main master\n\nmight be a way to adjust to the new world order without having me to\nchange my workflow. I can always update master, and main follows it\nwithout me being aware of it even being there. \n\nBut it did not work. Even worse, after doing so, running\n\n git update-ref refs/heads/main master\n\ncreated \".git/master\", which was a disaster. Of course\n\n git symbolic-ref refs/heads/main refs/heads/master\n\nwould have worked.\n\nI am not sure what workflow the \"nowhere\" thing is supposed to help.\nOf course, with s/nowhere/HEAD/, it is a perfectly sane repository\nimmediately after \"git init -b nowhere\", so whatever tightening we\ndo, we should make sure that\n\n git symbolic-ref refs/heads/main HEAD\n\nkeeps working.\n"},{"id":"460317","messageId":"YuXKaLXhnR3mVlWk@coredump.intra.peff.net","threadId":"58247","inReplyTo":"CAHk-=wg9LaHeg0UmZ90gLOaBpO-5fhoaH22iNNm=1eror95pFg@mail.gmail.com","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-31T00:18:48Z","receivedAt":"2022-07-31T00:18:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 30, 2022 at 01:21:50PM -0700, Linus Torvalds wrote:\n\n> Again - this is such a low-level plumbing thing that maybe nobody\n> cares, but it just struck me as a bad idea to have these kinds of\n> maintenance commands that can be used to just mess up your repository.\n> And if you have a bare repo, this really does look like the command\n> that *should* be used to change HEAD, since it's not about \"git\n> checkout\"\n\nI think it is probably worth addressing. I'm sure it has bitten me\nbefore[0], and the HEAD logic from afe5d3d516 (symbolic ref: refuse\nnon-ref targets in HEAD, 2009-01-29) was a cowardly attempt to fix the\nmost egregious cases without breaking anything.\n\n> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\n> index e547a08d6c..5354cfb4f1 100644\n> --- a/builtin/symbolic-ref.c\n> +++ b/builtin/symbolic-ref.c\n> @@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n> \t\tif (!strcmp(argv[0], \"HEAD\") &&\n> \t\t !starts_with(argv[1], \"refs/\"))\n> \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n> +\t\tif (check_refname_format(argv[1], 0) < 0)\n> +\t\t\tdie(\"Refusing to set '%s' to invalid ref '%s'\", argv[0], argv[1]);\n\nThis forbids syntactically-invalid refnames, which is good.\n\nSince you don't pass REFNAME_ALLOW_ONELEVEL, it also forbids nonsense\nnames like \"nowhere\". But that also breaks some probably-stupid cases\nthat are currently possible, like:\n\n git symbolic-ref refs/heads/foo FETCH_HEAD\n\nI'm not really sure why anybody would want to do that, but it does work\ncurrently. I'm tempted to say that the symref-reading code should\nactually complain about following something outside of \"refs/\", but that\ncarries an even higher possibility of breaking somebody. But it seems\nlike we should be consistent between what we allow to be read, and what\nwe allow to be written.\n\nAt any rate, with the code as you have it above, I think the \"make sure\nHEAD starts with refs/\" code is now redundant.\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 9723c2827c..b194c1b09b 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -723,9 +723,9 @@ test_expect_success 'deleting a symref' '\n> '\n> \n> test_expect_success 'deleting a dangling symref' '\n> -\tgit symbolic-ref refs/heads/dangling-symref nowhere &&\n> +\tgit symbolic-ref refs/heads/dangling-symref refs/heads/nowhere &&\n> \ttest_path_is_file .git/refs/heads/dangling-symref &&\n> -\techo \"Deleted branch dangling-symref (was nowhere).\" >expect &&\n> +\techo \"Deleted branch dangling-symref (was refs/heads/nowhere).\" >expect &&\n\nThis is a sensible change. With ALLOW_ONELEVEL, it wouldn't be\nnecessary, but I think it is representing a more plausible real-world\nscenario.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 6e66352558..427b06442d 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -2114,7 +2114,7 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '\n> \n> test_expect_success 'log diagnoses bogus HEAD symref' '\n> \tgit init empty &&\n> -\tgit --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&\n> +\techo \"ref: refs/heads/invalid.lock\" > empty/.git/HEAD &&\n> \ttest_must_fail git -C empty log 2>stderr &&\n> \ttest_i18ngrep broken stderr &&\n> \ttest_must_fail git -C empty log --default totally-bogus 2>stderr &&\n\nAfter this change, I think this would need to be marked with a REFFILES\nprereq similar to the test right before it.\n\n-Peff\n\n[0] Curiously there was a very similar patch to yours posted a while\n ago:\n\n https://lore.kernel.org/git/4FDE3D7D.4090502@elegosoft.com/\n\n There was some discussion and a followup:\n\n https://lore.kernel.org/git/1342440781-18816-1-git-send-email-mschub@elegosoft.com/\n\n but nothing seems to have been applied. I don't see any arguments\n against it there; I think the author simply didn't push it forward\n enough.\n\n There's also some bits in the sub-thread about limiting HEAD to\n \"refs/heads/\", which we couldn't quite do at the time. That might be\n worth revisiting, but definitely shouldn't hold up your patch.\n"},{"id":"460318","messageId":"YuXLtIBXYG+JBKdV@coredump.intra.peff.net","threadId":"58247","inReplyTo":"YuXKaLXhnR3mVlWk@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-31T00:24:20Z","receivedAt":"2022-07-31T00:24:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 30, 2022 at 08:18:48PM -0400, Jeff King wrote:\n\n> There's also some bits in the sub-thread about limiting HEAD to\n> \"refs/heads/\", which we couldn't quite do at the time. That might be\n> worth revisiting, but definitely shouldn't hold up your patch.\n\nHmph, maybe not. The sticking point was topgit, which points HEAD at\nrefs/top-bases. There's a fork here:\n\n https://github.com/mackyle/topgit\n\nwhich has been active in the last 12 months and which still uses that\nconvention. So maybe people really are still using it.\n\n(Again, neither here nor there for your patch).\n\n-Peff\n"},{"id":"460321","messageId":"CAHk-=wi5pfUcuaAUz=rifon9d51mshE7k6bkpMXddog0On9jow@mail.gmail.com","threadId":"58247","inReplyTo":"YuXLtIBXYG+JBKdV@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-07-31T00:44:25Z","receivedAt":"2022-07-31T00:44:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sat, Jul 30, 2022 at 5:24 PM Jeff King wrote:\n>>\n> Hmph, maybe not. The sticking point was topgit, which points HEAD at\n> refs/top-bases. There's a fork here:\n>\n> https://github.com/mackyle/topgit\n>\n> which has been active in the last 12 months and which still uses that\n> convention. So maybe people really are still using it.\n>\n> (Again, neither here nor there for your patch).\n\nWell, it *is* relevant for my patch in the sense that I clearly didn't\nthink of all the crazy things people might have been doing.\n\nThat\n\n git symbolic-ref refs/heads/foo FETCH_HEAD\n\nthat you mentioned in the other mail would obviously be entirely\ndisallowed by my patch, and again, I didn't for a second imagine that\nsomebody would do something that strange. Junio mentioned a similarly\nodd possible situation.\n\nSo while I think my patch is the right thing to do, I will also admit\nthat it's perhaps a \"we should always have done this, but we didn't\"\nsituation, and maybe those really odd cases need to be allowed.\n\nAdding ALLOW_ONELEVEL would make those things presumably still work,\nand would at least improve things *somewhat* - it would protect people\nfrom syntactically invalid branches (ie bad characters in the branch\nname etc).\n\nThat would imply still having to fix up that t4202-log.sh testcase,\nand I didn't even know or realize about that REFFILES prerequisite,\nsince obviously in all my use it has been true. I still use and test\nonly on Linux..\n\nYou are also right that without the ALLOW_ONELEVEL, the special-case\ncheck for HEAD should just be removed. That patch started out as the\nminimal possible \"let's just disallow invalid ref names\" patch, so I\ndidn't touch that odd special case code.\n\nPut another way: I think my patch is likely the right thing to do (and\nI'd personally prefer the stricter check without the ALLOW_ONELEVEL\nflag), but you and Junio are right about it being a bigger change than\nI in my naivete thought it was.\n\nSo I won't really push for this, I suspect this needs very much to be\na judgement call by you guys.\n\nThanks,\n\n Linus\n"},{"id":"460337","messageId":"xmqqzggpyu7q.fsf@gitster.g","threadId":"58247","inReplyTo":"YuXKaLXhnR3mVlWk@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-31T19:10:01Z","receivedAt":"2022-07-31T19:10:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm tempted to say that the symref-reading code should\n> actually complain about following something outside of \"refs/\", but that\n> carries an even higher possibility of breaking somebody. But it seems\n> like we should be consistent between what we allow to be read, and what\n> we allow to be written.\n>\n> At any rate, with the code as you have it above, I think the \"make sure\n> HEAD starts with refs/\" code is now redundant.\n\nIsn't the rule these days \"HEAD must be either detached or point\ninto refs/heads/\"? I thought \"checkout\" ensures that, and I am\ntempted to think that \"symbolic-ref\" that works on HEAD should be\nconsistent with \"checkout\". So \"make sure HEAD is within refs/\"\nwould certainly be \"not wrong per-se\" but not sufficiently tight,\nI suspect.\n\n"},{"id":"460360","messageId":"YugPER9UsH1z6MZo@coredump.intra.peff.net","threadId":"58247","inReplyTo":"xmqqzggpyu7q.fsf@gitster.g","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-01T17:36:17Z","receivedAt":"2022-08-01T17:36:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 31, 2022 at 12:10:01PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm tempted to say that the symref-reading code should\n> > actually complain about following something outside of \"refs/\", but that\n> > carries an even higher possibility of breaking somebody. But it seems\n> > like we should be consistent between what we allow to be read, and what\n> > we allow to be written.\n> >\n> > At any rate, with the code as you have it above, I think the \"make sure\n> > HEAD starts with refs/\" code is now redundant.\n> \n> Isn't the rule these days \"HEAD must be either detached or point\n> into refs/heads/\"? I thought \"checkout\" ensures that, and I am\n> tempted to think that \"symbolic-ref\" that works on HEAD should be\n> consistent with \"checkout\". So \"make sure HEAD is within refs/\"\n> would certainly be \"not wrong per-se\" but not sufficiently tight,\n> I suspect.\n\nNo, sadly, that isn't the rule. See afe5d3d516 (symbolic ref: refuse\nnon-ref targets in HEAD, 2009-01-29) which tightened it to \"refs/heads\"\nand then e9cc02f0e4 (symbolic-ref: allow refs/ in HEAD,\n2009-02-13) which had to loosen it.\n\nLikewise we seemed to touch the reading side at the same time, via\nb229d18a80 (validate_headref: tighten ref-matching to just branches,\n2009-01-29), and then 222b167386 (Revert \"validate_headref: tighten\nref-matching to just branches\", 2009-02-12).\n\nIn both cases it was refs/top-bases that triggered the revert. The\nrelevant thread is:\n\n https://lore.kernel.org/git/cc723f590902120009w432f5f61xd6550409835cdbb7@mail.gmail.com/\n\nThere's some discussion there about how topgit could do things\ndifferently, but I don't see any plan for moving away. However, the\nlatest changelog for it I could find has:\n\n [from https://mackyle.github.io/topgit/changelog.html]\n TopGit 0.19.4 (2017-02-14) introduced support for a new top-bases\n location under heads. This new location will become the default as of\n the TopGit 0.20.0 release. The current location under refs will\n continue to be supported in the future. See tg help migrate-bases for\n more details.\n\nSo it looks like there is some will there to switch. But the default\nhasn't flipped yet, and we'd be breaking any non-migrated installs. Not\nto mention that we don't know if any _other_ tools care. The topgit\nfolks reported the original problem in 2009 within a few weeks, and it\nnever made it to a release.\n\nSo yeah. We could certainly work out a deprecation/migration plan, but\nI'm not sure it's worth the effort. I do suspect there are other parts\nof Git that assume HEAD points at refs/heads/, especially the\nclone/fetch HEAD-selection code. But that code is getting the symref\ntarget from an untrusted remote, and is already careful about discarding\nnonsense and continuing.\n\nRegarding \"checkout\" versus \"symbolic-ref\", I do think \"checkout\"\nprobably does limit us to refs/heads/. But it is OK for porcelain to be\nopinionated and restrictive, but plumbing probably needs to support\nexisting possibly-silly use cases.\n\n-Peff\n"},{"id":"460361","messageId":"YugQqp4oN26OFOpt@coredump.intra.peff.net","threadId":"58247","inReplyTo":"CAHk-=wi5pfUcuaAUz=rifon9d51mshE7k6bkpMXddog0On9jow@mail.gmail.com","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-01T17:43:06Z","receivedAt":"2022-08-01T17:43:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 30, 2022 at 05:44:25PM -0700, Linus Torvalds wrote:\n\n> Put another way: I think my patch is likely the right thing to do (and\n> I'd personally prefer the stricter check without the ALLOW_ONELEVEL\n> flag), but you and Junio are right about it being a bigger change than\n> I in my naivete thought it was.\n> \n> So I won't really push for this, I suspect this needs very much to be\n> a judgement call by you guys.\n\nJust to lay out the options, I think we have:\n\n 1. Do nothing. This breaks nothing. ;)\n\n 2. Your patch, but with ALLOW_ONELEVEL. This fixes nonsense like\n \"foo..bar\", but doesn't break \"FETCH_HEAD\". Requires fixing t4202's\n \".lock\" example. Replaces the HEAD starts_with(\"refs/\") check.\n\n 3. Your patch as-is. Same as (2), but also breaks FETCH_HEAD.\n\n 4. Your patch, plus any extra tightening of HEAD to refs/heads/. I\n think this is probably breaking too much (I put more details\n elsewhere in the thread).\n\nI'd be in favor of (2), which is really just catching syntactically\ninvalid crap, and shouldn't break anyone. Technically it's possible\nsomebody could be using a symref pointing at arbitrary data for\nwho-knows-what reason, and extracting it with \"symbolic-ref\", but that\nis getting beyond far-fetched, I think.\n\nI'm also tempted by (3), but we should be prepared for obscure breakage\nreports.\n\n-Peff\n"},{"id":"460362","messageId":"YugRYN6sbP+RpxlJ@coredump.intra.peff.net","threadId":"58247","inReplyTo":"YugQqp4oN26OFOpt@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-01T17:46:08Z","receivedAt":"2022-08-01T17:46:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2022 at 01:43:06PM -0400, Jeff King wrote:\n\n> 2. Your patch, but with ALLOW_ONELEVEL. This fixes nonsense like\n> \"foo..bar\", but doesn't break \"FETCH_HEAD\". Requires fixing t4202's\n> \".lock\" example. Replaces the HEAD starts_with(\"refs/\") check.\n> \n> 3. Your patch as-is. Same as (2), but also breaks FETCH_HEAD.\n\nEr, sorry, the starts_with() thing is the other way around. It is\nredundant with your patch, but necessary to keep it for (2). Luckily\nt1401 catches the problem if you dare to try it. ;)\n\n-Peff\n"},{"id":"460363","messageId":"CAHk-=wg8EaHnM_OcHrw=+sT3VPAkTUpzeaQ8EjTDLUENK58HSw@mail.gmail.com","threadId":"58247","inReplyTo":"YugPER9UsH1z6MZo@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-08-01T17:49:26Z","receivedAt":"2022-08-01T17:49:48Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":")(\n\nOn Mon, Aug 1, 2022 at 10:36 AM Jeff King wrote:\n>\n> No, sadly, that isn't the rule. See afe5d3d516 (symbolic ref: refuse\n> non-ref targets in HEAD, 2009-01-29) which tightened it to \"refs/heads\"\n> and then e9cc02f0e4 (symbolic-ref: allow refs/ in HEAD,\n> 2009-02-13) which had to loosen it.\n\nBut it seems that at least this issue won't affect check_refname_format().\n\nHmm. Looking at that again - even without ALLOW_ONELEVEL I don't\nactually think check_refname_format() requires \"refs/\" per se. So the\nHEAD check isn't actually made redundant.\n\nI wonder what the intended semantic meaning of ALLOW_ONELEVEL really\nis supposed to be. It seems to really only require *one* slash - but\nit doesn't really end up checking that it's in the \"refs/\" hierarchy,\nit can be anywhere.\n\nI guess the only thing it disallows is literally HEAD and MERGE_HEAD\nand the like. But it's a bit odd, because it would seem to possibly\nallow you to use \"refs\" that point to the objects/ directory or\nsimilar.\n\nMaybe the refs/ protection comes in somewhere later, I didn't really\ngo around to check.\n\n Linus\n"},{"id":"460364","messageId":"YugVxaej5LdX4S8r@coredump.intra.peff.net","threadId":"58247","inReplyTo":"CAHk-=wg8EaHnM_OcHrw=+sT3VPAkTUpzeaQ8EjTDLUENK58HSw@mail.gmail.com","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-01T18:04:53Z","receivedAt":"2022-08-01T18:04:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2022 at 10:49:26AM -0700, Linus Torvalds wrote:\n\n> Hmm. Looking at that again - even without ALLOW_ONELEVEL I don't\n> actually think check_refname_format() requires \"refs/\" per se. So the\n> HEAD check isn't actually made redundant.\n> \n> I wonder what the intended semantic meaning of ALLOW_ONELEVEL really\n> is supposed to be. It seems to really only require *one* slash - but\n> it doesn't really end up checking that it's in the \"refs/\" hierarchy,\n> it can be anywhere.\n\nI'm actually not that surprised. I think the history of that flag\nis...weird. I think once upon a time, there was \"one-level\" checking\nwhich was meant to disallow \"refs/foo\" versus \"refs/heads/foo\". But\nthere were also spots that wanted to make sure we were in refs/, and not\ntouching MERGE_HEAD, etc.\n\nAnd because of the generic-ness of the flag name, those two cases got\nconflated. I think it's mostly been sorted out over the years, but I\nwon't be surprised if there are weird corner cases.\n\n> Maybe the refs/ protection comes in somewhere later, I didn't really\n> go around to check.\n\nI didn't check where, but I did confirm that the \"symbolic-ref HEAD foo\"\ncase in t1401 continues to pass even if we remove the special HEAD code.\nSo _something_ is doing it. ;)\n\n-Peff\n"},{"id":"460365","messageId":"YugYNzQYWqDCmOqN@coredump.intra.peff.net","threadId":"58247","inReplyTo":"YugQqp4oN26OFOpt@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-01T18:15:19Z","receivedAt":"2022-08-01T18:15:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2022 at 01:43:06PM -0400, Jeff King wrote:\n\n> I'd be in favor of (2), which is really just catching syntactically\n> invalid crap, and shouldn't break anyone. Technically it's possible\n> somebody could be using a symref pointing at arbitrary data for\n> who-knows-what reason, and extracting it with \"symbolic-ref\", but that\n> is getting beyond far-fetched, I think.\n\nJust to keep things moving forward, here it is with a commit message. I\nleft you as the author, but if you're OK with it, please tell Junio he\ncan forge your sign-off.\n\n-- >8 --\nFrom: Linus Torvalds \nSubject: [PATCH] symbolic-ref: refuse to set syntactically invalid target\n\nYou can feed absolute garbage to symbolic-ref as a target like:\n\n git symbolic-ref HEAD refs/heads/foo..bar\n\nWhile this doesn't technically break the repo entirely (our \"is it a git\ndirectory\" detector looks only for \"refs/\" at the start), we would never\nresolve such a ref, as the \"..\" is invalid within a refname.\n\nLet's flag these as invalid at creation time to help the caller realize\nthat what they're asking for is bogus.\n\nA few notes:\n\n - We use REFNAME_ALLOW_ONELEVEL here, which lets:\n\n git update-ref refs/heads/foo FETCH_HEAD\n\n continue to work. It's unclear whether anybody wants to do something\n so odd, but it does work now, so this is erring on the conservative\n side. There's a test to make sure we didn't accidentally break this,\n but don't take that test as an endorsement that it's a good idea, or\n something we might not change in the future.\n\n - The test in t4202-log.sh checks how we handle such an invalid ref on\n the reading side, so it has to be updated to touch the HEAD file\n directly.\n\n - We need to keep our HEAD-specific check for \"does it start with\n refs/\". The ALLOW_ONELEVEL flag means we won't be enforcing that for\n other refs, but HEAD is special here because of the checks in\n validate_headref().\n\nSigned-off-by: Jeff King \n---\n builtin/symbolic-ref.c | 2 ++\n t/t1401-symbolic-ref.sh | 10 ++++++++++\n t/t4202-log.sh | 4 ++--\n 3 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex e547a08d6c..1b0f10225f 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n \t\t !starts_with(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n+\t\tif (check_refname_format(argv[1], REFNAME_ALLOW_ONELEVEL) < 0)\n+\t\t\tdie(\"Refusing to set '%s' to invalid ref '%s'\", argv[0], argv[1]);\n \t\tret = !!create_symref(argv[0], argv[1], msg);\n \t\tbreak;\n \tdefault:\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 9fb0b90f25..0c204089b8 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -165,4 +165,14 @@ test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'symbolic-ref refuses invalid target for non-HEAD' '\n+\ttest_must_fail git symbolic-ref refs/heads/invalid foo..bar\n+'\n+\n+test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '\n+\tgit symbolic-ref refs/heads/top-level FETCH_HEAD &&\n+\tgit update-ref FETCH_HEAD HEAD &&\n+\ttest_cmp_rev top-level HEAD\n+'\n+\n test_done\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 6e66352558..f0aaa1fa02 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -2112,9 +2112,9 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '\n \ttest_i18ngrep broken stderr\n '\n \n-test_expect_success 'log diagnoses bogus HEAD symref' '\n+test_expect_success REFFILES 'log diagnoses bogus HEAD symref' '\n \tgit init empty &&\n-\tgit --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&\n+\techo \"ref: refs/heads/invalid.lock\" > empty/.git/HEAD &&\n \ttest_must_fail git -C empty log 2>stderr &&\n \ttest_i18ngrep broken stderr &&\n \ttest_must_fail git -C empty log --default totally-bogus 2>stderr &&\n-- \n2.37.1.804.g1775fa20e0\n\n"},{"id":"460371","messageId":"xmqqfsifyetv.fsf@gitster.g","threadId":"58247","inReplyTo":"YugYNzQYWqDCmOqN@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-01T18:54:36Z","receivedAt":"2022-08-01T18:54:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Just to keep things moving forward, here it is with a commit message. I\n> left you as the author, but if you're OK with it, please tell Junio he\n> can forge your sign-off.\n>\n> -- >8 --\n> From: Linus Torvalds \n> Subject: [PATCH] symbolic-ref: refuse to set syntactically invalid target\n>\n> You can feed absolute garbage to symbolic-ref as a target like:\n>\n> git symbolic-ref HEAD refs/heads/foo..bar\n>\n> While this doesn't technically break the repo entirely (our \"is it a git\n> directory\" detector looks only for \"refs/\" at the start), we would never\n> resolve such a ref, as the \"..\" is invalid within a refname.\n>\n> Let's flag these as invalid at creation time to help the caller realize\n> that what they're asking for is bogus.\n>\n> A few notes:\n>\n> - We use REFNAME_ALLOW_ONELEVEL here, which lets:\n>\n> git update-ref refs/heads/foo FETCH_HEAD\n>\n> continue to work. It's unclear whether anybody wants to do something\n> so odd, but it does work now, so this is erring on the conservative\n> side. There's a test to make sure we didn't accidentally break this,\n> but don't take that test as an endorsement that it's a good idea, or\n> something we might not change in the future.\n\nOK. Even if it were HEAD, it does look like a funny thing to do to\npoint at a shallower ref with a more concrete ref.\n\n> - The test in t4202-log.sh checks how we handle such an invalid ref on\n> the reading side, so it has to be updated to touch the HEAD file\n> directly.\n>\n> - We need to keep our HEAD-specific check for \"does it start with\n> refs/\". The ALLOW_ONELEVEL flag means we won't be enforcing that for\n> other refs, but HEAD is special here because of the checks in\n> validate_headref().\n\nOK.\n\n> Signed-off-by: Jeff King \n> ---\n> builtin/symbolic-ref.c | 2 ++\n> t/t1401-symbolic-ref.sh | 10 ++++++++++\n> t/t4202-log.sh | 4 ++--\n> 3 files changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\n> index e547a08d6c..1b0f10225f 100644\n> --- a/builtin/symbolic-ref.c\n> +++ b/builtin/symbolic-ref.c\n> @@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n> \t\tif (!strcmp(argv[0], \"HEAD\") &&\n> \t\t !starts_with(argv[1], \"refs/\"))\n> \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n> +\t\tif (check_refname_format(argv[1], REFNAME_ALLOW_ONELEVEL) < 0)\n> +\t\t\tdie(\"Refusing to set '%s' to invalid ref '%s'\", argv[0], argv[1]);\n\nMakes sense. Rejecting syntactically invalid thing like double-dot\nis something we should have done from day one.\n\n> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\n> index 9fb0b90f25..0c204089b8 100755\n> --- a/t/t1401-symbolic-ref.sh\n> +++ b/t/t1401-symbolic-ref.sh\n> @@ -165,4 +165,14 @@ test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'symbolic-ref refuses invalid target for non-HEAD' '\n> +\ttest_must_fail git symbolic-ref refs/heads/invalid foo..bar\n> +'\n\nGood.\n\n> +test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '\n> +\tgit symbolic-ref refs/heads/top-level FETCH_HEAD &&\n> +\tgit update-ref FETCH_HEAD HEAD &&\n> +\ttest_cmp_rev top-level HEAD\n> +'\n> test_done\n\nStrange, but OK.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 6e66352558..f0aaa1fa02 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -2112,9 +2112,9 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '\n> \ttest_i18ngrep broken stderr\n> '\n> \n> -test_expect_success 'log diagnoses bogus HEAD symref' '\n> +test_expect_success REFFILES 'log diagnoses bogus HEAD symref' '\n> \tgit init empty &&\n> -\tgit --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&\n> +\techo \"ref: refs/heads/invalid.lock\" > empty/.git/HEAD &&\n\nOK.\n\n> \ttest_must_fail git -C empty log 2>stderr &&\n> \ttest_i18ngrep broken stderr &&\n> \ttest_must_fail git -C empty log --default totally-bogus 2>stderr &&\n"},{"id":"460373","messageId":"CAHk-=wh64fFWR_=tMFLVW-ozHb5w2VoAGgpsMAuSNDVYexRRDw@mail.gmail.com","threadId":"58247","inReplyTo":"YugYNzQYWqDCmOqN@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-08-01T19:00:41Z","receivedAt":"2022-08-01T19:01:02Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Aug 1, 2022 at 11:15 AM Jeff King wrote:\n>\n> Just to keep things moving forward, here it is with a commit message. I\n> left you as the author, but if you're OK with it, please tell Junio he\n> can forge your sign-off.\n\nI'm certainly ok with that.\n\nThat said, I'm also ok with not getting the authorship credit for this\nat all. Not because you wrote the commit log and updated the patch\nwith REFNAME_ALLOW_ONELEVEL and added a test-case, but because clearly\nMichael Schubert already did basically the core of that patch ten\nyears ago.\n\nSo Junio, feel free to add my\n\n Signed-off-by: Linus Torvalds \n\nbut feel equally free to give credit to Jeff or - belatedly - Michael.\n\n Linus\n"},{"id":"460398","messageId":"Yuhz0VAX77qv4P5Z@coredump.intra.peff.net","threadId":"58247","inReplyTo":"xmqqfsifyetv.fsf@gitster.g","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-02T00:46:09Z","receivedAt":"2022-08-02T00:46:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2022 at 11:54:36AM -0700, Junio C Hamano wrote:\n\n> > +test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '\n> > +\tgit symbolic-ref refs/heads/top-level FETCH_HEAD &&\n> > +\tgit update-ref FETCH_HEAD HEAD &&\n> > +\ttest_cmp_rev top-level HEAD\n> > +'\n> > test_done\n> \n> Strange, but OK.\n\nI'd be OK to drop this if you hate it too much, btw. Mostly I wanted to\nmake sure that the various iterations behaved as I expected. But there\nis a test in t3200 (the one Linus found earlier) that incidentally does\ncheck that something like this works.\n\n-Peff\n"},{"id":"460400","messageId":"xmqqilnbjwcl.fsf@gitster.g","threadId":"58247","inReplyTo":"Yuhz0VAX77qv4P5Z@coredump.intra.peff.net","subject":"Re: \"git symbolic-ref\" doesn't do a very good job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-02T00:57:30Z","receivedAt":"2022-08-02T00:57:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 01, 2022 at 11:54:36AM -0700, Junio C Hamano wrote:\n>\n>> > +test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '\n>> > +\tgit symbolic-ref refs/heads/top-level FETCH_HEAD &&\n>> > +\tgit update-ref FETCH_HEAD HEAD &&\n>> > +\ttest_cmp_rev top-level HEAD\n>> > +'\n>> > test_done\n>> \n>> Strange, but OK.\n>\n> I'd be OK to drop this if you hate it too much, btw. Mostly I wanted to\n> make sure that the various iterations behaved as I expected. But there\n> is a test in t3200 (the one Linus found earlier) that incidentally does\n> check that something like this works.\n\nOh, no, I do not hate it (or like it) at all.\n\nThe \"strange\" was mostly referring to the order of the symbolic\nthing that refers to another thing that is being pointed at, which\nlooked backwards, i.e. \"git symbolic-ref HEAD refs/heads/main\" is\nwhat we usually expect (i.e. \"we use this short name HEAD to refer\nto the longer refs/heads/main ref\"), but after staring the one in\nthe test \"git symbolic-ref refs/heads/top-level FETCH_HEAD\" too\nlong, your eyes trick your brain into thinking we use the short name\nFETCH_HEAD to refer to the top-level branch, which is the other way\naround.\n\nWe've been allowing the one-level thing and I think the discussion\nhas established that we need to keep it supported. There is nothing\nto hate or like about it X-<.\n\nThanks.\n\n"}]}