From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 06 Nov 2025 13:04:25 GMT
Subject: Re: [PATCH v3 4/5] maintenance: add checking logic in `pack_refs_condition()`
Message-ID: <CAOLa=ZQ18H8WCp_m=7rzWt1HRRiWv5Ag63ceV2r=_BwyttyK5w@mail.gmail.com>
In-Reply-To: <aQyNSOdPWAxm15U3@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Nov 06, 2025 at 09:22:33AM +0100, Karthik Nayak wrote:
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index c6d62c74a7..c3e7a84ec2 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,
>> +		.flags = REFS_OPTIMIZE_PRUNE | REFS_OPTIMIZE_AUTO,
>> +	};
>> +	bool required;
>> +
>> +	/* Check for all refs, similar to 'git refs optimize --all'. */
>> +	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 == true;
>
> Tiny nit: I think in our codebase this can be written in a more
> idiomatic way by saying `!!required`.
>

Fair. Will change.

> Other than that I don't have anything more to add to this series.
> Thanks!
>
> Patrick

Thanks for your review!