threads / patch / 62274

patch, 3 partsobject-name: don't allow @ as a branch name

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

## tl;dr

18 messages between Oct 7, 2024 and Oct 18, 2024. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

Kristoffer Haugsbakk· Oct 7, 2024, 20:15 UTC · lore

I use `@` a lot for Git commands in the terminal. I accidentally did something that made me create a branch named `@`. This puzzled me since `HEAD` is not allowed.

Note that the bare/one-level `@` ref name is already banned. So this is just about not allowing `refs/heads/@`.

§ Research

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

Peff found a way for the branch name `HEAD` to be created While figuring out a solution:[4]

> Checking "HEAD" afterwards means you can't actually have a branch
> named "HEAD". Doing so is probably insane, but we probably really _do_
> want to just disallow the @-conversion here.

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

🔗 1: https://lore.kernel.org/git/20170302082306.n6kfc5uqz2kdxtpm@sigill.intra.peff.net/ 🔗 2: https://public-inbox.org/git/20170228121514.qajydm5bjdbzsucg@sigill.intra.peff.net/ 🔗 3: https://public-inbox.org/git/20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net/ 🔗 4: https://public-inbox.org/git/20170227090233.uk7dfruggytgmuw2@sigill.intra.peff.net/

  §2 Disallow `HEAD` as a branch name
This was done later in 2017:
https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/
  §2 `refs/heads/@` is apparently disallowed by git-refs(1)
See `t/t1508-at-combinations.sh`:

``` error: refs/heads/@: badRefName: invalid refname format ```

Kristoffer Haugsbakk (3):
  object-name: fix whitespace
  object-name: don't allow @ as a branch name
  t1402: exercise disallowed branch names
 object-name.c                         | 5 ++---
 t/t1402-check-ref-format.sh           | 4 ++++
 t/t3204-branch-name-interpretation.sh | 9 ++-------
 3 files changed, 8 insertions(+), 10 deletions(-)
-- 
2.46.1.641.g54e7913fcb6
Kristoffer Haugsbakk· Oct 7, 2024, 20:15 UTC · re: Kristoffer Haugsbakk · lore

[PATCH 1/3] object-name: fix whitespace

Fix double newlines according to `clang format`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 object-name.c | 2 --
 1 file changed, 2 deletions(-)
Show changes to object-name.c +0 −2
diff --git a/object-name.c b/object-name.c
index c892fbe80aa..42e3ba4a77a 100644
--- a/object-name.c
+++ b/object-name.c
@@ -482,7 +482,6 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 		strbuf_addf(sb, _("%s blob"), hash);
 	}
 
