On Fri, Mar 15 2019, Duy Nguyen wrote:
> On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason > <ava...@gmail.com> wrote: >> >> During reflog expiry, the cmd_reflog_expire() function first iterates >> over all reflogs in logs/*, and then one-by-one acquires the lock for >> each one to expire its reflog by getting a *.lock file on the >> corresponding loose ref[1] (even if the actual ref is packed). >> >> This lock is needed, but what isn't needed is locking the loose ref as >> a function of the OID we found from that first iteration. By the time >> we get around to re-visiting the reference some of the OIDs may have >> changed. >> >> Thus the verify_lock() function called by the lock_ref_oid_basic() >> function being changed here would fail with e.g. "ref '%s' is at %s >> but expected %s" if the repository was being updated concurrent to the >> "reflog expire". >> >> By not passing the OID to it we'll try to lock the reference >> regardless of it last known OID. Locking as a function of the OID >> would make "reflog expire" exit with a non-zero exit status under such >> contention, which in turn meant that a "gc" command (which expires >> reflogs before forking to the background) would encounter a hard >> error. > > Passing NULL oid has another side(?) effect, which I don't know if it > matters at all. Before, the mustexist flag in lock_ref_oid_basic() is > true. Now it's false. This affects refs_resolve_ref_unsafe() calls in > there. But that's where I'm stuck. This case was tricky, I ended up doing the same thing in v3, but now very carefully explain the rationale since none of it is obvious. >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index ef053f716c..4d4d226601 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store >> *ref_store, >> * reference itself, plus we might need to update the >> * reference if --updateref was specified: >> */ >> - lock = lock_ref_oid_basic(refs, refname, oid, >> + lock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */, > > Maybe mention this not oid in the comment block above. Or just drop > it. Reading this comment without the commit message does not really > help answer "why not oid?". Or perhaps /* don't verify oid */ Also fixed. >> NULL, NULL, REF_NO_DEREF, >> &type, &err); >> if (!lock) { >> --