scalar: add --no-maintenance option #1913
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,12 +9,12 @@ SYNOPSIS | |
| -------- | ||
| [verse] | ||
| scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] | ||
| [--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>] | ||
| scalar list | ||
| scalar register [--[no-]maintenance] [<enlistment>] | ||
|
There was a problem hiding this comment. 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.There was a problem hiding this comment. 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,
-StoleeThere was a problem hiding this comment. 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.
There was a problem hiding this comment. 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.
There was a problem hiding this comment. 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
There was a problem hiding this comment. 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.There was a problem hiding this comment. 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
There was a problem hiding this comment. 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. |
||
| scalar unregister [<enlistment>] | ||
| scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>] | ||
| scalar reconfigure [--maintenance=<mode>] [ --all | <enlistment> ] | ||
| scalar diagnose [<enlistment>] | ||
| scalar delete <enlistment> | ||
|
|
||
|
|
@@ -97,6 +97,11 @@ cloning. If the HEAD at the remote did not point at any branch when | |
| A sparse-checkout is initialized by default. This behavior can be | ||
| turned off via `--full-clone`. | ||
|
|
||
| --[no-]maintenance:: | ||
| By default, `scalar clone` configures the enlistment to use Git's | ||
| background maintenance feature. Use the `--no-maintenance` to skip | ||
| this configuration. | ||
|
|
||
| List | ||
| ~~~~ | ||
|
|
||
|
|
@@ -117,6 +122,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 | ||
| ~~~~~~~~~~ | ||
|
|
||
|
|
@@ -149,8 +160,19 @@ After a Scalar upgrade, or when the configuration of a Scalar enlistment | |
| was somehow corrupted or changed by mistake, this subcommand allows to | ||
| reconfigure the enlistment. | ||
|
|
||
| --all:: | ||
| When `--all` is specified, reconfigure all enlistments currently | ||
| registered with Scalar by the `scalar.repo` config key. Use this | ||
| option after each upgrade to get the latest features. | ||
|
|
||
| --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. | ||
|
|
||
| Diagnose | ||
| ~~~~~~~~ | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Derrick Stolee wrote (reply to this):