Allow scalar to succeed despite maintenance failures
cover Allow scalar to succeed despite maintenance failures
At $DAYJOB we received a report of issues with scalar clone and scalar register in a protected environment. Specifically, they couldn't run crontab or systemctl, which causes git maintenance start to fail. That failure was percolating through scalar clone and scalar register, even though the rest of their repository was properly set up.
Convert these hard failures into soft warnings.
This change could probably be done in a single commit, but I wanted to demonstrate the existing behavior before changing it. The first patch was also a related necessary step that will be helpful for future scalar tests.
Thanks,
* Stolee
Derrick Stolee (3):
t: allow 'scalar' in test_must_fail
t921*: test scalar behavior starting maintenance
scalar: only warn when background maintenance fails
scalar.c | 2 +- t/t9210-scalar.sh | 7 +++++++ t/t9211-scalar-clone.sh | 6 ++++++ t/test-lib-functions.sh | 2 +- 4 files changed, 15 insertions(+), 2 deletions(-)
base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1473%2Fderrickstolee%2Fscalar-register-warn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1473/derrickstolee/scalar-register-warn-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1473
Signature · 1 line
gitgitgadget
PATCH 1/3 t: allow 'scalar' in test_must_fail
From: Derrick Stolee <[email protected]>
This will enable scalar tests to use the test_must_fail helper, when necessary.
--- t/test-lib-functions.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/test-lib-functions.sh +1 -1
@@ -1016,7 +1016,7 @@ test_must_fail_acceptable () { fi case "$1" in - git|__git*|test-tool|test_terminal) + git|__git*|scalar|test-tool|test_terminal) return 0 ;; *)
Signature · 1 line
gitgitgadget
PATCH 2/3 t921*: test scalar behavior starting maintenance
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
A user recently reported issues with 'scalar register' and 'scalar clone' in that they failed when the system had permissions locked down so both 'crontab' and 'systemctl' commands failed when trying to enable background maintenance.
This hard error is undesirable, but let's create tests that demonstrate this behavior before modiying the behavior. We can use GIT_TEST_MAINT_SCHEDULER to guarantee failure and check the exit code and error message.
--- t/t9210-scalar.sh | 7 +++++++ t/t9211-scalar-clone.sh | 6 ++++++ 2 files changed, 13 insertions(+)
t/t9210-scalar.sh +7 -0
@@ -104,6 +104,13 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' ' test_cmp_config -C test/src true core.fsmonitor ' +test_expect_success 'scalar register fails when background maintenance fails' ' + git init register-repo && + GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ + test_must_fail scalar register register-repo 2>err && + grep "could not turn on maintenance" err +' + test_expect_success 'scalar unregister' ' git init vanish/src && scalar register vanish/src &&
t/t9211-scalar-clone.sh +6 -0
@@ -174,4 +174,10 @@ test_expect_success 'progress without tty' ' cleanup_clone $enlistment ' +test_expect_success 'scalar clone fails when background maintenance fails' ' + GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ + test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && + grep "could not turn on maintenance" err +' + test_done
Signature · 1 line
gitgitgadget
PATCH 3/3 scalar: only warn when background maintenance fails
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
A user reported issues with 'scalar clone' and 'scalar register' when working in an environment that had locked down the ability to run 'crontab' or 'systemctl' in that those commands registered as _failures_ instead of opportunistically reporting a success with just a warning about background maintenance.
As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a successful background maintenance, but this is not a viable strategy for long-term.
Update 'scalar register' and 'scalar clone' to no longer fail by modifying register_dir() to only warn when toggle_maintenance(1) fails.
Since background maintenance is a "nice to have" and not a requirement for a working repository, it is best to move this from hard error to gentle warning.
--- scalar.c | 2 +- t/t9210-scalar.sh | 4 ++-- t/t9211-scalar-clone.sh | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)
scalar.c +1 -1
@@ -262,7 +262,7 @@ static int register_dir(void)return error (_ ("could not set recommended config" ));if (toggle_maintenance (1 )) -return error (_ ("could not turn on maintenance" )); +warning (_ ("could not turn on maintenance" ));if (have_fsmonitor_support () &&start_fsmonitor_daemon ()) {return error (_ ("could not start the FSMonitor daemon" ));
t/t9210-scalar.sh +2 -2
@@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' ' test_cmp_config -C test/src true core.fsmonitor ' -test_expect_success 'scalar register fails when background maintenance fails' ' +test_expect_success 'scalar register warns when background maintenance fails' ' git init register-repo && GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ - test_must_fail scalar register register-repo 2>err && + scalar register register-repo 2>err && grep "could not turn on maintenance" err '
t/t9211-scalar-clone.sh +2 -2
@@ -174,9 +174,9 @@ test_expect_success 'progress without tty' ' cleanup_clone $enlistment ' -test_expect_success 'scalar clone fails when background maintenance fails' ' +test_expect_success 'scalar clone warns when background maintenance fails' ' GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ - test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && + scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && grep "could not turn on maintenance" err '
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 12 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
A user reported issues with 'scalar clone' and 'scalar register' when working in an environment that had locked down the ability to run 'crontab' or 'systemctl' in that those commands registered as _failures_ instead of opportunistically reporting a success with just a warning about background maintenance.
As a workaround, they can use
GIT_TEST_MAINT_SCHEDULERto fake a successful background maintenance, but this is not a viable strategy for long-term.
Quoted 6 lines
Update 'scalar register' and 'scalar clone' to no longer fail by modifying
register_dir()to only warn whentoggle_maintenance(1)fails.Since background maintenance is a "nice to have" and not a requirement for a working repository, it is best to move this from hard error to gentle warning.
Wasn't the main selling point of scalar that users do not have to worry about various minute details of configuration settings to maintain their clone of projects on the larger side? The "maintain their clone" certainly should include running periodic maintenance tasks without them having to worry about it. It feels like this is calling for an explicit "disable periodic maintenance tasks in this repository" option to help these esoteric environments that disable cron-like system services, while keeping the default safer, i.e. fail loudly when the periodic maintenance tasks that the users expect to happen cannot be enabled, or something.
Perhaps I am not the primary audience, but hmph, I have a feeling that this is not exactly going into a healthy direction.
Other two steps that led to this step looked quite sensible, though.
Thanks.
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 17 lines · Junio C Hamano
Quoted 15 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
A user reported issues with 'scalar clone' and 'scalar register' when working in an environment that had locked down the ability to run 'crontab' or 'systemctl' in that those commands registered as _failures_ instead of opportunistically reporting a success with just a warning about background maintenance.
As a workaround, they can use
GIT_TEST_MAINT_SCHEDULERto fake a successful background maintenance, but this is not a viable strategy for long-term.Update 'scalar register' and 'scalar clone' to no longer fail by modifying
register_dir()to only warn whentoggle_maintenance(1)fails.
Quoted 7 lines
Quoted 3 lines
Since background maintenance is a "nice to have" and not a requirement for a working repository, it is best to move this from hard error to gentle warning.
Wasn't the main selling point of scalar that users do not have to worry about various minute details of configuration settings to maintain their clone of projects on the larger side?
Yes, and that includes things like partial clone, sparse-checkout, and FS Monitor which are independent of this maintenance change.
Quoted 11 lines
The "maintain
their clone" certainly should include running periodic maintenance tasks without them having to worry about it. It feels like this is calling for an explicit "disable periodic maintenance tasks in this repository" option to help these esoteric environments that disable cron-like system services, while keeping the default safer, i.e. fail loudly when the periodic maintenance tasks that the users expect to happen cannot be enabled, or something.Perhaps I am not the primary audience, but hmph, I have a feeling that this is not exactly going into a healthy direction.
Here, we are in an environment where background maintenance is unavailable in an unexpected way. If that feature is not available to the user, should they not get the benefits of the others?
Not having background maintenance is definitely a downside, but it's different from failing to connect to the server or being unable to complete setting up the working directory. The warning communicates the issue, so users can realize the problem exists and work to resolve it in their own way.
Thanks,
-Stolee
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 9 lines · Derrick Stolee
Quoted 9 lines
The "maintain
their clone" certainly should include running periodic maintenance tasks without them having to worry about it. It feels like this is calling for an explicit "disable periodic maintenance tasks in this repository" option to help these esoteric environments that disable cron-like system services, while keeping the default safer, i.e. fail loudly when the periodic maintenance tasks that the users expect to happen cannot be enabled, or something.
Quoted 6 lines
Quoted 2 lines
Perhaps I am not the primary audience, but hmph, I have a feeling that this is not exactly going into a healthy direction.
Here, we are in an environment where background maintenance is unavailable in an unexpected way. If that feature is not available to the user, should they not get the benefits of the others?
That is not what I was saying. I just have expected to see a way for the user to give scalar an explicit "I understand that periodic maintenance does not happen in this repository" consent, instead of demoting an error detection for everybody to a warning that users will just ignore.
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 18 lines · Junio C Hamano
Quoted 9 lines · Derrick Stolee
Quoted 9 lines
The "maintain
their clone" certainly should include running periodic maintenance tasks without them having to worry about it. It feels like this is calling for an explicit "disable periodic maintenance tasks in this repository" option to help these esoteric environments that disable cron-like system services, while keeping the default safer, i.e. fail loudly when the periodic maintenance tasks that the users expect to happen cannot be enabled, or something.Quoted 6 lines
Quoted 2 lines
Perhaps I am not the primary audience, but hmph, I have a feeling that this is not exactly going into a healthy direction.
Here, we are in an environment where background maintenance is unavailable in an unexpected way. If that feature is not available to the user, should they not get the benefits of the others?
Quoted 5 lines
That is not what I was saying. I just have expected to see a way for the user to give scalar an explicit "I understand that periodic maintenance does not happen in this repository" consent, instead of demoting an error detection for everybody to a warning that users will just ignore.
Ah, so you'd prefer a --no-maintenance option for users who have this problem instead of just a warning. I'll do that in v2.
This could be a good time for me to upstream the --no-src option while I'm messing with arguments in 'scalar clone'.
Thanks,
-Stolee
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 4 lines · Derrick Stolee
Quoted 4 lines
Quoted 3 lines
Here, we are in an environment where background maintenance is unavailable in an unexpected way. If that feature is not available to the user, should they not get the benefits of the others?
Quoted 8 lines
Quoted 5 lines
That is not what I was saying. I just have expected to see a way for the user to give scalar an explicit "I understand that periodic maintenance does not happen in this repository" consent, instead of demoting an error detection for everybody to a warning that users will just ignore.
Ah, so you'd prefer a --no-maintenance option for users who have this problem instead of just a warning. I'll do that in v2.
Or a repository-local configuration to declare "no need to do the maintenance stuff here", probably? The expected use case you gave does not match per-invocation command line option very well, right?
Quoted 2 lines
This could be a good time for me to upstream the --no-src option while I'm messing with arguments in 'scalar clone'.
OK. Thanks.
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
In reply to Derrick StoleeQuoted 19 lines · Derrick Stolee
Quoted 18 lines · Junio C Hamano
Quoted 15 lines · Derrick Stolee
Quoted 11 lines
The "maintain
their clone" certainly should include running periodic maintenance tasks without them having to worry about it. It feels like this is calling for an explicit "disable periodic maintenance tasks in this repository" option to help these esoteric environments that disable cron-like system services, while keeping the default safer, i.e. fail loudly when the periodic maintenance tasks that the users expect to happen cannot be enabled, or something.Perhaps I am not the primary audience, but hmph, I have a feeling that this is not exactly going into a healthy direction.
Here, we are in an environment where background maintenance is unavailable in an unexpected way. If that feature is not available to the user, should they not get the benefits of the others?
Quoted 8 lines
Quoted 5 lines
That is not what I was saying. I just have expected to see a way for the user to give scalar an explicit "I understand that periodic maintenance does not happen in this repository" consent, instead of demoting an error detection for everybody to a warning that users will just ignore.
Ah, so you'd prefer a --no-maintenance option for users who have this problem instead of just a warning. I'll do that in v2.
I mentioned this earlier [1], but I want to reiterate that I really don't think a dedicated '--no-maintenance' option is a good approach to this problem. I understand wanting more active user acknowledgement that "I understand that periodic maintenance does not happen in this repository"; without that, users may (rightfully) be confused when they find their scalar-cloned repository full of loose objects. But, in the use case you've presented (where no scheduler is available), the user would need to - somewhat redundantly, I feel - acknowledge that for *every* repository they clone.
I'm also still worried about cluttering scalar's UX with options that toggle use of its internally-configured options and features. One of the big selling points for including scalar in the upstream project ([2], [3]) was its ability to "intelligently" configure all of the settings a user would need to optimize a large repository *without* a user needing to know what any of those options are/what they mean. These settings are inherently subject to change (due to use of experimental features); exposing a feature toggle entrenches that setting permanently within scalar and makes a user aware of implementation details that were intended to be hidden. At a high level, it pushes scalar towards simply being an "opinionated" 'git config'-configurator, which was a model I explicitly tried to move away from while upstreaming last year.
I still believe treating maintenance like FSMonitor - pre-determining whether the feature is available and only enabling it if possible - is the most consistent and user-friendly solution to the given problem within the context of scalar. But, if you feel that user acknowledgement is absolutely critical, I'd strongly prefer a config setting like 'maintenance.enabled'; it could be set globally (the appropriate scope in the case of a system that has no scheduler), or with '-c' with Scalar clone if it really needs to be per-repo.
[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/[email protected]/
Quoted 3 lines
This could be a good time for me to upstream the --no-src option while I'm messing with arguments in 'scalar clone'.
For what it's worth, my concerns about option clutter don't really apply to '--no-src' (cloning directly into a given directory, rather than '<directory>/src'). Unlike features like FSMonitor and maintenance, the 'src/' directory is a user-facing Scalar design decision. It's also something that seems to exist primarily for backward-compatibility reasons (if I'm interpreting your earlier comments [4] correctly). This could be a step on a deprecation path to make '--no-src' the default and remove the legacy enlistment structure? At the very least, it's sufficiently outside scalar's "configure for a large repo" scope for me to not worry about it setting a bad precedent.
[4] https://lore.kernel.org/git/[email protected]/
Quoted 3 lines
Thanks,
-Stolee
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 4 lines · Victoria Dye
I'm also still worried about cluttering scalar's UX with options that toggle use of its internally-configured options and features. One of the big selling points for including scalar in the upstream project ([2], [3]) was its ability to "intelligently" configure all of the settings a user would
Quoted 8 lines
need to optimize a large repository *without* a user needing to know what any of those options are/what they mean. These settings are inherently subject to change (due to use of experimental features); exposing a feature toggle entrenches that setting permanently within scalar and makes a user aware of implementation details that were intended to be hidden. At a high level, it pushes scalar towards simply being an "opinionated" 'git config'-configurator, which was a model I explicitly tried to move away from while upstreaming last year.
I personally do not think "opinionated configurator" is a bad model at all. And "this does not seem to work here, so let's silently disable it, as the user does not want to hear about minute details" is a valid opinion to have for such a tool.
I too share the aversion to command line option for this one. Disabled periodic task support is most likely system-wide, and passing --no-whatever every time you touch a new repository on the same system does not make much sense.
Thanks.
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 20 lines · Junio C Hamano
Quoted 4 lines · Victoria Dye
I'm also still worried about cluttering scalar's UX with options that toggle use of its internally-configured options and features. One of the big selling points for including scalar in the upstream project ([2], [3]) was its ability to "intelligently" configure all of the settings a user would
Quoted 8 lines
need to optimize a large repository *without* a user needing to know what any of those options are/what they mean. These settings are inherently subject to change (due to use of experimental features); exposing a feature toggle entrenches that setting permanently within scalar and makes a user aware of implementation details that were intended to be hidden. At a high level, it pushes scalar towards simply being an "opinionated" 'git config'-configurator, which was a model I explicitly tried to move away from while upstreaming last year.
I personally do not think "opinionated configurator" is a bad model at all. And "this does not seem to work here, so let's silently disable it, as the user does not want to hear about minute details" is a valid opinion to have for such a tool.
Quoted 4 lines
I too share the aversion to command line option for this one. Disabled periodic task support is most likely system-wide, and passing --no-whatever every time you touch a new repository on the same system does not make much sense.
Thanks, both. v2 will include --no-src, but not --no-maintenance.
-Stolee
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
In reply to Junio C HamanoQuoted 21 lines · Junio C Hamano
Quoted 12 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
A user reported issues with 'scalar clone' and 'scalar register' when working in an environment that had locked down the ability to run 'crontab' or 'systemctl' in that those commands registered as _failures_ instead of opportunistically reporting a success with just a warning about background maintenance.
As a workaround, they can use
GIT_TEST_MAINT_SCHEDULERto fake a successful background maintenance, but this is not a viable strategy for long-term.Quoted 6 lines
Update 'scalar register' and 'scalar clone' to no longer fail by modifying
register_dir()to only warn whentoggle_maintenance(1)fails.Since background maintenance is a "nice to have" and not a requirement for a working repository, it is best to move this from hard error to gentle warning.
Quoted 10 lines
Wasn't the main selling point of scalar that users do not have to worry about various minute details of configuration settings to maintain their clone of projects on the larger side? The "maintain their clone" certainly should include running periodic maintenance tasks without them having to worry about it. It feels like this is calling for an explicit "disable periodic maintenance tasks in this repository" option to help these esoteric environments that disable cron-like system services, while keeping the default safer, i.e. fail loudly when the periodic maintenance tasks that the users expect to happen cannot be enabled, or something.
I see Stolee's approach as more in line with how FSMonitor is treated by 'scalar', which is "only turn it on if it's supported, but otherwise do nothing" (the main difference here being that a warning is displayed if maintenance can't be turned on). That still fits the stated goal of 'scalar' ("configure all the niche large-repo settings for me when I clone/register"), but it makes 'scalar' more forgiving of system configurations that don't support maintenance.
That said, this doesn't distinguish between "maintenance couldn't be turned on because the system doesn't support it" and "maintenance couldn't be turned on because of an internal error". The latter might still be worth failing on, so maybe something like this would be more palatable?
--------8<--------8<--------8<--------
scalar.c +5 -1
@@ -252,6 +252,10 @@ static int stop_fsmonitor_daemon(void)return 0 ; } +static int have_maintenance_support (void ) { +/* check whether at least one scheduler is supported on the system */ +} +static int register_dir (void ) {if (add_or_remove_enlistment (1 ))
@@ -260,7 +264,7 @@ static int register_dir(void)if (set_recommended_config (0 ))return error (_ ("could not set recommended config" )); -if (toggle_maintenance (1 )) +if (have_maintenance_support () &&toggle_maintenance (1 ))return error (_ ("could not turn on maintenance" ));if (have_fsmonitor_support () &&start_fsmonitor_daemon ()) {
-------->8-------->8-------->8--------
Quoted 3 lines
Perhaps I am not the primary audience, but hmph, I have a feeling that this is not exactly going into a healthy direction.
I don't think this change would be the start of a larger pattern, since most Scalar-related settings aren't system-dependent (only FSMonitor and maintenance are, AFAIK). What would be worse, I think, is setting the maintenance behavior with a new command option - I could very easily see *that* leading to a slippery slope of endless options to toggle 'scalar's behaviors, completely defeating it's whole "set everything up for me" purpose.
Quoted 4 lines
Other two steps that led to this step looked quite sensible, though.
Thanks.
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 2 lines · Victoria Dye
Quoted 1 line · Junio C Hamano
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
This reply almost got lost in the shuffle, but mostly because it wasn't super-relevant if we were going with the --no-maintenance option. It's relevant again, so I wanted to point something out.
Quoted 30 lines
I see Stolee's approach as more in line with how FSMonitor is treated by 'scalar', which is "only turn it on if it's supported, but otherwise do nothing" (the main difference here being that a warning is displayed if maintenance can't be turned on). That still fits the stated goal of 'scalar' ("configure all the niche large-repo settings for me when I clone/register"), but it makes 'scalar' more forgiving of system configurations that don't support maintenance.
That said, this doesn't distinguish between "maintenance couldn't be turned on because the system doesn't support it" and "maintenance couldn't be turned on because of an internal error". The latter might still be worth failing on, so maybe something like this would be more palatable?
--------8<--------8<--------8<--------
scalar.c +4
-0@@ -252,6 +252,10 @@ static int stop_fsmonitor_daemon(void)return 0 ; } +static int have_maintenance_support (void ) { +/* check whether at least one scheduler is supported on the system */ +} +static int register_dir (void ) {if (add_or_remove_enlistment (1 ))
scalar.c static int register_dir(void) · line 270
Quoted 8 lines
scalar.c +1
-1@@ -260,7 +264,7 @@ static int register_dir(void)if (set_recommended_config (0 ))return error (_ ("could not set recommended config" )); -if (toggle_maintenance (1 )) +if (have_maintenance_support () &&toggle_maintenance (1 ))return error (_ ("could not turn on maintenance" ));if (have_fsmonitor_support () &&start_fsmonitor_daemon ()) {
The tricky thing about this is that have_fsmonitor_support() is something we can detect at compile time, while have_maintenance_support() would not (unless we start building for a new platform).
The case that brought this up is a platform that has both 'crontab' and 'systemctl' on the PATH, but when executing those commands an error occurs due to permissions.
So, if we wanted to distinguish between permissions issues and/or other unrelated failures, we would need to be able to parse the output of those commands. That seems fraught with potential errors, so it seems like we should _attempt_ to enable maintenance and warn with whatever failure is presented.
The only thing I could think is that we could define a custom exit code within 'git maintenance start' that means "we were able to start the scheduler process, but it failed" to differentiate from other kinds of failures, such as failing to write to global config.
Thanks,
-Stolee
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
In reply to Derrick Stolee via GitGitGadgetQuoted 31 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
A user reported issues with 'scalar clone' and 'scalar register' when working in an environment that had locked down the ability to run 'crontab' or 'systemctl' in that those commands registered as _failures_ instead of opportunistically reporting a success with just a warning about background maintenance.
As a workaround, they can use
GIT_TEST_MAINT_SCHEDULERto fake a successful background maintenance, but this is not a viable strategy for long-term.Update 'scalar register' and 'scalar clone' to no longer fail by modifying
register_dir()to only warn whentoggle_maintenance(1)fails.Since background maintenance is a "nice to have" and not a requirement for a working repository, it is best to move this from hard error to gentle warning.
Signed-off-by Derrick Stolee <[email protected]>--- scalar.c | 2 +- t/t9210-scalar.sh | 4 ++-- t/t9211-scalar-clone.sh | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)scalar.c +0
-0
scalar.c static int register_dir(void) · line 265
Quoted 5 lines
scalar.c +1
-1@@ -262,7 +262,7 @@ static int register_dir(void)return error (_ ("could not set recommended config" ));if (toggle_maintenance (1 )) -return error (_ ("could not turn on maintenance" )); +warning (_ ("could not turn on maintenance" ));
Should we do the same thing for 'unregister_dir()'? Unlike 'register_dir()', it doesn't break immediately (and finishes removing the enlistment), but it still returns a nonzero error code from 'scalar unregister'.
Quoted 25 lines
scalar.c +0
-0if (have_fsmonitor_support () &&start_fsmonitor_daemon ()) {return error (_ ("could not start the FSMonitor daemon" ));t/t9210-scalar.sh +2
-2@@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' ' test_cmp_config -C test/src true core.fsmonitor ' -test_expect_success 'scalar register fails when background maintenance fails' ' +test_expect_success 'scalar register warns when background maintenance fails' ' git init register-repo && GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ - test_must_fail scalar register register-repo 2>err && + scalar register register-repo 2>err && grep "could not turn on maintenance" err 't/t9211-scalar-clone.sh +0
-0
t/t9211-scalar-clone.sh test_expect_success 'progress without tty' ' · line 181
Quoted 10 lines
t/t9211-scalar-clone.sh +2
-2@@ -174,9 +174,9 @@ test_expect_success 'progress without tty' ' cleanup_clone $enlistment ' -test_expect_success 'scalar clone fails when background maintenance fails' ' +test_expect_success 'scalar clone warns when background maintenance fails' ' GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ - test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && + scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && grep "could not turn on maintenance" err '
Similarly, it might be nice to show how 'scalar unregister' behaves when maintenance fails in the tests.
Re: [PATCH 3/3] scalar: only warn when background maintenance fails
Quoted 8 lines · Victoria Dye
scalar.c static int register_dir(void) · line 265
Quoted 3 lines · Derrick Stolee via GitGitGadget
scalar.c +1
-1if (toggle_maintenance (1 )) -return error (_ ("could not turn on maintenance" )); +warning (_ ("could not turn on maintenance" ));Should we do the same thing for '
unregister_dir()'? Unlike 'register_dir()', it doesn't break immediately (and finishes removing the enlistment), but it still returns a nonzero error code from 'scalar unregister'.
The interesting thing about unregister_dir() is that toggle_maintenance(0) "turns of maintenance" by removing the maintenance.repo config value pointing to this repository, not by removing the maintenance schedule. Thus, we don't get the failure in the same way.
Quoted 10 lines
t/t9211-scalar-clone.sh test_expect_success 'progress without tty' ' · line 181
Quoted 7 lines
t/t9211-scalar-clone.sh +2
-2-test_expect_success 'scalar clone fails when background maintenance fails' ' +test_expect_success 'scalar clone warns when background maintenance fails' ' GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \ - test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && + scalar clone "file://$(pwd)/to-clone" maint-fail 2>err && grep "could not turn on maintenance" err 'Similarly, it might be nice to show how 'scalar unregister' behaves when maintenance fails in the tests.
And the way I found out was by making the same tests, but since they actually never failed or listed a warning (for this reason) I left it out.
Thanks,
-Stolee