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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 17, 2021, 04:11 UTC
Message-ID: <CAPig+cSCWHh9eKs2Wj3N8OBJ5LzQk=4sCEu6=s0aR76nmo8H5g@mail.gmail.com>
In-Reply-To: <5c4366e4-a891-0a3a-2534-af48ab0097c2@gmail.com>

On Wed, Jun 16, 2021 at 8:48 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 14 quoted lines

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

Show 11 quoted lines

> >     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.

Show 12 quoted lines

> > 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.