From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 03 Nov 2025 14:00:35 GMT
Subject: Re: [PATCH 5/5] maintenance: add 'is-needed' subcommand
Message-ID: <aQi1g9TX7FoDgo9n@pks.im>
In-Reply-To: <20251031-562-add-sub-command-to-check-if-maintenance-is-needed-v1-5-a03d53e28d0e@gmail.com>

On Fri, Oct 31, 2025 at 03:22:25PM +0100, Karthik Nayak wrote:
> diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc
> index 540b5cf68b..edcc88f4d0 100644
> --- a/Documentation/git-maintenance.adoc
> +++ b/Documentation/git-maintenance.adoc
> @@ -84,6 +85,11 @@ The `unregister` subcommand will report an error if the current repository
>  is not already registered. Use the `--force` option to return success even
>  when the current repository is not registered.
>  
> +is-needed::
> +    Check whether maintenance needs to be run without actually running it.
> +    Exits with a 0 status code if maintenance needs to be run, 1 otherwise.
> +    Can be used along with `--task`. Ideally should be used with '--auto'.

Okay. I assume when `--task` is not given we'll check all tasks
specified by the configured strategy? Might make sense to document if
so.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 72177305ff..4d20487ed6 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -3253,7 +3253,60 @@ static int maintenance_stop(int argc, const char **argv, const char *prefix,
>  	return update_background_schedule(NULL, 0);
>  }
>  
> -static const char * const builtin_maintenance_usage[] = {
> +static const char *const builtin_maintenance_is_needed_usage[] = {
> +	"git maintenance is-needed [--task=<task>] [--schedule]",
> +	NULL
> +};
> +
> +static int maintenance_is_needed(int argc, const char **argv, const char *prefix,
> +				 struct repository *repo UNUSED)
> +{
> +	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
> +	struct string_list selected_tasks = STRING_LIST_INIT_DUP;
> +	struct gc_config cfg = GC_CONFIG_INIT;
> +	struct option options[] = {
> +		OPT_BOOL(0, "auto", &opts.auto_flag,
> +			 N_("run tasks based on the state of the repository")),
> +		OPT_CALLBACK_F(0, "task", &selected_tasks, N_("task"),
> +			       N_("check a specific task"),
> +			       PARSE_OPT_NONEG, task_option_parse),
> +		OPT_END()
> +	};
> +	bool is_needed = false;
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     builtin_maintenance_is_needed_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	gc_config(&cfg);
> +	initialize_task_config(&opts, &selected_tasks);
> +
> +	if (argc)
> +		usage_with_options(builtin_maintenance_is_needed_usage, options);

Shouldn't this check be directly after the call to `parse_options()`?

> +	if (opts.auto_flag) {
> +		for (size_t i = 0; i < opts.tasks_nr; i++) {
> +			if (tasks[opts.tasks[i]].auto_condition &&
> +			    tasks[opts.tasks[i]].auto_condition(&cfg)) {
> +				is_needed = true;
> +				break;
> +			}
> +		}

Okay, we need to guard against the auto-condition not existing indeed.
This is only due to the "prefetch" task though, all the others do have
the callback.

> +	} else {
> +		/* When not using --auto, we should always require maintenance. */
> +		is_needed = true;
> +	}

I guess for now this is good enough, but it's not quite true. Some tasks
won't require maintenance even without `--auto`, like for example when
the reftable stack only has a single table.

Patrick