| Message ID | b88c2430f88b641d69e5f161d3a18cce113a81c9.1728331771.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | object-name: don't allow @ as a branch name | expand |
On Mon, Oct 07, 2024 at 10:15:18PM +0200, Kristoffer Haugsbakk wrote: > `HEAD` is an invalid branch name.[1] But the `@` synonym is allowed. > This is just as inconvenient since commands like `git checkout @` will, > quite sensibly, do `git checkout HEAD` instead of checking out that > branch; in turn there is no practical reason to use this as a branch > name since you cannot even check out the branch itself (only check out > the commit which `refs/heads/@` points to). > > † 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD}, > 2017-11-14) There's a bit of subtlety here which makes the term "invalid" somewhat vague. The refname "refs/heads/HEAD" is allowed by plumbing, as we try to maintain backwards compatibility there. So the current prohibition is just within the porcelain tools: we won't allow "git branch HEAD" because it's an easy mistake to make, even though you could still create it with "git update-ref". And naturally we'd want the same rules for "refs/heads/@". I think it might be worth adding "...in plumbing" to the end of the subject, and/or calling out this distinction in the text. It might also be worth mentioning some of the reasoning about the test you put in your cover letter, since that content is not otherwise in the Git history. I'm thinking something as simple as: Note that we are reversing the result of the test in t3204. But as the comment there notes, it was added only to check that "@" was not expanded. Asserting that the branch "@" can be created was only testing what happened to occur, and not an endorsement of the behavior. > diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh > index 594e3e43e12..7dcd1308f8c 100755 > --- a/t/t3204-branch-name-interpretation.sh > +++ b/t/t3204-branch-name-interpretation.sh > @@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' ' > expect_branch refs/heads/origin/previous two > ' > > -# 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. > -test_expect_success 'create branch named "@"' ' > - git branch -f @ one && > - expect_branch refs/heads/@ one > +test_expect_success 'disallow branch named "@"' ' > + test_must_fail git branch -f @ one > ' > > test_expect_success 'delete branch named "@"' ' I was a little surprised that the "delete branch named @" test immediately below did not need similar treatment. But I guess all of the "check refname" code in git-branch is split between those two cases, because we want to allow cleanup of broken names created through other means. So I think the patch is doing the right thing. But it might be worth mentioning this distinction in the commit message. -Peff
On Mon, Oct 7, 2024, at 22:44, Jeff King wrote: > On Mon, Oct 07, 2024 at 10:15:18PM +0200, Kristoffer Haugsbakk wrote: > >> `HEAD` is an invalid branch name.[1] But the `@` synonym is allowed. >> This is just as inconvenient since commands like `git checkout @` will, >> quite sensibly, do `git checkout HEAD` instead of checking out that >> branch; in turn there is no practical reason to use this as a branch >> name since you cannot even check out the branch itself (only check out >> the commit which `refs/heads/@` points to). >> >> † 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD}, >> 2017-11-14) > > There's a bit of subtlety here which makes the term "invalid" somewhat > vague. The refname "refs/heads/HEAD" is allowed by plumbing, as we try > to maintain backwards compatibility there. So the current prohibition is > just within the porcelain tools: we won't allow "git branch HEAD" > because it's an easy mistake to make, even though you could still create > it with "git update-ref". Got it. Creating this one (or something like `refs/heads/HEAD` for that matter) is allowed by the plumbing tools. But the porcelain ones are blocked. Also the plumbing query `git check-ref-format --branch @` now returns false. Since it has to harmonize with what the branch creation porcelain can do. > And naturally we'd want the same rules for "refs/heads/@". I think it > might be worth adding "...in plumbing" to the end of the subject, and/or > calling out this distinction in the text. Did you mean something like “disallow in porcelain”? > > It might also be worth mentioning some of the reasoning about the test > you put in your cover letter, since that content is not otherwise in the > Git history. I'm thinking something as simple as: > > Note that we are reversing the result of the test in t3204. But as the > comment there notes, it was added only to check that "@" was not > expanded. Asserting that the branch "@" can be created was only > testing what happened to occur, and not an endorsement of the > behavior. Sure. I didn’t even mention that removal since the comment stood so well on its own (i.e. explained its own presence). ;) > >> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh >> index 594e3e43e12..7dcd1308f8c 100755 >> --- a/t/t3204-branch-name-interpretation.sh >> +++ b/t/t3204-branch-name-interpretation.sh >> @@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' ' >> expect_branch refs/heads/origin/previous two >> ' >> >> -# 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. >> -test_expect_success 'create branch named "@"' ' >> - git branch -f @ one && >> - expect_branch refs/heads/@ one >> +test_expect_success 'disallow branch named "@"' ' >> + test_must_fail git branch -f @ one >> ' >> >> test_expect_success 'delete branch named "@"' ' > > I was a little surprised that the "delete branch named @" test > immediately below did not need similar treatment. But I guess all of the > "check refname" code in git-branch is split between those two cases, > because we want to allow cleanup of broken names created through other > means. > > So I think the patch is doing the right thing. But it might be worth > mentioning this distinction in the commit message. > > -Peff Yeah, I’ll do that.
Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > `HEAD` is an invalid branch name.[1] But the `@` synonym is allowed. > This is just as inconvenient since commands like `git checkout @` will, > quite sensibly, do `git checkout HEAD` instead of checking out that > branch; in turn there is no practical reason to use this as a branch > name since you cannot even check out the branch itself (only check out > the commit which `refs/heads/@` points to). I am not sure this is sensible at all, after all these years. I suspect that it is much more productive to deprecate and remove "@" that is a built-in synomym for HEAD (but "refs/remotes/origin/@" does not act as a synonym for "refs/remotes/origin/HEAD"). Having two ways to call the same thing merely adds to confusion in this case, unlike "HEAD" referring to 'master' (when 'master' is checked out), which is also to have two ways to call the same thing, but adds a true convenience. Those who really want to use @ can do something like $ echo "ref: HEAD" >.git/@ or something, perhaps.
On Mon, Oct 07, 2024 at 10:56:36PM +0200, Kristoffer Haugsbakk wrote: > > There's a bit of subtlety here which makes the term "invalid" somewhat > > vague. The refname "refs/heads/HEAD" is allowed by plumbing, as we try > > to maintain backwards compatibility there. So the current prohibition is > > just within the porcelain tools: we won't allow "git branch HEAD" > > because it's an easy mistake to make, even though you could still create > > it with "git update-ref". > > Got it. Creating this one (or something like `refs/heads/HEAD` for that > matter) is allowed by the plumbing tools. But the porcelain ones are > blocked. > > Also the plumbing query `git check-ref-format --branch @` now returns > false. Since it has to harmonize with what the branch creation > porcelain can do. Yeah, good point. I was thinking the existing test was purely about the git-branch porcelain, but "check-ref-format --branch" follows the same rules. > > And naturally we'd want the same rules for "refs/heads/@". I think it > > might be worth adding "...in plumbing" to the end of the subject, and/or > > calling out this distinction in the text. > > Did you mean something like “disallow in porcelain”? Oops, yes, I had it backwards. Good catch. -Peff
On Mon, Oct 07, 2024 at 03:01:29PM -0700, Junio C Hamano wrote: > Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > > > `HEAD` is an invalid branch name.[1] But the `@` synonym is allowed. > > This is just as inconvenient since commands like `git checkout @` will, > > quite sensibly, do `git checkout HEAD` instead of checking out that > > branch; in turn there is no practical reason to use this as a branch > > name since you cannot even check out the branch itself (only check out > > the commit which `refs/heads/@` points to). > > I am not sure this is sensible at all, after all these years. > > I suspect that it is much more productive to deprecate and remove > "@" that is a built-in synomym for HEAD (but "refs/remotes/origin/@" > does not act as a synonym for "refs/remotes/origin/HEAD"). Having > two ways to call the same thing merely adds to confusion in this > case, unlike "HEAD" referring to 'master' (when 'master' is checked > out), which is also to have two ways to call the same thing, but > adds a true convenience. I do not use "@" myself, but I feel like when removing it has been brought up before, it had its defenders. So I do not personally object, but I think you'd have to post a patch and see who screams. :) > Those who really want to use @ can do something like > > $ echo "ref: HEAD" >.git/@ > > or something, perhaps. I'm not sure if we'll allow that long-term. It does not match the root-ref syntax, so I'm not sure if it would pass check_ref_format(). (Sorry, I had a series a few months ago cleaning up some edges cases there, but I haven't gotten back to it yet). -Peff
On Mon, Oct 07, 2024 at 04:44:47PM -0400, Jeff King wrote: > The refname "refs/heads/HEAD" is allowed by plumbing, as we try > to maintain backwards compatibility there. So the current prohibition is > just within the porcelain tools: we won't allow "git branch HEAD" > because it's an easy mistake to make, even though you could still create > it with "git update-ref". Ah, your comment reminded me that something similar happened recently near me: $ git push origin some-ref:HEAD It caused a small disaster, although it was quickly fixed. The backwards compatibility you mentioned, which can also be understood as a non-limitation in this aspect, is worth maintaining. I haven't had time to investigate why git-push doesn't warn (or stop) the user when attempting that, but perhaps there's a small crack we want to fix. Or maybe it's something we actually want to allow...
diff --git a/object-name.c b/object-name.c index 42e3ba4a77a..56b288ff4c3 100644 --- a/object-name.c +++ b/object-name.c @@ -1763,7 +1763,8 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name) strbuf_splice(sb, 0, 0, "refs/heads/", 11); if (*name == '-' || - !strcmp(sb->buf, "refs/heads/HEAD")) + !strcmp(sb->buf, "refs/heads/HEAD") || + !strcmp(sb->buf, "refs/heads/@")) return -1; return check_refname_format(sb->buf, 0); diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh index 594e3e43e12..7dcd1308f8c 100755 --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -119,13 +119,8 @@ test_expect_success 'disallow deleting remote branch via @{-1}' ' expect_branch refs/heads/origin/previous two ' -# 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. -test_expect_success 'create branch named "@"' ' - git branch -f @ one && - expect_branch refs/heads/@ one +test_expect_success 'disallow branch named "@"' ' + test_must_fail git branch -f @ one ' test_expect_success 'delete branch named "@"' '
`HEAD` is an invalid branch name.[1] But the `@` synonym is allowed. This is just as inconvenient since commands like `git checkout @` will, quite sensibly, do `git checkout HEAD` instead of checking out that branch; in turn there is no practical reason to use this as a branch name since you cannot even check out the branch itself (only check out the commit which `refs/heads/@` points to). † 1: a625b092cc5 (branch: correctly reject refs/heads/{-dash,HEAD}, 2017-11-14) Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- object-name.c | 3 ++- t/t3204-branch-name-interpretation.sh | 9 ++------- 2 files changed, 4 insertions(+), 8 deletions(-)