scalar: add --no-maintenance option

[derrickstolee]

These patches add a new --no-maintenance option to the scalar register and scalar clone commands. My motivation is based on setting up Scalar clones in automated environments that set up a repo onto a disk image for use later. If background maintenance runs during later setup steps, then this introduces a variable that is unexpected at minimum and disruptive at worst. The disruption comes in if the automation has steps to run git maintenance run --task=<X> commands but those commands are blocked due to the maintenance.lock file.

Functionally, these leave the default behavior as-is but allow disabling the git maintenance start step when users opt-in to this difference. The idea of Scalar is to recommend the best practices for a typical user, but allowing customization for expert users.

Updates in v2

• The previous use of toggle_maintenance() in register_dir() would run the 'git maintenance unregister --force' command. There is a new patch 1 that is explicit about whether this should or should not happen and new tests are added to verify this behavior in the later patches.
• A new patch 4 adds the --[no-]maintenance option to scalar reconfigure.

Updates in v3

• Patch 4 converts the --[no-]maintenance option of scalar reconfigure to --maintenance=<mode> to keep the default behavior the same (enable maintenance) but also allow two other modes: disable maintenance and leave maintenance as configured.

Thanks,
-Stolee

cc: [email protected]
cc: [email protected]
cc: Patrick Steinhardt [email protected]

[derrickstolee]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1913/derrickstolee/scalar-no-maintenance-v1

To fetch this version to local tag pr-1913/derrickstolee/scalar-no-maintenance-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1913/derrickstolee/scalar-no-maintenance-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> These patches add a new --no-maintenance option to the scalar register and
> scalar clone commands. My motivation is based on setting up Scalar clones in
> automated environments that set up a repo onto a disk image for use later.
> If background maintenance runs during later setup steps, then this
> introduces a variable that is unexpected at minimum and disruptive at worst.
> The disruption comes in if the automation has steps to run git maintenance
> run --task=<X> commands but those commands are blocked due to the
> maintenance.lock file.
>
> Functionally, these leave the default behavior as-is but allow disabling the
> git maintenance start step when users opt-in to this difference. The idea of
> Scalar is to recommend the best practices for a typical user, but allowing
> customization for expert users.

The feature itself I do not have any objections to and I found the
reasoning given above very sound.

With these two patches, we still have an unconditional call to
toggle_maintenance(1) in cmd_reconfigure().  Shoudln't the call be
at least removed, which would mean that reconfiguring would not
change the auto-maintenance states, or made controllable from the
command line of "maintenance reconfigure"?

It somehow looks to me that the real culprit of making the result of
applying two patches still unsatisfactory is the original design to
have the toggle_maintenance() call made from inside register_dir()
in the first place.  Shouldn't a much higher layer entry points like
cmd_register() and cmd_clone() be where the decision is made if
maintenance task should be set up (or not set up) by calling
toggle_maintenance(), leaving the register_dir() responsible only
for "enlist the directory to the system"?

IOW it feels to me that enabling (and now optionally disabling)
maintenance is tied too deeply into the act of enlisting a
directory; if we need to disable maintenance (and a mode to add
enlistment without enabling maintenance), it is a sign that it
shouldn't be a parameter into the register_dir() function that
controls what register_dir() does, and rather it should be done by
letting the caller who calls register_dir() decide to call (or not)
toggle_maintenance().

Thanks.

[gitgitgadget]

This patch series was integrated into seen via git@cc522f2.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/30/2025 4:28 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> 
>> These patches add a new --no-maintenance option to the scalar register and
>> scalar clone commands. My motivation is based on setting up Scalar clones in
>> automated environments that set up a repo onto a disk image for use later.
>> If background maintenance runs during later setup steps, then this
>> introduces a variable that is unexpected at minimum and disruptive at worst.
>> The disruption comes in if the automation has steps to run git maintenance
>> run --task=<X> commands but those commands are blocked due to the
>> maintenance.lock file.
>>
>> Functionally, these leave the default behavior as-is but allow disabling the
>> git maintenance start step when users opt-in to this difference. The idea of
>> Scalar is to recommend the best practices for a typical user, but allowing
>> customization for expert users.
> 
> The feature itself I do not have any objections to and I found the
> reasoning given above very sound.
> 
> With these two patches, we still have an unconditional call to
> toggle_maintenance(1) in cmd_reconfigure().  Shoudln't the call be
> at least removed, which would mean that reconfiguring would not
> change the auto-maintenance states, or made controllable from the
> command line of "maintenance reconfigure"?

