From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 06 Nov 2025 12:02:47 GMT
Subject: Re: [PATCH v3 5/5] maintenance: add 'is-needed' subcommand
Message-ID: <aQyOZ0e6HO0_77Au@pks.im>
In-Reply-To: <20251106-562-add-sub-command-to-check-if-maintenance-is-needed-v3-5-d611a2a95cf5@gmail.com>

On Thu, Nov 06, 2025 at 09:22:34AM +0100, Karthik Nayak wrote:
> diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc
> index 540b5cf68b..37939510d4 100644
> --- a/Documentation/git-maintenance.adoc
> +++ b/Documentation/git-maintenance.adoc
> @@ -84,6 +85,16 @@ 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.
> +    Ideally used with the '--auto' flag.
> ++
> +If one or more `--task` options	are specified, then those tasks are checked

I spoke too soon, forgot that there's one more patch :) s/\t/ /

> +in that order. Otherwise, the tasks are determined by which
> +`maintenance.<task>.enabled` config options are true. By default, only
> +`maintenance.gc.enabled` is true.

This could use a pointer to "maintenance.strategy", but I see that you
took this explanation from the "run" subcommand. I think this is good
enough for now.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index c3e7a84ec2..e5ba2a2e72 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -3253,7 +3253,59 @@ 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);
> +	if (argc)
> +		usage_with_options(builtin_maintenance_is_needed_usage, options);
> +
> +	gc_config(&cfg);
> +	initialize_task_config(&opts, &selected_tasks);
> +
> +	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;
> +			}
> +		}
> +	} else {
> +		/* When not using --auto, we should always require maintenance. */

Nit: we might add a TODO comment here.

    /*
     * When not using --auto we always require maintenance right now.
     *
     * TODO: this certainly is too eager, as some maintenance tasks may
     * decide to not do anything because the data structures are already
     * fully optimized. We may eventually want to extend the auto
     * condition to also cover non-auto runs so that we can detect such
     * cases.
     /

Patrick