Re: [PATCH v6 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>`

On 6/14/2021 12:36 AM, Eric Sunshine wrote:

On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard [off-list ref] wrote:

...

quoted

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