On Mon, Oct 07, 2024 at 10:15:18PM +0200, Kristoffer Haugsbakk wrote:
> `HEAD` is an invalid branch name.[1] But the `@` synonym is allowed.
> This is just as inconvenient since commands like `git checkout @` will,
> quite sensibly, do `git checkout HEAD` instead of checking out that
> branch; in turn there is no practical reason to use this as a branch
> name since you cannot even check out the branch itself (only check out
> the commit which `refs/heads/@` points to).
>
> † 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD},
> 2017-11-14)
There's a bit of subtlety here which makes the term "invalid" somewhat
vague. The refname "refs/heads/HEAD" is allowed by plumbing, as we try
to maintain backwards compatibility there. So the current prohibition is
just within the porcelain tools: we won't allow "git branch HEAD"
because it's an easy mistake to make, even though you could still create
it with "git update-ref".
And naturally we'd want the same rules for "refs/heads/@". I think it
might be worth adding "...in plumbing" to the end of the subject, and/or
calling out this distinction in the text.
It might also be worth mentioning some of the reasoning about the test
you put in your cover letter, since that content is not otherwise in the
Git history. I'm thinking something as simple as:
Note that we are reversing the result of the test in t3204. But as the
comment there notes, it was added only to check that "@" was not
expanded. Asserting that the branch "@" can be created was only
testing what happened to occur, and not an endorsement of the
behavior.
> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
> index 594e3e43e12..7dcd1308f8c 100755
> --- a/t/t3204-branch-name-interpretation.sh
> +++ b/t/t3204-branch-name-interpretation.sh
> @@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' '
> expect_branch refs/heads/origin/previous two
> '
>
> -# The thing we are testing here is that "@" is the real branch refs/heads/@,
> -# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
> -# sane thing, but it _is_ technically allowed for now. If we disallow it, these
> -# can be switched to test_must_fail.
> -test_expect_success 'create branch named "@"' '
> - git branch -f @ one &&
> - expect_branch refs/heads/@ one
> +test_expect_success 'disallow branch named "@"' '
> + test_must_fail git branch -f @ one
> '
>
> test_expect_success 'delete branch named "@"' '
I was a little surprised that the "delete branch named @" test
immediately below did not need similar treatment. But I guess all of the
"check refname" code in git-branch is split between those two cases,
because we want to allow cleanup of broken names created through other
means.
So I think the patch is doing the right thing. But it might be worth
mentioning this distinction in the commit message.
-Peff
