git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git List <git@vger.kernel.org>, Derrick Stolee <stolee@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v3 3/4] maintenance: use launchctl on macOS
Date: Fri, 13 Nov 2020 15:19:25 -0500	[thread overview]
Message-ID: <CAPig+cQQrucwE-KSndQ5kCjG9Q1MtjxraAmA95+OQe=6bdks-Q@mail.gmail.com> (raw)
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:
> [...]
> 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
> +'

  reply	other threads:[~2020-11-13 20:19 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-03 14:03 [PATCH 0/3] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-11-03 14:03 ` [PATCH 1/3] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-03 14:03 ` [PATCH 2/3] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-03 18:45   ` Eric Sunshine
2020-11-03 21:21     ` Derrick Stolee
2020-11-03 22:27       ` Eric Sunshine
2020-11-04 13:33         ` Derrick Stolee
2020-11-04 14:17       ` Derrick Stolee
2020-11-03 14:03 ` [PATCH 3/3] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-03 19:06   ` Eric Sunshine
2020-11-03 21:23     ` Derrick Stolee
2020-11-03 20:18 ` [PATCH 0/3] Maintenance IV: Platform-specific background maintenance Junio C Hamano
2020-11-03 20:21 ` Junio C Hamano
2020-11-03 21:09   ` Derrick Stolee
2020-11-03 22:30     ` Junio C Hamano
2020-11-04 13:02       ` Derrick Stolee
2020-11-04 17:00         ` Junio C Hamano
2020-11-04 18:43           ` Derrick Stolee
2020-11-04 20:06 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2020-11-04 20:06   ` [PATCH v2 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-04 20:06   ` [PATCH v2 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-11  7:10     ` Eric Sunshine
2020-11-04 20:06   ` [PATCH v2 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-11  8:12     ` Eric Sunshine
2020-11-12 13:42       ` Derrick Stolee
2020-11-12 16:43         ` Eric Sunshine
2020-11-04 20:06   ` [PATCH v2 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-11  8:59     ` Eric Sunshine
2020-11-12 13:56       ` Derrick Stolee
2020-11-13 14:00   ` [PATCH v3 0/4] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-11-13 14:00     ` [PATCH v3 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-13 14:00     ` [PATCH v3 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-13 14:00     ` [PATCH v3 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-13 20:19       ` Eric Sunshine [this message]
2020-11-13 20:42         ` Derrick Stolee
2020-11-13 20:53           ` Eric Sunshine
2020-11-13 20:56             ` Eric Sunshine
2020-11-13 14:00     ` [PATCH v3 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-13 20:44       ` Eric Sunshine
2020-11-13 21:32         ` Derrick Stolee
2020-11-13 21:40           ` Eric Sunshine
2020-11-16 13:13             ` Derrick Stolee
2020-11-13 20:47     ` [PATCH v3 0/4] Maintenance IV: Platform-specific background maintenance Eric Sunshine
2020-11-14  9:23       ` Eric Sunshine
2020-11-16 13:17         ` Derrick Stolee
2020-11-17 21:13     ` [PATCH v4 " Derrick Stolee via GitGitGadget
2020-11-17 21:13       ` [PATCH v4 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-17 21:13       ` [PATCH v4 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-18  0:34         ` Eric Sunshine
2020-11-18 18:30           ` Derrick Stolee
2020-11-17 21:13       ` [PATCH v4 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-18  6:45         ` Eric Sunshine
2020-11-18 18:22           ` Derrick Stolee
2020-11-17 21:13       ` [PATCH v4 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-18  7:15         ` Eric Sunshine
2020-11-18 18:30           ` Derrick Stolee
2020-11-18 20:54             ` Eric Sunshine
2020-11-18 21:16               ` Derrick Stolee
2020-11-17 23:36       ` [PATCH v4 0/4] Maintenance IV: Platform-specific background maintenance Eric Sunshine
2020-11-24  2:20         ` Derrick Stolee
2020-11-24  2:59           ` Eric Sunshine
2020-11-17 23:54       ` Eric Sunshine
2020-11-24  4:16       ` [PATCH v5 " Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-27  9:08           ` Eric Sunshine
2020-12-09 19:28         ` [PATCH v6 0/4] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-12-09 19:28           ` [PATCH v6 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-12-09 19:29           ` [PATCH v6 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-12-09 19:29           ` [PATCH v6 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-12-09 19:29           ` [PATCH v6 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-12-10  0:32           ` [PATCH v6 0/4] Maintenance IV: Platform-specific background maintenance Junio C Hamano
2020-12-10  0:49             ` Eric Sunshine
2020-12-10  1:04               ` Junio C Hamano
2021-01-05 12:17                 ` Derrick Stolee
2021-01-05 13:08           ` [PATCH v7 " Derrick Stolee via GitGitGadget
2021-01-05 13:08             ` [PATCH v7 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2021-01-05 13:08             ` [PATCH v7 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2021-01-05 13:08             ` [PATCH v7 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2021-01-10  6:34               ` Eric Sunshine
2021-01-05 13:08             ` [PATCH v7 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPig+cQQrucwE-KSndQ5kCjG9Q1MtjxraAmA95+OQe=6bdks-Q@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH v3 3/4] maintenance: use launchctl on macOS' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).