On Mon, Aug 30 2021, Han-Wen Nienhuys wrote:
> On Mon, Aug 23, 2021 at 2:13 PM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>>
>> Since my "refs API: remove OID argument to reflog_expire()" we don't
>> have the "oid" as part of the reflog_expire() signature. Instead the
>> reflog_expire() should pass the OID of the tip of the "locked" ref to
>> the prepare_fn().
>>
>> In files_reflog_expire() we do that by getting the OID from
>> lock_ref_oid_basic(). I'm assuming (but am not familiar enough with
>> reftable...) that by the time we get here we've got a locked ref
>> already in some way, so let's just use
>> refs_resolve_ref_unsafe_with_errno() to lookup the current OID of that
>> presumably-locked ref.
>
> I quickly looked at the files code, but I don't understand why the OID
> needs to be passed-in (before your refactoring): in builtin/reflog.c
> (before), the current OID is read, with any protection. This means
> that its value can't be trusted.
>
> After your refactoring, you lock the ref. I guess in the files backend
> this protects against non-atomic update of (ref, reflog) racing with a
> concurrent reflog expiry? In reftable, the (ref,reflog) update is
> atomic, so there is no need for locking to properly sequence
> operations.
Before my [1] we'd do:
1. Read the OID for the branch in builtin/reflog.c
2. Pass it to refs/files-backend.c, it would lock at that OID (or fail if it changed)
3. Pass the OID with the now-locked OID to the builtin/reflog.c code
After that [1] we do:
1. Lock the branch in refs/files-backend.c
2. Pass the OID with the now-locked OID to the builtin/reflog.c code
(ditto #3 above)
Whatever reftable itself does with updates doesn't change that we need
to do that 2nd step of passing the OID to builtin/reflog.c, as it makes
use of it.
That code is a bit confusing, if you want to understand it I recommend
reading it at the tip of my yet-unsubmitted
avar/cleanup-refs-api-and-reflog-expire-post-no-eisdir, it makes the
control flow a lot cleaner.
So as far as what we do here is concerned, we're stuck with the refs
files backend inherently wanting to pass "I locked this for you, here's
the OID".
I guess it could also pass "OK, now go ahead and expire" and pass no
OID. We'd then in builtin/reflog.c lookup the current OID for the logic
there, but just having the reftable backend appease the common API by
looking up the OID and passing it seemed like the most straightforward
thing to do.
I haven't tested this or thought it through, but I don't understand how
reftable isn't going to race in reflog expiry then. Sure, the ref/reflog
update itself is atomic, so it won't suffer from the needing-a-lock
problem of two concurrent file backend writers doing the equivalent of:
echo $NEW_SHA1 >.git/refs/heads/some-branch
But we will need at least the optimistic locking of code like
builtin/reflog.c wanting to do an expiry, and deciding whether to do
that expiry based on a given state of the ref/reflog. I.e. we don't
want:
1. Start reflog expiry
2. Code in builtin/reflog.c looks up the OID
3. Code in builtin/reflog.c decides whether expire the reflog
4. Concurrent with #4, another writer updates the ref/reflog pair
5. Code in builtin/reflog.c says "OK, expire it!"
6. Reftable queues a delete/prune of the reflog per #5.
This would be a sequente of updates to the ref/reflog, none of whom were
racy as far as the reftable semantics itself are concerneb, but where
we'd do the wrong thing because the writer thought we had A when we
really had B. So we need the equivalent of an "git update-ref" with the
"<oldvalue>".
Is there a better way to do that in this case that I'm missing?
1. https://lore.kernel.org/git/patch-v5-09.13-aba12606cea-20210823T113115Z-avarab@xxxxxxxxx/
2. https://github.com/avar/git/compare/avar/files-backend-remove-dead-errno-eisdir-6...avar:avar/cleanup-refs-api-and-reflog-expire-post-no-eisdir
