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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/27/2016 08:55 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> 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?

Michael