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

Message

Kristoffer Haugsbakk Oct. 7, 2024, 8:15 p.m. UTC

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

Comments

Jeff King Oct. 7, 2024, 8:37 p.m. UTC | #1

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

Kristoffer Haugsbakk Oct. 7, 2024, 8:40 p.m. UTC | #2

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!

shejialuo Oct. 8, 2024, 1:19 p.m. UTC | #3

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/[e-mail]/
> 
>   §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, 2:19 p.m. UTC | #4

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/[e-mail]/
>> 
>>   §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.

Junio C Hamano Oct. 8, 2024, 6:17 p.m. UTC | #5

shejialuo <[e-mail]> 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).

shejialuo Oct. 9, 2024, noon UTC | #6

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

shejialuo Oct. 18, 2024, 2:21 p.m. UTC | #7

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/[e-mail]/
> >> 
> >>   §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