On 6/14/2021 12:36 AM, Eric Sunshine wrote: > On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard <lenaic@xxxxxxxxx> 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
