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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html