On 11/11/2020 3:12 AM, Eric Sunshine wrote: > On Wed, Nov 4, 2020 at 3:06 PM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> [...] >> The solution is to switch from cron to the Apple-recommended [1] >> 'launchd' tool. >> [...] >> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> --- >> 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
