Re: [PATCH v2 08/11] refs/files: add a comment about refs_reflog_exists() call

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org, "Han-Wen Nienhuys" <hanwen@google.com>,
	"Michael Haggerty" <mhagger@alum.mit.edu>
Subject: Re: [PATCH v2 08/11] refs/files: add a comment about refs_reflog_exists() call
Date: Mon, 19 Jul 2021 09:43:14 -0700	[thread overview]
Message-ID: <xmqqa6mintjx.fsf@gitster.g> (raw)
In-Reply-To: <YPI7k7TnfQSQM7YF@coredump.intra.peff.net> (Jeff King's message of "Fri, 16 Jul 2021 22:08:19 -0400")

Jeff King <peff@peff.net> writes:

> On Fri, Jul 16, 2021 at 04:13:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Add a comment about why it is that we need to check for the the
>
> s/the the//
>
>> existence of a reflog we're deleting after we've successfully acquired
>> the lock in files_reflog_expire(). As noted in [1] the lock protocol
>> for reflogs is somewhat intuitive.
>
> Did you mean unintuitive here?
>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index ec9c70d79cc..f73637fa087 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -3068,6 +3068,17 @@ static int files_reflog_expire(struct ref_store *ref_store,
>>  		strbuf_release(&err);
>>  		return -1;
>>  	}
>> +
>> +	/*
>> +	 * When refs are deleted their reflog is deleted before the
>> +	 * ref itself deleted. This race happens because there's no

"... before the ref itself gets deleted", or some verb there.  A
comma in "When refs are deleted, there reflog is ..." would help
making it more readable, too.

>> +	 * such thing as a lock on the reflog, instead we always lock
>> +	 * the "loose ref" (even if packet) above with
>> +	 * lock_ref_oid_basic().
>> +	 *
>> +	 * If race happens we've got nothing more to do, we were asked
>> +	 * to delete the reflog, and it's not there anymore. Great!
>> +	 */
>>  	if (!refs_reflog_exists(ref_store, refname)) {
>>  		unlock_ref(lock);
>>  		return 0;
>
> I think everything is accurate here, though I wouldn't have made the
> distinction with "locking the loose ref". There is no such thing as
> locking a packed ref; we always take the loose lock.
>
> s/packet/packed/, and maybe s/If race/If a race/.

Meaning, there is no such thing as locking a packed ref or a loose
ref, we just lock a "ref" and the way it is done in files backend is
by creating a lockfile as if we are creating/updating a loose one?

I do agree that the second sentence needs rewriting to make it less
confusing.  lock_ref_oid_basic() being the way to lock "a ref" is
not limited to cases where we want to do something to do to the
reflog.

Taking all together, perhaps:

	When refs are deleted, their reflog is deleted before the
	ref itself is deleted.  This is because there is no separate
	lock for reflog---instead we take a lock on the ref with
	lock_ref_oid_basic().

        If a race happens we've got nothing more to do...

>
> -Peff