* [PATCH 0/2] scalar: add --no-maintenance option
@ 2025-04-30 10:24 Derrick Stolee via GitGitGadget
2025-04-30 10:24 ` [PATCH 1/2] scalar register: " Derrick Stolee via GitGitGadget
` (3 more replies)
0 siblings, 4 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-04-30 10:24 UTC (permalink / raw)
To: git; +Cc: gitster, johannes.schindelin, Derrick Stolee
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.
Thanks, -Stolee
Derrick Stolee (2):
scalar register: add --no-maintenance option
scalar clone: add --no-maintenance option
Documentation/scalar.adoc | 15 +++++++++++++--
scalar.c | 23 ++++++++++++++---------
t/t9210-scalar.sh | 7 +++++++
t/t9211-scalar-clone.sh | 6 ++++++
4 files changed, 40 insertions(+), 11 deletions(-)
base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1913%2Fderrickstolee%2Fscalar-no-maintenance-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1913/derrickstolee/scalar-no-maintenance-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1913
--
gitgitgadget
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] scalar register: add --no-maintenance option
2025-04-30 10:24 [PATCH 0/2] scalar: add --no-maintenance option Derrick Stolee via GitGitGadget
@ 2025-04-30 10:24 ` Derrick Stolee via GitGitGadget
2025-05-02 9:15 ` Patrick Steinhardt
2025-04-30 10:24 ` [PATCH 2/2] scalar clone: " Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-04-30 10:24 UTC (permalink / raw)
To: git; +Cc: gitster, johannes.schindelin, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
When registering a repository with Scalar to get the latest opinionated
configuration, the 'scalar register' command will also set up background
maintenance. This is a recommended feature for most user scenarios.
However, this is not always recommended in some scenarios where
background modifications may interfere with foreground activities.
Specifically, setting up a clone for use in automation may require doing
certain maintenance steps in the foreground that could become blocked by
concurrent background maintenance operations.
Allow the user to specify --no-maintenance to 'scalar register'. This
requires updating the method prototype for register_dir(), so use the
default of enabling this value when otherwise specified.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 8 +++++++-
scalar.c | 17 ++++++++++-------
t/t9210-scalar.sh | 7 +++++++
3 files changed, 24 insertions(+), 8 deletions(-)
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()) {
@@ -550,7 +550,7 @@ static int cmd_clone(int argc, const char **argv)
if (res)
goto cleanup;
- res = register_dir();
+ res = register_dir(1);
cleanup:
free(branch_to_free);
@@ -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")),
OPT_END(),
};
const char * const usage[] = {
- N_("scalar register [<enlistment>]"),
+ N_("scalar register [--[no-]maintenance] [<enlistment>]"),
NULL
};
@@ -610,7 +613,7 @@ static int cmd_register(int argc, const char **argv)
setup_enlistment_directory(argc, argv, usage, options, NULL);
- return register_dir();
+ return register_dir(maintenance);
}
static int get_scalar_repos(const char *key, const char *value,
@@ -803,13 +806,13 @@ static int cmd_run(int argc, const char **argv)
strbuf_release(&buf);
if (i == 0)
- return register_dir();
+ return register_dir(1);
if (i > 0)
return run_git("maintenance", "run",
"--task", tasks[i].task, NULL);
- if (register_dir())
+ if (register_dir(1))
return -1;
for (i = 1; tasks[i].arg; i++)
if (run_git("maintenance", "run",
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
+'
+
test_expect_success 'set up repository to clone' '
test_commit first &&
test_commit second &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] scalar clone: add --no-maintenance option
2025-04-30 10:24 [PATCH 0/2] scalar: add --no-maintenance option Derrick Stolee via GitGitGadget
2025-04-30 10:24 ` [PATCH 1/2] scalar register: " Derrick Stolee via GitGitGadget
@ 2025-04-30 10:24 ` Derrick Stolee via GitGitGadget
2025-04-30 20:28 ` [PATCH 0/2] scalar: " Junio C Hamano
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
3 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-04-30 10:24 UTC (permalink / raw)
To: git; +Cc: gitster, johannes.schindelin, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
When creating a new enlistment via 'scalar clone', the default is to set
up situations that work for most user scenarios. Background maintenance
is one of those highly-recommended options for most users.
However, when using 'scalar clone' to create an enlistment in a
different situation, such as prepping a VM image, it may be valuable to
disable background maintenance so the manual maintenance steps do not
get blocked by concurrent background maintenance activities.
Add a new --no-maintenance option to 'scalar clone'.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 7 ++++++-
scalar.c | 8 +++++---
t/t9211-scalar-clone.sh | 6 ++++++
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
index b2b244a86499..7753df3b4352 100644
--- a/Documentation/scalar.adoc
+++ b/Documentation/scalar.adoc
@@ -9,7 +9,7 @@ SYNOPSIS
--------
[verse]
scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
- [--[no-]src] <url> [<enlistment>]
+ [--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>]
scalar list
scalar register [--[no-]maintenance] [<enlistment>]
scalar unregister [<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
~~~~
diff --git a/scalar.c b/scalar.c
index 2a21fd55f39b..90ea4da11b40 100644
--- a/scalar.c
+++ b/scalar.c
@@ -411,7 +411,7 @@ static int cmd_clone(int argc, const char **argv)
const char *branch = NULL;
char *branch_to_free = NULL;
int full_clone = 0, single_branch = 0, show_progress = isatty(2);
- int src = 1, tags = 1;
+ int src = 1, tags = 1, maintenance = 1;
struct option clone_options[] = {
OPT_STRING('b', "branch", &branch, N_("<branch>"),
N_("branch to checkout after clone")),
@@ -424,11 +424,13 @@ static int cmd_clone(int argc, const char **argv)
N_("create repository within 'src' directory")),
OPT_BOOL(0, "tags", &tags,
N_("specify if tags should be fetched during clone")),
+ OPT_BOOL(0, "maintenance", &maintenance,
+ N_("specify if background maintenance should be enabled")),
OPT_END(),
};
const char * const clone_usage[] = {
N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n"
- "\t[--[no-]src] [--[no-]tags] <url> [<enlistment>]"),
+ "\t[--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>]"),
NULL
};
const char *url;
@@ -550,7 +552,7 @@ static int cmd_clone(int argc, const char **argv)
if (res)
goto cleanup;
- res = register_dir(1);
+ res = register_dir(maintenance);
cleanup:
free(branch_to_free);
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index 01f71910f533..b9c130db6056 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -180,6 +180,12 @@ test_expect_success 'scalar clone warns when background maintenance fails' '
grep "could not turn on maintenance" err
'
+test_expect_success 'scalar clone --no-maintenance' '
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ scalar clone --no-maintenance "file://$(pwd)/to-clone" no-maint 2>err &&
+ ! grep "could not turn on maintenance" err
+'
+
test_expect_success '`scalar clone --no-src`' '
scalar clone --src "file://$(pwd)/to-clone" with-src &&
scalar clone --no-src "file://$(pwd)/to-clone" without-src &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] scalar: add --no-maintenance option
2025-04-30 10:24 [PATCH 0/2] scalar: add --no-maintenance option Derrick Stolee via GitGitGadget
2025-04-30 10:24 ` [PATCH 1/2] scalar register: " Derrick Stolee via GitGitGadget
2025-04-30 10:24 ` [PATCH 2/2] scalar clone: " Derrick Stolee via GitGitGadget
@ 2025-04-30 20:28 ` Junio C Hamano
2025-05-01 13:21 ` Derrick Stolee
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
3 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-04-30 20:28 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git, johannes.schindelin, Derrick Stolee
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] scalar: add --no-maintenance option
2025-04-30 20:28 ` [PATCH 0/2] scalar: " Junio C Hamano
@ 2025-05-01 13:21 ` Derrick Stolee
2025-05-01 16:38 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee @ 2025-05-01 13:21 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget; +Cc: git, johannes.schindelin
On 4/30/2025 4:28 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] scalar: add --no-maintenance option
2025-05-01 13:21 ` Derrick Stolee
@ 2025-05-01 16:38 ` Junio C Hamano
2025-05-01 18:20 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-05-01 16:38 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin
Derrick Stolee <stolee@gmail.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] scalar: add --no-maintenance option
2025-05-01 16:38 ` Junio C Hamano
@ 2025-05-01 18:20 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2025-05-01 18:20 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin
Junio C Hamano <gitster@pobox.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] scalar register: add --no-maintenance option
2025-04-30 10:24 ` [PATCH 1/2] scalar register: " Derrick Stolee via GitGitGadget
@ 2025-05-02 9:15 ` Patrick Steinhardt
2025-05-02 15:01 ` Derrick Stolee
0 siblings, 1 reply; 30+ messages in thread
From: Patrick Steinhardt @ 2025-05-02 9:15 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, johannes.schindelin, Derrick Stolee
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] scalar register: add --no-maintenance option
2025-05-02 9:15 ` Patrick Steinhardt
@ 2025-05-02 15:01 ` Derrick Stolee
0 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee @ 2025-05-02 15:01 UTC (permalink / raw)
To: Patrick Steinhardt, Derrick Stolee via GitGitGadget
Cc: git, gitster, johannes.schindelin
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 0/4] scalar: add --no-maintenance option
2025-04-30 10:24 [PATCH 0/2] scalar: add --no-maintenance option Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2025-04-30 20:28 ` [PATCH 0/2] scalar: " Junio C Hamano
@ 2025-05-05 15:27 ` Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
` (4 more replies)
3 siblings, 5 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-05 15:27 UTC (permalink / raw)
To: git; +Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee
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.
Thanks, -Stolee
Derrick Stolee (4):
scalar: customize register_dir()'s behavior
scalar register: add --no-maintenance option
scalar clone: add --no-maintenance option
scalar reconfigure: add --no-maintenance option
Documentation/scalar.adoc | 29 ++++++++++++++++++----
scalar.c | 51 +++++++++++++++++++++++++++++----------
t/t9210-scalar.sh | 20 +++++++++++++--
t/t9211-scalar-clone.sh | 11 ++++++++-
4 files changed, 90 insertions(+), 21 deletions(-)
base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1913%2Fderrickstolee%2Fscalar-no-maintenance-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1913/derrickstolee/scalar-no-maintenance-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1913
Range-diff vs v1:
-: ----------- > 1: f3a3cfe3ef1 scalar: customize register_dir()'s behavior
1: 4910bacd052 ! 2: 1b99a559520 scalar register: add --no-maintenance option
@@ Documentation/scalar.adoc: Note: when this subcommand is called in a worktree th
## scalar.c ##
-@@ scalar.c: 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"));
-@@ scalar.c: 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()) {
-@@ scalar.c: static int cmd_clone(int argc, const char **argv)
- if (res)
- goto cleanup;
-
-- res = register_dir();
-+ res = register_dir(1);
-
- cleanup:
- free(branch_to_free);
@@ scalar.c: static int cmd_list(int argc, const char **argv UNUSED)
static int cmd_register(int argc, const char **argv)
@@ scalar.c: static int cmd_register(int argc, const char **argv)
setup_enlistment_directory(argc, argv, usage, options, NULL);
-- return register_dir();
+- return register_dir(1);
++ /* If --no-maintenance, then leave maintenance as-is. */
+ return register_dir(maintenance);
}
static int get_scalar_repos(const char *key, const char *value,
-@@ scalar.c: static int cmd_run(int argc, const char **argv)
- strbuf_release(&buf);
-
- if (i == 0)
-- return register_dir();
-+ return register_dir(1);
-
- if (i > 0)
- return run_git("maintenance", "run",
- "--task", tasks[i].task, NULL);
-
-- if (register_dir())
-+ if (register_dir(1))
- return -1;
- for (i = 1; tasks[i].arg; i++)
- if (run_git("maintenance", "run",
## t/t9210-scalar.sh ##
+@@ t/t9210-scalar.sh: test_expect_success 'scalar register warns when background maintenance fails' '
+ git init register-repo &&
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ scalar register register-repo 2>err &&
+- grep "could not turn on maintenance" err
++ grep "could not toggle maintenance" err
+ '
+
+ test_expect_success 'scalar unregister' '
@@ t/t9210-scalar.sh: test_expect_success 'scalar unregister' '
scalar unregister vanish
'
+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_must_be_empty err &&
++ test_subcommand ! git maintenance unregister --force <no-maint.event
+'
+
test_expect_success 'set up repository to clone' '
2: 7ab1914b830 ! 3: e52b1282d93 scalar clone: add --no-maintenance option
@@ scalar.c: static int cmd_clone(int argc, const char **argv)
goto cleanup;
- res = register_dir(1);
++ /* If --no-maintenance, then skip maintenance command entirely. */
+ res = register_dir(maintenance);
cleanup:
free(branch_to_free);
## t/t9211-scalar-clone.sh ##
-@@ t/t9211-scalar-clone.sh: test_expect_success 'scalar clone warns when background maintenance fails' '
- grep "could not turn on maintenance" err
- '
-
+@@ t/t9211-scalar-clone.sh: test_expect_success 'progress without tty' '
+ test_expect_success 'scalar clone warns when background maintenance fails' '
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
+- grep "could not turn on maintenance" err
++ grep "could not toggle maintenance" err
++'
++
+test_expect_success 'scalar clone --no-maintenance' '
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
++ GIT_TRACE2_EVENT="$(pwd)/no-maint.event" \
++ GIT_TRACE2_EVENT_DEPTH=100 \
+ scalar clone --no-maintenance "file://$(pwd)/to-clone" no-maint 2>err &&
-+ ! grep "could not turn on maintenance" err
-+'
-+
++ ! grep "could not toggle maintenance" err &&
++ test_subcommand ! git maintenance unregister --force <no-maint.event
+ '
+
test_expect_success '`scalar clone --no-src`' '
- scalar clone --src "file://$(pwd)/to-clone" with-src &&
- scalar clone --no-src "file://$(pwd)/to-clone" without-src &&
-: ----------- > 4: 6fac9c4c394 scalar reconfigure: add --no-maintenance option
--
gitgitgadget
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/4] scalar: customize register_dir()'s behavior
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
@ 2025-05-05 15:27 ` Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 2/4] scalar register: add --no-maintenance option Derrick Stolee via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-05 15:27 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
In advance of adding a --[no-]maintenance option to several 'scalar'
subcommands, extend the register_dir() method to include an option for
how it should handle background maintenance.
It's important that we understand the context of toggle_maintenance()
that will enable _or disable_ maintenance depending on its input value.
Add a doc comment with this information.
Similarly, update register_dir() to either enable maintenance or leave
it alone.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
scalar.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/scalar.c b/scalar.c
index d359f08bb8e2..b20b063471a6 100644
--- a/scalar.c
+++ b/scalar.c
@@ -209,6 +209,12 @@ static int set_recommended_config(int reconfigure)
return 0;
}
+/**
+ * Enable or disable the maintenance mode for the current repository:
+ *
+ * * If 'enable' is nonzero, run 'git maintenance start'.
+ * * If 'enable' is zero, run 'git maintenance unregister --force'.
+ */
static int toggle_maintenance(int enable)
{
return run_git("maintenance",
@@ -259,7 +265,15 @@ static int stop_fsmonitor_daemon(void)
return 0;
}
-static int register_dir(void)
+/**
+ * Register the current directory as a Scalar enlistment, and set the
+ * recommended configuration.
+ *
+ * * If 'maintenance' is non-zero, then enable background maintenance.
+ * * If 'maintenance' is zero, then leave background maintenance as it is
+ * currently configured.
+ */
+static int register_dir(int maintenance)
{
if (add_or_remove_enlistment(1))
return error(_("could not add enlistment"));
@@ -267,8 +281,9 @@ static int register_dir(void)
if (set_recommended_config(0))
return error(_("could not set recommended config"));
- if (toggle_maintenance(1))
- warning(_("could not turn on maintenance"));
+ if (maintenance &&
+ toggle_maintenance(maintenance))
+ warning(_("could not toggle maintenance"));
if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
return error(_("could not start the FSMonitor daemon"));
@@ -550,7 +565,7 @@ static int cmd_clone(int argc, const char **argv)
if (res)
goto cleanup;
- res = register_dir();
+ res = register_dir(1);
cleanup:
free(branch_to_free);
@@ -610,7 +625,7 @@ static int cmd_register(int argc, const char **argv)
setup_enlistment_directory(argc, argv, usage, options, NULL);
- return register_dir();
+ return register_dir(1);
}
static int get_scalar_repos(const char *key, const char *value,
@@ -803,13 +818,13 @@ static int cmd_run(int argc, const char **argv)
strbuf_release(&buf);
if (i == 0)
- return register_dir();
+ return register_dir(1);
if (i > 0)
return run_git("maintenance", "run",
"--task", tasks[i].task, NULL);
- if (register_dir())
+ if (register_dir(1))
return -1;
for (i = 1; tasks[i].arg; i++)
if (run_git("maintenance", "run",
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/4] scalar register: add --no-maintenance option
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
@ 2025-05-05 15:27 ` Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 3/4] scalar clone: " Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-05 15:27 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
When registering a repository with Scalar to get the latest opinionated
configuration, the 'scalar register' command will also set up background
maintenance. This is a recommended feature for most user scenarios.
However, this is not always recommended in some scenarios where
background modifications may interfere with foreground activities.
Specifically, setting up a clone for use in automation may require doing
certain maintenance steps in the foreground that could become blocked by
concurrent background maintenance operations.
Allow the user to specify --no-maintenance to 'scalar register'. This
requires updating the method prototype for register_dir(), so use the
default of enabling this value when otherwise specified.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 8 +++++++-
scalar.c | 8 ++++++--
t/t9210-scalar.sh | 13 ++++++++++++-
3 files changed, 25 insertions(+), 4 deletions(-)
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 b20b063471a6..da0c46bc96cc 100644
--- a/scalar.c
+++ b/scalar.c
@@ -612,11 +612,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")),
OPT_END(),
};
const char * const usage[] = {
- N_("scalar register [<enlistment>]"),
+ N_("scalar register [--[no-]maintenance] [<enlistment>]"),
NULL
};
@@ -625,7 +628,8 @@ static int cmd_register(int argc, const char **argv)
setup_enlistment_directory(argc, argv, usage, options, NULL);
- return register_dir(1);
+ /* If --no-maintenance, then leave maintenance as-is. */
+ return register_dir(maintenance);
}
static int get_scalar_repos(const char *key, const char *value,
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index a81662713eb8..89a6a2a24d8b 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -108,7 +108,7 @@ test_expect_success 'scalar register warns when background maintenance fails' '
git init register-repo &&
GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
scalar register register-repo 2>err &&
- grep "could not turn on maintenance" err
+ grep "could not toggle maintenance" err
'
test_expect_success 'scalar unregister' '
@@ -129,6 +129,17 @@ test_expect_success 'scalar unregister' '
scalar unregister vanish
'
+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
+'
+
test_expect_success 'set up repository to clone' '
test_commit first &&
test_commit second &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 3/4] scalar clone: add --no-maintenance option
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 2/4] scalar register: add --no-maintenance option Derrick Stolee via GitGitGadget
@ 2025-05-05 15:27 ` Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 4/4] scalar reconfigure: " Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
4 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-05 15:27 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
When creating a new enlistment via 'scalar clone', the default is to set
up situations that work for most user scenarios. Background maintenance
is one of those highly-recommended options for most users.
However, when using 'scalar clone' to create an enlistment in a
different situation, such as prepping a VM image, it may be valuable to
disable background maintenance so the manual maintenance steps do not
get blocked by concurrent background maintenance activities.
Add a new --no-maintenance option to 'scalar clone'.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 7 ++++++-
scalar.c | 9 ++++++---
t/t9211-scalar-clone.sh | 11 ++++++++++-
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
index b2b244a86499..7753df3b4352 100644
--- a/Documentation/scalar.adoc
+++ b/Documentation/scalar.adoc
@@ -9,7 +9,7 @@ SYNOPSIS
--------
[verse]
scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
- [--[no-]src] <url> [<enlistment>]
+ [--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>]
scalar list
scalar register [--[no-]maintenance] [<enlistment>]
scalar unregister [<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
~~~~
diff --git a/scalar.c b/scalar.c
index da0c46bc96cc..dd6e1447e086 100644
--- a/scalar.c
+++ b/scalar.c
@@ -426,7 +426,7 @@ static int cmd_clone(int argc, const char **argv)
const char *branch = NULL;
char *branch_to_free = NULL;
int full_clone = 0, single_branch = 0, show_progress = isatty(2);
- int src = 1, tags = 1;
+ int src = 1, tags = 1, maintenance = 1;
struct option clone_options[] = {
OPT_STRING('b', "branch", &branch, N_("<branch>"),
N_("branch to checkout after clone")),
@@ -439,11 +439,13 @@ static int cmd_clone(int argc, const char **argv)
N_("create repository within 'src' directory")),
OPT_BOOL(0, "tags", &tags,
N_("specify if tags should be fetched during clone")),
+ OPT_BOOL(0, "maintenance", &maintenance,
+ N_("specify if background maintenance should be enabled")),
OPT_END(),
};
const char * const clone_usage[] = {
N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n"
- "\t[--[no-]src] [--[no-]tags] <url> [<enlistment>]"),
+ "\t[--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>]"),
NULL
};
const char *url;
@@ -565,7 +567,8 @@ static int cmd_clone(int argc, const char **argv)
if (res)
goto cleanup;
- res = register_dir(1);
+ /* If --no-maintenance, then skip maintenance command entirely. */
+ res = register_dir(maintenance);
cleanup:
free(branch_to_free);
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index 01f71910f533..bfbf22a46218 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -177,7 +177,16 @@ test_expect_success 'progress without tty' '
test_expect_success 'scalar clone warns when background maintenance fails' '
GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
- grep "could not turn on maintenance" err
+ grep "could not toggle maintenance" err
+'
+
+test_expect_success 'scalar clone --no-maintenance' '
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ GIT_TRACE2_EVENT="$(pwd)/no-maint.event" \
+ GIT_TRACE2_EVENT_DEPTH=100 \
+ scalar clone --no-maintenance "file://$(pwd)/to-clone" no-maint 2>err &&
+ ! grep "could not toggle maintenance" err &&
+ test_subcommand ! git maintenance unregister --force <no-maint.event
'
test_expect_success '`scalar clone --no-src`' '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 4/4] scalar reconfigure: add --no-maintenance option
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2025-05-05 15:27 ` [PATCH v2 3/4] scalar clone: " Derrick Stolee via GitGitGadget
@ 2025-05-05 15:27 ` Derrick Stolee via GitGitGadget
2025-05-05 21:47 ` Junio C Hamano
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
4 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-05 15:27 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
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.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 14 +++++++++++---
scalar.c | 9 ++++++---
t/t9210-scalar.sh | 7 ++++++-
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
index 7753df3b4352..2868b01988e4 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 [ --all | <enlistment> ]
+scalar reconfigure [--[no-]maintenance] [ --all | <enlistment> ]
scalar diagnose [<enlistment>]
scalar delete <enlistment>
@@ -160,8 +160,16 @@ 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.
-With the `--all` option, all enlistments currently registered with Scalar
-will be reconfigured. Use this option after each Scalar upgrade.
+--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.
+
+--[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.
Diagnose
~~~~~~~~
diff --git a/scalar.c b/scalar.c
index dd6e1447e086..0b8a63f6e5a6 100644
--- a/scalar.c
+++ b/scalar.c
@@ -667,14 +667,16 @@ static int remove_deleted_enlistment(struct strbuf *path)
static int cmd_reconfigure(int argc, const char **argv)
{
- int all = 0;
+ int all = 0, maintenance = 1;
struct option options[] = {
OPT_BOOL('a', "all", &all,
N_("reconfigure all registered enlistments")),
+ OPT_BOOL(0, "maintenance", &maintenance,
+ N_("specify if background maintenance should be enabled")),
OPT_END(),
};
const char * const usage[] = {
- N_("scalar reconfigure [--all | <enlistment>]"),
+ N_("scalar reconfigure [--[no-]maintenance] [--all | <enlistment>]"),
NULL
};
struct string_list scalar_repos = STRING_LIST_INIT_DUP;
@@ -758,7 +760,8 @@ static int cmd_reconfigure(int argc, const char **argv)
the_repository = old_repo;
repo_clear(&r);
- if (toggle_maintenance(1) >= 0)
+ if (maintenance &&
+ toggle_maintenance(1) >= 0)
succeeded = 1;
loop_end:
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 89a6a2a24d8b..34765a49fd07 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -210,7 +210,12 @@ test_expect_success 'scalar reconfigure' '
GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a &&
test_path_is_file one/src/cron.txt &&
test true = "$(git -C one/src config core.preloadIndex)" &&
- test_subcommand git maintenance start <reconfigure
+ test_subcommand git maintenance start <reconfigure &&
+ test_subcommand ! git maintenance unregister --force <reconfigure &&
+
+ GIT_TRACE2_EVENT="$(pwd)/reconfigure-maint" scalar reconfigure --no-maintenance -a &&
+ test_subcommand ! git maintenance start <reconfigure-maint &&
+ test_subcommand ! git maintenance unregister --force <reconfigure-maint
'
test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 4/4] scalar reconfigure: add --no-maintenance option
2025-05-05 15:27 ` [PATCH v2 4/4] scalar reconfigure: " Derrick Stolee via GitGitGadget
@ 2025-05-05 21:47 ` Junio C Hamano
2025-05-06 18:00 ` Derrick Stolee
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-05-05 21:47 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, johannes.schindelin, Patrick Steinhardt, Derrick Stolee
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <stolee@gmail.com>
>
> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 4/4] scalar reconfigure: add --no-maintenance option
2025-05-05 21:47 ` Junio C Hamano
@ 2025-05-06 18:00 ` Derrick Stolee
2025-05-06 22:16 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee @ 2025-05-06 18:00 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget
Cc: git, johannes.schindelin, Patrick Steinhardt
On 5/5/2025 5:47 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 4/4] scalar reconfigure: add --no-maintenance option
2025-05-06 18:00 ` Derrick Stolee
@ 2025-05-06 22:16 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2025-05-06 22:16 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin,
Patrick Steinhardt
Derrick Stolee <stolee@gmail.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 0/4] scalar: add --no-maintenance option
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
` (3 preceding siblings ...)
2025-05-05 15:27 ` [PATCH v2 4/4] scalar reconfigure: " Derrick Stolee via GitGitGadget
@ 2025-05-07 1:50 ` Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
` (4 more replies)
4 siblings, 5 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-07 1:50 UTC (permalink / raw)
To: git; +Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee
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
Derrick Stolee (4):
scalar: customize register_dir()'s behavior
scalar register: add --no-maintenance option
scalar clone: add --no-maintenance option
scalar reconfigure: add --maintenance=<mode> option
Documentation/scalar.adoc | 32 ++++++++++++++++---
scalar.c | 65 +++++++++++++++++++++++++++++++--------
t/t9210-scalar.sh | 26 ++++++++++++++--
t/t9211-scalar-clone.sh | 11 ++++++-
4 files changed, 114 insertions(+), 20 deletions(-)
base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1913%2Fderrickstolee%2Fscalar-no-maintenance-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1913/derrickstolee/scalar-no-maintenance-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1913
Range-diff vs v2:
1: f3a3cfe3ef1 = 1: f3a3cfe3ef1 scalar: customize register_dir()'s behavior
2: 1b99a559520 = 2: 1b99a559520 scalar register: add --no-maintenance option
3: e52b1282d93 = 3: e52b1282d93 scalar clone: add --no-maintenance option
4: 6fac9c4c394 ! 4: 684f04aaf7e scalar reconfigure: add --no-maintenance option
@@ Metadata
Author: Derrick Stolee <dstolee@microsoft.com>
## Commit message ##
- scalar reconfigure: add --no-maintenance option
+ scalar reconfigure: add --maintenance=<mode> option
When users want to enable the latest and greatest configuration options
recommended by Scalar after a Git upgrade, 'scalar reconfigure --all' is
@@ Commit message
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.
+ Add a new --maintenance=<mode> option to 'scalar reconfigure' that
+ provides options for enabling (default), disabling, or leaving
+ background maintenance config as-is.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## Documentation/scalar.adoc ##
@@ Documentation/scalar.adoc: scalar list
scalar unregister [<enlistment>]
scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
-scalar reconfigure [ --all | <enlistment> ]
-+scalar reconfigure [--[no-]maintenance] [ --all | <enlistment> ]
++scalar reconfigure [--maintenance=<mode>] [ --all | <enlistment> ]
scalar diagnose [<enlistment>]
scalar delete <enlistment>
@@ Documentation/scalar.adoc: After a Scalar upgrade, or when the configuration of
+ registered with Scalar by the `scalar.repo` config key. Use this
+ option after each upgrade to get the latest features.
+
-+--[no-]maintenance::
++--maintenance=<mode>::
+ 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.
++ 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
~~~~~~~~
## scalar.c ##
@@ scalar.c: static int remove_deleted_enlistment(struct strbuf *path)
-
static int cmd_reconfigure(int argc, const char **argv)
{
-- int all = 0;
-+ int all = 0, maintenance = 1;
+ int all = 0;
++ const char *maintenance_str = NULL;
++ int maintenance = 1; /* Enable maintenance by default. */
++
struct option options[] = {
OPT_BOOL('a', "all", &all,
N_("reconfigure all registered enlistments")),
-+ OPT_BOOL(0, "maintenance", &maintenance,
-+ N_("specify if background maintenance should be enabled")),
++ OPT_STRING(0, "maintenance", &maintenance_str,
++ N_("<mode>"),
++ N_("signal how to adjust background maintenance")),
OPT_END(),
};
const char * const usage[] = {
- N_("scalar reconfigure [--all | <enlistment>]"),
-+ N_("scalar reconfigure [--[no-]maintenance] [--all | <enlistment>]"),
++ N_("scalar reconfigure [--maintenance=<mode>] [--all | <enlistment>]"),
NULL
};
struct string_list scalar_repos = STRING_LIST_INIT_DUP;
+@@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
+ usage_msg_opt(_("--all or <enlistment>, but not both"),
+ usage, options);
+
++ 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);
++ }
++
+ git_config(get_scalar_repos, &scalar_repos);
+
+ for (size_t i = 0; i < scalar_repos.nr; i++) {
@@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
the_repository = old_repo;
repo_clear(&r);
- if (toggle_maintenance(1) >= 0)
-+ if (maintenance &&
-+ toggle_maintenance(1) >= 0)
++ if (maintenance >= 0 &&
++ toggle_maintenance(maintenance) >= 0)
succeeded = 1;
loop_end:
@@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure' '
+ test_subcommand git maintenance start <reconfigure &&
+ test_subcommand ! git maintenance unregister --force <reconfigure &&
+
-+ GIT_TRACE2_EVENT="$(pwd)/reconfigure-maint" scalar reconfigure --no-maintenance -a &&
-+ test_subcommand ! git maintenance start <reconfigure-maint &&
-+ test_subcommand ! git maintenance unregister --force <reconfigure-maint
++ GIT_TRACE2_EVENT="$(pwd)/reconfigure-maint-disable" \
++ scalar reconfigure -a --maintenance=disable &&
++ test_subcommand ! git maintenance start <reconfigure-maint-disable &&
++ test_subcommand git maintenance unregister --force <reconfigure-maint-disable &&
++
++ GIT_TRACE2_EVENT="$(pwd)/reconfigure-maint-keep" \
++ scalar reconfigure --maintenance=keep -a &&
++ test_subcommand ! git maintenance start <reconfigure-maint-keep &&
++ test_subcommand ! git maintenance unregister --force <reconfigure-maint-keep
'
test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '
--
gitgitgadget
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 1/4] scalar: customize register_dir()'s behavior
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
@ 2025-05-07 1:50 ` Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 2/4] scalar register: add --no-maintenance option Derrick Stolee via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-07 1:50 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
In advance of adding a --[no-]maintenance option to several 'scalar'
subcommands, extend the register_dir() method to include an option for
how it should handle background maintenance.
It's important that we understand the context of toggle_maintenance()
that will enable _or disable_ maintenance depending on its input value.
Add a doc comment with this information.
Similarly, update register_dir() to either enable maintenance or leave
it alone.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
scalar.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/scalar.c b/scalar.c
index d359f08bb8e2..b20b063471a6 100644
--- a/scalar.c
+++ b/scalar.c
@@ -209,6 +209,12 @@ static int set_recommended_config(int reconfigure)
return 0;
}
+/**
+ * Enable or disable the maintenance mode for the current repository:
+ *
+ * * If 'enable' is nonzero, run 'git maintenance start'.
+ * * If 'enable' is zero, run 'git maintenance unregister --force'.
+ */
static int toggle_maintenance(int enable)
{
return run_git("maintenance",
@@ -259,7 +265,15 @@ static int stop_fsmonitor_daemon(void)
return 0;
}
-static int register_dir(void)
+/**
+ * Register the current directory as a Scalar enlistment, and set the
+ * recommended configuration.
+ *
+ * * If 'maintenance' is non-zero, then enable background maintenance.
+ * * If 'maintenance' is zero, then leave background maintenance as it is
+ * currently configured.
+ */
+static int register_dir(int maintenance)
{
if (add_or_remove_enlistment(1))
return error(_("could not add enlistment"));
@@ -267,8 +281,9 @@ static int register_dir(void)
if (set_recommended_config(0))
return error(_("could not set recommended config"));
- if (toggle_maintenance(1))
- warning(_("could not turn on maintenance"));
+ if (maintenance &&
+ toggle_maintenance(maintenance))
+ warning(_("could not toggle maintenance"));
if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
return error(_("could not start the FSMonitor daemon"));
@@ -550,7 +565,7 @@ static int cmd_clone(int argc, const char **argv)
if (res)
goto cleanup;
- res = register_dir();
+ res = register_dir(1);
cleanup:
free(branch_to_free);
@@ -610,7 +625,7 @@ static int cmd_register(int argc, const char **argv)
setup_enlistment_directory(argc, argv, usage, options, NULL);
- return register_dir();
+ return register_dir(1);
}
static int get_scalar_repos(const char *key, const char *value,
@@ -803,13 +818,13 @@ static int cmd_run(int argc, const char **argv)
strbuf_release(&buf);
if (i == 0)
- return register_dir();
+ return register_dir(1);
if (i > 0)
return run_git("maintenance", "run",
"--task", tasks[i].task, NULL);
- if (register_dir())
+ if (register_dir(1))
return -1;
for (i = 1; tasks[i].arg; i++)
if (run_git("maintenance", "run",
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 2/4] scalar register: add --no-maintenance option
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
@ 2025-05-07 1:50 ` Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 3/4] scalar clone: " Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-07 1:50 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
When registering a repository with Scalar to get the latest opinionated
configuration, the 'scalar register' command will also set up background
maintenance. This is a recommended feature for most user scenarios.
However, this is not always recommended in some scenarios where
background modifications may interfere with foreground activities.
Specifically, setting up a clone for use in automation may require doing
certain maintenance steps in the foreground that could become blocked by
concurrent background maintenance operations.
Allow the user to specify --no-maintenance to 'scalar register'. This
requires updating the method prototype for register_dir(), so use the
default of enabling this value when otherwise specified.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 8 +++++++-
scalar.c | 8 ++++++--
t/t9210-scalar.sh | 13 ++++++++++++-
3 files changed, 25 insertions(+), 4 deletions(-)
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 b20b063471a6..da0c46bc96cc 100644
--- a/scalar.c
+++ b/scalar.c
@@ -612,11 +612,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")),
OPT_END(),
};
const char * const usage[] = {
- N_("scalar register [<enlistment>]"),
+ N_("scalar register [--[no-]maintenance] [<enlistment>]"),
NULL
};
@@ -625,7 +628,8 @@ static int cmd_register(int argc, const char **argv)
setup_enlistment_directory(argc, argv, usage, options, NULL);
- return register_dir(1);
+ /* If --no-maintenance, then leave maintenance as-is. */
+ return register_dir(maintenance);
}
static int get_scalar_repos(const char *key, const char *value,
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index a81662713eb8..89a6a2a24d8b 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -108,7 +108,7 @@ test_expect_success 'scalar register warns when background maintenance fails' '
git init register-repo &&
GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
scalar register register-repo 2>err &&
- grep "could not turn on maintenance" err
+ grep "could not toggle maintenance" err
'
test_expect_success 'scalar unregister' '
@@ -129,6 +129,17 @@ test_expect_success 'scalar unregister' '
scalar unregister vanish
'
+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
+'
+
test_expect_success 'set up repository to clone' '
test_commit first &&
test_commit second &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 3/4] scalar clone: add --no-maintenance option
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 2/4] scalar register: add --no-maintenance option Derrick Stolee via GitGitGadget
@ 2025-05-07 1:50 ` Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option Derrick Stolee via GitGitGadget
2025-05-14 13:52 ` [PATCH 5/4] scalar reconfigure: improve --maintenance docs Derrick Stolee
4 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-07 1:50 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
When creating a new enlistment via 'scalar clone', the default is to set
up situations that work for most user scenarios. Background maintenance
is one of those highly-recommended options for most users.
However, when using 'scalar clone' to create an enlistment in a
different situation, such as prepping a VM image, it may be valuable to
disable background maintenance so the manual maintenance steps do not
get blocked by concurrent background maintenance activities.
Add a new --no-maintenance option to 'scalar clone'.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 7 ++++++-
scalar.c | 9 ++++++---
t/t9211-scalar-clone.sh | 11 ++++++++++-
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
index b2b244a86499..7753df3b4352 100644
--- a/Documentation/scalar.adoc
+++ b/Documentation/scalar.adoc
@@ -9,7 +9,7 @@ SYNOPSIS
--------
[verse]
scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
- [--[no-]src] <url> [<enlistment>]
+ [--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>]
scalar list
scalar register [--[no-]maintenance] [<enlistment>]
scalar unregister [<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
~~~~
diff --git a/scalar.c b/scalar.c
index da0c46bc96cc..dd6e1447e086 100644
--- a/scalar.c
+++ b/scalar.c
@@ -426,7 +426,7 @@ static int cmd_clone(int argc, const char **argv)
const char *branch = NULL;
char *branch_to_free = NULL;
int full_clone = 0, single_branch = 0, show_progress = isatty(2);
- int src = 1, tags = 1;
+ int src = 1, tags = 1, maintenance = 1;
struct option clone_options[] = {
OPT_STRING('b', "branch", &branch, N_("<branch>"),
N_("branch to checkout after clone")),
@@ -439,11 +439,13 @@ static int cmd_clone(int argc, const char **argv)
N_("create repository within 'src' directory")),
OPT_BOOL(0, "tags", &tags,
N_("specify if tags should be fetched during clone")),
+ OPT_BOOL(0, "maintenance", &maintenance,
+ N_("specify if background maintenance should be enabled")),
OPT_END(),
};
const char * const clone_usage[] = {
N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n"
- "\t[--[no-]src] [--[no-]tags] <url> [<enlistment>]"),
+ "\t[--[no-]src] [--[no-]tags] [--[no-]maintenance] <url> [<enlistment>]"),
NULL
};
const char *url;
@@ -565,7 +567,8 @@ static int cmd_clone(int argc, const char **argv)
if (res)
goto cleanup;
- res = register_dir(1);
+ /* If --no-maintenance, then skip maintenance command entirely. */
+ res = register_dir(maintenance);
cleanup:
free(branch_to_free);
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index 01f71910f533..bfbf22a46218 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -177,7 +177,16 @@ test_expect_success 'progress without tty' '
test_expect_success 'scalar clone warns when background maintenance fails' '
GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
- grep "could not turn on maintenance" err
+ grep "could not toggle maintenance" err
+'
+
+test_expect_success 'scalar clone --no-maintenance' '
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ GIT_TRACE2_EVENT="$(pwd)/no-maint.event" \
+ GIT_TRACE2_EVENT_DEPTH=100 \
+ scalar clone --no-maintenance "file://$(pwd)/to-clone" no-maint 2>err &&
+ ! grep "could not toggle maintenance" err &&
+ test_subcommand ! git maintenance unregister --force <no-maint.event
'
test_expect_success '`scalar clone --no-src`' '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2025-05-07 1:50 ` [PATCH v3 3/4] scalar clone: " Derrick Stolee via GitGitGadget
@ 2025-05-07 1:50 ` Derrick Stolee via GitGitGadget
2025-05-07 21:46 ` Junio C Hamano
2025-05-14 13:52 ` [PATCH 5/4] scalar reconfigure: improve --maintenance docs Derrick Stolee
4 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2025-05-07 1:50 UTC (permalink / raw)
To: git
Cc: gitster, johannes.schindelin, Patrick Steinhardt, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
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 --maintenance=<mode> option to 'scalar reconfigure' that
provides options for enabling (default), disabling, or leaving
background maintenance config as-is.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/scalar.adoc | 17 ++++++++++++++---
scalar.c | 23 +++++++++++++++++++++--
t/t9210-scalar.sh | 13 ++++++++++++-
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
index 7753df3b4352..387527be1ea5 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 [ --all | <enlistment> ]
+scalar reconfigure [--maintenance=<mode>] [ --all | <enlistment> ]
scalar diagnose [<enlistment>]
scalar delete <enlistment>
@@ -160,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.
-With the `--all` option, all enlistments currently registered with Scalar
-will be reconfigured. Use this option after each Scalar upgrade.
+--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
~~~~~~~~
diff --git a/scalar.c b/scalar.c
index dd6e1447e086..847d2dd2f58a 100644
--- a/scalar.c
+++ b/scalar.c
@@ -668,13 +668,19 @@ static int remove_deleted_enlistment(struct strbuf *path)
static int cmd_reconfigure(int argc, const char **argv)
{
int all = 0;
+ const char *maintenance_str = NULL;
+ int maintenance = 1; /* Enable maintenance by default. */
+
struct option options[] = {
OPT_BOOL('a', "all", &all,
N_("reconfigure all registered enlistments")),
+ OPT_STRING(0, "maintenance", &maintenance_str,
+ N_("<mode>"),
+ N_("signal how to adjust background maintenance")),
OPT_END(),
};
const char * const usage[] = {
- N_("scalar reconfigure [--all | <enlistment>]"),
+ N_("scalar reconfigure [--maintenance=<mode>] [--all | <enlistment>]"),
NULL
};
struct string_list scalar_repos = STRING_LIST_INIT_DUP;
@@ -694,6 +700,18 @@ static int cmd_reconfigure(int argc, const char **argv)
usage_msg_opt(_("--all or <enlistment>, but not both"),
usage, options);
+ 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);
+ }
+
git_config(get_scalar_repos, &scalar_repos);
for (size_t i = 0; i < scalar_repos.nr; i++) {
@@ -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;
loop_end:
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 89a6a2a24d8b..bd6f0c40d229 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -210,7 +210,18 @@ test_expect_success 'scalar reconfigure' '
GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a &&
test_path_is_file one/src/cron.txt &&
test true = "$(git -C one/src config core.preloadIndex)" &&
- test_subcommand git maintenance start <reconfigure
+ test_subcommand git maintenance start <reconfigure &&
+ test_subcommand ! git maintenance unregister --force <reconfigure &&
+
+ GIT_TRACE2_EVENT="$(pwd)/reconfigure-maint-disable" \
+ scalar reconfigure -a --maintenance=disable &&
+ test_subcommand ! git maintenance start <reconfigure-maint-disable &&
+ test_subcommand git maintenance unregister --force <reconfigure-maint-disable &&
+
+ GIT_TRACE2_EVENT="$(pwd)/reconfigure-maint-keep" \
+ scalar reconfigure --maintenance=keep -a &&
+ test_subcommand ! git maintenance start <reconfigure-maint-keep &&
+ test_subcommand ! git maintenance unregister --force <reconfigure-maint-keep
'
test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
2025-05-07 1:50 ` [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option Derrick Stolee via GitGitGadget
@ 2025-05-07 21:46 ` Junio C Hamano
2025-05-12 14:34 ` Derrick Stolee
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-05-07 21:46 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, johannes.schindelin, Patrick Steinhardt, Derrick Stolee
"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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
2025-05-07 21:46 ` Junio C Hamano
@ 2025-05-12 14:34 ` Derrick Stolee
2025-05-12 17:44 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee @ 2025-05-12 14:34 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget
Cc: git, johannes.schindelin, Patrick Steinhardt
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
2025-05-12 14:34 ` Derrick Stolee
@ 2025-05-12 17:44 ` Junio C Hamano
2025-05-12 18:02 ` Derrick Stolee
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-05-12 17:44 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin,
Patrick Steinhardt
Derrick Stolee <stolee@gmail.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
2025-05-12 17:44 ` Junio C Hamano
@ 2025-05-12 18:02 ` Derrick Stolee
2025-05-14 12:28 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee @ 2025-05-12 18:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin,
Patrick Steinhardt
On 5/12/2025 1:44 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option
2025-05-12 18:02 ` Derrick Stolee
@ 2025-05-14 12:28 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2025-05-14 12:28 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin,
Patrick Steinhardt
Derrick Stolee <stolee@gmail.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 5/4] scalar reconfigure: improve --maintenance docs
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
` (3 preceding siblings ...)
2025-05-07 1:50 ` [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option Derrick Stolee via GitGitGadget
@ 2025-05-14 13:52 ` Derrick Stolee
2025-05-14 22:16 ` Junio C Hamano
4 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee @ 2025-05-14 13:52 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, johannes.schindelin, Patrick Steinhardt
From 0f5dc1cb6d697c7d8d3c126f3640c2f58fcfda43 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <stolee@gmail.com>
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 <stolee@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 5/4] scalar reconfigure: improve --maintenance docs
2025-05-14 13:52 ` [PATCH 5/4] scalar reconfigure: improve --maintenance docs Derrick Stolee
@ 2025-05-14 22:16 ` Junio C Hamano
2025-05-16 16:36 ` Derrick Stolee
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-05-14 22:16 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin,
Patrick Steinhardt
Derrick Stolee <stolee@gmail.com> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/4] scalar reconfigure: improve --maintenance docs
2025-05-14 22:16 ` Junio C Hamano
@ 2025-05-16 16:36 ` Derrick Stolee
0 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee @ 2025-05-16 16:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, johannes.schindelin,
Patrick Steinhardt
On 5/14/2025 6:16 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> 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
Thank you for fixing up this whitespace issue.
I'm happy with your copy of ds/scalar-no-maintenance.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2025-05-16 16:36 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-30 10:24 [PATCH 0/2] scalar: add --no-maintenance option Derrick Stolee via GitGitGadget
2025-04-30 10:24 ` [PATCH 1/2] scalar register: " Derrick Stolee via GitGitGadget
2025-05-02 9:15 ` Patrick Steinhardt
2025-05-02 15:01 ` Derrick Stolee
2025-04-30 10:24 ` [PATCH 2/2] scalar clone: " Derrick Stolee via GitGitGadget
2025-04-30 20:28 ` [PATCH 0/2] scalar: " Junio C Hamano
2025-05-01 13:21 ` Derrick Stolee
2025-05-01 16:38 ` Junio C Hamano
2025-05-01 18:20 ` Junio C Hamano
2025-05-05 15:27 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 2/4] scalar register: add --no-maintenance option Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 3/4] scalar clone: " Derrick Stolee via GitGitGadget
2025-05-05 15:27 ` [PATCH v2 4/4] scalar reconfigure: " Derrick Stolee via GitGitGadget
2025-05-05 21:47 ` Junio C Hamano
2025-05-06 18:00 ` Derrick Stolee
2025-05-06 22:16 ` Junio C Hamano
2025-05-07 1:50 ` [PATCH v3 0/4] scalar: " Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 1/4] scalar: customize register_dir()'s behavior Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 2/4] scalar register: add --no-maintenance option Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 3/4] scalar clone: " Derrick Stolee via GitGitGadget
2025-05-07 1:50 ` [PATCH v3 4/4] scalar reconfigure: add --maintenance=<mode> option Derrick Stolee via GitGitGadget
2025-05-07 21:46 ` Junio C Hamano
2025-05-12 14:34 ` Derrick Stolee
2025-05-12 17:44 ` Junio C Hamano
2025-05-12 18:02 ` Derrick Stolee
2025-05-14 12:28 ` Junio C Hamano
2025-05-14 13:52 ` [PATCH 5/4] scalar reconfigure: improve --maintenance docs Derrick Stolee
2025-05-14 22:16 ` Junio C Hamano
2025-05-16 16:36 ` Derrick Stolee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).