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

Jeff King [off-list ref] writes:

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

quoted

On 01/11/2016 04:52 PM, Jeff King wrote:

quoted

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.

Thanks, I did notice this and wondered the same while reviewing, but
totally forgot about it when I queued X-<.

quoted

quoted

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.

Sounds good.  Thanks, both.