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>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Son Luong Ngoc" <sluongng@gmail.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v2 3/4] maintenance: use launchctl on macOS
Date: Wed, 11 Nov 2020 03:12:19 -0500	[thread overview]
Message-ID: <CAPig+cT=DytbMH6KkC6ipD3jbWNa7jgW9G0Q76rwJoEsLGn_ow@mail.gmail.com> (raw)
In-Reply-To: <0fafd75d100f343f7cff6471772ed9b12793f81e.1604520368.git.gitgitgadget@gmail.com>

On Wed, Nov 4, 2020 at 3:06 PM 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/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> +While macOS technically supports `cron`, using `crontab -e` requires
> +elevated privileges and the executed process do not have a full user

Either s/process/processes/ or s/do/does/

> +context. Without a full user context, Git and its credential helpers
> +cannot access stored credentials, so some maintenance tasks are not
> +functional.

Nicely explained.

> +Instead, `git maintenance start` interacts with the `launchctl` tool,
> +which is the recommended way to
> +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html[schedule timed jobs in macOS].

Nit: I worry a bit about links to Apple documentation becoming
outdated. It might not hurt to omit this link altogether, or perhaps
demote it to a footnote (which might allow it to be somewhat usable
even when Git documentation is rendered into something other than
HTML).

> +Scheduling maintenance through `git maintenance (start|stop)` requires
> +some `launchctl` features available only in macOS 10.11 or later.

Nit: This leaves the reader wondering what modern features are needed.
Would it make sense to mention that "bootstrap" is used in place of
"load" in older versions of 'launchctl'?

> +Your user-specific scheduled tasks are stored as XML-formatted `.plist`
> +files in `~/Library/LaunchAgents/`. You can see the currently-registered
> +tasks using the following command:
> +
> +-----------------------------------------------------------------------
> +$ ls ~/Library/LaunchAgents/ | grep org.git-scm.git

Alternately (unimportant):

    ls ~/Library/LaunchAgents/org.git-scm.git.*

although that would emit "No such file" if you don't have any
registered, which might suggest:

    find ~/Library/LaunchAgents -name 'org.git-scm.git.*'

> +To create more advanced customizations to your background tasks, see
> +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLaunchdJobs.html#//apple_ref/doc/uid/TP40001762-104142[the `launchctl` documentation]
> +for more information.

I really worry about this sort of URL becoming outdated. Would it make
sense instead to just point the user at the man page,
launchd.plist(5)? It's not quite the same, as it doesn't provide the
range of examples as the URL you cite, but it should get the user
started.

> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -1491,6 +1491,214 @@ static int maintenance_unregister(void)
> +static int remove_plist(enum schedule_priority schedule)
> +{
> +       const char *frequency = get_frequency(schedule);
> +       char *name = get_service_name(frequency);
> +       char *filename = get_service_filename(name);
> +       int result = bootout(filename);
> +       free(filename);
> +       free(name);
> +       return result;
> +}
>
> +static int remove_plists(void)
> +{
> +       return remove_plist(SCHEDULE_HOURLY) ||
> +               remove_plist(SCHEDULE_DAILY) ||
> +               remove_plist(SCHEDULE_WEEKLY);
> +}

The new documentation you added says that the plist files will be
deleted after they are deregistered using launchctl, but I don't see
anything actually deleting them. Am I missing something obvious?

> +static int schedule_plist(const char *exec_path, enum schedule_priority schedule)
> +{
> +       plist = fopen(filename, "w");
> +       if (!plist)
> +               die(_("failed to open '%s'"), filename);

As mentioned previously, these could be replaced with a simple xfopen().

In fact, I'm having trouble seeing changes in this re-roll which you
had planned on making, such as consolidating the repeated code in
bootout() and bootstrap(), and ensuring that bootout() doesn't
complain if the plist files are already missing, and so forth. Did you
opt to not make those changes? (Which would be fine; they were minor
suggestions.)

> +       preamble = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
> +                  "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
> +                  "<plist version=\"1.0\">"
> +                  "<dict>\n"
> +                  "<key>Label</key><string>%s</string>\n"
> +                  "<key>ProgramArguments</key>\n"
> +                  "<array>\n"
> +                  "<string>%s/git</string>\n"
> +                  "<string>--exec-path=%s</string>\n"
> +                  "<string>for-each-repo</string>\n"
> +                  "<string>--config=maintenance.repo</string>\n"
> +                  "<string>maintenance</string>\n"
> +                  "<string>run</string>\n"
> +                  "<string>--schedule=%s</string>\n"
> +                  "</array>\n"
> +                  "<key>StartCalendarInterval</key>\n"
> +                  "<array>\n";
> +       fprintf(plist, preamble, name, exec_path, exec_path, frequency);

The Git test framework ensures that this will be written into the test
directory rather than the user's actual ~/Library/LaunchAgents
directory during testing. Okay.

> +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
> +       echo "#!/bin/sh\necho \$@ >>args" >print-args &&
> +       chmod a+x print-args &&

Earlier review already mentioned write_script() and "$@". (Not
necessarily worth a re-roll.)

> +       for frequency in hourly daily weekly
> +       do
> +               PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
> +               xmllint "$PLIST" >/dev/null &&

Do we really need to suppress xmllint's stdout?

> +               grep schedule=$frequency "$PLIST" &&
> +               echo "bootout gui/$UID $PLIST" >>expect &&
> +               echo "bootstrap gui/$UID $PLIST" >>expect || return 1
> +       done &&
> +       test_cmp expect args &&
> +
> +       rm -f args &&
> +       GIT_TEST_CRONTAB="./print-args"  git maintenance stop &&

There is still an extra space between the closing quote and git
command (mentioned previously).

> +       # stop does not unregister the repo
> +       git config --get --global maintenance.repo "$(pwd)" &&
> +
> +       # stop does not remove plist files, but boots them out

Documentation added in this re-roll claims that the plist files do get deleted.

  reply	other threads:[~2020-11-11  8:12 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 [this message]
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
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+cT=DytbMH6KkC6ipD3jbWNa7jgW9G0Q76rwJoEsLGn_ow@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=congdanhqx@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=sluongng@gmail.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH v2 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).