Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2021, 17:40 UTC
Message-ID: <xmqq35s7efb3.fsf@gitster.g>
In-Reply-To: <patch-06.12-295594fe8ae-20210720T102051Z-avarab@gmail.com>

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

Show 35 quoted lines

>  static void reflog_expiry_prepare(const char *refname,
> -				  const struct object_id *oid,
> +				  struct object_id *locked_oid,
>  				  void *cb_data)
>  {
>  	struct expire_reflog_policy_cb *cb = cb_data;
> @@ -361,7 +361,7 @@ static void reflog_expiry_prepare(const char *refname,
>  		cb->unreachable_expire_kind = UE_HEAD;
>  	} else {
>  		cb->tip_commit = lookup_commit_reference_gently(the_repository,
> -								oid, 1);
> +								locked_oid, 1);
>  		if (!cb->tip_commit)
>  			cb->unreachable_expire_kind = UE_ALWAYS;
>  		else
> diff --git a/refs.h b/refs.h
> index 48970dfc7e0..c009707438d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -796,7 +796,7 @@ enum expire_reflog_flags {
>   * expiration policy that is desired.
>   *
>   * reflog_expiry_prepare_fn -- Called once after the reference is
> - *     locked.
> + *     locked. Called with the OID of the locked reference.
>   *
>   * reflog_expiry_should_prune_fn -- Called once for each entry in the
>   *     existing reflog. It should return true iff that entry should be
> @@ -806,7 +806,7 @@ enum expire_reflog_flags {
>   *     unlocked again.
>   */
>  typedef void reflog_expiry_prepare_fn(const char *refname,
> -				      const struct object_id *oid,
> +				      struct object_id *lock_oid,
>  				      void *cb_data);

The files_reflog_expire() helper still takes const oid but it can tolerate this change because it uses the lockfile API as its implementation detail, and passes &(lock->old_oid) (which is no longer a const).

But do we expect that all backends to take an on-filesystem lock via the lockfile API and pass it down to prepare_fn?

This obviously breaks the latest round of reftable topic, as it still wants this type to take const oid, and I do not think using on-filesystem lock as if we are using the files backend is not a good solution.