[PATCH 0/3] Allow scalar to succeed despite maintenance failures

[PATCH 0/3] Allow scalar to succeed despite maintenance failures

[Derrick Stolee via GitGitGadget]

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
-- 
gitgitgadget

[PATCH 1/3] t: allow 'scalar' in test_must_fail

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

This will enable scalar tests to use the test_must_fail helper, when
necessary.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 79922227030..75b8ee95e7f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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
 		;;
 	*)
-- 
gitgitgadget

[PATCH 2/3] t921*: test scalar behavior starting maintenance

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t9210-scalar.sh       | 7 +++++++
 t/t9211-scalar-clone.sh | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 25f500cf682..13a4f6dbcf4 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -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 &&
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index 02d32fb6ede..a6156da29ac 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -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
-- 
gitgitgadget

[PATCH 3/3] scalar: only warn when background maintenance fails

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 scalar.c                | 2 +-
 t/t9210-scalar.sh       | 4 ++--
 t/t9211-scalar-clone.sh | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/scalar.c b/scalar.c
index f25d5f1d0ef..ca19b95ce46 100644
--- a/scalar.c
+++ b/scalar.c
@@ -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"));
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 13a4f6dbcf4..4432a30d10b 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -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
 '
 
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index a6156da29ac..872ad1c9c2b 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -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
 '
 
-- 
gitgitgadget

Re: [PATCH 3/3] scalar: only warn when background maintenance fails

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> 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.

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

[Derrick Stolee]

On 1/27/2023 3:36 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> 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.
> 
> 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.

>  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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>>  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?

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

[Derrick Stolee]

On 1/27/2023 7:32 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>>>  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?
> 
> 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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

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

> 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

[Victoria Dye]

Derrick Stolee wrote:
> On 1/27/2023 7:32 PM, Junio C Hamano wrote:
>> Derrick Stolee <derrickstolee@github.com> writes:
>>
>>>>  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?
>>
>> 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/3ade6d9f-8477-40c2-d683-d629e863c6ab@github.com/
[2] https://lore.kernel.org/git/pull.1005.git.1630359290.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/pull.1275.git.1656521925.gitgitgadget@gmail.com/

> 
> 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/82716e5b-3522-68f5-7479-1b39811e0cb2@github.com/

> 
> Thanks,
> -Stolee

Re: [PATCH 3/3] scalar: only warn when background maintenance fails

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

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

[Derrick Stolee]

On 1/30/2023 1:58 PM, Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
> 
>> 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 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, both. v2 will include --no-src, but not --no-maintenance.

-Stolee

Re: [PATCH 3/3] scalar: only warn when background maintenance fails

[Victoria Dye]

Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> 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.
> 
> 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<--------
diff --git a/scalar.c b/scalar.c
index 6c52243cdf1..138780abe5f 100644
--- a/scalar.c
+++ b/scalar.c
@@ -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--------

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

> 
> Other two steps that led to this step looked quite sensible, though.
> 
> Thanks.

Re: [PATCH 3/3] scalar: only warn when background maintenance fails

[Derrick Stolee]

On 1/27/2023 5:18 PM, Victoria Dye wrote:
> Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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.

> 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<--------
> diff --git a/scalar.c b/scalar.c
> index 6c52243cdf1..138780abe5f 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -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()) {

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

[Victoria Dye]

Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> 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.
> 
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  scalar.c                | 2 +-
>  t/t9210-scalar.sh       | 4 ++--
>  t/t9211-scalar-clone.sh | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/scalar.c b/scalar.c
> index f25d5f1d0ef..ca19b95ce46 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -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'.

>  
>  	if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
>  		return error(_("could not start the FSMonitor daemon"));
> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
> index 13a4f6dbcf4..4432a30d10b 100755
> --- a/t/t9210-scalar.sh
> +++ b/t/t9210-scalar.sh
> @@ -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
>  '
>  
> diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
> index a6156da29ac..872ad1c9c2b 100755
> --- a/t/t9211-scalar-clone.sh
> +++ b/t/t9211-scalar-clone.sh
> @@ -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

[Derrick Stolee]

On 1/27/2023 5:06 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>>  	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'.

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.

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