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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 15, 2019, 15:52 UTC
Message-ID: <87zhpw9ltk.fsf@evledraar.gmail.com>
In-Reply-To: <CACsJy8B_rOBB5SvqnQs9hkui+3txGVPcz+H62hWE7oPGb789bg@mail.gmail.com>

On Fri, Mar 15 2019, Duy Nguyen wrote:

Show 29 quoted lines

> On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason
> <avarab@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.

Show 14 quoted lines

>> 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) {
>> --