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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Michael Haggerty <mhagger@alum.mit.edu>,
	Stefan Beller <stefanbeller@gmail.com>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v2 7/7] reflog expire: don't assert the OID when locking refs
Date: Fri, 15 Mar 2019 16:52:39 +0100	[thread overview]
Message-ID: <87zhpw9ltk.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CACsJy8B_rOBB5SvqnQs9hkui+3txGVPcz+H62hWE7oPGb789bg@mail.gmail.com>

On Fri, Mar 15 2019, Duy Nguyen wrote:

> 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.

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