Re: [PATCH v2 4/4] scalar reconfigure: add --no-maintenance option

From: Junio C Hamano <gitster@pobox.com>
Date: May 5, 2025, 21:47 UTC
Message-ID: <xmqqselin30f.fsf@gitster.g>
In-Reply-To: <6fac9c4c394b9882a2f7af6209af389edf384e4d.1746458844.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> From: Derrick Stolee <stolee@gmail.com>
>
> When users want to enable the latest and greatest configuration options
> recommended by Scalar after a Git upgrade, 'scalar reconfigure --all' is
> a great option that iterates over all repos in the multi-valued
> 'scalar.repos' config key.
>
> However, this feature previously forced users to enable background
> maintenance. In some environments this is not preferred.
>
> Add a new --[no-]maintenance option to 'scalar reconfigure' that avoids
> running 'git maintenance start' on these enlistments.

It makes sense for --maintenance option to be between enable and disable when registering a new directory to the system, and when cloning somebody else's repository that causes a new directory to be created and enlisting the resulting new directory to the system.

But wouldn't users want "leave maintenance-enrollment status alone" option when reconfiguring an existing already enlisted directory?

As written, the design easily allows enabling of maintenance as part of reconfiguring, but disabling cannot be done the same way (i.e. individual enlistments need to be visited and their maintenance disabled manually).

IOW, it is a bit counter-intuitive

Show 5 quoted lines

> +--[no-]maintenance::
> +	By default, Scalar configures the enlistment to use Git's
> +	background maintenance feature. Use the `--no-maintenance` to skip
> +	this configuration and leave the repositories in whatever state is
> +	currently configured.

that for clone and register, --maintenance means "enable" and "--no-maintenance" means "disable", but when reconfiguring an already registered directory, it would be natural to expect that "--no-maintenance" would explicitly tell the command to disable scheduled maintenance.

> -		if (toggle_maintenance(1) >= 0)
> +		if (maintenance &&
> +		    toggle_maintenance(1) >= 0)
>  			succeeded = 1;

A 3-way approach would make this part something like ...

	switch (maintenance) {
	default:	BUG("..."); break;
	case ENABLE:	res = toggle_maintenance(1); break;
	case DISABLE:	res = toggle_maintenance(0); break;
	case ASIS:	res = 0; break;
	}
	if (res >= 0)
		succeeded = 1;

... which would allow people to easily say "leave the existing maintenance state alone".

I dunno.