Re: [PATCH] maintenance: disable cron on macOS

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stolee,

On Wed, 10 Nov 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> In eba1ba9 (maintenance: `git maintenance run` learned
> `--scheduler=<scheduler>`, 2021-09-04), we introduced the ability to
> specify a scheduler explicitly. This led to some extra checks around
> whether an alternative scheduler was available. This added the
> functionality of removing background maintenance from schedulers other
> than the one selected.
>
> On macOS, cron is technically available, but running 'crontab' triggers
> a UI prompt asking for special permissions. This is the major reason why
> launchctl is used as the default scheduler. The is_crontab_available()
> method triggers this UI prompt, causing user disruption.
>
> Remove this disruption by using an #ifdef to prevent running crontab
> this way on macOS. This has the unfortunate downside that if a user
> manually selects cron via the '--scheduler' option, then adjusting the
> scheduler later will not remove the schedule from cron. The
> '--scheduler' option ignores the is_available checks, which is how we
> can get into this situation.
>
> Extract the new check_crontab_process() method to avoid making the
> 'child' variable unused on macOS. The method is marked MAYBE_UNUSED
> because it has no callers on macOS.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     [For 2.34.0 Release] maintenance: disable cron on macOS
>
>     This one is really tricky because we can't notice anything is wrong
>     without running git maintenance start or git maintenance stop
>     interactively on macOS. The tests pass just fine because the UI alert
>     gets automatically ignored during the test suite.
>
>     This is a bit of a half-fix: it avoids the UI alert, but has a corner
>     case of not un-doing the cron schedule if a user manages to select it
>     (under suitable permissions such that it succeeds). For the purpose of
>     the timing of the release, I think this is an appropriate hedge.

I agree.

We can revisit this once v2.34.0 is released, and then determine a better
layer to prevent this, or alternatively warn very loudly about it.

Thanks,
Dscho

>
>     Thanks! -Stolee
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1075%2Fderrickstolee%2Fmaintenance-cron-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1075/derrickstolee/maintenance-cron-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1075
>
>  builtin/gc.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 26709311601..bcef6a4c8da 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1999,15 +1999,11 @@ static int schtasks_update_schedule(int run_maintenance, int fd)
>  		return schtasks_remove_tasks();
>  }
>
> -static int is_crontab_available(void)
> +MAYBE_UNUSED
> +static int check_crontab_process(const char *cmd)
>  {
> -	const char *cmd = "crontab";
> -	int is_available;
>  	struct child_process child = CHILD_PROCESS_INIT;
>
> -	if (get_schedule_cmd(&cmd, &is_available))
> -		return is_available;
> -
>  	strvec_split(&child.args, cmd);
>  	strvec_push(&child.args, "-l");
>  	child.no_stdin = 1;
> @@ -2022,6 +2018,25 @@ static int is_crontab_available(void)
>  	return 1;
>  }
>
> +static int is_crontab_available(void)
> +{
> +	const char *cmd = "crontab";
> +	int is_available;
> +
> +	if (get_schedule_cmd(&cmd, &is_available))
> +		return is_available;
> +
> +#ifdef __APPLE__
> +	/*
> +	 * macOS has cron, but it requires special permissions and will
> +	 * create a UI alert when attempting to run this command.
> +	 */
> +	return 0;
> +#else
> +	return check_crontab_process(cmd);
> +#endif
> +}
> +
>  #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
>  #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
>
>
> base-commit: 6c220937e2b26d85920bf2d38ff2464a0d57fd6b
> --
> gitgitgadget
>
>