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
