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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

The locking protocol for reflogs involves getting a *.lock file on the
loose ref[1] (even if the actual ref is packed). This isn't needed to
expire the reflog, and needlessly results promotes reference update
contention to hard errors in e.g. "gc".

During reflog expiry, the cmd_reflog_expire() function first iterates
over all known references, and then one-by-one acquires the lock for
each one to expire its reflog. By the time it gets around to
re-visiting the references some of the OIDs may have changed.

This has been the case ever since "reflog expire" was initially
implemented in 4264dc15e1 ("git reflog expire", 2006-12-19). As seen
in that simpler initial version of the code (and the same is the case
before this change) we subsequently use the OID to inform the expiry,
but never needed to use it to lock the reference associated with the
reflog.

Thus the verify_lock() function would fail with e.g. "ref '%s' is at
%s but expected %s" if the repository was being updated concurrent to
the "reflog expire".

This made "reflog expire" exit with a non-zero exit status, which in
turn meant that a "gc" command (which expires reflogs before forking
to the background) would encounter a hard error in such a scenario.

If we instead lock the reference without considering what OID we last
saw it at, we won't encounter user-visible contention to the extent
that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 ("refs:
retry acquiring reference locks for 100ms", 2017-08-21).

Unfortunately this sort of probabilistic contention is hard to turn
into a test. I've tested this by running the following three subshells
in concurrent terminals:

    (
        cd /tmp &&
        rm -rf git &&
        git init git &&
        cd git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >out &&
            git add out &&
            git commit -m"out"
        done
    )

    (
        cd /tmp &&
        rm -rf git-clone &&
        git clone file:///tmp/git git-clone &&
        cd git-clone &&
        while git pull
        do
            date
        done
    )

    (
        cd /tmp/git-clone &&
        while git reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with
a "but expected" error. After this change both the "pull" and "reflog
expire" will run for a while, but eventually fail because I get
unlucky with core.filesRefLockTimeout (the "reflog expire" is in a
really tight loop). That can be resolved by being more generous with
higher values of core.filesRefLockTimeout than the 100ms default.

1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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! */,
 				  NULL, NULL, REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
-- 
2.21.0.360.g471c308f928