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

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

[Kristoffer Haugsbakk]

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

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

[Kristoffer Haugsbakk]

Fix double newlines according to `clang format`.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 object-name.c | 2 --
 1 file changed, 2 deletions(-)

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

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

[Kristoffer Haugsbakk]

`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(-)

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

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

[Jeff King]

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

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

[Kristoffer Haugsbakk]

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 but any Christopher-variation is fine

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

[Jeff King]

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

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

[Rubén Justo]

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Kristoffer Haugsbakk]

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t1402-check-ref-format.sh | 4 ++++
 1 file changed, 4 insertions(+)

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Kristoffer Haugsbakk]

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

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

[shejialuo]

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

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

[Kristoffer Haugsbakk]

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.

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

[shejialuo]

On Tue, Oct 08, 2024 at 04:19:10PM +0200, Kristoffer Haugsbakk wrote:
> 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

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

[Junio C Hamano]

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

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

[shejialuo]

On Tue, Oct 08, 2024 at 11:17:36AM -0700, Junio C Hamano wrote:
> 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.