Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2025, 21:46 UTC
Message-ID: <xmqqcyckayb4.fsf@gitster.g>
In-Reply-To: <684f04aaf7e87f22ab0b00a4fd42d2943304ef04.1746582637.git.gitgitgadget@gmail.com>

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

> Add a new --maintenance=<mode> option to 'scalar reconfigure' that
> provides options for enabling (default), disabling, or leaving
> background maintenance config as-is.

Hmph, this is a bit unexpected.

Show 8 quoted lines

> +--maintenance=<mode>::
> +	By default, Scalar configures the enlistment to use Git's
> +	background maintenance feature; this is the same as using the
> +	`--maintenance=enable` value for this option. Use the
> +	`--maintenance=disable` to remove each considered enlistment
> +	from background maintenance. Use `--maitnenance=keep' to leave
> +	the background maintenance configuration untouched for These
> +	repositories.

If I understand it correctly, here is the only place that the users can learn what the valid choices are, and it is not even an enumeration. They are forced to read the entire paragraph to learn what the choices are.

> +		OPT_STRING(0, "maintenance", &maintenance_str,
> +			 N_("<mode>"),
> +			 N_("signal how to adjust background maintenance")),

And there is no hint what are the right <mode> strings are.

Show 5 quoted lines

>  	const char * const usage[] = {
> -		N_("scalar reconfigure [--all | <enlistment>]"),
> +		N_("scalar reconfigure [--maintenance=<mode>] [--all | <enlistment>]"),
>  		NULL
>  	};

So "scalar reconfigure -h" would not tell readers what the right choices are, either.

Show 10 quoted lines

> +	if (maintenance_str) {
> +		if (!strcmp(maintenance_str, "enable"))
> +			maintenance = 1;
> +		else if (!strcmp(maintenance_str, "disable"))
> +			maintenance = 0;
> +		else if (!strcmp(maintenance_str, "keep"))
> +			maintenance = -1;
> +		else
> +			die(_("unknown mode for --maintenance option: %s"),
> +			    maintenance_str);

Those who say "scalar reconfigure --maintenance=yes" gets this message that tells 'yes' is not a known mode, without saying that they meant 'enable'.

The --opt=<mode> interface is good when we expect the vocabulary for <mode> to grow, but I am not sure if it is warranted in this case. Is there a strong reason why 'reconfigure' MUST enable the maintenance by default, even if it were originally disabled in the enlistment? If there isn't, initializing maintenance to -1 and setting it with OPT_BOOL() would make the UI consistent with the register and clone subcommands, and also we can lose the above block to parse out a string. Also the code below ...

Show 8 quoted lines

> @@ -758,7 +776,8 @@ static int cmd_reconfigure(int argc, const char **argv)
>  		the_repository = old_repo;
>  		repo_clear(&r);
>  
> -		if (toggle_maintenance(1) >= 0)
> +		if (maintenance >= 0 &&
> +		    toggle_maintenance(maintenance) >= 0)
>  			succeeded = 1;

... which does make perfect sense, would still be applicable.

I dunno. I just feel that 3-way "mode" interface is too much hassle to get right (e.g., give hints to guide the users who forgot what modes are available and how they are spelled) for this code path.

Anyway, will replace the previous round with this one.

Thanks.