On Mon, Oct 7, 2024, at 22:44, Jeff King wrote:
> 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".
Got it. Creating this one (or something like `refs/heads/HEAD` for that
matter) is allowed by the plumbing tools. But the porcelain ones are
blocked.
Also the plumbing query `git check-ref-format --branch @` now returns
false. Since it has to harmonize with what the branch creation
porcelain can do.
> 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.
Did you mean something like “disallow in porcelain”?
>
> 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.
Sure. I didn’t even mention that removal since the comment stood so
well on its own (i.e. explained its own presence). ;)
>
>> 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
Yeah, I’ll do that.
--
Kristoffer but any Christopher-variation is fine
