From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 03 Nov 2025 17:18:35 GMT
Subject: Re: [PATCH 5/5] maintenance: add 'is-needed' subcommand
Message-ID: <CAOLa=ZSsEygvz1_aj4KomfF0Jo0vJi3yVLtJbhLX=RLgW6_GzQ@mail.gmail.com>
In-Reply-To: <aQi1g9TX7FoDgo9n@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

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

Actually no. It's similar to the 'run' command, if nothing is specified,
we check `maintenance.<task>.enabled`. By default it is only enabled for
'gc'. This is important information, I will add it in.

>> 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()`?
>

Yes, I moved it around, will fix it.

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

Yup, that's correct.

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

Good point. Thought I'm not sure how we'd go about it. Initially I
wanted to not have an `--auto` flag and simply make it the default
behavior. But that would restrict us from introducing the `schedule`
flag in the future. Which I think might be a worthwhile addition.