Re: [PATCH 3/3] branch: forbid refs/heads/HEAD

[Kaartic Sivaraam]

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(, 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

[Junio C Hamano]

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

[Kaartic Sivaraam]

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

[Jeff King]

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]

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

[Junio C Hamano]

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

[Jeff King]

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