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

From: Derrick Stolee <stolee@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>,
	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>,
	"Đ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: Thu, 12 Nov 2020 08:42:52 -0500	[thread overview]
Message-ID: <5e0307b8-8e63-f82d-b417-7007d4a3a5b3@gmail.com> (raw)
In-Reply-To: <CAPig+cT=DytbMH6KkC6ipD3jbWNa7jgW9G0Q76rwJoEsLGn_ow@mail.gmail.com>

On 11/11/2020 3:12 AM, Eric Sunshine wrote:
> 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.

I shared similar concerns. I'll use the man page references instead.
All of the information should be a short web search away after the
user is given the right terminology.

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

As mentioned below, this was a change that I made but somehow lost
while juggling multiple copies of my branch.

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

No, I definitely made those changes _somewhere_ but I must have
gotten confused as to which of my machines had those changes. I
guess that's part of the risk of testing across three platforms.

Thank you for noticing, and I'll be more careful from now on.

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

I'm going to go back to all of your earlier comments to make sure
they are _actually_ applied in v3.

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

It outputs the XML itself. Maybe there is a command to stop that from
happening, but nulling stdout keeps the test log clean.

Thanks,
-Stolee