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

From: Jeff King <peff@peff.net>
Date: Oct 7, 2024, 20:37 UTC
Message-ID: <20241007203720.GA603285@coredump.intra.peff.net>
In-Reply-To: <cover.1728331771.git.code@khaugsbakk.name>

On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:

Show 15 quoted lines

> 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