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

[Junio C Hamano <gitster@pobox.com>]

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

> 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

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