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