-
 out:
 	/*
 	 * TRANSLATORS: This is line item of ambiguous object output
@@ -1965,7 +1964,6 @@ static void diagnose_invalid_index_path(struct repository *r,
 	strbuf_release(&fullname);
 }
 
-
 static char *resolve_relative_path(struct repository *r, const char *rel)
 {
 	if (!starts_with(rel, "./") && !starts_with(rel, "../"))

-- 
2.46.1.641.g54e7913fcb6
Kristoffer Haugsbakk· Oct 7, 2024, 20:15 UTC · re: Kristoffer Haugsbakk · lore

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

`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(-)
Show changes to 2 files +4 −8

object-name.c, t/t3204-branch-name-interpretation.sh

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 "@"' '

-- 
2.46.1.641.g54e7913fcb6
Jeff King· Oct 7, 2024, 20:44 UTC · re: Kristoffer Haugsbakk · lore

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

On Mon, Oct 07, 2024 at 10:15:18PM +0200, Kristoffer Haugsbakk wrote:
Show 9 quoted lines
> `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.
Show 20 quoted lines
> 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
Kristoffer Haugsbakk· Oct 7, 2024, 20:56 UTC · re: Jeff King · lore

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

On Mon, Oct 7, 2024, at 22:44, Jeff King wrote:
Show 18 quoted lines
> 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”?
Show 10 quoted lines
>
> 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). ;)

Show 32 quoted lines
>
>> 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 but any Christopher-variation is fine
Jeff King· Oct 8, 2024, 06:52 UTC · re: Kristoffer Haugsbakk · lore

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

On Mon, Oct 07, 2024 at 10:56:36PM +0200, Kristoffer Haugsbakk wrote:
Show 14 quoted lines
> > 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.

Show 5 quoted lines
> > 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
Rubén Justo· Oct 8, 2024, 20:37 UTC · re: Jeff King · lore

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

On Mon, Oct 07, 2024 at 04:44:47PM -0400, Jeff King wrote:
Show 5 quoted lines
> 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...

Junio C Hamano· Oct 7, 2024, 22:01 UTC · re: Kristoffer Haugsbakk · lore

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

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
Show 6 quoted lines
> `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.
Jeff King· Oct 8, 2024, 06:54 UTC · re: Junio C Hamano · lore

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

On Mon, Oct 07, 2024 at 03:01:29PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> 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. :)

Show 5 quoted lines
> 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
Kristoffer Haugsbakk· Oct 7, 2024, 20:15 UTC · re: Kristoffer Haugsbakk · lore

[PATCH 3/3] t1402: exercise disallowed branch names

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t1402-check-ref-format.sh | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to t/t1402-check-ref-format.sh +4 −0
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 5ed9d7318e0..06ef54c6091 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -68,6 +68,10 @@ valid_ref 'heads/foo*/bar' --refspec-pattern
 valid_ref 'heads/f*o/bar' --refspec-pattern
 invalid_ref 'heads/f*o*/bar' --refspec-pattern
 invalid_ref 'heads/foo*/bar*' --refspec-pattern
+invalid_ref 'HEAD' --branch
+invalid_ref '@' --branch
+invalid_ref '-' --branch
+invalid_ref '-something' --branch
 
 ref='foo'
 invalid_ref "$ref"

-- 
2.46.1.641.g54e7913fcb6
Jeff King· Oct 7, 2024, 20:47 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH 3/3] t1402: exercise disallowed branch names

On Mon, Oct 07, 2024 at 10:15:19PM +0200, Kristoffer Haugsbakk wrote:
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---

Can we give some more details here? These are cases that already passed before your series, right? At least I'd expect the "HEAD" one to do so, but I guess the "@" fix is new?

I don't need a lengthy explanation, just trying to understand how this change fits into your series. It might be simpler if this came first, adding new tests for existing behavior that was not covered, and then your 2/3 patch just adds the "@" line in the same spot.

-Peff
Jeff King· Oct 7, 2024, 20:37 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH 0/3] object-name: don't allow @ as a branch 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
Kristoffer Haugsbakk· Oct 7, 2024, 20:40 UTC · re: Jeff King · lore

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! ;)
: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
shejialuo· Oct 8, 2024, 13:19 UTC · re: Kristoffer Haugsbakk · lore

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

On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
[snip]
Show 14 quoted lines
>   §2 Disallow `HEAD` as a branch name
> 
> This was done later in 2017:
> 
> https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/
> 
>   §2 `refs/heads/@` is apparently disallowed by git-refs(1)
> 
> See `t/t1508-at-combinations.sh`:
> 
> ```
> error: refs/heads/@: badRefName: invalid refname format
> ```
>

It's true that using "git refs verify" will report "refs/heads/@" is a bad refname.

From the man page of the "git-check-ref-format(1)", it is clear that
    9. They cannot be the single character @.

Because I am interesting in this patch which is highly relevant with my recent work, so I try somethings here and find some interesting results as below shows.

    $ git check-ref-format refs/heads/@
    $ echo $? # will be 0
    # git check-ref-format --allow-onelevel @
    # echo $? # will be 1

The reason why "git refs verify" will report this error is that in the code implementation, I have to iterate every file in the filesystem. So it's convenient for me to do the following:

    if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
        ret = fsck_report(...);
    }

Because I specify "REFNAME_ALLOW_ONELEVEL" here, so it will follow the "git check-ref-format --allow-onelevel" command thus reporting an error to the user.