I agree that this was a miss on my part. Thanks for the careful eye.
 
> It somehow looks to me that the real culprit of making the result of
> applying two patches still unsatisfactory is the original design to
> have the toggle_maintenance() call made from inside register_dir()
> in the first place.  Shouldn't a much higher layer entry points like
> cmd_register() and cmd_clone() be where the decision is made if
> maintenance task should be set up (or not set up) by calling
> toggle_maintenance(), leaving the register_dir() responsible only
> for "enlist the directory to the system"?
> 
> IOW it feels to me that enabling (and now optionally disabling)
> maintenance is tied too deeply into the act of enlisting a
> directory; if we need to disable maintenance (and a mode to add
> enlistment without enabling maintenance), it is a sign that it
> shouldn't be a parameter into the register_dir() function that
> controls what register_dir() does, and rather it should be done by
> letting the caller who calls register_dir() decide to call (or not)
> toggle_maintenance().
I will continue thinking about this while playing with different
options for a v2. My gut reaction is that register_dir() is our
centralized way to "set up recommended configuration" which includes
config options and background maintenance. We're now introducing a
way to customize this centralized operation based on decentralized
options, hence a new option to the method.

Is the right solution to move the toggle_maintenance() out of
register_dir()? If this is the only way we plan to customize the
config, then yes. Otherwise, the second or third customization will
start to lead to copied logic through these three locations.

Again, I'm mostly thinking out loud here before I go and dig into
the code. I'll report back with v2.

Thanks,
-Stolee

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <[email protected]> writes:

> Is the right solution to move the toggle_maintenance() out of
> register_dir()? If this is the only way we plan to customize the
> config, then yes. Otherwise, the second or third customization will
> start to lead to copied logic through these three locations.

It is mostly philosophical, I think, but I actually think the
callers that are allowed to be different is a good thing.  The
callers can pass different parameters to register_dir(), but the
distinction between these different callers would become more subtle
and not immediately obvious to readers.  With an explicit call to
toggle_maintenance() at each callsite, it becomes more obvious to
see who sets up the maintenance job and how.

If this is and will stay the only way, I would not care too much
either way, but if we are planning to extend, then I would say that
it is more important to allow callers to be more explicit.

Besides, you'd need to call toggle_maintenance() to disable it in a
caller outside register_dir(), so it is not like you can hide the
tweaks from readers of the code and make it appear to be simpler.
They need to be aware of what goes on anyway.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes:

> It is mostly philosophical, I think, ...

In other words, I think making each helper function do one thing and
one thing well is a good thing, and "register" should focus on
registering the new repository to the system.

By the way, it is excellent that the new option honors the positive
form (i.e. "clone --maintenance" and "clone --no-maintenance").  I
was re-reading the patches and was pleasantly surprised, as I would
have forgotten to do so while focusing too much on the shiny new
feature of being able to disable if I were doing these patches.

[gitgitgadget]

This branch is now known as ds/scalar-no-maintenance.

[gitgitgadget]

This patch series was integrated into seen via git@bfe24fd.

