From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 16 Oct 2025 12:03:25 GMT
Subject: Re: [PATCH 1/4] refs: move to using the '.optimize' functions
Message-ID: <CAOLa=ZQ-Ri7_SgPttqphRNNB7-AryNcSbcRsjWF_ua9jQxM92g@mail.gmail.com>
In-Reply-To: <zah4lafziwkqty2qw6erkbq3wfilfneom5vm6vcfqk5b644iff@rcebhmsrgmmd>

Justin Tobler <jltobler@gmail.com> writes:

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

Will fix.

>> 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.
>

Since there are no users of `refs_pack_refs()` which uses the
'.pack_refs' field, there are no users of 'debug_pack_refs()'.

But if tracing is enabled then the debug backend would wrap around the
other backends and any calls to `refs_optimize()` would call
`debug_optimize()`.

>>  	.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

It's done in the next commit, let me just squash that in to make it
easier.