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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 13, 2020, 20:19 UTC
Message-ID: <CAPig+cQQrucwE-KSndQ5kCjG9Q1MtjxraAmA95+OQe=6bdks-Q@mail.gmail.com>
In-Reply-To: <1629bcfcf82dbc2ed9889a0e9ea2d08427901c4e.1605276024.git.gitgitgadget@gmail.com>

On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 15 quoted lines

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

Show 8 quoted lines

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

Show 16 quoted lines

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

Show 9 quoted lines

> +       # 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
> +'