From: Patrick Steinhardt <ps@pks.im>
To: David Lin <davidzylin@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, David Lin <davidlin@stripe.com>
Subject: Re: [PATCH] builtin/maintenance: accept "none" as a maintenance strategy
Date: Tue, 4 Aug 2026 14:58:55 +0200 [thread overview]
Message-ID: <anHiDzJbXUAgPRbO@pks.im> (raw)
In-Reply-To: <20260729194006.75317-1-davidlin@stripe.com>
On Wed, Jul 29, 2026 at 03:40:06PM -0400, David Lin wrote:
> Commit d465be2327 (builtin/maintenance: don't silently ignore invalid
> strategy, 2025-10-24) changed scheduled maintenance to error on an
> unknown maintenance strategy instead of silently defaulting to the
> `none` strategy.
>
> However, `parse_maintenance_strategy()` does not recognize `none`, so
> Git rejects a valid and documented strategy that can be used to override
> an existing strategy and disable maintenance tasks.
Oh, indeed.
> Accept `none` as a valid maintenance strategy and add tests to ensure
> it's accepted.
Makes sense. You can of course achieve the same thing by disabling
maintenance altogether, but it's a documented thing and users thus
rightfully expect the "none" strategy to exist.
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 46999a99ab..3d1e39d46a 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1922,6 +1922,8 @@ static const struct maintenance_strategy geometric_strategy = {
>
> static struct maintenance_strategy parse_maintenance_strategy(const char *name)
> {
> + if (!strcasecmp(name, "none"))
> + return none_strategy;
> if (!strcasecmp(name, "incremental"))
> return incremental_strategy;
> if (!strcasecmp(name, "gc"))
Yup, looks obviously correct.
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index a8d691719d..130c971b15 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -1022,6 +1022,9 @@ test_expect_success 'maintenance.strategy is respected' '
> test_must_fail git -c maintenance.strategy=unknown maintenance run 2>err &&
> test_grep "unknown maintenance strategy: .unknown." err &&
>
> + test_strategy none </dev/null &&
> + test_strategy none --schedule=weekly </dev/null &&
> +
> test_strategy incremental <<-\EOF &&
> git pack-refs --all --prune
> git reflog expire --all
And test looks obviously correct to me, too.
So other than Junio's remark about the SOB this patch looks good to me.
Thanks!
Patrick
prev parent reply other threads:[~2026-08-04 12:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-29 19:40 [PATCH] builtin/maintenance: accept "none" as a maintenance strategy David Lin
2026-07-30 2:58 ` Junio C Hamano
2026-08-04 12:58 ` Patrick Steinhardt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=anHiDzJbXUAgPRbO@pks.im \
--to=ps@pks.im \
--cc=davidlin@stripe.com \
--cc=davidzylin@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.