Re: [PATCH 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs

[Jeff King <peff@peff.net>]

On Tue, Jan 12, 2016 at 05:55:25AM +0100, Michael Haggerty wrote:

> On 01/11/2016 04:52 PM, Jeff King wrote:
> > We sometimes call lock_ref_sha1_basic both with REF_NODEREF
> > to operate directly on a symbolic ref.
> 
> ^^^ This sentence seems to be missing some words.

I think it has one too many. :)

It was originally "both with a regular ref and with a symref", but I
shortened it since we only care about the symref case. I think just
getting rid of "both" is the right thing.

> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 180c837..ea67d82 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -1901,6 +1901,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
> >  
> >  	refname = resolve_ref_unsafe(refname, resolve_flags,
> >  				     lock->old_oid.hash, &type);
> > +	if (!refname && (flags & REF_NODEREF))
> > +		refname = resolve_ref_unsafe(orig_refname,
> > +					     resolve_flags | RESOLVE_REF_NO_RECURSE,
> > +					     lock->old_oid.hash, &type);
> > [...]
> 
> The main risk for this change would be that this new recovery code
> allows the function to continue, but one of the outputs of the second
> function invocation is not correct for the code that follows. Let me
> think out loud:
> 
> * refname -- now will be equal to orig_refname. I think the main effect
> is that it will be passed to verify_refname_available_dir(). This seems
> to be what we want.
> 
> * type -- now reflects orig_refname; i.e., usually REF_ISSYMREF. This
> also seems correct.
> 
> * lock->old_oid.hash -- is now ZEROS. This might get compared to the
> caller's old_sha1 in verify_lock(), and it will also be written to the
> reflog as the "old" value. I think this is also what we want.
> 
> So this change looks good to me.

Thanks. I had a nagging feeling that I hadn't considered all cases, but
the way you've framed it makes sense to me.

-Peff