I am curious why "git check-ref-format refs/heads/@" will succeed, so I try to use "git symbolic-ref" and "git update-ref" to verify to test the behavior.

    $ git symbolic-ref refs/heads/@ refs/heads/master
    error: cannot lock ref 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference broken
    $ git update-ref refs/heads/@ refs/heads/master
    fatal: update_ref failed for ref 'refs/heads/@': cannot lock ref 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference broken

So, we are not consistent here. I guess the reason why "git check-ref-format refs/heads/@" will succeed is that we allow user create this kind of branch.

If we decide to not allow user to create such refs. We should also change the behavior of the "check_refname_format" function. (I am not familiar with the internal implementation, this is my guess)

Thanks, Jialuo

Kristoffer Haugsbakk· Oct 8, 2024, 14:19 UTC · re: shejialuo · lore

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

On Tue, Oct 8, 2024, at 15:19, shejialuo wrote:
Show 69 quoted lines
> On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
>
> [snip]
>
>>   §2 Disallow `HEAD` as a branch name
>> 
>> This was done later in 2017:
>> 
>> https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/
>> 
>>   §2 `refs/heads/@` is apparently disallowed by git-refs(1)
>> 
>> See `t/t1508-at-combinations.sh`:
>> 
>> ```
>> error: refs/heads/@: badRefName: invalid refname format
>> ```
>> 
>
> It's true that using "git refs verify" will report "refs/heads/@" is a
> bad refname.
>
> From the man page of the "git-check-ref-format(1)", it is clear that
>
>     9. They cannot be the single character @.
>
> Because I am interesting in this patch which is highly relevant with my
> recent work, so I try somethings here and find some interesting results
> as below shows.
>
>     $ git check-ref-format refs/heads/@
>     $ echo $? # will be 0
>     # git check-ref-format --allow-onelevel @
>     # echo $? # will be 1
>
> The reason why "git refs verify" will report this error is that in the
> code implementation, I have to iterate every file in the filesystem. So
> it's convenient for me to do the following:
>
>     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
>         ret = fsck_report(...);
>     }
>
> Because I specify "REFNAME_ALLOW_ONELEVEL" here, so it will follow the
> "git check-ref-format --allow-onelevel" command thus reporting an error
> to the user.
>
> I am curious why "git check-ref-format refs/heads/@" will succeed, so I
> try to use "git symbolic-ref" and "git update-ref" to verify to test the
> behavior.
>
>     $ git symbolic-ref refs/heads/@ refs/heads/master
>     error: cannot lock ref 'refs/heads/@': unable to resolve reference 
> 'refs/heads/@': reference broken
>     $ git update-ref refs/heads/@ refs/heads/master
>     fatal: update_ref failed for ref 'refs/heads/@': cannot lock ref 
> 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference 
> broken
>
> So, we are not consistent here. I guess the reason why "git
> check-ref-format refs/heads/@" will succeed is that we allow user create
> this kind of branch.
>
> If we decide to not allow user to create such refs. We should also
> change the behavior of the "check_refname_format" function. (I am not
> familiar with the internal implementation, this is my guess)
>
> Thanks,
> Jialuo
Thanks for the careful analysis.
shejialuo· Oct 18, 2024, 14:21 UTC · re: Kristoffer Haugsbakk · lore

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

