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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 29, 2016, 10:57 UTC
Message-ID: <57233E19.4000200@alum.mit.edu>
In-Reply-To: <xmqqvb3023v0.fsf@gitster.mtv.corp.google.com>

On 04/29/2016 10:53 AM, Junio C Hamano wrote:

Show 27 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> Could you explain s/sha1/NULL/ here in the proposed log message?
>>
>> Good question.
>>
>> Passing sha1 to delete_ref() doesn't add any safety, because the same
>> sha1 was just read a moment before, and it is not used for anything
>> else.
> 
> "... and it is guaranteed that no other process in the meantime
> wanted to update the ref we are trying to delete because it wants to
> see the ref with its updated value." is something I expected to see
> at the end.
> 
>> So the check only protects us from a concurrent update to
>> newrefname between the call to read_ref_full() and the call to
>> delete_ref(). But such a race is indistinguishable from the case that a
>> modification to newrefname happens just before the call to
>> read_ref_full(), which would have the same outcome as the new code.
> 
> In other words, when a transaction that contains a deletion of a ref
> races with another one that updates the ref, the latter transaction
> may come after the former one, but the ref may not survive in the
> end result and can be left deleted?
> 
> Puzzled...

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

It is not possible for (1) to happen after (2b) because the former's check of the old value of newrefname would fail. So there are two possible interleavings:

* 1, 2a, 2b
* 2a, 1, 2b

With the new code, both of these interleavings end up with newrefname deleted.

With the old code, the second interleaving would fail.

But the only difference is the relative order of the read-only operation (2a), whose SHA-1 result is never used. So neither process actually cares which of those two interleavings occurred, and it is legitimate to treat them the same.

Note that the first transaction *did* successfully set newrefname to value B in both cases and indeed knows for sure that the update was successful. It's just that newrefname was deleted immediately afterwards.

Michael