On Thursday 02 November 2017 01:21 PM, Junio C Hamano wrote:
Kaartic Sivaraam <kaarticsivaraam91196@xxxxxxxxx> 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
