Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
On Sat, 2017-10-21 at 17:57 +0900, Junio C Hamano wrote: > ... The code may already > handle it, or there may need even more code to support the rename; I > didn't check. > As far as I could see there the code does seem to already handle the rename. This part of builtin/branch.c is what seems to be ensuring that, if (strbuf_check_branch_ref(&oldref, oldname)) { /* * Bad name --- this could be an attempt to rename a * ref that we used to allow to be created by accident. */ if (ref_exists(oldref.buf)) recovery = 1; else die(_("Invalid branch name: '%s'"), oldname); } -- Kaartic
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Kaartic Sivaraam writes: >> The only difference is improved tests where we use show-ref to make >> sure refs/heads/HEAD does not exist when it shouldn't, exercise >> update-ref to create and delete refs/heads/HEAD, and also make sure >> it can be deleted with "git branch -d". > > In which case you might also like to ensure that it's possible to > "rename" the branch with a name "HEAD" to recover from historical > mistakes. Perhaps. I didn't think it was all that needed---as long as you can delete, you can recreate at the same commit with a more desirable name, and it is not like users have tons of repositories with misnamed branches that they need to fix. The code may already handle it, or there may need even more code to support the rename; I didn't check. >> int strbuf_check_branch_ref(struct strbuf *sb, const char *name) >> { >> strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL); >> -if (name[0] == '-') >> +if (*name == '-' || !strcmp(name, "HEAD")) >> return -1; > > I guess this makes the check for "HEAD" in builtin/branch::cmd_branch() > (line 796) redundant. May be it could be removed? Perhaps. But I think that is better done as a follow-up "now the lower level consistently handles, let's remove the extra check that has become unnecessary" separate patch. > So, may be the following test could also be added (untested yet), > > test_expect_success 'branch -m can rename refs/heads/HEAD' ' > git update-ref refs/heads/HEAD HEAD^ && > git branch -m HEAD head && > test_must_fail git show-ref refs/heads/HEAD > ' Yeah, that would be a good material for that separate follow-up patch. Thanks.
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Junio C Hamano writes: > > > Perhaps. Also we may want to make sure that "git branch -D HEAD" > > still works as a way to recover from historical mistakes. > > The only difference is improved tests where we use show-ref to make > sure refs/heads/HEAD does not exist when it shouldn't, exercise > update-ref to create and delete refs/heads/HEAD, and also make sure > it can be deleted with "git branch -d". > In which case you might also like to ensure that it's possible to "rename" the branch with a name "HEAD" to recover from historical mistakes. > diff --git a/sha1_name.c b/sha1_name.c > index c7c5ab376c..1b8c503095 100644 > --- a/sha1_name.c > +++ b/sha1_name.c > @@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char > *name, unsigned allowed) > int strbuf_check_branch_ref(struct strbuf *sb, const char *name) > { > strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL); > - if (name[0] == '-') > + if (*name == '-' || !strcmp(name, "HEAD")) > return -1; I guess this makes the check for "HEAD" in builtin/branch::cmd_branch() (line 796) redundant. May be it could be removed? > strbuf_splice(sb, 0, 0, "refs/heads/", 11); > return check_refname_format(sb->buf, 0); > diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh > index e88349c8a0..4556aa66b8 100755 > --- a/t/t1430-bad-ref-name.sh > +++ b/t/t1430-bad-ref-name.sh > @@ -331,4 +331,27 @@ test_expect_success 'update-ref --stdin -z fails delete > with bad ref name' ' > grep "fatal: invalid ref format: ~a" err > ' > > +test_expect_success 'branch rejects HEAD as a branch name' ' > + test_must_fail git branch HEAD HEAD^ && > + test_must_fail git show-ref refs/heads/HEAD > +' > + > +test_expect_success 'checkout -b rejects HEAD as a branch name' ' > + test_must_fail git checkout -B HEAD HEAD^ && > + test_must_fail git show-ref refs/heads/HEAD > +' > + > +test_expect_success 'update-ref can operate on refs/heads/HEAD' ' > + git update-ref refs/heads/HEAD HEAD^ && > + git show-ref refs/heads/HEAD && > + git update-ref -d refs/heads/HEAD && > + test_must_fail git show-ref refs/heads/HEAD > +' > + > +test_expect_success 'branch -d can remove refs/heads/HEAD' ' > + git update-ref refs/heads/HEAD HEAD^ && > + git branch -d HEAD && > + test_must_fail git show-ref refs/heads/HEAD > +' > + > test_done So, may be the following test could also be added (untested yet), test_expect_success 'branch -m can rename refs/heads/HEAD' ' git update-ref refs/heads/HEAD HEAD^ && git branch -m HEAD head && test_must_fail git show-ref refs/heads/HEAD ' -- Kaartic
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
On Sat, Oct 14, 2017 at 11:20:11AM +0900, Junio C Hamano wrote: > Junio C Hamano writes: > > >> Should we test that: > >> > >> git update-ref refs/heads/HEAD HEAD^ > >> > >> continues to work? > > > > Perhaps. Also we may want to make sure that "git branch -D HEAD" > > still works as a way to recover from historical mistakes. > > The only difference is improved tests where we use show-ref to make > sure refs/heads/HEAD does not exist when it shouldn't, exercise > update-ref to create and delete refs/heads/HEAD, and also make sure > it can be deleted with "git branch -d". Thanks, this looks good to me. -Peff
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Junio C Hamano writes: >> Should we test that: >> >> git update-ref refs/heads/HEAD HEAD^ >> >> continues to work? > > Perhaps. Also we may want to make sure that "git branch -D HEAD" > still works as a way to recover from historical mistakes. The only difference is improved tests where we use show-ref to make sure refs/heads/HEAD does not exist when it shouldn't, exercise update-ref to create and delete refs/heads/HEAD, and also make sure it can be deleted with "git branch -d". -- >8 -- strbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'. Use it to also reject "HEAD" as a branch name. Helped-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_name.c | 2 +- t/t1430-bad-ref-name.sh | 23 +++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/sha1_name.c b/sha1_name.c index c7c5ab376c..1b8c503095 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) int strbuf_check_branch_ref(struct strbuf *sb, const char *name) { strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL); - if (name[0] == '-') + if (*name == '-' || !strcmp(name, "HEAD")) return -1; strbuf_splice(sb, 0, 0, "refs/heads/", 11); return check_refname_format(sb->buf, 0); diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index e88349c8a0..4556aa66b8 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -331,4 +331,27 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' ' grep "fatal: invalid ref format: ~a" err ' +test_expect_success 'branch rejects HEAD as a branch name' ' + test_must_fail git branch HEAD HEAD^ && + test_must_fail git show-ref refs/heads/HEAD +' + +test_expect_success 'checkout -b rejects HEAD as a branch name' ' + test_must_fail git checkout -B HEAD HEAD^ && + test_must_fail git show-ref refs/heads/HEAD +' + +test_expect_success 'update-ref can operate on refs/heads/HEAD' ' + git update-ref refs/heads/HEAD HEAD^ && + git show-ref refs/heads/HEAD && + git update-ref -d refs/heads/HEAD && + test_must_fail git show-ref refs/heads/HEAD +' + +test_expect_success 'branch -d can remove refs/heads/HEAD' ' + git update-ref refs/heads/HEAD HEAD^ && + git branch -d HEAD && + test_must_fail git show-ref refs/heads/HEAD +' + test_done -- 2.15.0-rc1-158-gbd035ae683
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Jeff King writes: > On Fri, Oct 13, 2017 at 02:11:32PM +0900, Junio C Hamano wrote: > >> strbuf_check_branch_ref() is the central place where many codepaths >> see if a proposed name is suitable for the name of a branch. It was >> designed to allow us to get stricter than the check_refname_format() >> check used for refnames in general, and we already use it to reject >> a branch whose name begins with a '-'. >> >> Use it to also reject "HEAD" as a branch name. > > Heh, I just pointed somebody to this a day or two ago as #leftoverbit. I > guess it's taken now. :) Didn't notice /remember it; sorry about that. I can retract it if you want, but perhaps they cannot unsee it ;-) >> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh >> index e88349c8a0..3ecb2eab0c 100755 >> --- a/t/t1430-bad-ref-name.sh >> +++ b/t/t1430-bad-ref-name.sh >> @@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete >> with bad ref name' ' >> grep "fatal: invalid ref format: ~a" err >> ' >> >> +test_expect_success 'branch rejects HEAD as a branch name' ' >> +test_must_fail git branch HEAD HEAD^ >> +' >> + >> +test_expect_success 'checkout -b rejects HEAD as a branch name' ' >> +test_must_fail git checkout -B HEAD HEAD^ >> +' > > Should we test that: > > git update-ref refs/heads/HEAD HEAD^ > > continues to work? Perhaps. Also we may want to make sure that "git branch -D HEAD" still works as a way to recover from historical mistakes.
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
On Fri, Oct 13, 2017 at 02:11:32PM +0900, Junio C Hamano wrote: > strbuf_check_branch_ref() is the central place where many codepaths > see if a proposed name is suitable for the name of a branch. It was > designed to allow us to get stricter than the check_refname_format() > check used for refnames in general, and we already use it to reject > a branch whose name begins with a '-'. > > Use it to also reject "HEAD" as a branch name. Heh, I just pointed somebody to this a day or two ago as #leftoverbit. I guess it's taken now. :) Related to this: should we do a better job of confirming that the refname is available for use? If you do: git branch foo git checkout -b foo/bar then "foo/bar" is not available. And for "checkout -b", we'd notice when we tried to create the ref. But for: git checkout --orphan foo/bar we'd update HEAD with a non-viable name, and only find out later during "git commit". That's not the end of the world, but it might be nice to complain when writing the symlink. Largely orthogonal to the problem you're solving here, but I suspect it may touch the same code, so it might be worth thinking about while we're here. > diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh > index e88349c8a0..3ecb2eab0c 100755 > --- a/t/t1430-bad-ref-name.sh > +++ b/t/t1430-bad-ref-name.sh > @@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete > with bad ref name' ' > grep "fatal: invalid ref format: ~a" err > ' > > +test_expect_success 'branch rejects HEAD as a branch name' ' > + test_must_fail git branch HEAD HEAD^ > +' > + > +test_expect_success 'checkout -b rejects HEAD as a branch name' ' > + test_must_fail git checkout -B HEAD HEAD^ > +' Should we test that: git update-ref refs/heads/HEAD HEAD^ continues to work? -Peff
[PATCH 3/3] branch: forbid refs/heads/HEAD
strbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'. Use it to also reject "HEAD" as a branch name. Signed-off-by: Junio C Hamano --- sha1_name.c | 2 +- t/t1430-bad-ref-name.sh | 8 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/sha1_name.c b/sha1_name.c index c7c5ab376c..1b8c503095 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) int strbuf_check_branch_ref(struct strbuf *sb, const char *name) { strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL); - if (name[0] == '-') + if (*name == '-' || !strcmp(name, "HEAD")) return -1; strbuf_splice(sb, 0, 0, "refs/heads/", 11); return check_refname_format(sb->buf, 0); diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index e88349c8a0..3ecb2eab0c 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' ' grep "fatal: invalid ref format: ~a" err ' +test_expect_success 'branch rejects HEAD as a branch name' ' + test_must_fail git branch HEAD HEAD^ +' + +test_expect_success 'checkout -b rejects HEAD as a branch name' ' + test_must_fail git checkout -B HEAD HEAD^ +' + test_done -- 2.15.0-rc1-158-gbd035ae683