Re: [PATCH 2/3] object-name: don't allow @ as a branch name

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Oct 7, 2024, 20:56 UTC
Message-ID: <9a64a58b-3d08-44b4-96a5-9031863de4f1@app.fastmail.com>
In-Reply-To: <20241007204447.GB603285@coredump.intra.peff.net>

On Mon, Oct 7, 2024, at 22:44, Jeff King wrote:

Show 18 quoted lines

> 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”?

Show 10 quoted lines

>
> 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). ;)

Show 32 quoted lines

>
>> 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