Re: [PATCH 5/5] maintenance: add 'is-needed' subcommand

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

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

Yes, and the default strategy is to run 'gc'.

I miss-read your earlier comment, you were talking about the configured
strategy. I thought you were asking if no '--task' is given, we'd run
all available tasks.

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

I think that's a fair conclusion. I'll leave this as is for now then.

Thanks for the review.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]