Re: [PATCH v2 7/7] reflog expire: don't assert the OID when locking refs

On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason
[off-list ref] 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.

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 */

                                  NULL, NULL, REF_NO_DEREF,
                                  &type, &err);
        if (!lock) {
--

-- 
Duy