| Message ID | cover.1728331771.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| Headers | show |
| Series | object-name: don't allow @ as a branch name | expand |
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
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!
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
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.
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).
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.