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