Michael Haggerty <mhagger@xxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html