Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'

From: Karthik Nayak <karthik.188@gmail.com>
Date: Oct 13, 2025, 12:37 UTC
Message-ID: <CAOLa=ZQz3Fho2N1KBrbaj3OykXeCx1b5NTjBoPs0uL5uBm3z0g@mail.gmail.com>
In-Reply-To: <aOjsjpE1vuFUXXbh@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 11 quoted lines

> On Fri, Oct 10, 2025 at 12:27:13PM +0200, Karthik Nayak wrote:
>> The 'git pack-refs' and 'git refs optimize' commands allow users to
>> optimize their reference backends. However they provide no functionality
>> to check if optimization is required without performing it.
>>
>> Add a '--required' flag to these commands to do that. This is useful
>> on the server side where this information can be utilized to perform
>> more targetted maintenance runs of the repository.
>
> s/targetted/targeted/
>

My spell check says both are accepted. But the latter seems to be the general spelling.

Show 9 quoted lines

>> Add a corresponding test for the files backend. For the reftable
>> backend, this cannot be tested easily as it performs auto-compaction.
>> However, an earlier commit ensured the functionality was covered by
>> unit test.
>
> You can disable auto-compaction via the
> "GIT_TEST_REFTABLE_AUTOCOMPACTION" environment variable, so it should be
> rather easy to add a test.
>

Good point, let me add a test.

Show 15 quoted lines

>> diff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc
>> index fde9f2f294..62bc01b29b 100644
>> --- a/Documentation/git-pack-refs.adoc
>> +++ b/Documentation/git-pack-refs.adoc
>> @@ -9,6 +9,7 @@ SYNOPSIS
>>  --------
>>  [verse]
>>  'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
>> +                [--required]
>
> Hm, I'm not a huge fan of that name. "--dry-run" might be a better name,
> but on the other hand one might expect that such a command also yields
> information about what happens or that the compaction should be
> successful. So I cannot come up with a better name, either.
>

Yea, I too had the same thought process. Something like `--dry-run` would create the actual compacted files but not replace the existing files.

We could perhaps use `--check` ? But I'm okay with either.

Show 19 quoted lines

>> diff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc
>> index 0b11282941..66d69530b9 100644
>> --- a/Documentation/pack-refs-options.adoc
>> +++ b/Documentation/pack-refs-options.adoc
>> @@ -50,3 +50,8 @@ the provided `--exclude` patterns.
>>  +
>>  When used with `--include`, refs provided to `--include`, minus refs that are
>>  provided to `--exclude` will be packed.
>> +
>> +--required::
>> +
>> +Check if pack-refs is required to run, without actually performing the changes.
>
> Let's not say "pack-refs" here anymore, as we're migrating away from
> that name towards more generic terminology. Maybe:
>
>     Check whether the reference store needs to be optimized without
>     actually performing the changes.
>

This is better, will amend.

Show 44 quoted lines

>> diff --git a/pack-refs.c b/pack-refs.c
>> index fee77fbf9f..5d4d4266de 100644
>> --- a/pack-refs.c
>> +++ b/pack-refs.c
>> @@ -21,6 +21,7 @@ int pack_refs_core(int argc,
>>  	};
>>  	struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
>>  	struct string_list_item *item;
>> +	bool check_required = false;
>>  	int pack_all = 0;
>>  	int ret;
>>
>> @@ -28,6 +29,7 @@ int pack_refs_core(int argc,
>>  		OPT_BOOL(0, "all",   &pack_all, N_("pack everything")),
>>  		OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), OPTIMIZE_REFS_PRUNE),
>>  		OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), OPTIMIZE_REFS_AUTO),
>> +		OPT_BOOL(0, "required", &check_required, N_("check if optimization is required")),
>>  		OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
>>  			N_("references to include")),
>>  		OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
>> @@ -47,7 +49,14 @@ int pack_refs_core(int argc,
>>  	if (!pack_refs_opts.includes->nr)
>>  		string_list_append(pack_refs_opts.includes, "refs/tags/*");
>>
>> -	ret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);
>> +	if (check_required) {
>> +		bool required = false;
>> +		ret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,
>> +					     &required);
>> +		ret |= !required;
>
> I think we shouldn't use `|=` here, and furthermore I think we need to
> define semantics more specifically. E.g.:
>
>     - Return 0 in case optimization is required.
>
>     - Return 2 in case no optimization is required. I mostly shy away
>       from `1` here because it might already be used? We'll have to
>       double check though.
>
>     - Return any other non-zero error code in case an error occurs.
>
> This also needs to be documented.
>

Fair. So I checked and the only backend which returns an error is the reftable backend and this is explicitly only the `REFTABLE_OUT_OF_MEMORY_ERROR` error, which is equal to -13.

So let's do 0 for optimization required, 1 for no optimization required and other non-zero for error.

I'll modify accordingly.

Show 19 quoted lines

>> diff --git a/refs.c b/refs.c
>> index 514fb85af2..59a48b36b7 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
>>  	return refs->be->optimize(refs, opts);
>>  }
>>
>> +int refs_optimize_required(struct ref_store *refs,
>> +			   struct optimize_refs_opts *opts,
>> +			   bool *required)
>> +{
>> +	return refs->be->optimize_required(refs, opts, required);
>> +}
>> +
>
> I feel like this should be introduced in a preceding commit.
>
> Patrick

Yeah, It would be better there, will move it.