| Message ID | 20210612165043.165579-3-lenaic@lhuard.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintenance: add support for systemd timers on Linux | expand |
On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard <lenaic@lhuard.fr> wrote: > Depending on the system, different schedulers can be used to schedule > the hourly, daily and weekly executions of `git maintenance run`: > * `launchctl` for MacOS, > * `schtasks` for Windows and > * `crontab` for everything else. > > `git maintenance run` now has an option to let the end-user explicitly > choose which scheduler he wants to use: > `--scheduler=auto|crontab|launchctl|schtasks`. > > When `git maintenance start --scheduler=XXX` is run, it not only > registers `git maintenance run` tasks in the scheduler XXX, it also > removes the `git maintenance run` tasks from all the other schedulers to > ensure we cannot have two schedulers launching concurrent identical > tasks. > > The default value is `auto` which chooses a suitable scheduler for the > system. > > `git maintenance stop` doesn't have any `--scheduler` parameter because > this command will try to remove the `git maintenance run` tasks from all > the available schedulers. > > Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> Thanks. Unfortunately, I haven't been following this series too closely since I reviewed v1, so I set aside time to review v6, which I have now done. The material in the cover letter and individual commit messages was helpful in understanding the nuances of the changes, and the series seems pretty well complete at this point. (If, however, you do happen to re-roll for some reason, please consider using the --range-diff option of git-format-patch as an aid to reviewers.) I did leave a number of comments below regarding possible improvements to the code and documentation, however, they're probably mostly subjective and don't necessarily warrant a re-roll; I'd have no problem seeing this accepted as-is without the suggestions applied. (They can always be applied later on if someone considers them important enough.) I do, though, have one question (below) about is_crontab_available() for which I could not figure out the answer. > --- > diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt > @@ -179,6 +179,17 @@ OPTIONS > +--scheduler=auto|crontab|launchctl|schtasks:: > + When combined with the `start` subcommand, specify the scheduler > + to use to run the hourly, daily and weekly executions of > + `git maintenance run`. > + The possible values for `<scheduler>` depend on the system: `crontab` > + is available on POSIX systems, `launchctl` is available on > + MacOS and `schtasks` is available on Windows. > + By default or when `auto` is specified, a suitable scheduler for > + the system is used. On MacOS, `launchctl` is used. On Windows, > + `schtasks` is used. On all other systems, `crontab` is used. The above description is somewhat redundant. Another way to write it without the redundancy might be: Specify the scheduler -- in combination with subcommand `start` -- for running the hourly, daily and weekly invocations of `git maintenance run`. Possible values for `<scheduler>` are `auto`, `crontab` (POSIX), `launchctl` (macOS), and `schtasks` (Windows). When `auto` is specified, the appropriate platform-specific scheduler is used. Default is `auto`. > diff --git a/builtin/gc.c b/builtin/gc.c > @@ -1529,6 +1529,59 @@ static const char *get_frequency(enum schedule_priority schedule) > +static int get_schedule_cmd(const char **cmd, int *is_available) > +{ > + char *item; > + char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER")); > + > + if (!testing) > + return 0; > + > + if (is_available) > + *is_available = 0; > + > + for (item = testing;;) { > + char *sep; > + char *end_item = strchr(item, ','); > + if (end_item) > + *end_item = '\0'; > + > + sep = strchr(item, ':'); > + if (!sep) > + die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing); > + *sep = '\0'; > + > + if (!strcmp(*cmd, item)) { > + *cmd = sep + 1; > + if (is_available) > + *is_available = 1; > + UNLEAK(testing); > + return 1; > + } > + > + if (!end_item) > + break; > + item = end_item + 1; > + } > + > + free(testing); > + return 1; > +} I ended up studying this implementation several times since I had to come back to it repeatedly after reading calling code in order to (I hope) fully understand all the different conditions represented by its three distinct return values (the function return value, and the values returned in **cmd and **is_available). That it required several readings might warrant a comment block explaining what the function does and what the various return conditions mean. As a bonus, an explanation of the value of GIT_TEST_MAINT_SCHEDULER -- a comma-separated list of colon-delimited tuples, and what those tuples represent -- could be helpful. > +static int is_launchctl_available(void) > +{ > + const char *cmd = "launchctl"; > + int is_available; > + if (get_schedule_cmd(&cmd, &is_available)) > + return is_available; > + > +#ifdef __APPLE__ > + return 1; > +#else > + return 0; > +#endif > +} On this project, we usually frown upon #if conditionals within functions since the code often can become unreadable. The usage in this function doesn't suffer from that problem, however, resolve_auto_scheduler() is somewhat ugly. An alternative would be to set up these values outside of all functions, perhaps like this: #ifdef __APPLE__ #define MAINT_SCHEDULER SCHEDULER_LAUNCHCTL #elif GIT_WINDOWS_NATIVE #define MAINT_SCHEDULER SCHEDULER_SCHTASKS #else #define MAINT_SCHEDULER SCHEDULER_CRON #endif and then: static int is_launchctl_available(void) { if (get_schedule_cmd(...)) return is_available; return MAINT_SCHEDULER == SCHEDULER_LAUNCHCTL; } static void resolve_auto_scheduler(enum scheduler *scheduler) { if (*scheduler == SCHEDULER_AUTO) *scheduler = MAINT_SCHEDULER; } > +static int is_crontab_available(void) > +{ > + const char *cmd = "crontab"; > + int is_available; > + struct child_process child = CHILD_PROCESS_INIT; > + > + if (get_schedule_cmd(&cmd, &is_available) && !is_available) > + return 0; > + > + strvec_split(&child.args, cmd); > + strvec_push(&child.args, "-l"); > + child.no_stdin = 1; > + child.no_stdout = 1; > + child.no_stderr = 1; > + child.silent_exec_failure = 1; > + > + if (start_command(&child)) > + return 0; > + /* Ignore exit code, as an empty crontab will return error. */ > + finish_command(&child); > + return 1; > } If I understand get_schedule_cmd() correctly, it will always return true if GIT_TEST_MAINT_SCHEDULER is present in the environment, however, it will only set `is_available` to true if GIT_TEST_MAINT_SCHEDULER contains a matching entry for `cmd` (which in this case is "crontab"). Assuming this understanding is correct, then I'm having trouble understanding why this: if (get_schedule_cmd(&cmd, &is_available) && !is_available) return 0; isn't instead written like this: if (get_schedule_cmd(&cmd, &is_available)) return is_available; That is, why doesn't is_crontab_available() trust the result of get_schedule_cmd(), instead going ahead and trying to invoke `crontab` itself? Am I missing something which makes the `!is_available` case special? > +static void resolve_auto_scheduler(enum scheduler *scheduler) > +{ > + if (*scheduler != SCHEDULER_AUTO) > + return; > + > #if defined(__APPLE__) > + *scheduler = SCHEDULER_LAUNCHCTL; > + return; > + > #elif defined(GIT_WINDOWS_NATIVE) > + *scheduler = SCHEDULER_SCHTASKS; > + return; > + > #else > + *scheduler = SCHEDULER_CRON; > + return; > #endif > +} (See above for a way to simplify this implementation.) Is there a strong reason which I'm missing that this function alters its argument rather than simply returning the resolved scheduler? static enum scheduler resolve_scheduler(enum scheduler x) {...} Or is it just personal preference? (Minor: I took the liberty of shortening the function name since it doesn't feel like the longer name adds much value.) > +static int maintenance_start(int argc, const char **argv, const char *prefix) > { > + struct maintenance_start_opts opts = { 0 }; > + struct option options[] = { > + OPT_CALLBACK_F( > + 0, "scheduler", &opts.scheduler, N_("scheduler"), > + N_("scheduler to use to trigger git maintenance run"), Dropping "to use" would make this more concise without losing clarity: "scheduler to trigger git maintenance run" > + PARSE_OPT_NONEG, maintenance_opt_scheduler), > + OPT_END() > + }; > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > @@ -494,8 +494,21 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' ' > +test_expect_success 'start --scheduler=<scheduler>' ' > + test_expect_code 129 git maintenance start --scheduler=foo 2>err && > + test_i18ngrep "unrecognized --scheduler argument" err && > + > + test_expect_code 129 git maintenance start --no-scheduler 2>err && > + test_i18ngrep "unknown option" err && > + > + test_expect_code 128 \ > + env GIT_TEST_MAINT_SCHEDULER="launchctl:true,schtasks:true" \ > + git maintenance start --scheduler=crontab 2>err && > + test_i18ngrep "fatal: crontab scheduler is not available" err > +' Why does this test care about the exact exit codes rather than simply using test_must_fail() as is typically done elsewhere in the test suite, especially since we're also checking the error message itself? Am I missing some non-obvious property of the error codes? I don't see `auto` being tested anywhere. Do we want such a test? (It seems like it should be doable, though perhaps the complexity is too high -- I haven't thought it through fully.)
On 6/14/2021 12:36 AM, Eric Sunshine wrote: > On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard <lenaic@lhuard.fr> wrote: ... >> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt >> @@ -179,6 +179,17 @@ OPTIONS >> +--scheduler=auto|crontab|launchctl|schtasks:: >> + When combined with the `start` subcommand, specify the scheduler >> + to use to run the hourly, daily and weekly executions of >> + `git maintenance run`. >> + The possible values for `<scheduler>` depend on the system: `crontab` >> + is available on POSIX systems, `launchctl` is available on >> + MacOS and `schtasks` is available on Windows. >> + By default or when `auto` is specified, a suitable scheduler for >> + the system is used. On MacOS, `launchctl` is used. On Windows, >> + `schtasks` is used. On all other systems, `crontab` is used. > > The above description is somewhat redundant. Another way to write it > without the redundancy might be: > > Specify the scheduler -- in combination with subcommand `start` -- I think this change to the start is not contributing to the drop in redundancy, but _does_ break from the pattern of previous options. These start with "When combined with the `X` subcommand" to clarify when they can be used. I'm not against improving that pattern. I'm just saying that we should do it across the whole file if we do it at all. > for running the hourly, daily and weekly invocations of `git > maintenance run`. Possible values for `<scheduler>` are `auto`, > `crontab` (POSIX), `launchctl` (macOS), and `schtasks` (Windows). > When `auto` is specified, the appropriate platform-specific > scheduler is used. Default is `auto`. I find this to be a good way to reorganize the paragraph to be very clear. How do you propose adding `systemd-timer` in the next patch? Is it simply adding "`systemd-timer (Linux)`" or do we need to be more careful using "(when available)"? Unlike the others, the availability of that option is not as cut-and-dry. ... > I don't see `auto` being tested anywhere. Do we want such a test? (It > seems like it should be doable, though perhaps the complexity is too > high -- I haven't thought it through fully.) Unfortunately, `auto` seems to live in a world where we need to actually run commands such as crontab and systemctl to determine if they are available, but that happens only when GIT_TEST_MAINT_SCHEDULER is unset. But, we don't want to actually edit the timing information for the test runner, so some other abstraction needs to be inserted at the proper layer. It's worth a shot, but I expect it to be challenging to get right. Thanks, -Stolee
On Wed, Jun 16, 2021 at 8:48 PM Derrick Stolee <stolee@gmail.com> wrote: > On 6/14/2021 12:36 AM, Eric Sunshine wrote: > > On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard <lenaic@lhuard.fr> wrote: > > The above description is somewhat redundant. Another way to write it > > without the redundancy might be: > > > > Specify the scheduler -- in combination with subcommand `start` -- > > I think this change to the start is not contributing to the drop > in redundancy, but _does_ break from the pattern of previous > options. These start with "When combined with the `X` subcommand" > to clarify when they can be used. > > I'm not against improving that pattern. I'm just saying that we > should do it across the whole file if we do it at all. Indeed, it's not a big deal. As mentioned in my review, all my comments were subjective, and none of them should prevent this from being accepted as-is if everyone is happy with it. > > for running the hourly, daily and weekly invocations of `git > > maintenance run`. Possible values for `<scheduler>` are `auto`, > > `crontab` (POSIX), `launchctl` (macOS), and `schtasks` (Windows). > > When `auto` is specified, the appropriate platform-specific > > scheduler is used. Default is `auto`. > > I find this to be a good way to reorganize the paragraph to be > very clear. How do you propose adding `systemd-timer` in the next > patch? Is it simply adding "`systemd-timer (Linux)`" or do we > need to be more careful using "(when available)"? Unlike the others, > the availability of that option is not as cut-and-dry. It should be easy enough. For instance: Possible values for `<scheduler>` are `auto`, `crontab` (POSIX), `systemd-timer` (Linux), `launchctl` (macOS), and `schtasks` (Windows). When `auto` is specified, the appropriate platform- specific scheduler is used; on Linux, `systemd-timer` is used if available, otherwise `crontab`. Default is `auto`. But this sort of minor rewrite can always be done later. > > I don't see `auto` being tested anywhere. Do we want such a test? (It > > seems like it should be doable, though perhaps the complexity is too > > high -- I haven't thought it through fully.) > > Unfortunately, `auto` seems to live in a world where we need to > actually run commands such as crontab and systemctl to determine > if they are available, but that happens only when > GIT_TEST_MAINT_SCHEDULER is unset. But, we don't want to actually > edit the timing information for the test runner, so some other > abstraction needs to be inserted at the proper layer. > > It's worth a shot, but I expect it to be challenging to get right. I imagine that it can be mocked up with GIT_TEST_MAINT_SCHEDULER in some fashion, but it's not worth holding up this series or expecting a re-roll for that one little question which popped out of my brain. As I mentioned in my review, my comments were subjective, and I think the series is in a state in which it can be accepted as-is without additional re-rolls[1] if everyone is happy with it. [1]: Except for the one question I asked about is_crontab_available(); I don't understand that one bit of logic, thus can't tell if the behavior makes sense or not.
On 14/06/2021 05:36, Eric Sunshine wrote: > On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard <lenaic@lhuard.fr> wrote: >> Depending on the system, different schedulers can be used to schedule >> the hourly, daily and weekly executions of `git maintenance run`: >> * `launchctl` for MacOS, >> * `schtasks` for Windows and >> * `crontab` for everything else. >> >> `git maintenance run` now has an option to let the end-user explicitly >> choose which scheduler he wants to use: >> `--scheduler=auto|crontab|launchctl|schtasks`. >> >> When `git maintenance start --scheduler=XXX` is run, it not only >> registers `git maintenance run` tasks in the scheduler XXX, it also >> removes the `git maintenance run` tasks from all the other schedulers to >> ensure we cannot have two schedulers launching concurrent identical >> tasks. >> >> The default value is `auto` which chooses a suitable scheduler for the >> system. >> >> `git maintenance stop` doesn't have any `--scheduler` parameter because >> this command will try to remove the `git maintenance run` tasks from all >> the available schedulers. >> >> Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> > > Thanks. Unfortunately, I haven't been following this series too > closely since I reviewed v1, so I set aside time to review v6, which I > have now done. The material in the cover letter and individual commit > messages was helpful in understanding the nuances of the changes, and > the series seems pretty well complete at this point. (If, however, you > do happen to re-roll for some reason, please consider using the > --range-diff option of git-format-patch as an aid to reviewers.) > > I did leave a number of comments below regarding possible improvements > to the code and documentation, however, they're probably mostly > subjective and don't necessarily warrant a re-roll; I'd have no > problem seeing this accepted as-is without the suggestions applied. > (They can always be applied later on if someone considers them > important enough.) > > I do, though, have one question (below) about is_crontab_available() > for which I could not figure out the answer. I think that is a bug >> [...] >> diff --git a/builtin/gc.c b/builtin/gc.c >> @@ -1529,6 +1529,59 @@ static const char *get_frequency(enum schedule_priority schedule) >> +static int get_schedule_cmd(const char **cmd, int *is_available) >> +{ >> + char *item; >> + char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER")); >> + >> + if (!testing) >> + return 0; >> + >> + if (is_available) >> + *is_available = 0; >> + >> + for (item = testing;;) { >> + char *sep; >> + char *end_item = strchr(item, ','); >> + if (end_item) >> + *end_item = '\0'; >> + >> + sep = strchr(item, ':'); >> + if (!sep) >> + die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing); >> + *sep = '\0'; >> + >> + if (!strcmp(*cmd, item)) { >> + *cmd = sep + 1; >> + if (is_available) >> + *is_available = 1; >> + UNLEAK(testing); >> + return 1; >> + } >> + >> + if (!end_item) >> + break; >> + item = end_item + 1; >> + } >> + >> + free(testing); >> + return 1; >> +} > > I ended up studying this implementation several times since I had to > come back to it repeatedly after reading calling code in order to (I > hope) fully understand all the different conditions represented by its > three distinct return values (the function return value, and the > values returned in **cmd and **is_available). That it required several > readings might warrant a comment block explaining what the function > does and what the various return conditions mean. As a bonus, an > explanation of the value of GIT_TEST_MAINT_SCHEDULER -- a > comma-separated list of colon-delimited tuples, and what those tuples > represent -- could be helpful. I agree documenting GIT_TEST_MAINT_SCHEDULER would be useful >> +static int is_launchctl_available(void) >> +{ >> + const char *cmd = "launchctl"; >> + int is_available; >> + if (get_schedule_cmd(&cmd, &is_available)) >> + return is_available; >> + >> +#ifdef __APPLE__ >> + return 1; >> +#else >> + return 0; >> +#endif >> +} > > On this project, we usually frown upon #if conditionals within > functions since the code often can become unreadable. The usage in > this function doesn't suffer from that problem, however, > resolve_auto_scheduler() is somewhat ugly. An alternative would be to > set up these values outside of all functions, perhaps like this: > > #ifdef __APPLE__ > #define MAINT_SCHEDULER SCHEDULER_LAUNCHCTL > #elif GIT_WINDOWS_NATIVE > #define MAINT_SCHEDULER SCHEDULER_SCHTASKS > #else > #define MAINT_SCHEDULER SCHEDULER_CRON > #endif > > and then: > > static int is_launchctl_available(void) > { > if (get_schedule_cmd(...)) > return is_available; > return MAINT_SCHEDULER == SCHEDULER_LAUNCHCTL; > } > > static void resolve_auto_scheduler(enum scheduler *scheduler) > { > if (*scheduler == SCHEDULER_AUTO) > *scheduler = MAINT_SCHEDULER; > } > >> +static int is_crontab_available(void) >> +{ >> + const char *cmd = "crontab"; >> + int is_available; >> + struct child_process child = CHILD_PROCESS_INIT; >> + >> + if (get_schedule_cmd(&cmd, &is_available) && !is_available) >> + return 0; >> + >> + strvec_split(&child.args, cmd); >> + strvec_push(&child.args, "-l"); >> + child.no_stdin = 1; >> + child.no_stdout = 1; >> + child.no_stderr = 1; >> + child.silent_exec_failure = 1; >> + >> + if (start_command(&child)) >> + return 0; >> + /* Ignore exit code, as an empty crontab will return error. */ >> + finish_command(&child); >> + return 1; >> } > > If I understand get_schedule_cmd() correctly, it will always return > true if GIT_TEST_MAINT_SCHEDULER is present in the environment, > however, it will only set `is_available` to true if > GIT_TEST_MAINT_SCHEDULER contains a matching entry for `cmd` (which in > this case is "crontab"). Assuming this understanding is correct, then > I'm having trouble understanding why this: > > if (get_schedule_cmd(&cmd, &is_available) && !is_available) > return 0; > > isn't instead written like this: > > if (get_schedule_cmd(&cmd, &is_available)) > return is_available; > > That is, why doesn't is_crontab_available() trust the result of > get_schedule_cmd(), instead going ahead and trying to invoke `crontab` > itself? Am I missing something which makes the `!is_available` case > special? I agree, I think we should be returning is_available irrespective of its value if get_schedule_cmd() returns true. This is what is_systemd_timer_available() does in the next patch. Well spotted. Best Wishes Phillip >> +static void resolve_auto_scheduler(enum scheduler *scheduler) >> +{ >> + if (*scheduler != SCHEDULER_AUTO) >> + return; >> + >> #if defined(__APPLE__) >> + *scheduler = SCHEDULER_LAUNCHCTL; >> + return; >> + >> #elif defined(GIT_WINDOWS_NATIVE) >> + *scheduler = SCHEDULER_SCHTASKS; >> + return; >> + >> #else >> + *scheduler = SCHEDULER_CRON; >> + return; >> #endif >> +} > > (See above for a way to simplify this implementation.) > > Is there a strong reason which I'm missing that this function alters > its argument rather than simply returning the resolved scheduler? > > static enum scheduler resolve_scheduler(enum scheduler x) {...} > > Or is it just personal preference? > > (Minor: I took the liberty of shortening the function name since it > doesn't feel like the longer name adds much value.) > >> +static int maintenance_start(int argc, const char **argv, const char *prefix) >> { >> + struct maintenance_start_opts opts = { 0 }; >> + struct option options[] = { >> + OPT_CALLBACK_F( >> + 0, "scheduler", &opts.scheduler, N_("scheduler"), >> + N_("scheduler to use to trigger git maintenance run"), > > Dropping "to use" would make this more concise without losing clarity: > > "scheduler to trigger git maintenance run" > >> + PARSE_OPT_NONEG, maintenance_opt_scheduler), >> + OPT_END() >> + }; >> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh >> @@ -494,8 +494,21 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' ' >> +test_expect_success 'start --scheduler=<scheduler>' ' >> + test_expect_code 129 git maintenance start --scheduler=foo 2>err && >> + test_i18ngrep "unrecognized --scheduler argument" err && >> + >> + test_expect_code 129 git maintenance start --no-scheduler 2>err && >> + test_i18ngrep "unknown option" err && >> + >> + test_expect_code 128 \ >> + env GIT_TEST_MAINT_SCHEDULER="launchctl:true,schtasks:true" \ >> + git maintenance start --scheduler=crontab 2>err && >> + test_i18ngrep "fatal: crontab scheduler is not available" err >> +' > > Why does this test care about the exact exit codes rather than simply > using test_must_fail() as is typically done elsewhere in the test > suite, especially since we're also checking the error message itself? > Am I missing some non-obvious property of the error codes? > > I don't see `auto` being tested anywhere. Do we want such a test? (It > seems like it should be doable, though perhaps the complexity is too > high -- I haven't thought it through fully.) >
Le lundi 14 juin 2021, 06:36:09 CEST Eric Sunshine a écrit : > Thanks. Unfortunately, I haven't been following this series too > closely since I reviewed v1, so I set aside time to review v6, which I > have now done. > … > I do, though, have one question (below) about is_crontab_available() > for which I could not figure out the answer. Thank you very much for your review. I’ve just submitted a new re-roll that should address the points you raised and in particular the `is_crontab_available` unexpected behavior. … > > +static int is_launchctl_available(void) > > +{ > > + const char *cmd = "launchctl"; > > + int is_available; > > + if (get_schedule_cmd(&cmd, &is_available)) > > + return is_available; > > + > > +#ifdef __APPLE__ > > + return 1; > > +#else > > + return 0; > > +#endif > > +} > > On this project, we usually frown upon #if conditionals within > functions since the code often can become unreadable. The usage in > this function doesn't suffer from that problem, however, > resolve_auto_scheduler() is somewhat ugly. An alternative would be to > set up these values outside of all functions, perhaps like this: > > #ifdef __APPLE__ > #define MAINT_SCHEDULER SCHEDULER_LAUNCHCTL > #elif GIT_WINDOWS_NATIVE > #define MAINT_SCHEDULER SCHEDULER_SCHTASKS > #else > #define MAINT_SCHEDULER SCHEDULER_CRON > #endif > > and then: > > static int is_launchctl_available(void) > { > if (get_schedule_cmd(...)) > return is_available; > return MAINT_SCHEDULER == SCHEDULER_LAUNCHCTL; > } > > static void resolve_auto_scheduler(enum scheduler *scheduler) > { > if (*scheduler == SCHEDULER_AUTO) > *scheduler = MAINT_SCHEDULER; > } > This approach would unfortunately work only for the second patch of this series where a single scheduler is available on each platform. With the third patch of this series, `resolve_auto_scheduler` doesn’t return a value that is fully determined at compilation time anymore. On Linux, both `crontab` and `systemd-timers` are susceptible to be available and this is checked at runtime. So, with the third patch of this series, it wouldn’t be possible anymore to define a single value for `MAINT_SCHEDULER` and to base `resolve_auto_scheduler` on it. … > > + PARSE_OPT_NONEG, maintenance_opt_scheduler), > > + OPT_END() > > + }; > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > > @@ -494,8 +494,21 @@ test_expect_success !MINGW 'register and unregister > > with regex metacharacters' ' +test_expect_success 'start > > --scheduler=<scheduler>' ' > > + test_expect_code 129 git maintenance start --scheduler=foo 2>err > > && > > + test_i18ngrep "unrecognized --scheduler argument" err && > > + > > + test_expect_code 129 git maintenance start --no-scheduler 2>err && > > + test_i18ngrep "unknown option" err && > > + > > + test_expect_code 128 \ > > + env > > GIT_TEST_MAINT_SCHEDULER="launchctl:true,schtasks:true" \ + > > git maintenance start --scheduler=crontab 2>err && + test_i18ngrep > > "fatal: crontab scheduler is not available" err +' > > Why does this test care about the exact exit codes rather than simply > using test_must_fail() as is typically done elsewhere in the test > suite, especially since we're also checking the error message itself? > Am I missing some non-obvious property of the error codes? I have no strong opinion on this. I only mimicked the `help text` test that is at the top of the `t7900- maintenance.sh` file as it was also testing invalid commands and checking the resulting error message. > I don't see `auto` being tested anywhere. Do we want such a test? (It > seems like it should be doable, though perhaps the complexity is too > high -- I haven't thought it through fully.) My main problem with the `auto` is that a big part of its logic is determined at compilation time with `#if` statements based on the platform. And it seems that the tests are designed so far to test the same things on all platforms. A solution could be to completely get rid of the platform `#if` statements and to turn all the detection logic as runtime tests. But it would mean that, for ex., the git binary for Linux would systematically check if the MacOS and Windows specific schedulers are available. Cheers, Lénaïc.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 1e738ad398..07065ed4f3 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -179,6 +179,17 @@ OPTIONS `maintenance.<task>.enabled` configured as `true` are considered. See the 'TASKS' section for the list of accepted `<task>` values. +--scheduler=auto|crontab|launchctl|schtasks:: + When combined with the `start` subcommand, specify the scheduler + to use to run the hourly, daily and weekly executions of + `git maintenance run`. + The possible values for `<scheduler>` depend on the system: `crontab` + is available on POSIX systems, `launchctl` is available on + MacOS and `schtasks` is available on Windows. + By default or when `auto` is specified, a suitable scheduler for + the system is used. On MacOS, `launchctl` is used. On Windows, + `schtasks` is used. On all other systems, `crontab` is used. + TROUBLESHOOTING --------------- diff --git a/builtin/gc.c b/builtin/gc.c index f05d2f0a1a..4a5ed7cb6f 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1529,6 +1529,59 @@ static const char *get_frequency(enum schedule_priority schedule) } } +static int get_schedule_cmd(const char **cmd, int *is_available) +{ + char *item; + char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER")); + + if (!testing) + return 0; + + if (is_available) + *is_available = 0; + + for (item = testing;;) { + char *sep; + char *end_item = strchr(item, ','); + if (end_item) + *end_item = '\0'; + + sep = strchr(item, ':'); + if (!sep) + die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing); + *sep = '\0'; + + if (!strcmp(*cmd, item)) { + *cmd = sep + 1; + if (is_available) + *is_available = 1; + UNLEAK(testing); + return 1; + } + + if (!end_item) + break; + item = end_item + 1; + } + + free(testing); + return 1; +} + +static int is_launchctl_available(void) +{ + const char *cmd = "launchctl"; + int is_available; + if (get_schedule_cmd(&cmd, &is_available)) + return is_available; + +#ifdef __APPLE__ + return 1; +#else + return 0; +#endif +} + static char *launchctl_service_name(const char *frequency) { struct strbuf label = STRBUF_INIT; @@ -1555,19 +1608,17 @@ static char *launchctl_get_uid(void) return xstrfmt("gui/%d", getuid()); } -static int launchctl_boot_plist(int enable, const char *filename, const char *cmd) +static int launchctl_boot_plist(int enable, const char *filename) { + const char *cmd = "launchctl"; int result; struct child_process child = CHILD_PROCESS_INIT; char *uid = launchctl_get_uid(); + get_schedule_cmd(&cmd, NULL); strvec_split(&child.args, cmd); - if (enable) - strvec_push(&child.args, "bootstrap"); - else - strvec_push(&child.args, "bootout"); - strvec_push(&child.args, uid); - strvec_push(&child.args, filename); + strvec_pushl(&child.args, enable ? "bootstrap" : "bootout", uid, + filename, NULL); child.no_stderr = 1; child.no_stdout = 1; @@ -1581,26 +1632,26 @@ static int launchctl_boot_plist(int enable, const char *filename, const char *cm return result; } -static int launchctl_remove_plist(enum schedule_priority schedule, const char *cmd) +static int launchctl_remove_plist(enum schedule_priority schedule) { const char *frequency = get_frequency(schedule); char *name = launchctl_service_name(frequency); char *filename = launchctl_service_filename(name); - int result = launchctl_boot_plist(0, filename, cmd); + int result = launchctl_boot_plist(0, filename); unlink(filename); free(filename); free(name); return result; } -static int launchctl_remove_plists(const char *cmd) +static int launchctl_remove_plists(void) { - return launchctl_remove_plist(SCHEDULE_HOURLY, cmd) || - launchctl_remove_plist(SCHEDULE_DAILY, cmd) || - launchctl_remove_plist(SCHEDULE_WEEKLY, cmd); + return launchctl_remove_plist(SCHEDULE_HOURLY) || + launchctl_remove_plist(SCHEDULE_DAILY) || + launchctl_remove_plist(SCHEDULE_WEEKLY); } -static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd) +static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule) { FILE *plist; int i; @@ -1669,8 +1720,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit fclose(plist); /* bootout might fail if not already running, so ignore */ - launchctl_boot_plist(0, filename, cmd); - if (launchctl_boot_plist(1, filename, cmd)) + launchctl_boot_plist(0, filename); + if (launchctl_boot_plist(1, filename)) die(_("failed to bootstrap service %s"), filename); free(filename); @@ -1678,21 +1729,35 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit return 0; } -static int launchctl_add_plists(const char *cmd) +static int launchctl_add_plists(void) { const char *exec_path = git_exec_path(); - return launchctl_schedule_plist(exec_path, SCHEDULE_HOURLY, cmd) || - launchctl_schedule_plist(exec_path, SCHEDULE_DAILY, cmd) || - launchctl_schedule_plist(exec_path, SCHEDULE_WEEKLY, cmd); + return launchctl_schedule_plist(exec_path, SCHEDULE_HOURLY) || + launchctl_schedule_plist(exec_path, SCHEDULE_DAILY) || + launchctl_schedule_plist(exec_path, SCHEDULE_WEEKLY); } -static int launchctl_update_schedule(int run_maintenance, int fd, const char *cmd) +static int launchctl_update_schedule(int run_maintenance, int fd) { if (run_maintenance) - return launchctl_add_plists(cmd); + return launchctl_add_plists(); else - return launchctl_remove_plists(cmd); + return launchctl_remove_plists(); +} + +static int is_schtasks_available(void) +{ + const char *cmd = "schtasks"; + int is_available; + if (get_schedule_cmd(&cmd, &is_available)) + return is_available; + +#ifdef GIT_WINDOWS_NATIVE + return 1; +#else + return 0; +#endif } static char *schtasks_task_name(const char *frequency) @@ -1702,13 +1767,15 @@ static char *schtasks_task_name(const char *frequency) return strbuf_detach(&label, NULL); } -static int schtasks_remove_task(enum schedule_priority schedule, const char *cmd) +static int schtasks_remove_task(enum schedule_priority schedule) { + const char *cmd = "schtasks"; int result; struct strvec args = STRVEC_INIT; const char *frequency = get_frequency(schedule); char *name = schtasks_task_name(frequency); + get_schedule_cmd(&cmd, NULL); strvec_split(&args, cmd); strvec_pushl(&args, "/delete", "/tn", name, "/f", NULL); @@ -1719,15 +1786,16 @@ static int schtasks_remove_task(enum schedule_priority schedule, const char *cmd return result; } -static int schtasks_remove_tasks(const char *cmd) +static int schtasks_remove_tasks(void) { - return schtasks_remove_task(SCHEDULE_HOURLY, cmd) || - schtasks_remove_task(SCHEDULE_DAILY, cmd) || - schtasks_remove_task(SCHEDULE_WEEKLY, cmd); + return schtasks_remove_task(SCHEDULE_HOURLY) || + schtasks_remove_task(SCHEDULE_DAILY) || + schtasks_remove_task(SCHEDULE_WEEKLY); } -static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule, const char *cmd) +static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule) { + const char *cmd = "schtasks"; int result; struct child_process child = CHILD_PROCESS_INIT; const char *xml; @@ -1736,6 +1804,8 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority char *name = schtasks_task_name(frequency); struct strbuf tfilename = STRBUF_INIT; + get_schedule_cmd(&cmd, NULL); + strbuf_addf(&tfilename, "%s/schedule_%s_XXXXXX", get_git_common_dir(), frequency); tfile = xmks_tempfile(tfilename.buf); @@ -1840,28 +1910,52 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority return result; } -static int schtasks_schedule_tasks(const char *cmd) +static int schtasks_schedule_tasks(void) { const char *exec_path = git_exec_path(); - return schtasks_schedule_task(exec_path, SCHEDULE_HOURLY, cmd) || - schtasks_schedule_task(exec_path, SCHEDULE_DAILY, cmd) || - schtasks_schedule_task(exec_path, SCHEDULE_WEEKLY, cmd); + return schtasks_schedule_task(exec_path, SCHEDULE_HOURLY) || + schtasks_schedule_task(exec_path, SCHEDULE_DAILY) || + schtasks_schedule_task(exec_path, SCHEDULE_WEEKLY); } -static int schtasks_update_schedule(int run_maintenance, int fd, const char *cmd) +static int schtasks_update_schedule(int run_maintenance, int fd) { if (run_maintenance) - return schtasks_schedule_tasks(cmd); + return schtasks_schedule_tasks(); else - return schtasks_remove_tasks(cmd); + return schtasks_remove_tasks(); +} + +static int is_crontab_available(void) +{ + const char *cmd = "crontab"; + int is_available; + struct child_process child = CHILD_PROCESS_INIT; + + if (get_schedule_cmd(&cmd, &is_available) && !is_available) + return 0; + + strvec_split(&child.args, cmd); + strvec_push(&child.args, "-l"); + child.no_stdin = 1; + child.no_stdout = 1; + child.no_stderr = 1; + child.silent_exec_failure = 1; + + if (start_command(&child)) + return 0; + /* Ignore exit code, as an empty crontab will return error. */ + finish_command(&child); + return 1; } #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE" #define END_LINE "# END GIT MAINTENANCE SCHEDULE" -static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd) +static int crontab_update_schedule(int run_maintenance, int fd) { + const char *cmd = "crontab"; int result = 0; int in_old_region = 0; struct child_process crontab_list = CHILD_PROCESS_INIT; @@ -1869,6 +1963,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd) FILE *cron_list, *cron_in; struct strbuf line = STRBUF_INIT; + get_schedule_cmd(&cmd, NULL); strvec_split(&crontab_list.args, cmd); strvec_push(&crontab_list.args, "-l"); crontab_list.in = -1; @@ -1945,66 +2040,163 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd) return result; } +enum scheduler { + SCHEDULER_INVALID = -1, + SCHEDULER_AUTO, + SCHEDULER_CRON, + SCHEDULER_LAUNCHCTL, + SCHEDULER_SCHTASKS, +}; + +static const struct { + const char *name; + int (*is_available)(void); + int (*update_schedule)(int run_maintenance, int fd); +} scheduler_fn[] = { + [SCHEDULER_CRON] = { + .name = "crontab", + .is_available = is_crontab_available, + .update_schedule = crontab_update_schedule, + }, + [SCHEDULER_LAUNCHCTL] = { + .name = "launchctl", + .is_available = is_launchctl_available, + .update_schedule = launchctl_update_schedule, + }, + [SCHEDULER_SCHTASKS] = { + .name = "schtasks", + .is_available = is_schtasks_available, + .update_schedule = schtasks_update_schedule, + }, +}; + +static enum scheduler parse_scheduler(const char *value) +{ + if (!value) + return SCHEDULER_INVALID; + else if (!strcasecmp(value, "auto")) + return SCHEDULER_AUTO; + else if (!strcasecmp(value, "cron") || !strcasecmp(value, "crontab")) + return SCHEDULER_CRON; + else if (!strcasecmp(value, "launchctl")) + return SCHEDULER_LAUNCHCTL; + else if (!strcasecmp(value, "schtasks")) + return SCHEDULER_SCHTASKS; + else + return SCHEDULER_INVALID; +} + +static int maintenance_opt_scheduler(const struct option *opt, const char *arg, + int unset) +{ + enum scheduler *scheduler = opt->value; + + BUG_ON_OPT_NEG(unset); + + *scheduler = parse_scheduler(arg); + if (*scheduler == SCHEDULER_INVALID) + return error(_("unrecognized --scheduler argument '%s'"), arg); + return 0; +} + +struct maintenance_start_opts { + enum scheduler scheduler; +}; + +static void resolve_auto_scheduler(enum scheduler *scheduler) +{ + if (*scheduler != SCHEDULER_AUTO) + return; + #if defined(__APPLE__) -static const char platform_scheduler[] = "launchctl"; + *scheduler = SCHEDULER_LAUNCHCTL; + return; + #elif defined(GIT_WINDOWS_NATIVE) -static const char platform_scheduler[] = "schtasks"; + *scheduler = SCHEDULER_SCHTASKS; + return; + #else -static const char platform_scheduler[] = "crontab"; + *scheduler = SCHEDULER_CRON; + return; #endif +} -static int update_background_schedule(int enable) +static void validate_scheduler(enum scheduler scheduler) { - int result; - const char *scheduler = platform_scheduler; - const char *cmd = scheduler; - char *testing; + if (scheduler == SCHEDULER_INVALID) + BUG("invalid scheduler"); + if (scheduler == SCHEDULER_AUTO) + BUG("resolve_auto_scheduler should have been called before"); + + if (!scheduler_fn[scheduler].is_available()) + die(_("%s scheduler is not available"), + scheduler_fn[scheduler].name); +} + +static int update_background_schedule(const struct maintenance_start_opts *opts, + int enable) +{ + unsigned int i; + int result = 0; struct lock_file lk; char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); - testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER")); - if (testing) { - char *sep = strchr(testing, ':'); - if (!sep) - die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing); - *sep = '\0'; - scheduler = testing; - cmd = sep + 1; + if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) { + free(lock_path); + return error(_("another process is scheduling background maintenance")); } - if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) { - result = error(_("another process is scheduling background maintenance")); - goto cleanup; + for (i = 1; i < ARRAY_SIZE(scheduler_fn); i++) { + if (enable && opts->scheduler == i) + continue; + if (!scheduler_fn[i].is_available()) + continue; + scheduler_fn[i].update_schedule(0, get_lock_file_fd(&lk)); } - if (!strcmp(scheduler, "launchctl")) - result = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd); - else if (!strcmp(scheduler, "schtasks")) - result = schtasks_update_schedule(enable, get_lock_file_fd(&lk), cmd); - else if (!strcmp(scheduler, "crontab")) - result = crontab_update_schedule(enable, get_lock_file_fd(&lk), cmd); - else - die("unknown background scheduler: %s", scheduler); + if (enable) + result = scheduler_fn[opts->scheduler].update_schedule( + 1, get_lock_file_fd(&lk)); rollback_lock_file(&lk); -cleanup: free(lock_path); - free(testing); return result; } -static int maintenance_start(void) +static const char *const builtin_maintenance_start_usage[] = { + N_("git maintenance start [--scheduler=<scheduler>]"), + NULL +}; + +static int maintenance_start(int argc, const char **argv, const char *prefix) { + struct maintenance_start_opts opts = { 0 }; + struct option options[] = { + OPT_CALLBACK_F( + 0, "scheduler", &opts.scheduler, N_("scheduler"), + N_("scheduler to use to trigger git maintenance run"), + PARSE_OPT_NONEG, maintenance_opt_scheduler), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + builtin_maintenance_start_usage, 0); + if (argc) + usage_with_options(builtin_maintenance_start_usage, options); + + resolve_auto_scheduler(&opts.scheduler); + validate_scheduler(opts.scheduler); + if (maintenance_register()) warning(_("failed to add repo to global config")); - - return update_background_schedule(1); + return update_background_schedule(&opts, 1); } static int maintenance_stop(void) { - return update_background_schedule(0); + return update_background_schedule(NULL, 0); } static const char builtin_maintenance_usage[] = N_("git maintenance <subcommand> [<options>]"); @@ -2018,7 +2210,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix) if (!strcmp(argv[1], "run")) return maintenance_run(argc - 1, argv + 1, prefix); if (!strcmp(argv[1], "start")) - return maintenance_start(); + return maintenance_start(argc - 1, argv + 1, prefix); if (!strcmp(argv[1], "stop")) return maintenance_stop(); if (!strcmp(argv[1], "register")) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index b93ae014ee..b36b7f5fb0 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -494,8 +494,21 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' ' maintenance.repo "$(pwd)/$META" ' +test_expect_success 'start --scheduler=<scheduler>' ' + test_expect_code 129 git maintenance start --scheduler=foo 2>err && + test_i18ngrep "unrecognized --scheduler argument" err && + + test_expect_code 129 git maintenance start --no-scheduler 2>err && + test_i18ngrep "unknown option" err && + + test_expect_code 128 \ + env GIT_TEST_MAINT_SCHEDULER="launchctl:true,schtasks:true" \ + git maintenance start --scheduler=crontab 2>err && + test_i18ngrep "fatal: crontab scheduler is not available" err +' + test_expect_success 'start from empty cron table' ' - GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start && + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start --scheduler=crontab && # start registers the repo git config --get --global --fixed-value maintenance.repo "$(pwd)" && @@ -518,7 +531,7 @@ test_expect_success 'stop from existing schedule' ' test_expect_success 'start preserves existing schedule' ' echo "Important information!" >cron.txt && - GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start && + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start --scheduler=crontab && grep "Important information!" cron.txt ' @@ -547,7 +560,7 @@ test_expect_success 'start and stop macOS maintenance' ' EOF rm -f args && - GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start && + GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start --scheduler=launchctl && # start registers the repo git config --get --global --fixed-value maintenance.repo "$(pwd)" && @@ -598,7 +611,7 @@ test_expect_success 'start and stop Windows maintenance' ' EOF rm -f args && - GIT_TEST_MAINT_SCHEDULER="schtasks:./print-args" git maintenance start && + GIT_TEST_MAINT_SCHEDULER="schtasks:./print-args" git maintenance start --scheduler=schtasks && # start registers the repo git config --get --global --fixed-value maintenance.repo "$(pwd)" && @@ -621,6 +634,40 @@ test_expect_success 'start and stop Windows maintenance' ' test_cmp expect args ' +test_expect_success 'start and stop when several schedulers are available' ' + write_script print-args <<-\EOF && + printf "%s\n" "$*" | sed "s:gui/[0-9][0-9]*:gui/[UID]:; s:\(schtasks /create .* /xml\).*:\1:;" >>args + EOF + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=launchctl && + printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \ + hourly daily weekly >expect && + for frequency in hourly daily weekly + do + PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && + echo "launchctl bootout gui/[UID] $PLIST" >>expect && + echo "launchctl bootstrap gui/[UID] $PLIST" >>expect || return 1 + done && + test_cmp expect args && + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=schtasks && + printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ + hourly daily weekly >expect && + printf "schtasks /create /tn Git Maintenance (%s) /f /xml\n" \ + hourly daily weekly >>expect && + test_cmp expect args && + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance stop && + printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ + hourly daily weekly >expect && + printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \ + hourly daily weekly >>expect && + test_cmp expect args +' + test_expect_success 'register preserves existing strategy' ' git config maintenance.strategy none && git maintenance register &&
Depending on the system, different schedulers can be used to schedule the hourly, daily and weekly executions of `git maintenance run`: * `launchctl` for MacOS, * `schtasks` for Windows and * `crontab` for everything else. `git maintenance run` now has an option to let the end-user explicitly choose which scheduler he wants to use: `--scheduler=auto|crontab|launchctl|schtasks`. When `git maintenance start --scheduler=XXX` is run, it not only registers `git maintenance run` tasks in the scheduler XXX, it also removes the `git maintenance run` tasks from all the other schedulers to ensure we cannot have two schedulers launching concurrent identical tasks. The default value is `auto` which chooses a suitable scheduler for the system. `git maintenance stop` doesn't have any `--scheduler` parameter because this command will try to remove the `git maintenance run` tasks from all the available schedulers. Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> --- Documentation/git-maintenance.txt | 11 + builtin/gc.c | 334 +++++++++++++++++++++++------- t/t7900-maintenance.sh | 55 ++++- 3 files changed, 325 insertions(+), 75 deletions(-)