From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 06 Nov 2025 11:58:00 GMT
Subject: Re: [PATCH v3 4/5] maintenance: add checking logic in `pack_refs_condition()`
Message-ID: <aQyNSOdPWAxm15U3@pks.im>
In-Reply-To: <20251106-562-add-sub-command-to-check-if-maintenance-is-needed-v3-4-d611a2a95cf5@gmail.com>

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

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

Patrick