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

[David Turner <dturner@twopensource.com>]

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 <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?

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.