Re: [PATCH] maintenance: disable cron on macOS
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 10, 2021, 18:58 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2111101953300.21127@tvgsbejvaqbjf.bet>
- In-Reply-To
- <pull.1075.git.1636569360002.gitgitgadget@gmail.com>
Hi Stolee,
On Wed, 10 Nov 2021, Derrick Stolee via GitGitGadget wrote:
Show 38 quoted lines
> 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
Show 64 quoted lines
>
> 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
>
>