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

["Kristoffer Haugsbakk" <code@khaugsbakk.name>]

On Mon, Oct 7, 2024, at 22:37, Jeff King wrote:
> On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
>
>> This has come up before.  There even is a test which guards the current
>> behavior (allow `@` as a branch name) with the comment:[1]
>> 
>> ```
>> # 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.
>> ```
>> 
>> There was no reply to this change in neither the first[2] nor second
>> version.
>> 
>> That series points back to a bug report thread[3] which is about
>> expanding `@` to a branch named `HEAD`.
>
> Yeah. The series you found was about not expanding "@" in the wrong
> contexts. So the test made sure we did not do so, but of course it was
> then left asserting the weird behavior that was left over. So this:
>
>> So that was tangential to the bug fix (`HEAD` as a branch name was not
>> disallowed in the patch series that resulted from this bug).
>
> is accurate. Those tests are no reason we should not consider
> disallowing "@" as a branch name.
>
>   As an aside, I have a couple times left these sort of "do not take
>   this test as an endorsement of the behavior" comments when working in
>   crufty corners of the code base. I am happy that one is finally paying
>   off! ;)

:D

> So I think the aim of your series is quite reasonable. The
> implementation mostly looks good, but I have a few comments which I'll
> leave on the individual patches.

Excellent. Thanks!

-- 
Kristoffer but any Christopher-variation is fine