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

Michael Haggerty [off-list ref] writes:

quoted

quoted

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.

quoted

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