git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Karthik NayakNext: Patrick Steinhardt
Message 3 of 14 in “refs: cleanup code around optimizations”
  1. 0/4 refs: cleanup code around optimizationsKarthik Nayak, Oct 15, 2025
  2. 1/4 refs: move to using the '.optimize' functionsKarthik Nayak, Oct 15, 2025
  3. Justin ToblerOct 15, 2025
  4. Patrick SteinhardtOct 16, 2025
  5. Karthik NayakOct 16, 2025
  6. Patrick SteinhardtOct 16, 2025
  7. Karthik NayakOct 16, 2025
  8. 2/4 refs: cleanup code around optimizationKarthik Nayak, Oct 15, 2025
  9. Justin ToblerOct 15, 2025
  10. 3/4 refs: rename 'pack_refs_opts' to 'refs_optimize_opts'Karthik Nayak, Oct 15, 2025
  11. Justin ToblerOct 15, 2025
  12. Karthik NayakOct 16, 2025
  13. 4/4 t/pack-refs-tests: move the 'test_done' to calleesKarthik Nayak, Oct 15, 2025
  14. Patrick SteinhardtOct 16, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.