On Tue, Oct 08, 2024 at 04:19:10PM +0200, Kristoffer Haugsbakk wrote:
Show 72 quoted lines
> On Tue, Oct 8, 2024, at 15:19, shejialuo wrote:
> > On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
> >
> > [snip]
> >
> >>   §2 Disallow `HEAD` as a branch name
> >> 
> >> This was done later in 2017:
> >> 
> >> https://lore.kernel.org/git/20171114114259.8937-1-kaartic.sivaraam@gmail.com/
> >> 
> >>   §2 `refs/heads/@` is apparently disallowed by git-refs(1)
> >> 
> >> See `t/t1508-at-combinations.sh`:
> >> 
> >> ```
> >> error: refs/heads/@: badRefName: invalid refname format
> >> ```
> >> 
> >
> > It's true that using "git refs verify" will report "refs/heads/@" is a
> > bad refname.
> >
> > From the man page of the "git-check-ref-format(1)", it is clear that
> >
> >     9. They cannot be the single character @.
> >
> > Because I am interesting in this patch which is highly relevant with my
> > recent work, so I try somethings here and find some interesting results
> > as below shows.
> >
> >     $ git check-ref-format refs/heads/@
> >     $ echo $? # will be 0
> >     # git check-ref-format --allow-onelevel @
> >     # echo $? # will be 1
> >
> > The reason why "git refs verify" will report this error is that in the
> > code implementation, I have to iterate every file in the filesystem. So
> > it's convenient for me to do the following:
> >
> >     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
> >         ret = fsck_report(...);
> >     }
> >
> > Because I specify "REFNAME_ALLOW_ONELEVEL" here, so it will follow the
> > "git check-ref-format --allow-onelevel" command thus reporting an error
> > to the user.
> >
> > I am curious why "git check-ref-format refs/heads/@" will succeed, so I
> > try to use "git symbolic-ref" and "git update-ref" to verify to test the
> > behavior.
> >
> >     $ git symbolic-ref refs/heads/@ refs/heads/master
> >     error: cannot lock ref 'refs/heads/@': unable to resolve reference 
> > 'refs/heads/@': reference broken
> >     $ git update-ref refs/heads/@ refs/heads/master
> >     fatal: update_ref failed for ref 'refs/heads/@': cannot lock ref 
> > 'refs/heads/@': unable to resolve reference 'refs/heads/@': reference 
> > broken
> >
> > So, we are not consistent here. I guess the reason why "git
> > check-ref-format refs/heads/@" will succeed is that we allow user create
> > this kind of branch.
> >
> > If we decide to not allow user to create such refs. We should also
> > change the behavior of the "check_refname_format" function. (I am not
> > familiar with the internal implementation, this is my guess)
> >
> > Thanks,
> > Jialuo
> 
> Thanks for the careful analysis.

Please ignore the above analysis which is not true. (Today I am writing code for my work). Currently, we truly allow "refs/heads/@" as the refname. And also for "git check-ref-format", "git update-ref" and "git symbolic-ref"

When I did the experiments above, I forgot to clear the state which makes the "git update-ref" and "git symbolic-ref" fail. So, there are some faults in "git refs verify". I will fix in my current work.

So, if we decide to not allow "refs/heads/@", we should also update "git check-ref-format", "git update-ref" and "git symbolic-ref" to align with this behavior.

Thanks, Jialuo

Junio C Hamano· Oct 8, 2024, 18:17 UTC · re: shejialuo · lore

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

shejialuo <shejialuo@gmail.com> writes:
Show 7 quoted lines
> The reason why "git refs verify" will report this error is that in the
> code implementation, I have to iterate every file in the filesystem. So
> it's convenient for me to do the following:
>
>     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
>         ret = fsck_report(...);
>     }

It may be convenient, but I think it is wrong. HEAD may be allowed at the top, but refs/heads/HEAD is not, and checking only the single level name as you descend into .git/refs directory hierarchy and find files would not be a good design to begin with (and it would not work if your backend is reftable).

shejialuo· Oct 9, 2024, 12:00 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 08, 2024 at 11:17:36AM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> shejialuo <shejialuo@gmail.com> writes:
> 
> > The reason why "git refs verify" will report this error is that in the
> > code implementation, I have to iterate every file in the filesystem. So
> > it's convenient for me to do the following:
> >
> >     if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
> >         ret = fsck_report(...);
> >     }
> 
> It may be convenient, but I think it is wrong.  HEAD may be allowed
> at the top, but refs/heads/HEAD is not, and checking only the single
> level name as you descend into .git/refs directory hierarchy and
> find files would not be a good design to begin with (and it would
> not work if your backend is reftable).

In my current work, I will introduce worktree check here and then I will use the fullname to check and this will not be a problem. Thanks for reminding here.

← back to recent threads