Re: [PATCH 5/5] maintenance: add 'is-needed' subcommand
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Nov 3, 2025, 14:00 UTC
- 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:
Show 12 quoted lines
> 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.
Show 39 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()`?
Show 8 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.
> + } 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