From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 15 Oct 2025 22:05:46 GMT
Subject: Re: [PATCH 1/4] refs: move to using the '.optimize' functions
Message-ID: <zah4lafziwkqty2qw6erkbq3wfilfneom5vm6vcfqk5b644iff@rcebhmsrgmmd>
In-Reply-To: <20251015-refs-code-cleanup-v1-1-550fdd8a3b41@gmail.com>

On 25/10/15 11:25PM, Karthik Nayak wrote:
> The `struct ref_store` variable, exposes two ways to optimize a reftable

s/variable,/variable/

> backend:
> 
>   1. pack_refs
>   2. optimize
> 
> The former was specific to the 'files' + 'packed' refs backend. The
> latter is more generic and covers all backends. While the naming is
> different, both of these functions perform the same functionality.
> 
> In the following commit, we will consolidate this code to only maintain
> the 'optimize' functions. In preparation, modify the backends so that
> they exclusively implement the `optimize` callback, only. All users of
> the refs subsystem already use the 'optimize' function so there is no
> changes needed on the callee side.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/debug.c            |  8 ++++----
>  refs/files-backend.c    | 14 ++------------
>  refs/packed-backend.c   |  6 +++---
>  refs/reftable-backend.c | 13 +++----------
>  4 files changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/refs/debug.c b/refs/debug.c
> index 01499b9033..40cd1d9c15 100644
> --- a/refs/debug.c
> +++ b/refs/debug.c
> @@ -116,11 +116,11 @@ static int debug_transaction_abort(struct ref_store *refs,
>  	return res;
>  }
>  
> -static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)
> +static int debug_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)
>  {
>  	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
> -	int res = drefs->refs->be->pack_refs(drefs->refs, opts);
> -	trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
> +	int res = drefs->refs->be->optimize(drefs->refs, opts);
> +	trace_printf_key(&trace_refs, "optimize: %d\n", res);
>  	return res;
>  }
>  
> @@ -430,7 +430,7 @@ struct ref_storage_be refs_be_debug = {
>  	.transaction_finish = debug_transaction_finish,
>  	.transaction_abort = debug_transaction_abort,
>  
> -	.pack_refs = debug_pack_refs,
> +	.optimize = debug_optimize,

question: Was the debug backend not using either of these callbacks?
From the commit message, it sounds like all the backends were using the
optimize callback.

>  	.rename_ref = debug_rename_ref,
>  	.copy_ref = debug_copy_ref,
>  
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index ed8a1729d6..92d90fc508 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1444,8 +1444,8 @@ static int should_pack_refs(struct files_ref_store *refs,
>  	return 0;
>  }
>  
> -static int files_pack_refs(struct ref_store *ref_store,
> -			   struct pack_refs_opts *opts)
> +static int files_optimize(struct ref_store *ref_store,
> +			  struct pack_refs_opts *opts)
>  {
>  	struct files_ref_store *refs =
>  		files_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,
> @@ -1512,15 +1512,6 @@ static int files_pack_refs(struct ref_store *ref_store,
>  	return 0;
>  }
>  
> -static int files_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)
> -{
> -	/*
> -	 * For the "files" backend, "optimizing" is the same as "packing".
> -	 * So, we just call the existing worker function for packing.
> -	 */
> -	return files_pack_refs(ref_store, opts);
> -}
> -
>  /*
>   * People using contrib's git-new-workdir have .git/logs/refs ->
>   * /some/other/path/.git/logs/refs, and that may live on another device.
> @@ -3969,7 +3960,6 @@ struct ref_storage_be refs_be_files = {
>  	.transaction_finish = files_transaction_finish,
>  	.transaction_abort = files_transaction_abort,
>  
> -	.pack_refs = files_pack_refs,
>  	.optimize = files_optimize,

Ok, we are removing the "pack_refs" callback and its implementations
from all the backends in favor of using the just using the more generic
"optimize" callback. Make sense.

It does look like we still have `refs_pack_refs()` which references the
"optimize" callback. It looks like there are no users, but should we
also remove it as part of this patch?

-Justin