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

On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:

On 04/27/2016 08:55 PM, Junio C Hamano wrote:

quoted

Michael Haggerty [off-list ref] writes:

quoted

@@ -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.