Re: [BUG] Incosistent repository state when trying to rename HEAD in the middle of a rebase

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Date: Nov 3, 2017, 09:23 UTC
Message-ID: <49563f7c-354e-334e-03a6-c3a40884b6d0@gmail.com>
In-Reply-To: <xmqqd151qft5.fsf@gitster.mtv.corp.google.com>

On Thursday 02 November 2017 01:21 PM, Junio C Hamano wrote:

Show 9 quoted lines

> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> 
>> I was able to spare some time to dig into this and found a few things.
>>
>> First, it seems that the issue is more generic and the BUG kicks in
>> whenever HEAD is not a symbolic ref.
> 
> Interesting.
>

Let me detail a little more about my observations just for the sake of completeness. The change that forbid refs/heads/HEAD caused issues only when HEAD wasn't a symbolic link because of the following reasons,

1) The change resulted in 'strbuf_check_branch_ref' returning with 
failure when the name it received (sb) was HEAD *without* interpreting 
it as "refs/heads/HEAD" into 'ref'. This resulted in the violation of 
the expectation of it's callers that it would have interpret 'ref' which 
was the major cause of the issue.

It wouldn't have been an issue if we had checked for the existence of a "branch" (refs/heads/) rather than checking for the existence of a "ref" (which allowed HEAD to pass the test).

2) This did not cause issues when HEAD was a symbolic ref because there 
was a check for attempting to rename in a symbolic ref in 
'files_copy_or_rename_ref'. The check throws an error when trying to 
rename a symbolic ref which resulted in suspicious error message I got.

So, IIUC the issue doesn't occur when 'ref' is intrepreted before the check for 'HEAD'. That's possibly why the diff patch I sent in the previous mail fixed the issue.

Also, it would be nice if we check for the existence of a "branch" when we want to know whether a branch exists rather than simply doing a 'ref_exists' on the interpreted ref.

> Shortly we'll be rewinding the tip of 'next', and it is a good
> opportunity to kick any not-so-well-cooked topic back to 'pu',
> so let's figure out what is going on after that happens (at which
> point let's eject the "branch name sanity" topic out of 'next').

Does the above explanation give an idea about what's happening?

--- Kaartic