[gitgitgadget]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Apr 30, 2025 at 10:24:39AM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
> index 7e4259c6743f..b2b244a86499 100644
> --- a/Documentation/scalar.adoc
> +++ b/Documentation/scalar.adoc
> @@ -11,7 +11,7 @@ SYNOPSIS
>  scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
>  	[--[no-]src] <url> [<enlistment>]
>  scalar list
> -scalar register [<enlistment>]
> +scalar register [--[no-]maintenance] [<enlistment>]
>  scalar unregister [<enlistment>]
>  scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
>  scalar reconfigure [ --all | <enlistment> ]
> @@ -117,6 +117,12 @@ Note: when this subcommand is called in a worktree that is called `src/`, its
>  parent directory is considered to be the Scalar enlistment. If the worktree is
>  _not_ called `src/`, it itself will be considered to be the Scalar enlistment.
>  
> +--[no-]maintenance::
> +	By default, `scalar register` configures the enlistment to use Git's
> +	background maintenance feature. Use the `--no-maintenance` to skip
> +	this configuration. This does not disable any maintenance that may
> +	already be enabled in other ways.
> +
>  Unregister
>  ~~~~~~~~~~
>  
> diff --git a/scalar.c b/scalar.c
> index d359f08bb8e2..2a21fd55f39b 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -259,7 +259,7 @@ static int stop_fsmonitor_daemon(void)
>  	return 0;
>  }
>  
> -static int register_dir(void)
> +static int register_dir(int maintenance)
>  {
>  	if (add_or_remove_enlistment(1))
>  		return error(_("could not add enlistment"));
> @@ -267,7 +267,7 @@ static int register_dir(void)
>  	if (set_recommended_config(0))
>  		return error(_("could not set recommended config"));
>  
> -	if (toggle_maintenance(1))
> +	if (toggle_maintenance(maintenance))
>  		warning(_("could not turn on maintenance"));
>  
>  	if (have_fsmonitor_support() && start_fsmonitor_daemon()) {

Isn't this change contrary to what the docs say? `toggle_maintenance(0)`
would cause us to execute `git maintenance unregister --force`, which
deregisters maintenance for us.

> @@ -597,11 +597,14 @@ static int cmd_list(int argc, const char **argv UNUSED)
>  
>  static int cmd_register(int argc, const char **argv)
>  {
> +	int maintenance = 1;
>  	struct option options[] = {
> +		OPT_BOOL(0, "maintenance", &maintenance,
> +			 N_("specify if background maintenance should be enabled")),

Maybe s/if/whether/? Might just be me not being a native speaker,
though.

> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
> index a81662713eb8..a488f72de9fe 100755
> --- a/t/t9210-scalar.sh
> +++ b/t/t9210-scalar.sh
> @@ -129,6 +129,13 @@ test_expect_success 'scalar unregister' '
>  	scalar unregister vanish
>  '
>  
> +test_expect_success 'scalar register --no-maintenance' '
> +	git init register-no-maint &&
> +	GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
> +		scalar register --no-maintenance register-no-maint 2>err &&
> +	test_must_be_empty err
> +'
> +

We should probably have a test that verifies that we don't deregister
maintenance.

Patrick

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 5/2/2025 5:15 AM, Patrick Steinhardt wrote:
> On Wed, Apr 30, 2025 at 10:24:39AM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
>> index 7e4259c6743f..b2b244a86499 100644
>> --- a/Documentation/scalar.adoc
>> +++ b/Documentation/scalar.adoc
>> @@ -11,7 +11,7 @@ SYNOPSIS
>>  scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
>>  	[--[no-]src] <url> [<enlistment>]
>>  scalar list
>> -scalar register [<enlistment>]
>> +scalar register [--[no-]maintenance] [<enlistment>]
>>  scalar unregister [<enlistment>]
>>  scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
>>  scalar reconfigure [ --all | <enlistment> ]
>> @@ -117,6 +117,12 @@ Note: when this subcommand is called in a worktree that is called `src/`, its
>>  parent directory is considered to be the Scalar enlistment. If the worktree is
>>  _not_ called `src/`, it itself will be considered to be the Scalar enlistment.
>>  
>> +--[no-]maintenance::
>> +	By default, `scalar register` configures the enlistment to use Git's
>> +	background maintenance feature. Use the `--no-maintenance` to skip
>> +	this configuration. This does not disable any maintenance that may
>> +	already be enabled in other ways.

>> -	if (toggle_maintenance(1))
>> +	if (toggle_maintenance(maintenance))
>>  		warning(_("could not turn on maintenance"));
>>  
>>  	if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
> 
> Isn't this change contrary to what the docs say? `toggle_maintenance(0)`
> would cause us to execute `git maintenance unregister --force`, which
> deregisters maintenance for us.

You are right. I've confirmed this using 'test_subcommand' in my test, so I'll
plumb the ability to skip toggle_maintenance() in certain cases. I'll carefully
document this in code as well.

Thanks for the careful eye. 
>> @@ -597,11 +597,14 @@ static int cmd_list(int argc, const char **argv UNUSED)
>>  
>>  static int cmd_register(int argc, const char **argv)
>>  {
>> +	int maintenance = 1;
>>  	struct option options[] = {
>> +		OPT_BOOL(0, "maintenance", &maintenance,
>> +			 N_("specify if background maintenance should be enabled")),
> 
> Maybe s/if/whether/? Might just be me not being a native speaker,
> though.

I like your choice here.

>> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
>> index a81662713eb8..a488f72de9fe 100755
>> --- a/t/t9210-scalar.sh
>> +++ b/t/t9210-scalar.sh
>> @@ -129,6 +129,13 @@ test_expect_success 'scalar unregister' '
>>  	scalar unregister vanish
>>  '
>>  
>> +test_expect_success 'scalar register --no-maintenance' '
>> +	git init register-no-maint &&
>> +	GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
>> +		scalar register --no-maintenance register-no-maint 2>err &&
>> +	test_must_be_empty err
>> +'
>> +
> 
> We should probably have a test that verifies that we don't deregister
> maintenance.
Yes, will do. The currently-passing test that confirms the unregister is
happening would look like this:

test_expect_success 'scalar register --no-maintenance' '
	git init register-no-maint &&
	event_log="$(pwd)/no-maint.event" &&
	GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
	GIT_TRACE2_EVENT="$event_log" \
	GIT_TRACE2_EVENT_DEPTH=100 \
		scalar register --no-maintenance register-no-maint 2>err &&
	test_must_be_empty err &&
	test_subcommand git maintenance unregister --force <no-maint.event
'When using test_subcommand, it's really important to verify that this kindof test passes before changing the behavior and turning the test into a
negation, since it's easier to accidentally "pass" a negative test if the
test is malformed.

Thanks,
-Stolee

[gitgitgadget]

User Patrick Steinhardt <[email protected]> has been added to the cc: list.

[gitgitgadget]

This patch series was integrated into seen via git@62ce17a.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch ds/scalar-no-maintenance on the Git mailing list:

Two "scalar" subcommands that adds a repository that hasn't been
under "scalar"'s control are taught an option not to enable the
scheduled maintenance on it.

Expecting a reroll.
cf. <[email protected]>
source: <[email protected]>

[derrickstolee]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1913/derrickstolee/scalar-no-maintenance-v2

To fetch this version to local tag pr-1913/derrickstolee/scalar-no-maintenance-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1913/derrickstolee/scalar-no-maintenance-v2

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> From: Derrick Stolee <[email protected]>
>
> 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.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 5/5/2025 5:47 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> 
>> From: Derrick Stolee <[email protected]>
>>
>> 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.

I can see how this command is different from the other two, and thus
a three-way flipper can actually result in three different behaviors:

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

This does mean that we'd need to have a different toggle from the
typical OPT_BOOL().

What do you think about something of the form --maintenance=<option>
where <option> is one of these:

 * "enable" (default) runs 'git maintenance start'
 * "disable" runs 'git maintenance unregister'
 * "keep" does not mess with maintenance config.

Does this sort of option seem to make sense? I'll wait to see if
any further adjustments are recommended before I start rolling a
new version.

Thanks,
-Stolee

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <[email protected]> writes:

> What do you think about something of the form --maintenance=<option>
> where <option> is one of these:
>
>  * "enable" (default) runs 'git maintenance start'
>  * "disable" runs 'git maintenance unregister'
>  * "keep" does not mess with maintenance config.

I think it makes superb sense.  

Certainly much less ambiguous than "--[no-]maintenance" given that
we need to handle "reconfigure".  Without the need to deal with
"reconfigure", it certainly is attractive if we can treat it as a
simple Boolean, though.

It is also tempting to just initialize the internal variable to -1
and keep using OPT_BOOL() though.  Then after config and command
line parsing is done, clone and register would turn -1 the user did
not touch into 1 (i.e. enable is default for these two operations),
while reconfigure treats -1 as "leave it as-is".  It would make it
very cumbersome if we ever change our mind and give a default other
than "leave it as-is" to "reconfigure", but other than that minor
downside, it may be easier to use from end-user's point of view.

I have no strong opinion between the two.

Thanks.





    

[gitgitgadget]

This patch series was integrated into seen via git@68cd517.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/scalar-no-maintenance on the Git mailing list:

Two "scalar" subcommands that adds a repository that hasn't been
under "scalar"'s control are taught an option not to enable the
scheduled maintenance on it.

Will merge to 'next'?
source: <[email protected]>

[gitgitgadget]

This patch series was integrated into seen via git@1d646df.

[derrickstolee]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1913/derrickstolee/scalar-no-maintenance-v3

To fetch this version to local tag pr-1913/derrickstolee/scalar-no-maintenance-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1913/derrickstolee/scalar-no-maintenance-v3

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> 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.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 5/7/25 5:46 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> 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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <[email protected]> writes:

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

Ahh, OK.  I misread your intention.

If it is common for existing users to disable maintenance, perhaps
by mistake, together with configuration changes that are not quite
right, perhaps also by mistake, and if they used reconfigure to
recover from such mistakes, it indeed may make sense to nuke the
current setting and enable maintenance unconditionally.

As you suggested in a part of your response I omitted, we can
annotate <mode> to give hints on the valid choices to help users,
without changing the default behaviour.  I am personally fine either
way, as long as we clearly document the reasoning behind our design.

Thanks.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 5/12/2025 1:44 PM, Junio C Hamano wrote:
> Derrick Stolee <[email protected]> writes:
> 
>> 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".
> 
> Ahh, OK.  I misread your intention.
> 
> If it is common for existing users to disable maintenance, perhaps
> by mistake, together with configuration changes that are not quite
> right, perhaps also by mistake, and if they used reconfigure to
> recover from such mistakes, it indeed may make sense to nuke the
> current setting and enable maintenance unconditionally.

The original intent of updating background maintenance in the
"reconfigure" command was primarily about updating the schedule, if
needed.

The recent bug in the macOS scheduler is a good example of this.
Users will need to rerun 'git maintenance start' somewhere in order
to get the "correct" schedule. This bug only needs this run for any
single repo, which makes the 'scalar reconfigure -a' command a
somewhat strange place to get the fix.

> As you suggested in a part of your response I omitted, we can
> annotate <mode> to give hints on the valid choices to help users,
> without changing the default behaviour.  I am personally fine either
> way, as long as we clearly document the reasoning behind our design.

I'll create a new patch on top of the current series version that
does this, calling it out as an intentional pattern. It's previously
been used by these examples:

 * --fixup=[(amend|reword):]<commit>
 * --diff-filter=[(A|C|D|M|R|T|U|X|B)...[*]]
 * --tool=[g,n,]vimdiff
 * --exclude-hidden=[fetch|receive|uploadpack]

One place where this kind of notation could be helpful, but appears
to be absent, is the '-L(<n>:<m>)|(:<method>):<file>' argument for
'git log' and 'git rev-list'. Perhaps this is too dense, though, so
it would be better split into '-L<n>:<m>:<file>' and
'-L:<method>:<file>'.

Thanks,
-Stolee

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <[email protected]> writes:

>> As you suggested in a part of your response I omitted, we can
>> annotate <mode> to give hints on the valid choices to help users,
>> without changing the default behaviour.  I am personally fine either
>> way, as long as we clearly document the reasoning behind our design.
>
> I'll create a new patch on top of the current series version that
> does this, calling it out as an intentional pattern. It's previously
> been used by these examples:
>
>  * --fixup=[(amend|reword):]<commit>
>  * --diff-filter=[(A|C|D|M|R|T|U|X|B)...[*]]
>  * --tool=[g,n,]vimdiff
>  * --exclude-hidden=[fetch|receive|uploadpack]

Yup, these are good things to have in "git cmd -h" to help users jog
their memory what the available choices are.  We do not have to
always verbosely explain what these mean everywhere, of course, but
if we said in "git commit -h" something like

    --fixup=<choice>

that would be almost hostile to the users.  And in documentation
pages, of course we can describe what each of the available choices
mean.

> One place where this kind of notation could be helpful, but appears
> to be absent, is the '-L(<n>:<m>)|(:<method>):<file>' argument for
> 'git log' and 'git rev-list'. Perhaps this is too dense, though, so
> it would be better split into '-L<n>:<m>:<file>' and
> '-L:<method>:<file>'.

Yup.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/scalar-no-maintenance on the Git mailing list:

Two "scalar" subcommands that adds a repository that hasn't been
under "scalar"'s control are taught an option not to enable the
scheduled maintenance on it.

Will merge to 'next'.
source: <[email protected]>

[gitgitgadget]

This patch series was integrated into seen via git@8c8d7d0.

[gitgitgadget]

This patch series was integrated into seen via git@96fcc68.

[gitgitgadget]

This patch series was integrated into next via git@1006cdd.

[gitgitgadget]

This patch series was integrated into seen via git@caf5766.

[gitgitgadget]

This patch series was integrated into seen via git@cdcb281.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/scalar-no-maintenance on the Git mailing list:

Two "scalar" subcommands that adds a repository that hasn't been
under "scalar"'s control are taught an option not to enable the
scheduled maintenance on it.

Will merge to 'master'.
source: <[email protected]>

[gitgitgadget]

This patch series was integrated into seen via git@920a7a3.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

From 0f5dc1cb6d697c7d8d3c126f3640c2f58fcfda43 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <[email protected]>
Date: Wed, 14 May 2025 09:50:32 -0400
Subject: [PATCH 5/4] scalar reconfigure: improve --maintenance docs

The --maintenance option for 'scalar reconfigure' has three possible
values. Improve the documentation by specifying the option in the -h
help menu and usage information.

Signed-off-by: Derrick Stolee <[email protected]>
---

Adding this extra patch on top to improve the docs. I could resend
as a full v4 if needed.

Thanks,
-Stolee


 Documentation/scalar.adoc | 13 ++++++-------
 scalar.c                  |  4 ++--
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
index 387527be1ea..4bd5b150e8e 100644
--- a/Documentation/scalar.adoc
+++ b/Documentation/scalar.adoc
@@ -14,7 +14,7 @@ scalar list
 scalar register [--[no-]maintenance] [<enlistment>]
 scalar unregister [<enlistment>]
 scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
-scalar reconfigure [--maintenance=<mode>] [ --all | <enlistment> ]
+scalar reconfigure [--maintenance=(enable|disable|keep)] [ --all | <enlistment> ]
 scalar diagnose [<enlistment>]
 scalar delete <enlistment>

@@ -165,14 +165,13 @@ reconfigure the enlistment.
 	registered with Scalar by the `scalar.repo` config key. Use this
 	option after each upgrade to get the latest features.

---maintenance=<mode>::
+--maintenance=(enable|disable|keep)::
 	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.
+	`enable` value for this option. Use the	`disable` value to
+	remove each considered enlistment from background maintenance.
+	Use `keep' to leave the background maintenance configuration
+	untouched for these repositories.

 Diagnose
 ~~~~~~~~
diff --git a/scalar.c b/scalar.c
index 847d2dd2f58..355baf75e49 100644
--- a/scalar.c
+++ b/scalar.c
@@ -675,12 +675,12 @@ static int cmd_reconfigure(int argc, const char **argv)
 		OPT_BOOL('a', "all", &all,
 			 N_("reconfigure all registered enlistments")),
 		OPT_STRING(0, "maintenance", &maintenance_str,
-			 N_("<mode>"),
+			 N_("(enable|disable|keep)"),
 			 N_("signal how to adjust background maintenance")),
 		OPT_END(),
 	};
 	const char * const usage[] = {
-		N_("scalar reconfigure [--maintenance=<mode>] [--all | <enlistment>]"),
+		N_("scalar reconfigure [--maintenance=(enable|disable|keep)] [--all | <enlistment>]"),
 		NULL
 	};
 	struct string_list scalar_repos = STRING_LIST_INIT_DUP;
-- 
2.47.2.vfs.0.2

[gitgitgadget]

This patch series was integrated into seen via git@cd279d8.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <[email protected]> writes:

> Adding this extra patch on top to improve the docs. I could resend
> as a full v4 if needed.

Nah, the other four patches have been beaten to death, I think.
Please double check the result when I push it out later today, as
I've got the following when running "git am".

    warning: Patch sent with format=flowed; space at the end of lines might be lost.
    Applying: scalar reconfigure: improve --maintenance docs

Thanks.