Re: [PATCH 19/29] refs: don't dereference on rename

[Jeff King <peff@peff.net>]

On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:

> Remember, we're talking about rename_ref() only, not reference deletion
> in general. rename_ref() is not very robust anyway--it doesn't happen in
> a single transaction, and it is vulnerable to being defeated by
> simultaneous reference updates by other processes. It *does* wrap the
> deletion of newrefname in a transaction; the only question is whether an
> old_sha1 is supplied to that transaction.
> 
> So, suppose that newrefname starts at value A, and there are two updates
> running simultaneously:
> 
> 1. An update of reference newrefname from A -> B
> 
> 2. A rename of reference oldrefname to newrefname, which includes
>    a. read_ref_full("newrefname") and
>    b. delete_ref("newrefname").

I wondered at first if you meant "oldrefname" in 2b. That is, a rename
would involve writing to "newrefname" and then deleting "oldrefname",
and not doing the old_sha1 and normal locking on the deletion of
oldrefname would be bad (in case somebody else updated it while we were
operating).

But reading the patch again, that's not what's going on. You're talking
just about the case where we force-overwrite an existing newrefname, and
delete it first to do so. But what I don't understand is:

  1. Why do we read_ref_full() at all? Since it is not done under lock
     anyway, it is useless for helping with race conditions, and I think
     that is what you are arguing (that we should just be deleting
     regardless). But then why not just call delete_ref(), and handle
     the ENOENT case as a noop (which closes another race if somebody
     deletes it between your 2a and 2b).

  2. Why delete it at all? The point is to overwrite, so I guess it is
     trying to make room. But we could just rename the loose ref file
     and reflog overtop the old, couldn't we?

Or am I totally misreading the purpose of this code?

-Peff