Re: [PATCH 0/3] object-name: don't allow @ as a branch name
On Mon, Oct 7, 2024, at 22:37, Jeff King wrote:
Show 32 quoted lines
> 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! ;)
> 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.
--
Kristoffer but any Christopher-variation is fine