Re: [PATCH v3 3/4] maintenance: use launchctl on macOS

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> The solution is to switch from cron to the Apple-recommended [1]
> 'launchd' tool.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -1491,6 +1491,200 @@ static int maintenance_unregister(void)
> +static int boot_plist(int enable, const char *filename)
> +{
> +       struct child_process child = CHILD_PROCESS_INIT;
> +       child.no_stderr = 1;
> +       child.no_stdout = 1;
> +       if (start_command(&child))
> +               die(_("failed to start launchctl"));

Not necessarily worth a re-roll -- in fact, it could be done atop this
series to avoid holding this series up -- but this too-succinct error
reporting won't help users diagnose the failure. An alternative would
be to capture stdout and stderr and only print them if the command
fails. Perhaps something like this:

    struct strbuf out = STRBUF_INIT;
    struct strbuf err = STRBUF_INIT;
    ...
    if (pipe_command(child, NULL, 0, &out, 0, &err, 0) {
        if (out.len && err.len)
            strbuf_addstr(&out, "; ");
        strbuf_addbuf(&out, &err);
        die(_("launchctl failed: %s"), out.buf);
    }

By the way, won't this die() be a problem when schedule_plist() calls
boot_plist() to remove the old scheduled tasks before calling it again
to register the new ones? If the old ones don't exist, then it will
die() unnecessarily and never register the new ones. Or am I
misunderstanding? (I'm guessing that I must be misunderstanding since
the test script presumably passes.)

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -389,12 +389,54 @@ test_expect_success 'stop from existing schedule' '
> +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
> +       write_script print-args "#!/bin/sh\necho \$* >>args" &&

write_script() takes the script body as stdin, not as an argument, and
you don't need to specify /bin/sh. What you have here works by
accident only because write_script() takes an optional second argument
specifying the shell to use in place of the default /bin/sh.
Nevertheless, it should really be written:

    write_script print-args <<-\EOF
    echo $*
    EOF

Patch [4/4] uses write_script() correctly.

> +       rm -f args &&
> +       GIT_TEST_CRONTAB="./print-args" git maintenance start &&
> +
> +       # start registers the repo
> +       git config --get --global maintenance.repo "$(pwd)" &&
> +
> +       # ~/Library/LaunchAgents
> +       ls "$HOME/Library/LaunchAgents" >actual &&

Not sure what the comment above the `ls` is meant to be conveying.
Could be dropped but not itself worth a re-roll.

> +       cat >expect <<-\EOF &&
> +       org.git-scm.git.daily.plist
> +       org.git-scm.git.hourly.plist
> +       org.git-scm.git.weekly.plist
> +       EOF
> +       test_cmp expect actual &&
> +
> +       rm expect &&
> +       for frequency in hourly daily weekly
> +       do
> +               PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
> +               xmllint --noout "$PLIST" &&
> +               grep schedule=$frequency "$PLIST" &&
> +               echo "bootout gui/$UID $PLIST" >>expect &&
> +               echo "bootstrap gui/$UID $PLIST" >>expect || return 1
> +       done &&

This is still relying upon $UID picked up from the users environment
(as far as I can tell), which seems fragile. As mentioned in my first
review, it probably would be more robust to compute UID manually the
same way git-maintenance itself does.

> +       test_cmp expect args &&
> +
> +       rm -f args &&
> +       GIT_TEST_CRONTAB="./print-args" git maintenance stop &&

Minor: No need for the quotes around ./print-args (though they don't
hurt either, and certainly not worth re-rolling just to drop them, and
it's subjective so don't drop them just for my sake).

> +       # stop does not unregister the repo
> +       git config --get --global maintenance.repo "$(pwd)" &&
> +
> +       printf "bootout gui/$UID $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> +               hourly daily weekly >expect &&
> +       test_cmp expect args &&
> +       ls "$HOME/Library/LaunchAgents" >actual &&
> +       test_line_count = 0 actual
> +'