From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 03 Nov 2025 14:00:27 GMT
Subject: Re: [PATCH 4/5] maintenance: add checking logic in `pack_refs_condition()`
Message-ID: <aQi1e0zWfRaxSKtz@pks.im>
In-Reply-To: <20251031-562-add-sub-command-to-check-if-maintenance-is-needed-v1-4-a03d53e28d0e@gmail.com>

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/

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

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

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

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

> +		.flags = REFS_OPTIMIZE_PRUNE | REFS_OPTIMIZE_AUTO,
> +	};
> +	bool required;
> +
> +	// Check for all refs, similar to 'git refs optimize --all'.

Style: this should use `/* */` comments.

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