From: Junio C Hamano Date: Tue, 12 Jan 2016 19:41:17 GMT Subject: Re: [PATCH v3 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs Message-ID: In-Reply-To: <20160112135522.GA23255@sigill.intra.peff.net> Jeff King writes: > I also notice that if we are deleting, we _do_ set > RESOLVE_REF_NO_RECURSE from the very beginning, which means we would > generally not get a valid lock->old_oid.hash for a symref. But I'm not > sure what it would mean to delete a symref while asking for its current > value (it cannot have one!). So I don't think it is a bug. I started scratching my head after noticing that the NO_RECURSE bit set in the DELETING codepath before reading the above, and I am still doing so. A transaction that attempts to delete an existing symref presumably wants to make sure that the "old" value it read hasn't changed, but ensuring the object name (obtained by reading the ref that is pointed by the symref by dereferencing) are the same is not the right way to ensure nobody raced with us in the meantime anyway (we should rather be making sure that the symref is still pointing at the same ref), so in that sense, in the context of acquiring the lock, old oid value is meaningless for symrefs. This patch is a strict improvement as the behaviour for REF_DELETING case is unchanged by it (an idempotent resolve-ref-unsafe may be called one more time in some cases), and other cases are better, I think.