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

Kaartic Sivaraam [off-list ref] writes:

quoted

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.

quoted

 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.

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