Re: [PATCH 1/4] refs: move to using the '.optimize' functions
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Oct 15, 2025, 22:05 UTC
- 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/
Show 48 quoted lines
> 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.
Show 40 quoted lines
> .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