{"thread":{"id":"62274","subject":"[PATCH 0/3] object-name: don't allow @ as a branch name","startedAt":"2024-10-07T20:16:12Z","lastAt":"2024-10-18T14:21:11Z","messageCount":18,"participants":["Kristoffer Haugsbakk","Jeff King","Junio C Hamano","shejialuo","Rubén Justo"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"504352","messageId":"cover.1728331771.git.code@khaugsbakk.name","threadId":"62274","inReplyTo":null,"subject":"[PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-07T20:15:16Z","receivedAt":"2024-10-07T20:16:12Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"I use `@` a lot for Git commands in the terminal.  I accidentally did\nsomething that made me create a branch named `@`.  This puzzled me since\n`HEAD` is not allowed.\n\nNote that the bare/one-level `@` ref name is already banned.  So this is\njust about not allowing `refs/heads/@`.\n\n§ Research\n\nThis has come up before.  There even is a test which guards the current\nbehavior (allow `@` as a branch name) with the comment:[1]\n\n```\n# The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n# sane thing, but it _is_ technically allowed for now. If we disallow it, these\n# can be switched to test_must_fail.\n```\n\nThere was no reply to this change in neither the first[2] nor second\nversion.\n\nThat series points back to a bug report thread[3] which is about\nexpanding `@` to a branch named `HEAD`.\n\nPeff found a way for the branch name `HEAD` to be created While figuring\nout a solution:[4]\n\n> Checking \"HEAD\" afterwards means you can't actually have a branch\n> named \"HEAD\". Doing so is probably insane, but we probably really _do_\n> want to just disallow the @-conversion here.\n\nSo that was tangential to the bug fix (`HEAD` as a branch name was not\ndisallowed in the patch series that resulted from this bug).\n\n🔗 1: https://lore.kernel.org/git/20170302082306.n6kfc5uqz2kdxtpm@sigill.intra.peff.net/\n🔗 2: https://public-inbox.org/git/20170228121514.qajydm5bjdbzsucg@sigill.intra.peff.net/\n🔗 3: https://public-inbox.org/git/20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net/\n🔗 4: https://public-inbox.org/git/20170227090233.uk7dfruggytgmuw2@sigill.intra.peff.net/\n\n  §2 Disallow `HEAD` as a branch name\n\nThis was done later in 2017:\n\nhttps://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/\n\n  §2 `refs/heads/@` is apparently disallowed by git-refs(1)\n\nSee `t/t1508-at-combinations.sh`:\n\n```\nerror: refs/heads/@: badRefName: invalid refname format\n```\n\nKristoffer Haugsbakk (3):\n  object-name: fix whitespace\n  object-name: don't allow @ as a branch name\n  t1402: exercise disallowed branch names\n\n object-name.c                         | 5 ++---\n t/t1402-check-ref-format.sh           | 4 ++++\n t/t3204-branch-name-interpretation.sh | 9 ++-------\n 3 files changed, 8 insertions(+), 10 deletions(-)\n\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"504353","messageId":"689eb69554480343b9f6db15ee6bef2c505717ad.1728331771.git.code@khaugsbakk.name","threadId":"62274","inReplyTo":"cover.1728331771.git.code@khaugsbakk.name","subject":"[PATCH 1/3] object-name: fix whitespace","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-07T20:15:17Z","receivedAt":"2024-10-07T20:16:15Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Fix double newlines according to `clang format`.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n object-name.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80aa..42e3ba4a77a 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -482,7 +482,6 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)\n \t\tstrbuf_addf(sb, _(\"%s blob\"), hash);\n \t}\n \n-\n out:\n \t/*\n \t * TRANSLATORS: This is line item of ambiguous object output\n@@ -1965,7 +1964,6 @@ static void diagnose_invalid_index_path(struct repository *r,\n \tstrbuf_release(&fullname);\n }\n \n-\n static char *resolve_relative_path(struct repository *r, const char *rel)\n {\n \tif (!starts_with(rel, \"./\") && !starts_with(rel, \"../\"))\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"504354","messageId":"b88c2430f88b641d69e5f161d3a18cce113a81c9.1728331771.git.code@khaugsbakk.name","threadId":"62274","inReplyTo":"cover.1728331771.git.code@khaugsbakk.name","subject":"[PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-07T20:15:18Z","receivedAt":"2024-10-07T20:16:18Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"`HEAD` is an invalid branch name.[1]  But the `@` synonym is allowed.\nThis is just as inconvenient since commands like `git checkout @` will,\nquite sensibly, do `git checkout HEAD` instead of checking out that\nbranch; in turn there is no practical reason to use this as a branch\nname since you cannot even check out the branch itself (only check out\nthe commit which `refs/heads/@` points to).\n\n† 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD},\n    2017-11-14)\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n object-name.c                         | 3 ++-\n t/t3204-branch-name-interpretation.sh | 9 ++-------\n 2 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 42e3ba4a77a..56b288ff4c3 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1763,7 +1763,8 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \n \tif (*name == '-' ||\n-\t    !strcmp(sb->buf, \"refs/heads/HEAD\"))\n+\t    !strcmp(sb->buf, \"refs/heads/HEAD\") ||\n+\t    !strcmp(sb->buf, \"refs/heads/@\"))\n \t\treturn -1;\n \n \treturn check_refname_format(sb->buf, 0);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 594e3e43e12..7dcd1308f8c 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' '\n \texpect_branch refs/heads/origin/previous two\n '\n \n-# The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n-# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n-# sane thing, but it _is_ technically allowed for now. If we disallow it, these\n-# can be switched to test_must_fail.\n-test_expect_success 'create branch named \"@\"' '\n-\tgit branch -f @ one &&\n-\texpect_branch refs/heads/@ one\n+test_expect_success 'disallow branch named \"@\"' '\n+\ttest_must_fail git branch -f @ one\n '\n \n test_expect_success 'delete branch named \"@\"' '\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"504355","messageId":"8262b81141bbd36cd7a17e6abe5eb6bb688290f3.1728331771.git.code@khaugsbakk.name","threadId":"62274","inReplyTo":"cover.1728331771.git.code@khaugsbakk.name","subject":"[PATCH 3/3] t1402: exercise disallowed branch names","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-07T20:15:19Z","receivedAt":"2024-10-07T20:16:22Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n t/t1402-check-ref-format.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 5ed9d7318e0..06ef54c6091 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -68,6 +68,10 @@ valid_ref 'heads/foo*/bar' --refspec-pattern\n valid_ref 'heads/f*o/bar' --refspec-pattern\n invalid_ref 'heads/f*o*/bar' --refspec-pattern\n invalid_ref 'heads/foo*/bar*' --refspec-pattern\n+invalid_ref 'HEAD' --branch\n+invalid_ref '@' --branch\n+invalid_ref '-' --branch\n+invalid_ref '-something' --branch\n \n ref='foo'\n invalid_ref \"$ref\"\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"504356","messageId":"20241007203720.GA603285@coredump.intra.peff.net","threadId":"62274","inReplyTo":"cover.1728331771.git.code@khaugsbakk.name","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-07T20:37:20Z","receivedAt":"2024-10-07T20:37:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:\n\n> This has come up before.  There even is a test which guards the current\n> behavior (allow `@` as a branch name) with the comment:[1]\n> \n> ```\n> # The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n> # and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n> # sane thing, but it _is_ technically allowed for now. If we disallow it, these\n> # can be switched to test_must_fail.\n> ```\n> \n> There was no reply to this change in neither the first[2] nor second\n> version.\n> \n> That series points back to a bug report thread[3] which is about\n> expanding `@` to a branch named `HEAD`.\n\nYeah. The series you found was about not expanding \"@\" in the wrong\ncontexts. So the test made sure we did not do so, but of course it was\nthen left asserting the weird behavior that was left over. So this:\n\n> So that was tangential to the bug fix (`HEAD` as a branch name was not\n> disallowed in the patch series that resulted from this bug).\n\nis accurate. Those tests are no reason we should not consider\ndisallowing \"@\" as a branch name.\n\n  As an aside, I have a couple times left these sort of \"do not take\n  this test as an endorsement of the behavior\" comments when working in\n  crufty corners of the code base. I am happy that one is finally paying\n  off! ;)\n\nSo I think the aim of your series is quite reasonable. The\nimplementation mostly looks good, but I have a few comments which I'll\nleave on the individual patches.\n\n-Peff\n"},{"id":"504357","messageId":"a727888c-9960-44a9-b0b6-a54d5ecaa5d6@app.fastmail.com","threadId":"62274","inReplyTo":"20241007203720.GA603285@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-07T20:40:00Z","receivedAt":"2024-10-07T20:40:23Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Oct 7, 2024, at 22:37, Jeff King wrote:\n> On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:\n>\n>> This has come up before.  There even is a test which guards the current\n>> behavior (allow `@` as a branch name) with the comment:[1]\n>> \n>> ```\n>> # The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n>> # and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n>> # sane thing, but it _is_ technically allowed for now. If we disallow it, these\n>> # can be switched to test_must_fail.\n>> ```\n>> \n>> There was no reply to this change in neither the first[2] nor second\n>> version.\n>> \n>> That series points back to a bug report thread[3] which is about\n>> expanding `@` to a branch named `HEAD`.\n>\n> Yeah. The series you found was about not expanding \"@\" in the wrong\n> contexts. So the test made sure we did not do so, but of course it was\n> then left asserting the weird behavior that was left over. So this:\n>\n>> So that was tangential to the bug fix (`HEAD` as a branch name was not\n>> disallowed in the patch series that resulted from this bug).\n>\n> is accurate. Those tests are no reason we should not consider\n> disallowing \"@\" as a branch name.\n>\n>   As an aside, I have a couple times left these sort of \"do not take\n>   this test as an endorsement of the behavior\" comments when working in\n>   crufty corners of the code base. I am happy that one is finally paying\n>   off! ;)\n\n:D\n\n> So I think the aim of your series is quite reasonable. The\n> implementation mostly looks good, but I have a few comments which I'll\n> leave on the individual patches.\n\nExcellent. Thanks!\n\n-- \nKristoffer but any Christopher-variation is fine\n"},{"id":"504358","messageId":"20241007204447.GB603285@coredump.intra.peff.net","threadId":"62274","inReplyTo":"b88c2430f88b641d69e5f161d3a18cce113a81c9.1728331771.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-07T20:44:47Z","receivedAt":"2024-10-07T20:44:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 10:15:18PM +0200, Kristoffer Haugsbakk wrote:\n\n> `HEAD` is an invalid branch name.[1]  But the `@` synonym is allowed.\n> This is just as inconvenient since commands like `git checkout @` will,\n> quite sensibly, do `git checkout HEAD` instead of checking out that\n> branch; in turn there is no practical reason to use this as a branch\n> name since you cannot even check out the branch itself (only check out\n> the commit which `refs/heads/@` points to).\n> \n> † 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD},\n>     2017-11-14)\n\nThere's a bit of subtlety here which makes the term \"invalid\" somewhat\nvague. The refname \"refs/heads/HEAD\" is allowed by plumbing, as we try\nto maintain backwards compatibility there. So the current prohibition is\njust within the porcelain tools: we won't allow \"git branch HEAD\"\nbecause it's an easy mistake to make, even though you could still create\nit with \"git update-ref\".\n\nAnd naturally we'd want the same rules for \"refs/heads/@\". I think it\nmight be worth adding \"...in plumbing\" to the end of the subject, and/or\ncalling out this distinction in the text.\n\nIt might also be worth mentioning some of the reasoning about the test\nyou put in your cover letter, since that content is not otherwise in the\nGit history. I'm thinking something as simple as:\n\n  Note that we are reversing the result of the test in t3204. But as the\n  comment there notes, it was added only to check that \"@\" was not\n  expanded. Asserting that the branch \"@\" can be created was only\n  testing what happened to occur, and not an endorsement of the\n  behavior.\n\n> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\n> index 594e3e43e12..7dcd1308f8c 100755\n> --- a/t/t3204-branch-name-interpretation.sh\n> +++ b/t/t3204-branch-name-interpretation.sh\n> @@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' '\n>  \texpect_branch refs/heads/origin/previous two\n>  '\n>  \n> -# The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n> -# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n> -# sane thing, but it _is_ technically allowed for now. If we disallow it, these\n> -# can be switched to test_must_fail.\n> -test_expect_success 'create branch named \"@\"' '\n> -\tgit branch -f @ one &&\n> -\texpect_branch refs/heads/@ one\n> +test_expect_success 'disallow branch named \"@\"' '\n> +\ttest_must_fail git branch -f @ one\n>  '\n>  \n>  test_expect_success 'delete branch named \"@\"' '\n\nI was a little surprised that the \"delete branch named @\" test\nimmediately below did not need similar treatment. But I guess all of the\n\"check refname\" code in git-branch is split between those two cases,\nbecause we want to allow cleanup of broken names created through other\nmeans.\n\nSo I think the patch is doing the right thing. But it might be worth\nmentioning this distinction in the commit message.\n\n-Peff\n"},{"id":"504359","messageId":"20241007204735.GC603285@coredump.intra.peff.net","threadId":"62274","inReplyTo":"8262b81141bbd36cd7a17e6abe5eb6bb688290f3.1728331771.git.code@khaugsbakk.name","subject":"Re: [PATCH 3/3] t1402: exercise disallowed branch names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-07T20:47:35Z","receivedAt":"2024-10-07T20:47:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 10:15:19PM +0200, Kristoffer Haugsbakk wrote:\n\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n\nCan we give some more details here? These are cases that already passed\nbefore your series, right? At least I'd expect the \"HEAD\" one to do so,\nbut I guess the \"@\" fix is new?\n\nI don't need a lengthy explanation, just trying to understand how this\nchange fits into your series. It might be simpler if this came first,\nadding new tests for existing behavior that was not covered, and then\nyour 2/3 patch just adds the \"@\" line in the same spot.\n\n-Peff\n"},{"id":"504360","messageId":"9a64a58b-3d08-44b4-96a5-9031863de4f1@app.fastmail.com","threadId":"62274","inReplyTo":"20241007204447.GB603285@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-07T20:56:36Z","receivedAt":"2024-10-07T20:56:57Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Oct 7, 2024, at 22:44, Jeff King wrote:\n> On Mon, Oct 07, 2024 at 10:15:18PM +0200, Kristoffer Haugsbakk wrote:\n>\n>> `HEAD` is an invalid branch name.[1]  But the `@` synonym is allowed.\n>> This is just as inconvenient since commands like `git checkout @` will,\n>> quite sensibly, do `git checkout HEAD` instead of checking out that\n>> branch; in turn there is no practical reason to use this as a branch\n>> name since you cannot even check out the branch itself (only check out\n>> the commit which `refs/heads/@` points to).\n>>\n>> † 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD},\n>>     2017-11-14)\n>\n> There's a bit of subtlety here which makes the term \"invalid\" somewhat\n> vague. The refname \"refs/heads/HEAD\" is allowed by plumbing, as we try\n> to maintain backwards compatibility there. So the current prohibition is\n> just within the porcelain tools: we won't allow \"git branch HEAD\"\n> because it's an easy mistake to make, even though you could still create\n> it with \"git update-ref\".\n\nGot it.  Creating this one (or something like `refs/heads/HEAD` for that\nmatter) is allowed by the plumbing tools.  But the porcelain ones are\nblocked.\n\nAlso the plumbing query `git check-ref-format --branch @` now returns\nfalse.  Since it has to harmonize with what the branch creation\nporcelain can do.\n\n> And naturally we'd want the same rules for \"refs/heads/@\". I think it\n> might be worth adding \"...in plumbing\" to the end of the subject, and/or\n> calling out this distinction in the text.\n\nDid you mean something like “disallow in porcelain”?\n\n>\n> It might also be worth mentioning some of the reasoning about the test\n> you put in your cover letter, since that content is not otherwise in the\n> Git history. I'm thinking something as simple as:\n>\n>   Note that we are reversing the result of the test in t3204. But as the\n>   comment there notes, it was added only to check that \"@\" was not\n>   expanded. Asserting that the branch \"@\" can be created was only\n>   testing what happened to occur, and not an endorsement of the\n>   behavior.\n\nSure.  I didn’t even mention that removal since the comment stood so\nwell on its own (i.e. explained its own presence).  ;)\n\n>\n>> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\n>> index 594e3e43e12..7dcd1308f8c 100755\n>> --- a/t/t3204-branch-name-interpretation.sh\n>> +++ b/t/t3204-branch-name-interpretation.sh\n>> @@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' '\n>>  \texpect_branch refs/heads/origin/previous two\n>>  '\n>>\n>> -# The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n>> -# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n>> -# sane thing, but it _is_ technically allowed for now. If we disallow it, these\n>> -# can be switched to test_must_fail.\n>> -test_expect_success 'create branch named \"@\"' '\n>> -\tgit branch -f @ one &&\n>> -\texpect_branch refs/heads/@ one\n>> +test_expect_success 'disallow branch named \"@\"' '\n>> +\ttest_must_fail git branch -f @ one\n>>  '\n>>\n>>  test_expect_success 'delete branch named \"@\"' '\n>\n> I was a little surprised that the \"delete branch named @\" test\n> immediately below did not need similar treatment. But I guess all of the\n> \"check refname\" code in git-branch is split between those two cases,\n> because we want to allow cleanup of broken names created through other\n> means.\n>\n> So I think the patch is doing the right thing. But it might be worth\n> mentioning this distinction in the commit message.\n>\n> -Peff\n\nYeah, I’ll do that.\n\n-- \nKristoffer but any Christopher-variation is fine\n\n"},{"id":"504367","messageId":"xmqqy12z7eti.fsf@gitster.g","threadId":"62274","inReplyTo":"b88c2430f88b641d69e5f161d3a18cce113a81c9.1728331771.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-07T22:01:29Z","receivedAt":"2024-10-07T22:01:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> `HEAD` is an invalid branch name.[1]  But the `@` synonym is allowed.\n> This is just as inconvenient since commands like `git checkout @` will,\n> quite sensibly, do `git checkout HEAD` instead of checking out that\n> branch; in turn there is no practical reason to use this as a branch\n> name since you cannot even check out the branch itself (only check out\n> the commit which `refs/heads/@` points to).\n\nI am not sure this is sensible at all, after all these years.\n\nI suspect that it is much more productive to deprecate and remove\n\"@\" that is a built-in synomym for HEAD (but \"refs/remotes/origin/@\"\ndoes not act as a synonym for \"refs/remotes/origin/HEAD\").  Having\ntwo ways to call the same thing merely adds to confusion in this\ncase, unlike \"HEAD\" referring to 'master' (when 'master' is checked\nout), which is also to have two ways to call the same thing, but\nadds a true convenience.\n\nThose who really want to use @ can do something like\n\n\t$ echo \"ref: HEAD\" >.git/@\n\nor something, perhaps.\n"},{"id":"504392","messageId":"20241008065255.GA676291@coredump.intra.peff.net","threadId":"62274","inReplyTo":"9a64a58b-3d08-44b4-96a5-9031863de4f1@app.fastmail.com","subject":"Re: [PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-08T06:52:55Z","receivedAt":"2024-10-08T06:53:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 10:56:36PM +0200, Kristoffer Haugsbakk wrote:\n\n> > There's a bit of subtlety here which makes the term \"invalid\" somewhat\n> > vague. The refname \"refs/heads/HEAD\" is allowed by plumbing, as we try\n> > to maintain backwards compatibility there. So the current prohibition is\n> > just within the porcelain tools: we won't allow \"git branch HEAD\"\n> > because it's an easy mistake to make, even though you could still create\n> > it with \"git update-ref\".\n> \n> Got it.  Creating this one (or something like `refs/heads/HEAD` for that\n> matter) is allowed by the plumbing tools.  But the porcelain ones are\n> blocked.\n> \n> Also the plumbing query `git check-ref-format --branch @` now returns\n> false.  Since it has to harmonize with what the branch creation\n> porcelain can do.\n\nYeah, good point. I was thinking the existing test was purely about the\ngit-branch porcelain, but \"check-ref-format --branch\" follows the same\nrules.\n\n> > And naturally we'd want the same rules for \"refs/heads/@\". I think it\n> > might be worth adding \"...in plumbing\" to the end of the subject, and/or\n> > calling out this distinction in the text.\n> \n> Did you mean something like “disallow in porcelain”?\n\nOops, yes, I had it backwards. Good catch.\n\n-Peff\n"},{"id":"504393","messageId":"20241008065451.GB676291@coredump.intra.peff.net","threadId":"62274","inReplyTo":"xmqqy12z7eti.fsf@gitster.g","subject":"Re: [PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-08T06:54:51Z","receivedAt":"2024-10-08T06:54:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 03:01:29PM -0700, Junio C Hamano wrote:\n\n> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n> \n> > `HEAD` is an invalid branch name.[1]  But the `@` synonym is allowed.\n> > This is just as inconvenient since commands like `git checkout @` will,\n> > quite sensibly, do `git checkout HEAD` instead of checking out that\n> > branch; in turn there is no practical reason to use this as a branch\n> > name since you cannot even check out the branch itself (only check out\n> > the commit which `refs/heads/@` points to).\n> \n> I am not sure this is sensible at all, after all these years.\n> \n> I suspect that it is much more productive to deprecate and remove\n> \"@\" that is a built-in synomym for HEAD (but \"refs/remotes/origin/@\"\n> does not act as a synonym for \"refs/remotes/origin/HEAD\").  Having\n> two ways to call the same thing merely adds to confusion in this\n> case, unlike \"HEAD\" referring to 'master' (when 'master' is checked\n> out), which is also to have two ways to call the same thing, but\n> adds a true convenience.\n\nI do not use \"@\" myself, but I feel like when removing it has been\nbrought up before, it had its defenders. So I do not personally object,\nbut I think you'd have to post a patch and see who screams. :)\n\n> Those who really want to use @ can do something like\n> \n> \t$ echo \"ref: HEAD\" >.git/@\n> \n> or something, perhaps.\n\nI'm not sure if we'll allow that long-term. It does not match the\nroot-ref syntax, so I'm not sure if it would pass check_ref_format().\n(Sorry, I had a series a few months ago cleaning up some edges cases\nthere, but I haven't gotten back to it yet).\n\n-Peff\n"},{"id":"504420","messageId":"ZwUxdz_HobRGF9yq@ArchLinux","threadId":"62274","inReplyTo":"cover.1728331771.git.code@khaugsbakk.name","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-08T13:19:51Z","receivedAt":"2024-10-08T13:19:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:\n\n[snip]\n\n>   §2 Disallow `HEAD` as a branch name\n> \n> This was done later in 2017:\n> \n> https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/\n> \n>   §2 `refs/heads/@` is apparently disallowed by git-refs(1)\n> \n> See `t/t1508-at-combinations.sh`:\n> \n> ```\n> error: refs/heads/@: badRefName: invalid refname format\n> ```\n> \n\nIt's true that using \"git refs verify\" will report \"refs/heads/@\" is a\nbad refname.\n\nFrom the man page of the \"git-check-ref-format(1)\", it is clear that\n\n    9. They cannot be the single character @.\n\nBecause I am interesting in this patch which is highly relevant with my\nrecent work, so I try somethings here and find some interesting results\nas below shows.\n\n    $ git check-ref-format refs/heads/@\n    $ echo $? # will be 0\n    # git check-ref-format --allow-onelevel @\n    # echo $? # will be 1\n\nThe reason why \"git refs verify\" will report this error is that in the\ncode implementation, I have to iterate every file in the filesystem. So\nit's convenient for me to do the following:\n\n    if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {\n        ret = fsck_report(...);\n    }\n\nBecause I specify \"REFNAME_ALLOW_ONELEVEL\" here, so it will follow the\n\"git check-ref-format --allow-onelevel\" command thus reporting an error\nto the user.\n\nI am curious why \"git check-ref-format refs/heads/@\" will succeed, so I\ntry to use \"git symbolic-ref\" and \"git update-ref\" to verify to test the\nbehavior.\n\n    $ git symbolic-ref refs/heads/@ refs/heads/master\n    error: cannot lock ref 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference broken\n    $ git update-ref refs/heads/@ refs/heads/master\n    fatal: update_ref failed for ref 'refs/heads/@': cannot lock ref 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference broken\n\nSo, we are not consistent here. I guess the reason why \"git\ncheck-ref-format refs/heads/@\" will succeed is that we allow user create\nthis kind of branch.\n\nIf we decide to not allow user to create such refs. We should also\nchange the behavior of the \"check_refname_format\" function. (I am not\nfamiliar with the internal implementation, this is my guess)\n\nThanks,\nJialuo\n"},{"id":"504444","messageId":"3af78a3c-afb9-4ce7-aea0-a5bbddd4f34a@app.fastmail.com","threadId":"62274","inReplyTo":"ZwUxdz_HobRGF9yq@ArchLinux","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-08T14:19:10Z","receivedAt":"2024-10-08T14:19:33Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Oct 8, 2024, at 15:19, shejialuo wrote:\n> On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:\n>\n> [snip]\n>\n>>   §2 Disallow `HEAD` as a branch name\n>> \n>> This was done later in 2017:\n>> \n>> https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/\n>> \n>>   §2 `refs/heads/@` is apparently disallowed by git-refs(1)\n>> \n>> See `t/t1508-at-combinations.sh`:\n>> \n>> ```\n>> error: refs/heads/@: badRefName: invalid refname format\n>> ```\n>> \n>\n> It's true that using \"git refs verify\" will report \"refs/heads/@\" is a\n> bad refname.\n>\n> From the man page of the \"git-check-ref-format(1)\", it is clear that\n>\n>     9. They cannot be the single character @.\n>\n> Because I am interesting in this patch which is highly relevant with my\n> recent work, so I try somethings here and find some interesting results\n> as below shows.\n>\n>     $ git check-ref-format refs/heads/@\n>     $ echo $? # will be 0\n>     # git check-ref-format --allow-onelevel @\n>     # echo $? # will be 1\n>\n> The reason why \"git refs verify\" will report this error is that in the\n> code implementation, I have to iterate every file in the filesystem. So\n> it's convenient for me to do the following:\n>\n>     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {\n>         ret = fsck_report(...);\n>     }\n>\n> Because I specify \"REFNAME_ALLOW_ONELEVEL\" here, so it will follow the\n> \"git check-ref-format --allow-onelevel\" command thus reporting an error\n> to the user.\n>\n> I am curious why \"git check-ref-format refs/heads/@\" will succeed, so I\n> try to use \"git symbolic-ref\" and \"git update-ref\" to verify to test the\n> behavior.\n>\n>     $ git symbolic-ref refs/heads/@ refs/heads/master\n>     error: cannot lock ref 'refs/heads/@': unable to resolve reference \n> 'refs/heads/@': reference broken\n>     $ git update-ref refs/heads/@ refs/heads/master\n>     fatal: update_ref failed for ref 'refs/heads/@': cannot lock ref \n> 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference \n> broken\n>\n> So, we are not consistent here. I guess the reason why \"git\n> check-ref-format refs/heads/@\" will succeed is that we allow user create\n> this kind of branch.\n>\n> If we decide to not allow user to create such refs. We should also\n> change the behavior of the \"check_refname_format\" function. (I am not\n> familiar with the internal implementation, this is my guess)\n>\n> Thanks,\n> Jialuo\n\nThanks for the careful analysis.\n"},{"id":"504465","messageId":"xmqqjzei1mtb.fsf@gitster.g","threadId":"62274","inReplyTo":"ZwUxdz_HobRGF9yq@ArchLinux","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T18:17:36Z","receivedAt":"2024-10-08T18:17:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo <shejialuo@gmail.com> writes:\n\n> The reason why \"git refs verify\" will report this error is that in the\n> code implementation, I have to iterate every file in the filesystem. So\n> it's convenient for me to do the following:\n>\n>     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {\n>         ret = fsck_report(...);\n>     }\n\nIt may be convenient, but I think it is wrong.  HEAD may be allowed\nat the top, but refs/heads/HEAD is not, and checking only the single\nlevel name as you descend into .git/refs directory hierarchy and\nfind files would not be a good design to begin with (and it would\nnot work if your backend is reftable).\n"},{"id":"504484","messageId":"b1f2b664-34ea-4d9d-9bf5-fb6632b265f5@gmail.com","threadId":"62274","inReplyTo":"20241007204447.GB603285@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] object-name: don't allow @ as a branch name","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-10-08T20:37:13Z","receivedAt":"2024-10-08T20:37:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Oct 07, 2024 at 04:44:47PM -0400, Jeff King wrote:\n\n> The refname \"refs/heads/HEAD\" is allowed by plumbing, as we try\n> to maintain backwards compatibility there. So the current prohibition is\n> just within the porcelain tools: we won't allow \"git branch HEAD\"\n> because it's an easy mistake to make, even though you could still create\n> it with \"git update-ref\".\n\nAh, your comment reminded me that something similar happened recently\nnear me:\n\n   $ git push origin some-ref:HEAD\n\nIt caused a small disaster, although it was quickly fixed.\n\nThe backwards compatibility you mentioned, which can also be understood\nas a non-limitation in this aspect, is worth maintaining.\n\nI haven't had time to investigate why git-push doesn't warn (or stop)\nthe user when attempting that, but perhaps there's a small crack we\nwant to fix.  Or maybe it's something we actually want to allow...\n"},{"id":"504548","messageId":"ZwZwdvlQv9AMRJpI@ArchLinux","threadId":"62274","inReplyTo":"xmqqjzei1mtb.fsf@gitster.g","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-09T12:00:54Z","receivedAt":"2024-10-09T12:00:51Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Oct 08, 2024 at 11:17:36AM -0700, Junio C Hamano wrote:\n> shejialuo <shejialuo@gmail.com> writes:\n> \n> > The reason why \"git refs verify\" will report this error is that in the\n> > code implementation, I have to iterate every file in the filesystem. So\n> > it's convenient for me to do the following:\n> >\n> >     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {\n> >         ret = fsck_report(...);\n> >     }\n> \n> It may be convenient, but I think it is wrong.  HEAD may be allowed\n> at the top, but refs/heads/HEAD is not, and checking only the single\n> level name as you descend into .git/refs directory hierarchy and\n> find files would not be a good design to begin with (and it would\n> not work if your backend is reftable).\n\nIn my current work, I will introduce worktree check here and then I will\nuse the fullname to check and this will not be a problem. Thanks for\nreminding here.\n"},{"id":"505444","messageId":"ZxJu13wpSiJ_kDdB@ArchLinux","threadId":"62274","inReplyTo":"3af78a3c-afb9-4ce7-aea0-a5bbddd4f34a@app.fastmail.com","subject":"Re: [PATCH 0/3] object-name: don't allow @ as a branch name","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-18T14:21:11Z","receivedAt":"2024-10-18T14:21:11Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Oct 08, 2024 at 04:19:10PM +0200, Kristoffer Haugsbakk wrote:\n> On Tue, Oct 8, 2024, at 15:19, shejialuo wrote:\n> > On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:\n> >\n> > [snip]\n> >\n> >>   §2 Disallow `HEAD` as a branch name\n> >> \n> >> This was done later in 2017:\n> >> \n> >> https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/\n> >> \n> >>   §2 `refs/heads/@` is apparently disallowed by git-refs(1)\n> >> \n> >> See `t/t1508-at-combinations.sh`:\n> >> \n> >> ```\n> >> error: refs/heads/@: badRefName: invalid refname format\n> >> ```\n> >> \n> >\n> > It's true that using \"git refs verify\" will report \"refs/heads/@\" is a\n> > bad refname.\n> >\n> > From the man page of the \"git-check-ref-format(1)\", it is clear that\n> >\n> >     9. They cannot be the single character @.\n> >\n> > Because I am interesting in this patch which is highly relevant with my\n> > recent work, so I try somethings here and find some interesting results\n> > as below shows.\n> >\n> >     $ git check-ref-format refs/heads/@\n> >     $ echo $? # will be 0\n> >     # git check-ref-format --allow-onelevel @\n> >     # echo $? # will be 1\n> >\n> > The reason why \"git refs verify\" will report this error is that in the\n> > code implementation, I have to iterate every file in the filesystem. So\n> > it's convenient for me to do the following:\n> >\n> >     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {\n> >         ret = fsck_report(...);\n> >     }\n> >\n> > Because I specify \"REFNAME_ALLOW_ONELEVEL\" here, so it will follow the\n> > \"git check-ref-format --allow-onelevel\" command thus reporting an error\n> > to the user.\n> >\n> > I am curious why \"git check-ref-format refs/heads/@\" will succeed, so I\n> > try to use \"git symbolic-ref\" and \"git update-ref\" to verify to test the\n> > behavior.\n> >\n> >     $ git symbolic-ref refs/heads/@ refs/heads/master\n> >     error: cannot lock ref 'refs/heads/@': unable to resolve reference \n> > 'refs/heads/@': reference broken\n> >     $ git update-ref refs/heads/@ refs/heads/master\n> >     fatal: update_ref failed for ref 'refs/heads/@': cannot lock ref \n> > 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference \n> > broken\n> >\n> > So, we are not consistent here. I guess the reason why \"git\n> > check-ref-format refs/heads/@\" will succeed is that we allow user create\n> > this kind of branch.\n> >\n> > If we decide to not allow user to create such refs. We should also\n> > change the behavior of the \"check_refname_format\" function. (I am not\n> > familiar with the internal implementation, this is my guess)\n> >\n> > Thanks,\n> > Jialuo\n> \n> Thanks for the careful analysis.\n\nPlease ignore the above analysis which is not true. (Today I am writing\ncode for my work). Currently, we truly allow \"refs/heads/@\" as the refname.\nAnd also for \"git check-ref-format\", \"git update-ref\" and \"git symbolic-ref\"\n\nWhen I did the experiments above, I forgot to clear the state which\nmakes the \"git update-ref\" and \"git symbolic-ref\" fail. So, there are\nsome faults in \"git refs verify\". I will fix in my current work.\n\nSo, if we decide to not allow \"refs/heads/@\", we should also update \"git\ncheck-ref-format\", \"git update-ref\" and \"git symbolic-ref\" to align with\nthis behavior.\n\nThanks,\nJialuo\n"}]}