{"thread":{"id":"56871","subject":"[PATCH] maintenance: disable cron on macOS","startedAt":"2021-11-10T18:36:06Z","lastAt":"2021-11-10T21:06:09Z","messageCount":6,"participants":["Derrick Stolee via GitGitGadget","Johannes Schindelin","Junio C Hamano","Derrick Stolee","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440858","messageId":"pull.1075.git.1636569360002.gitgitgadget@gmail.com","threadId":"56871","inReplyTo":null,"subject":"[PATCH] maintenance: disable cron on macOS","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-10T18:35:59Z","receivedAt":"2021-11-10T18:36:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nIn eba1ba9 (maintenance: `git maintenance run` learned\n`--scheduler=<scheduler>`, 2021-09-04), we introduced the ability to\nspecify a scheduler explicitly. This led to some extra checks around\nwhether an alternative scheduler was available. This added the\nfunctionality of removing background maintenance from schedulers other\nthan the one selected.\n\nOn macOS, cron is technically available, but running 'crontab' triggers\na UI prompt asking for special permissions. This is the major reason why\nlaunchctl is used as the default scheduler. The is_crontab_available()\nmethod triggers this UI prompt, causing user disruption.\n\nRemove this disruption by using an #ifdef to prevent running crontab\nthis way on macOS. This has the unfortunate downside that if a user\nmanually selects cron via the '--scheduler' option, then adjusting the\nscheduler later will not remove the schedule from cron. The\n'--scheduler' option ignores the is_available checks, which is how we\ncan get into this situation.\n\nExtract the new check_crontab_process() method to avoid making the\n'child' variable unused on macOS. The method is marked MAYBE_UNUSED\nbecause it has no callers on macOS.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    [For 2.34.0 Release] maintenance: disable cron on macOS\n    \n    This one is really tricky because we can't notice anything is wrong\n    without running git maintenance start or git maintenance stop\n    interactively on macOS. The tests pass just fine because the UI alert\n    gets automatically ignored during the test suite.\n    \n    This is a bit of a half-fix: it avoids the UI alert, but has a corner\n    case of not un-doing the cron schedule if a user manages to select it\n    (under suitable permissions such that it succeeds). For the purpose of\n    the timing of the release, I think this is an appropriate hedge.\n    \n    Thanks! -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1075%2Fderrickstolee%2Fmaintenance-cron-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1075/derrickstolee/maintenance-cron-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1075\n\n builtin/gc.c | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 26709311601..bcef6a4c8da 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1999,15 +1999,11 @@ static int schtasks_update_schedule(int run_maintenance, int fd)\n \t\treturn schtasks_remove_tasks();\n }\n \n-static int is_crontab_available(void)\n+MAYBE_UNUSED\n+static int check_crontab_process(const char *cmd)\n {\n-\tconst char *cmd = \"crontab\";\n-\tint is_available;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tif (get_schedule_cmd(&cmd, &is_available))\n-\t\treturn is_available;\n-\n \tstrvec_split(&child.args, cmd);\n \tstrvec_push(&child.args, \"-l\");\n \tchild.no_stdin = 1;\n@@ -2022,6 +2018,25 @@ static int is_crontab_available(void)\n \treturn 1;\n }\n \n+static int is_crontab_available(void)\n+{\n+\tconst char *cmd = \"crontab\";\n+\tint is_available;\n+\n+\tif (get_schedule_cmd(&cmd, &is_available))\n+\t\treturn is_available;\n+\n+#ifdef __APPLE__\n+\t/*\n+\t * macOS has cron, but it requires special permissions and will\n+\t * create a UI alert when attempting to run this command.\n+\t */\n+\treturn 0;\n+#else\n+\treturn check_crontab_process(cmd);\n+#endif\n+}\n+\n #define BEGIN_LINE \"# BEGIN GIT MAINTENANCE SCHEDULE\"\n #define END_LINE \"# END GIT MAINTENANCE SCHEDULE\"\n \n\nbase-commit: 6c220937e2b26d85920bf2d38ff2464a0d57fd6b\n-- \ngitgitgadget\n"},{"id":"440859","messageId":"nycvar.QRO.7.76.6.2111101953300.21127@tvgsbejvaqbjf.bet","threadId":"56871","inReplyTo":"pull.1075.git.1636569360002.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: disable cron on macOS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T18:58:12Z","receivedAt":"2021-11-10T19:00:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Wed, 10 Nov 2021, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> In eba1ba9 (maintenance: `git maintenance run` learned\n> `--scheduler=<scheduler>`, 2021-09-04), we introduced the ability to\n> specify a scheduler explicitly. This led to some extra checks around\n> whether an alternative scheduler was available. This added the\n> functionality of removing background maintenance from schedulers other\n> than the one selected.\n>\n> On macOS, cron is technically available, but running 'crontab' triggers\n> a UI prompt asking for special permissions. This is the major reason why\n> launchctl is used as the default scheduler. The is_crontab_available()\n> method triggers this UI prompt, causing user disruption.\n>\n> Remove this disruption by using an #ifdef to prevent running crontab\n> this way on macOS. This has the unfortunate downside that if a user\n> manually selects cron via the '--scheduler' option, then adjusting the\n> scheduler later will not remove the schedule from cron. The\n> '--scheduler' option ignores the is_available checks, which is how we\n> can get into this situation.\n>\n> Extract the new check_crontab_process() method to avoid making the\n> 'child' variable unused on macOS. The method is marked MAYBE_UNUSED\n> because it has no callers on macOS.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     [For 2.34.0 Release] maintenance: disable cron on macOS\n>\n>     This one is really tricky because we can't notice anything is wrong\n>     without running git maintenance start or git maintenance stop\n>     interactively on macOS. The tests pass just fine because the UI alert\n>     gets automatically ignored during the test suite.\n>\n>     This is a bit of a half-fix: it avoids the UI alert, but has a corner\n>     case of not un-doing the cron schedule if a user manages to select it\n>     (under suitable permissions such that it succeeds). For the purpose of\n>     the timing of the release, I think this is an appropriate hedge.\n\nI agree.\n\nWe can revisit this once v2.34.0 is released, and then determine a better\nlayer to prevent this, or alternatively warn very loudly about it.\n\nThanks,\nDscho\n\n>\n>     Thanks! -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1075%2Fderrickstolee%2Fmaintenance-cron-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1075/derrickstolee/maintenance-cron-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1075\n>\n>  builtin/gc.c | 27 +++++++++++++++++++++------\n>  1 file changed, 21 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 26709311601..bcef6a4c8da 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1999,15 +1999,11 @@ static int schtasks_update_schedule(int run_maintenance, int fd)\n>  \t\treturn schtasks_remove_tasks();\n>  }\n>\n> -static int is_crontab_available(void)\n> +MAYBE_UNUSED\n> +static int check_crontab_process(const char *cmd)\n>  {\n> -\tconst char *cmd = \"crontab\";\n> -\tint is_available;\n>  \tstruct child_process child = CHILD_PROCESS_INIT;\n>\n> -\tif (get_schedule_cmd(&cmd, &is_available))\n> -\t\treturn is_available;\n> -\n>  \tstrvec_split(&child.args, cmd);\n>  \tstrvec_push(&child.args, \"-l\");\n>  \tchild.no_stdin = 1;\n> @@ -2022,6 +2018,25 @@ static int is_crontab_available(void)\n>  \treturn 1;\n>  }\n>\n> +static int is_crontab_available(void)\n> +{\n> +\tconst char *cmd = \"crontab\";\n> +\tint is_available;\n> +\n> +\tif (get_schedule_cmd(&cmd, &is_available))\n> +\t\treturn is_available;\n> +\n> +#ifdef __APPLE__\n> +\t/*\n> +\t * macOS has cron, but it requires special permissions and will\n> +\t * create a UI alert when attempting to run this command.\n> +\t */\n> +\treturn 0;\n> +#else\n> +\treturn check_crontab_process(cmd);\n> +#endif\n> +}\n> +\n>  #define BEGIN_LINE \"# BEGIN GIT MAINTENANCE SCHEDULE\"\n>  #define END_LINE \"# END GIT MAINTENANCE SCHEDULE\"\n>\n>\n> base-commit: 6c220937e2b26d85920bf2d38ff2464a0d57fd6b\n> --\n> gitgitgadget\n>\n>\n"},{"id":"440866","messageId":"xmqqh7cjyeqb.fsf@gitster.g","threadId":"56871","inReplyTo":"pull.1075.git.1636569360002.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: disable cron on macOS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T19:47:56Z","receivedAt":"2021-11-10T19:48:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     This one is really tricky because we can't notice anything is wrong\n>     without running git maintenance start or git maintenance stop\n>     interactively on macOS. The tests pass just fine because the UI alert\n>     gets automatically ignored during the test suite.\n>     \n>     This is a bit of a half-fix: it avoids the UI alert, but has a corner\n>     case of not un-doing the cron schedule if a user manages to select it\n>     (under suitable permissions such that it succeeds). For the purpose of\n>     the timing of the release, I think this is an appropriate hedge.\n\nReverting the \"git maintainance run --scheduler=<scheduler>\" topic\naltogether to give us time to stop and think may be another obvious\noption, but it probably is OK to tell macOS users that they may have\ncron but Git won't work with their cron with this patch.\n\nIn any case, let's leave it (slightly) broken for macOS folks by\napplying this quickfix.\n\nThanks.\n\n\nPS.\n\nI was puzzled by the lack of a call to validate_scheduler() at the\nvery beginning of resolve_scheduler(), by the way.  Or even earlier\nduring the verification of the command line options.  I wonder if\non macOS, --scheduler=nosuch and --scheduler=cron should behave the\nsame way, both saying \"unrecognized\", instead of saying that only to\nthe former and \"not available\" to the latter.\n"},{"id":"440867","messageId":"d4564a73-792c-4ad5-03da-e64d1571d092@gmail.com","threadId":"56871","inReplyTo":"xmqqh7cjyeqb.fsf@gitster.g","subject":"Re: [PATCH] maintenance: disable cron on macOS","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-11-10T20:01:58Z","receivedAt":"2021-11-10T20:02:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/10/2021 2:47 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>>     This one is really tricky because we can't notice anything is wrong\n>>     without running git maintenance start or git maintenance stop\n>>     interactively on macOS. The tests pass just fine because the UI alert\n>>     gets automatically ignored during the test suite.\n>>     \n>>     This is a bit of a half-fix: it avoids the UI alert, but has a corner\n>>     case of not un-doing the cron schedule if a user manages to select it\n>>     (under suitable permissions such that it succeeds). For the purpose of\n>>     the timing of the release, I think this is an appropriate hedge.\n> \n> Reverting the \"git maintainance run --scheduler=<scheduler>\" topic\n> altogether to give us time to stop and think may be another obvious\n> option, but it probably is OK to tell macOS users that they may have\n> cron but Git won't work with their cron with this patch.\n> \n> In any case, let's leave it (slightly) broken for macOS folks by\n> applying this quickfix.\n\nI agree that we don't need to revert the topic.\n\n> I was puzzled by the lack of a call to validate_scheduler() at the\n> very beginning of resolve_scheduler(), by the way.  Or even earlier\n> during the verification of the command line options.  I wonder if\n> on macOS, --scheduler=nosuch and --scheduler=cron should behave the\n> same way, both saying \"unrecognized\", instead of saying that only to\n> the former and \"not available\" to the latter.\n\nIt's a bit strange, but the crux is that selecting --scheduler=X\nmakes the 'X' scheduler pass through the 'enable' path, while the\nrest of the \"available\" schedulers goes through the 'disable' path.\n\nThis UI prompt occurs because we are trying to 'disable' cron\nbefore enabling launchctl.\n\nI think the biggest issue is how macOS uses non-standard ways of\nrequesting user permissions, because if 'crontab' simply exited\nwith a non-zero status, there would be no work to do here. Because\nof that, I think we will always need to tiptoe around cron on macOS.\n\nThanks,\n-Stolee\n"},{"id":"440873","messageId":"211110.86y25vpw1y.gmgdl@evledraar.gmail.com","threadId":"56871","inReplyTo":"pull.1075.git.1636569360002.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: disable cron on macOS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T20:56:13Z","receivedAt":"2021-11-10T20:58:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 10 2021, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> In eba1ba9 (maintenance: `git maintenance run` learned\n> `--scheduler=<scheduler>`, 2021-09-04), we introduced the ability to\n> specify a scheduler explicitly. This led to some extra checks around\n> whether an alternative scheduler was available. This added the\n> functionality of removing background maintenance from schedulers other\n> than the one selected.\n>\n> On macOS, cron is technically available, but running 'crontab' triggers\n> a UI prompt asking for special permissions. This is the major reason why\n> launchctl is used as the default scheduler. The is_crontab_available()\n> method triggers this UI prompt, causing user disruption.\n>\n> Remove this disruption by using an #ifdef to prevent running crontab\n> this way on macOS. This has the unfortunate downside that if a user\n> manually selects cron via the '--scheduler' option, then adjusting the\n> scheduler later will not remove the schedule from cron. The\n> '--scheduler' option ignores the is_available checks, which is how we\n> can get into this situation.\n>\n> Extract the new check_crontab_process() method to avoid making the\n> 'child' variable unused on macOS. The method is marked MAYBE_UNUSED\n> because it has no callers on macOS.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     [For 2.34.0 Release] maintenance: disable cron on macOS\n>     \n>     This one is really tricky because we can't notice anything is wrong\n>     without running git maintenance start or git maintenance stop\n>     interactively on macOS. The tests pass just fine because the UI alert\n>     gets automatically ignored during the test suite.\n>     \n>     This is a bit of a half-fix: it avoids the UI alert, but has a corner\n>     case of not un-doing the cron schedule if a user manages to select it\n>     (under suitable permissions such that it succeeds). For the purpose of\n>     the timing of the release, I think this is an appropriate hedge.\n>     \n>     Thanks! -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1075%2Fderrickstolee%2Fmaintenance-cron-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1075/derrickstolee/maintenance-cron-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1075\n>\n>  builtin/gc.c | 27 +++++++++++++++++++++------\n>  1 file changed, 21 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 26709311601..bcef6a4c8da 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1999,15 +1999,11 @@ static int schtasks_update_schedule(int run_maintenance, int fd)\n>  \t\treturn schtasks_remove_tasks();\n>  }\n>  \n> -static int is_crontab_available(void)\n> +MAYBE_UNUSED\n> +static int check_crontab_process(const char *cmd)\n>  {\n> -\tconst char *cmd = \"crontab\";\n> -\tint is_available;\n>  \tstruct child_process child = CHILD_PROCESS_INIT;\n>  \n> -\tif (get_schedule_cmd(&cmd, &is_available))\n> -\t\treturn is_available;\n> -\n>  \tstrvec_split(&child.args, cmd);\n>  \tstrvec_push(&child.args, \"-l\");\n>  \tchild.no_stdin = 1;\n> @@ -2022,6 +2018,25 @@ static int is_crontab_available(void)\n>  \treturn 1;\n>  }\n>  \n> +static int is_crontab_available(void)\n> +{\n> +\tconst char *cmd = \"crontab\";\n> +\tint is_available;\n> +\n> +\tif (get_schedule_cmd(&cmd, &is_available))\n> +\t\treturn is_available;\n> +\n> +#ifdef __APPLE__\n> +\t/*\n> +\t * macOS has cron, but it requires special permissions and will\n> +\t * create a UI alert when attempting to run this command.\n> +\t */\n> +\treturn 0;\n> +#else\n> +\treturn check_crontab_process(cmd);\n> +#endif\n> +}\n> +\n>  #define BEGIN_LINE \"# BEGIN GIT MAINTENANCE SCHEDULE\"\n>  #define END_LINE \"# END GIT MAINTENANCE SCHEDULE\"\n>  \n>\n> base-commit: 6c220937e2b26d85920bf2d38ff2464a0d57fd6b\n\nI haven't tested, but isn't a smaller fix for this to just re-arrange\nthe array where we declare the methods to check to have \"cron\" come\nafter all the OS-specific ones, or at least after launchctl?\n\nI.e. we already have an ifdef to pick launchctl and never cron for OSX\non \"start\", so this is only for the case where we loop through the array\nlooking for something to select.\n\nThat wouldn't work if that user can run cron, but can't use launchctl at\nall, but in that case won't they be happy to get the prompt?\n\n"},{"id":"440874","messageId":"4996fb32-8262-b2af-b78a-c337041d897a@gmail.com","threadId":"56871","inReplyTo":"211110.86y25vpw1y.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] maintenance: disable cron on macOS","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-11-10T21:06:00Z","receivedAt":"2021-11-10T21:06:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/10/2021 3:56 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Nov 10 2021, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> In eba1ba9 (maintenance: `git maintenance run` learned\n>> `--scheduler=<scheduler>`, 2021-09-04), we introduced the ability to\n>> specify a scheduler explicitly. This led to some extra checks around\n>> whether an alternative scheduler was available. This added the\n>> functionality of removing background maintenance from schedulers other\n>> than the one selected.\n\nNote this last sentence.\n\n> I haven't tested, but isn't a smaller fix for this to just re-arrange\n> the array where we declare the methods to check to have \"cron\" come\n> after all the OS-specific ones, or at least after launchctl?\n> \n> I.e. we already have an ifdef to pick launchctl and never cron for OSX\n> on \"start\", so this is only for the case where we loop through the array\n> looking for something to select.\n> \n> That wouldn't work if that user can run cron, but can't use launchctl at\n> all, but in that case won't they be happy to get the prompt?\n \nYour suggestion doesn't work because this isn't about picking cron\nover launchctl, it's about disabling cron (and systemd or whatever is\navailable) when launchctl was selected.\n\nThanks,\n-Stolee\n"}]}