From: Phillip Wood Date: Tue, 06 Jul 2021 13:18:44 GMT Subject: Re: [PATCH v7 0/3] maintenance: add support for systemd timers on Linux Message-ID: <387ad277-a75c-92ca-b7af-163d10d3b4a4@gmail.com> In-Reply-To: <20210702142556.99864-1-lenaic@lhuard.fr> Hi Lénaïc On 02/07/2021 15:25, Lénaïc Huard wrote: > Hello, > > Please find hereafter a new reroll of my patchset to add support for > systemd timers on Linux for the `git maintenance start` command. > > The changes compared to the previous version address the remarks > raised during the previous review. > ... > Diff-intervalle contre v6 : > -: ---------- > 1: 899b11ed5b cache.h: Introduce a generic "xdg_config_home_for(…)" function > 1: 604627f347 ! 2: f3e2f0256b maintenance: `git maintenance run` learned `--scheduler=` > @@ Documentation/git-maintenance.txt: 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 > ++ for running the hourly, daily and weekly executions of > + `git maintenance run`. > -+ The possible values for `` 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. > ++ Possible values for `` are `auto`, `crontab` (POSIX), > ++ `launchctl` (macOS), and `schtasks` (Windows). > ++ When `auto` is specified, the appropriate platform-specific > ++ scheduler is used. Default is `auto`. > + > > TROUBLESHOOTING > @@ builtin/gc.c: static const char *get_frequency(enum schedule_priority schedule) > } > } > > ++/* > ++ * get_schedule_cmd` reads the GIT_TEST_MAINT_SCHEDULER environment variable > ++ * to mock the schedulers that `git maintenance start` rely on. > ++ * > ++ * For test purpose, GIT_TEST_MAINT_SCHEDULER can be set to a comma-separated > ++ * list of colon-separated key/value pairs where each pair contains a scheduler > ++ * and its corresponding mock. > ++ * > ++ * * If $GET_TEST_MAINT_SCHEDULER is not set, return false and leave the > ++ * arguments unmodified. > ++ * > ++ * * If $GET_TEST_MAINT_SCHEDULER is set, return true. > ++ * In this case, the *cmd value is read as input. > ++ * > ++ * * if the input value *cmd is the key of one of the comma-separated list > ++ * item, then *is_available is set to true and *cmd is modified and becomes > ++ * the mock command. > ++ * > ++ * * if the input value *cmd isn’t the key of any of the comma-separated list > ++ * item, then *is_available is set to false. > ++ * > ++ * Ex.: > ++ * GIT_TEST_MAINT_SCHEDULER not set > ++ * ┏━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ > ++ * ┃ Input ┃ Output ┃ > ++ * ┃ *cmd ┃ return code │ *cmd │ *is_available ┃ > ++ * ┣━━━━━━━╋━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━┫ > ++ * ┃ "foo" ┃ false │ "foo" (unchanged) │ (unchanged) ┃ > ++ * ┗━━━━━━━┻━━━━━━━━━━━━━┷━━━━━━━━━━━━━━━━━━━┷━━━━━━━━━━━━━━━┛ > ++ * > ++ * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh” > ++ * ┏━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ > ++ * ┃ Input ┃ Output ┃ > ++ * ┃ *cmd ┃ return code │ *cmd │ *is_available ┃ > ++ * ┣━━━━━━━╋━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━┫ > ++ * ┃ "foo" ┃ true │ "./mock.foo.sh" │ true ┃ > ++ * ┃ "qux" ┃ true │ "qux" (unchanged) │ false ┃ > ++ * ┗━━━━━━━┻━━━━━━━━━━━━━┷━━━━━━━━━━━━━━━━━━━┷━━━━━━━━━━━━━━━┛ > ++ */ > +static int get_schedule_cmd(const char **cmd, int *is_available) > +{ > + char *item; > @@ builtin/gc.c: static int schtasks_schedule_task(const char *exec_path, enum sche > + int is_available; > + struct child_process child = CHILD_PROCESS_INIT; > + > -+ if (get_schedule_cmd(&cmd, &is_available) && !is_available) > -+ return 0; > ++ if (get_schedule_cmd(&cmd, &is_available)) > ++ return is_available; This fixes the bug that Eric found with the last version - excellent. > + > + strvec_split(&child.args, cmd); > + strvec_push(&child.args, "-l"); > @@ builtin/gc.c: static int crontab_update_schedule(int run_maintenance, int fd, co > + enum scheduler scheduler; > +}; > + > -+static void resolve_auto_scheduler(enum scheduler *scheduler) > ++static enum scheduler resolve_scheduler(enum scheduler scheduler) > +{ > -+ if (*scheduler != SCHEDULER_AUTO) > -+ return; > ++ if (scheduler != SCHEDULER_AUTO) > ++ return scheduler; > + > #if defined(__APPLE__) > -static const char platform_scheduler[] = "launchctl"; > -+ *scheduler = SCHEDULER_LAUNCHCTL; > -+ return; > ++ return SCHEDULER_LAUNCHCTL; > + > #elif defined(GIT_WINDOWS_NATIVE) > -static const char platform_scheduler[] = "schtasks"; > -+ *scheduler = SCHEDULER_SCHTASKS; > -+ return; > ++ return SCHEDULER_SCHTASKS; > + > #else > -static const char platform_scheduler[] = "crontab"; > -+ *scheduler = SCHEDULER_CRON; > -+ return; > ++ return SCHEDULER_CRON; > #endif > +} This is one of the changes that Eric suggested, I agree it improves the code. Thanks for your work on these patches, I've scanned the rest of the range-diff and I'd be happy to see this version merged Best Wishes Phillip > @@ builtin/gc.c: static int crontab_update_schedule(int run_maintenance, int fd, co > + if (scheduler == SCHEDULER_INVALID) > + BUG("invalid scheduler"); > + if (scheduler == SCHEDULER_AUTO) > -+ BUG("resolve_auto_scheduler should have been called before"); > ++ BUG("resolve_scheduler should have been called before"); > + > + if (!scheduler_fn[scheduler].is_available()) > + die(_("%s scheduler is not available"), > @@ builtin/gc.c: static int crontab_update_schedule(int run_maintenance, int fd, co > + struct option options[] = { > + OPT_CALLBACK_F( > + 0, "scheduler", &opts.scheduler, N_("scheduler"), > -+ N_("scheduler to use to trigger git maintenance run"), > ++ N_("scheduler to trigger git maintenance run"), > + PARSE_OPT_NONEG, maintenance_opt_scheduler), > + OPT_END() > + }; > @@ builtin/gc.c: static int crontab_update_schedule(int run_maintenance, int fd, co > + if (argc) > + usage_with_options(builtin_maintenance_start_usage, options); > + > -+ resolve_auto_scheduler(&opts.scheduler); > ++ opts.scheduler = resolve_scheduler(opts.scheduler); > + validate_scheduler(opts.scheduler); > + > if (maintenance_register()) > 2: 29628b5a92 ! 3: 0ea5b2fc45 maintenance: add support for systemd timers on Linux > @@ Documentation/git-maintenance.txt: OPTIONS > ---scheduler=auto|crontab|launchctl|schtasks:: > +--scheduler=auto|crontab|systemd-timer|launchctl|schtasks:: > When combined with the `start` subcommand, specify the scheduler > - to use to run the hourly, daily and weekly executions of > + for running the hourly, daily and weekly executions of > `git maintenance run`. > - The possible values for `` depend on the system: `crontab` > -- is available on POSIX systems, `launchctl` is available on > -- MacOS and `schtasks` is available on Windows. > -+ is available on POSIX systems, `systemd-timer` is available on Linux > -+ 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. > -+ `schtasks` is used. On Linux, `systemd-timer` is used if user systemd > -+ timers are available, otherwise, `crontab` is used. On all other systems, > -+ `crontab` is used. > +- Possible values for `` are `auto`, `crontab` (POSIX), > +- `launchctl` (macOS), and `schtasks` (Windows). > +- When `auto` is specified, the appropriate platform-specific > +- scheduler is used. Default is `auto`. > ++ Possible values for `` 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`. > > > TROUBLESHOOTING > @@ builtin/gc.c: static enum scheduler parse_scheduler(const char *value) > else if (!strcasecmp(value, "launchctl")) > return SCHEDULER_LAUNCHCTL; > else if (!strcasecmp(value, "schtasks")) > -@@ builtin/gc.c: static void resolve_auto_scheduler(enum scheduler *scheduler) > - *scheduler = SCHEDULER_SCHTASKS; > - return; > +@@ builtin/gc.c: static enum scheduler resolve_scheduler(enum scheduler scheduler) > + #elif defined(GIT_WINDOWS_NATIVE) > + return SCHEDULER_SCHTASKS; > > +#elif defined(__linux__) > + if (is_systemd_timer_available()) > -+ *scheduler = SCHEDULER_SYSTEMD; > ++ return SCHEDULER_SYSTEMD; > + else if (is_crontab_available()) > -+ *scheduler = SCHEDULER_CRON; > ++ return SCHEDULER_CRON; > + else > + die(_("neither systemd timers nor crontab are available")); > -+ return; > + > #else > - *scheduler = SCHEDULER_CRON; > - return; > + return SCHEDULER_CRON; > + #endif > > ## t/t7900-maintenance.sh ## > @@ t/t7900-maintenance.sh: test_xmllint () { >