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

[Junio C Hamano <gitster@pobox.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.

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

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

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


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