From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 12 May 2025 14:34:22 GMT
Subject: Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
Message-ID: <35ecaa1d-589f-4d28-9d97-78bae8de2d41@gmail.com>
In-Reply-To: <xmqqcyckayb4.fsf@gitster.g>

On 5/7/25 5:46 PM, Junio C Hamano wrote:
> "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.

I suppose this could be fixed by changing the `<mode>` to be of the
form "[enable|disable|keep]".

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

My intention was to bend over backwards to prevent a behavior change
in the default case. However, I'm coming around to understand that
we don't need this background maintenance to be redone every time
and can become a no-op by default. (Other new configuration will
still happen.)

In the case where we're fine changing the default behavior, then
the standard --[no-]maintenance option will work, though it is a
three-way switch where the lack of its existence means "don't do
either mode".

I've got a new version of this patch doing what you asked for in
the first place.

Thanks,
-Stolee