Re: [PATCH v4 08/11] reflog expire: don't lock reflogs using previously seen OID

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Han-Wen Nienhuys <hanwen@google.com>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH v4 08/11] reflog expire: don't lock reflogs using previously seen OID
Date: Mon, 02 Aug 2021 10:26:39 -0700	[thread overview]
Message-ID: <xmqqeebbwyeo.fsf@gitster.g> (raw)
In-Reply-To: <patch-08.11-c9c2da3599-20210726T234237Z-avarab@gmail.com> (=?utf-8?B?IsOGdmFyCUFybmZqw7Zyw7A=?= Bjarmason"'s message of "Tue, 27 Jul 2021 01:44:24 +0200")

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> During reflog expiry, the cmd_reflog_expire() function first iterates
> ...
> I'm leaving behind now-unused code the refs API etc. that takes the
> now-NULL "unused_oid" argument, and other code that can be simplified now
> that we never have on OID in that context, that'll be cleaned up in
> subsequent commits, but for now let's narrowly focus on fixing the
> "git gc" issue. As the modified assert() shows we always pass a NULL
> oid to reflog_expire() now.

OK.  Nicely described.

>  static int files_reflog_expire(struct ref_store *ref_store,
> -			       const char *refname, const struct object_id *oid,
> +			       const char *refname, const struct object_id *unused_oid,
>  			       unsigned int flags,
>  			       reflog_expiry_prepare_fn prepare_fn,
>  			       reflog_expiry_should_prune_fn should_prune_fn,
> @@ -3049,6 +3049,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  	int status = 0;
>  	int type;
>  	struct strbuf err = STRBUF_INIT;
> +	const struct object_id *oid;
>  
>  	memset(&cb, 0, sizeof(cb));
>  	cb.flags = flags;
> @@ -3060,7 +3061,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,
>  				  REF_NO_DEREF,
>  				  &type, &err);
>  	if (!lock) {
> @@ -3068,6 +3069,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  		strbuf_release(&err);
>  		return -1;
>  	}
> +	oid = &lock->old_oid;

OK.  That makes it more clear that the object name the locking code
read is what gets used.

> @@ -3111,6 +3113,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  		}
>  	}
>  
> +	assert(!unused_oid);
>  	(*prepare_fn)(refname, oid, cb.policy_cb);
>  	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
>  	(*cleanup_fn)(cb.policy_cb);

The preference in this codebase is

	ptr_to_function(params);

over

	(*ptr_to_function)(params);

Once it is written and committed, it is not worth changing, but just
for the record...

THanks.