Jeff King <peff@xxxxxxxx> 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
