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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2017, 08:57 UTC
Message-ID: <xmqqpo9gg9pa.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1508561424.2516.19.camel@gmail.com>

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

Show 8 quoted lines

>> 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.

Show 9 quoted lines

>>  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.

Show 7 quoted lines

> 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.