Re: [PATCH 4/5] maintenance: add checking logic in `pack_refs_condition()`

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3426 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Oct 31, 2025 at 03:22:24PM +0100, Karthik Nayak wrote:
>> The 'git-maintenance(1)' command support an '--auto' flag. Usage of the
>
> s/support/&s/
>

Ah, will change.

>> flag ensures to run maintenance tasks only if certain thresholds are
>> met. The heuristic is defined on a task level, wherein each task defines
>> a 'auto_condition', which states if the task should be run.
>
> s/a/an/

Yup, thanks!

>
>> The 'pack-refs' task is hard-coded to return 1 as:
>> 1. There was never a way to check if the reference backend needs to be
>> optimized without actually performing the optimization.
>> 2. We can pass in the '--auto' flag to 'git-pack-refs(1)' which would
>> optimize based on heuristics.
>>
>> The previous commit added a `refs_optimize_required()` function, which
>> can be used to check if a reference backend required optimization. Use
>> this within `pack_refs_condition()`.
>>
>> This allows us to add a 'git maintenance is-needed' subcommand which can
>> notify the user if maintenance is needed without actually performing the
>> optimization, without this change, the reference backend would always
>
> s/optimize, without/optimize. Without/
>

Thanks, this is better.

>> state that optimization is needed.
>>
>> Since we import 'revision.h', we need to remove the definition for
>> 'SEEN' which is duplicated in the included header.
>
> Quite weird that it was redefined in the first place. Feels like a nice
> side effect.
>

Indeed.

>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index c6d62c74a7..72177305ff 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -285,12 +286,26 @@ static void maintenance_run_opts_release(struct maintenance_run_opts *opts)
>>
>>  static int pack_refs_condition(UNUSED struct gc_config *cfg)
>>  {
>> -	/*
>> -	 * The auto-repacking logic for refs is handled by the ref backends and
>> -	 * exposed via `git pack-refs --auto`. We thus always return truish
>> -	 * here and let the backend decide for us.
>> -	 */
>> -	return 1;
>> +	struct string_list included_refs = STRING_LIST_INIT_NODUP;
>> +	struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
>> +	struct refs_optimize_opts optimize_opts = {
>> +		.exclusions = &excludes,
>> +		.includes = &included_refs,
>
> A bit weird that we have to declare these two fields even though we
> don't really care for either of them. But I don't mind that too much.
>

Yeah, I think there is some cleanup to be done in the files backend. But
I don't think it should be part of this series. If we don't add these,
we crash with a SIGSEGV.

>> +		.flags = REFS_OPTIMIZE_PRUNE | REFS_OPTIMIZE_AUTO,
>> +	};
>> +	bool required;
>> +
>> +	// Check for all refs, similar to 'git refs optimize --all'.
>
> Style: this should use `/* */` comments.
>

Thanks, will fix.

>> +	string_list_append(optimize_opts.includes, "*");
>> +
>> +	if (refs_optimize_required(get_main_ref_store(the_repository),
>> +				   &optimize_opts, &required))
>> +		return 0;
>> +
>> +	clear_ref_exclusions(&excludes);
>> +	string_list_clear(&included_refs, 0);
>> +
>> +	return required;
>
> You return a boolean, but the function is declared to return an integer.
> This works, but it feels wrong.
>
> Patrick

I get what you're saying but returning `required == true` also feel like
a bool return to me (even though it is an int in C).

Anyways, I'll make the change. I don't care much for either.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]