Jeff King <peff@xxxxxxxx> writes: > 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. Thanks, I did notice this and wondered the same while reviewing, but totally forgot about it when I queued X-<. >> > 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. -- 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