On Wed, Jun 16, 2021 at 8:48 PM Derrick Stolee <stolee@xxxxxxxxx> 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@xxxxxxxxx> 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.
