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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 30, 2023, 19:25 UTC
Message-ID: <1958e192-3ffe-ac10-4097-6da5eba02f4f@github.com>
In-Reply-To: <3ade6d9f-8477-40c2-d683-d629e863c6ab@github.com>

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.

Show 38 quoted 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<--------
> 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