Re: [PATCH] receive-pack: crash when checking with non-exist HEAD

[Jeff King <peff@peff.net>]

On Wed, Jul 22, 2015 at 01:30:00PM -0700, Junio C Hamano wrote:

> I see a similar "if head_name is NULL, don't bother." check in
> is_ref_checked_out() so in that sense this is a correct fix to the
> immediate problem.  That check came from 986e8239 (receive-pack:
> detect push to current branch of non-bare repo, 2008-11-08).

Yeah, I think this patch makes sense for the same reason as the check in
986e8239: if our HEAD ref does not point to a branch, we cannot possibly
be trying to delete it.

I do think the check is a little racy, though I'm not sure it is easy to
fix. E.g., imagine one client pushes to create refs/heads/master just as
somebody else is trying to delete it. I don't think this is much
different than the case where somebody redirects HEAD to
refs/heads/master as we are trying to delete it, though. The checks are
inherently racy because they are not done under locks (you would need to
lock both HEAD and refs/heads/master in that case). It's probably not a
big deal in practice.

> This is a tangent, but if HEAD points at an unborn branch that
> cannot be created, wouldn't all other things break?  
> 
> For example, in order to "git commit" from such a state to create
> the root commit on that branch, existing unrelated branches whose
> names collide with the branch must be removed, which would mean one
> of two things, either (1) you end up losing many unrelated work, or
> (2) the command refuses to work, not letting you to record the
> commit.  Neither is satisfactory, but we seem to choose (2), which
> is at least the safer of the two:
> 
>     $ git checkout master
>     $ git checkout --orphan master/1
>     $ git commit -m foo
>     fatal: cannot lock ref 'HEAD': 'refs/heads/master' exists;
>     cannot create 'refs/heads/master/1'

Yeah, that seems sensible. I think the "way out" for the user here would
presumably be:

  git symbolic-ref HEAD refs/heads/something-else

though of course they could also rename the other ref.

> We may want to avoid putting us in such a situation in the first
> place.  Giving "checkout --orphan" an extra check might be a simple
> small thing we can do, i.e.
> 
>     $ git checkout master
>     $ git checkout --orphan master/1
>     fatal: 'master' branch exists, cannot create 'master/1'
> 
> But I suspect it would not protect us from different avenues that
> can cause this kind of thing; e.g. to prevent this:
> 
>     $ git branch -D next
>     $ git checkout --orphan next/1
>     $ git fetch origin master:refs/heads/next
> 
> creation of a ref "refs/heads/next" here must notice HEAD points
> at "refs/heads/next/1" (that does not yet exist) and do something
> intelligent about it.

Right. You'd have to teach the is_refname_available() check to always
check what HEAD points to, and consider it as "taken", even if the ref
itself doesn't exist. But what about other symbolic refs? The
"refs/remotes/origin/HEAD" symref may point to
"refs/remotes/origin/master" even though "refs/remotes/origin/master/1"
exists. I doubt that will cause real problems in practice, but it points
out that special cases like "the value of HEAD is magic and reserved"
will later end up being insufficient as the code is extended.

I think I'd be willing to simply punt on the whole thing as being too
rare to come up in practice.

-Peff