patch, 3 partsAllow scalar to succeed despite maintenance failures

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

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

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

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

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

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

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

> "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()) {

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

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

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