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

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