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

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 04/29/2016 02:12 PM, Jeff King wrote:
>> 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:
>
> It's beyond the ambition of this patch to fix this old rename_ref()
> code, but...

Thanks for a clear explanation.  The NULL might want to be explained
in in-code comment (e.g. "here we do not care about verifying old
value because..."), then.