On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote: > On 04/27/2016 08:55 PM, Junio C Hamano wrote: > > Michael Haggerty <[email protected]> writes: > > > > > @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, > > > const char *newrefname, const char *logms > > > goto rollback; > > > } > > > > > > - if (!read_ref_full(newrefname, RESOLVE_REF_READING, > > > sha1, NULL) && > > > - delete_ref(newrefname, sha1, REF_NODEREF)) { > > > + if (!read_ref_full(newrefname, resolve_flags, sha1, > > > NULL) && > > > + delete_ref(newrefname, NULL, REF_NODEREF)) { > > > > 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. 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. > So > the "sha1" check only adds ways for the rename() to fail in > situations > where nothing harmful would have happened anyway. > > That being said, this is a very unlikely race, and I don't think it > matters much either way. In any case, the change s/sha1/NULL/ here > seems > orthogonal to the rest of the patch. > > David, you wrote the original version of this patch. Am I overlooking > something?
I think I might have been handling some weird case related to symbolic refs, but I don't recall the details. Your argument seems right to me. -- 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
