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

From: Jeff King <peff@peff.net>
Date: Jul 17, 2021, 02:08 UTC
Message-ID: <YPI7k7TnfQSQM7YF@coredump.intra.peff.net>
In-Reply-To: <patch-08.11-1e25b7c59c5-20210716T140631Z-avarab@gmail.com>

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?

Show 22 quoted lines

> 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
> +	 * 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/.

-Peff