Michael Haggerty <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
