From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 04 Nov 2025 05:54:06 GMT
Subject: Re: [PATCH 5/5] maintenance: add 'is-needed' subcommand
Message-ID: <aQmU_hOPO55_ojw2@pks.im>
In-Reply-To: <CAOLa=ZSsEygvz1_aj4KomfF0Jo0vJi3yVLtJbhLX=RLgW6_GzQ@mail.gmail.com>

On Mon, Nov 03, 2025 at 09:18:35AM -0800, Karthik Nayak wrote:
> 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.

But we use `initialize_task_config()`, and that function knows to use
the configured strategy unless it's given an explicit list of tasks. So
we do use the maintenance strategy.

> >> diff --git a/builtin/gc.c b/builtin/gc.c
> >> index 72177305ff..4d20487ed6 100644
> >> --- a/builtin/gc.c
> >> +++ b/builtin/gc.c
[snip]
> >> +	} 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.

Yeah, agreed.

I guess eventually we could extend `auto_condition()` to honor the
"--auto" flag:

  - If it's set the task verifies that it needs to trigger housekeeping
    tasks with heuristics.

  - Otherwise it checks whether there even is anything that could be
    cleaned up.

But that's certainly out of scope of this patch series, I think it's
good enough to bail on that specific part for now.

Patrick