Re: [PATCH 5/5] maintenance: add 'is-needed' subcommand
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Nov 3, 2025, 17:18 UTC
- Message-ID
- <CAOLa=ZSsEygvz1_aj4KomfF0Jo0vJi3yVLtJbhLX=RLgW6_GzQ@mail.gmail.com>
- In-Reply-To
- <aQi1g9TX7FoDgo9n@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 18 quoted lines
> 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.
Show 42 quoted lines
>> 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.
Show 13 quoted lines
>> + 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.
Show 10 quoted lines
>> + } 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.
>
> PatrickGood 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.