object-name: don't allow @ as a branch name
cover object-name: don't allow @ as a branch name
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]
Quoted 3 lines
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/[email protected]/
🔗 2: https://public-inbox.org/git/[email protected]/
🔗 3: https://public-inbox.org/git/[email protected]/
🔗 4: https://public-inbox.org/git/[email protected]/
§2 Disallow HEAD as a branch name
This was done later in 2017:
https://lore.kernel.org/git/[email protected]/
§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(-)
Signature · 1 line
2.46.1.641.g54e7913fcb6
PATCH 1/3 object-name: fix whitespace
Fix double newlines according to clang format.
--- object-name.c | 2 -- 1 file changed, 2 deletions(-)
object-name.c +0 -2
@@ -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,"../" ))
Signature · 1 line
2.46.1.641.g54e7913fcb6
PATCH 2/3 object-name: don't allow @ as a branch name
In reply to Kristoffer HaugsbakkHEAD 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)
--- object-name.c | 3 ++- t/t3204-branch-name-interpretation.sh | 9 ++------- 2 files changed, 4 insertions(+), 8 deletions(-)
object-name.c +2 -1
@@ -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 );
t/t3204-branch-name-interpretation.sh +2 -7
@@ -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 "@"' '
Signature · 1 line
2.46.1.641.g54e7913fcb6
Quoted 9 lines · Kristoffer Haugsbakk
HEADis an invalid branch name.[1] But the@synonym is allowed. This is just as inconvenient since commands likegit checkout @will, quite sensibly, dogit checkout HEADinstead 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 whichrefs/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.
Quoted 10 lines
t/t3204-branch-name-interpretation.sh +0
-2@@ -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
t/t3204-branch-name-interpretation.sh test_expect_success 'disallow deleting remote branch via @{-1}' ' · line 126
Quoted 10 lines
t/t3204-branch-name-interpretation.sh +2
-5-# 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
Quoted 12 lines · Jeff King
Quoted 9 lines · Kristoffer Haugsbakk
HEADis an invalid branch name.[1] But the@synonym is allowed. This is just as inconvenient since commands likegit checkout @will, quite sensibly, dogit checkout HEADinstead 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 whichrefs/heads/@points to).† 1:
a625b092cc5(branch: correctly reject refs/heads/{-dash,HEAD},2017-11-14)
Quoted 6 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.
Quoted 3 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”?
Quoted 10 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). ;)
Quoted 28 lines
Quoted 11 lines
t/t3204-branch-name-interpretation.sh +0
-2@@ -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 at/t3204-branch-name-interpretation.sh test_expect_success 'disallow deleting remote branch via @{-1}' ' · line 126
Quoted 10 lines
t/t3204-branch-name-interpretation.sh +2
-5-# 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.
Quoted 4 lines
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.
Signature · 1 line
Kristoffer but any Christopher-variation is fine
Quoted 7 lines · Kristoffer Haugsbakk
Quoted 6 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".
Quoted 7 lines
Got it. Creating this one (or something like
refs/heads/HEADfor 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.
Quoted 5 lines
Quoted 3 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
Quoted 5 lines · Jeff King
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...
Quoted 6 lines · Kristoffer Haugsbakk
HEADis an invalid branch name.[1] But the@synonym is allowed. This is just as inconvenient since commands likegit checkout @will, quite sensibly, dogit checkout HEADinstead 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 whichrefs/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.
Quoted 11 lines · Junio C Hamano
Quoted 6 lines · Kristoffer Haugsbakk
HEADis an invalid branch name.[1] But the@synonym is allowed. This is just as inconvenient since commands likegit checkout @will, quite sensibly, dogit checkout HEADinstead 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 whichrefs/heads/@points to).I am not sure this is sensible at all, after all these years.
Quoted 7 lines
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. :)
Quoted 5 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
PATCH 3/3 t1402: exercise disallowed branch names
In reply to Kristoffer Haugsbakk--- t/t1402-check-ref-format.sh | 4 ++++ 1 file changed, 4 insertions(+)
t/t1402-check-ref-format.sh +4 -0
@@ -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"
Signature · 1 line
2.46.1.641.g54e7913fcb6
Re: [PATCH 3/3] t1402: exercise disallowed branch names
Quoted 2 lines · Kristoffer Haugsbakk
Signed-off-by Kristoffer Haugsbakk <[email protected]>---
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
Quoted 10 lines · Kristoffer Haugsbakk
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. ```
Quoted 5 lines
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 namedHEAD.
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:
Quoted 2 lines
So that was tangential to the bug fix (
HEADas 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
Quoted 25 lines · Jeff King
Quoted 10 lines · Kristoffer Haugsbakk
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. ```Quoted 5 lines
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 namedHEAD.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:
Quoted 2 lines
So that was tangential to the bug fix (
HEADas a branch name was not disallowed in the patch series that resulted from this bug).
Quoted 7 lines
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
Quoted 3 lines
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!
Signature · 1 line
Kristoffer but any Christopher-variation is fine
On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
[snip]
Quoted 6 lines
§2 Disallow
HEADas a branch nameThis was done later in 2017:
Quoted 8 lines
§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 1The 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 brokenSo, 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
Quoted 63 lines · shejialuo
On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
[snip]
Quoted 6 lines
§2 Disallow
HEADas a branch nameThis was done later in 2017:
Quoted 8 lines
§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 1The 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 brokenSo, 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.
Quoted 6 lines
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.
Quoted 64 lines · Kristoffer Haugsbakk
Quoted 63 lines · shejialuo
On Mon, Oct 07, 2024 at 10:15:16PM +0200, Kristoffer Haugsbakk wrote:
[snip]
Quoted 6 lines
§2 Disallow
HEADas a branch nameThis was done later in 2017:
Quoted 8 lines
§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 1The 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 brokenSo, 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.
Quoted 8 lines
Quoted 6 lines
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,
JialuoThanks 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
Quoted 7 lines · shejialuo
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).
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · shejialuo
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(...); }
Quoted 5 lines
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.