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=<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 `<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.
++ 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`.
+
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 `<scheduler>` 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 `<scheduler>` 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 `<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`.
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 () {
