This is based on ds/maintenance-part-3. After sitting with the background maintenance as it has been cooking, I wanted to come back around and implement the background maintenance for Windows. However, I noticed that there were some things bothering me with background maintenance on my macOS machine. These are detailed in PATCH 3, but the tl;dr is that 'cron' is not recommended by Apple and instead 'launchd' satisfies our needs. This series implements the background scheduling so git maintenance (start|stop) works on those platforms. I've been operating with these schedules for a while now without the problems described in the patches. There is a particularly annoying case about console windows popping up on Windows, but PATCH 4 describes a plan to get around that. Updates in V3 ============= * This actually includes the feedback responses I had intended for v2. Sorry about that! * One major change is the use of a 'struct child_process' instead of just run_command_v_opt() so we can suppress error messages from the schedule helpers. We will rely on exit code and present our own error messages, as necessary. * Some doc and test fixes. Updates in V2 ============= * This is a faster turnaround for a v2 than I would normally like, but Eric inspired extra documentation about how to customize background schedules. * New extensions to git-maintenance.txt include guidelines for inspecting what git maintenance start does and how to customize beyond that. This includes a new PATCH 2 that includes documentation for 'cron' on non-macOS non-Windows systems. * Several improvements, especially in the tests, are included. * While testing manually, I noticed that somehow I had incorrectly had an opening <dict> tag instead of a closing </dict> tag in the hourly format on macOS. I found that the xmllint tool can verify the XML format of a file, which catches the bug. This seems like a good approach since the test is macOS-only. Does anyone have concerns about adding this dependency? Thanks, -Stolee cc: jrnieder@xxxxxxxxx [jrnieder@xxxxxxxxx], jonathantanmy@xxxxxxxxxx [jonathantanmy@xxxxxxxxxx], sluongng@xxxxxxxxx [sluongng@xxxxxxxxx]cc: Derrick Stolee stolee@xxxxxxxxx [stolee@xxxxxxxxx]cc: Đoàn Trần Công Danh congdanhqx@xxxxxxxxx [congdanhqx@xxxxxxxxx]cc: Martin Ågren martin.agren@xxxxxxxxx [martin.agren@xxxxxxxxx]cc: Eric Sunshine sunshine@xxxxxxxxxxxxxx [sunshine@xxxxxxxxxxxxxx]cc: Derrick Stolee stolee@xxxxxxxxx [stolee@xxxxxxxxx] Derrick Stolee (4): maintenance: extract platform-specific scheduling maintenance: include 'cron' details in docs maintenance: use launchctl on macOS maintenance: use Windows scheduled tasks Documentation/git-maintenance.txt | 116 +++++++++ builtin/gc.c | 417 ++++++++++++++++++++++++++++-- t/t7900-maintenance.sh | 75 +++++- t/test-lib.sh | 4 + 4 files changed, 592 insertions(+), 20 deletions(-) base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-776%2Fderrickstolee%2Fmaintenance%2FmacOS-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-776/derrickstolee/maintenance/macOS-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/776 Range-diff vs v2: 1: d35f1aa162 = 1: d35f1aa162 maintenance: extract platform-specific scheduling 2: 709a173720 ! 2: 0dfe53092e maintenance: include 'cron' details in docs @@ Commit message baseline can provide a way forward for users who have never worked with cron schedules. + Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> ## Documentation/git-maintenance.txt ## @@ Documentation/git-maintenance.txt: Further, the `git gc` command should not be c +--------------------------------------- + +The standard mechanism for scheduling background tasks on POSIX systems -+is `cron`. This tool executes commands based on a given schedule. The ++is cron(8). This tool executes commands based on a given schedule. The +current list of user-scheduled tasks can be found by running `crontab -l`. +The schedule written by `git maintenance start` is similar to this: + @@ Documentation/git-maintenance.txt: Further, the `git gc` command should not be c +Any modifications within this region will be completely deleted by +`git maintenance stop` or overwritten by `git maintenance start`. + -+The `<path>` string is loaded to specifically use the location for the -+`git` executable used in the `git maintenance start` command. This allows -+for multiple versions to be compatible. However, if the same user runs -+`git maintenance start` with multiple Git executables, then only the -+latest executable will be used. ++The `crontab` entry specifies the full path of the `git` executable to ++ensure that the executed `git` command is the same one with which ++`git maintenance start` was issued independent of `PATH`. If the same user ++runs `git maintenance start` with multiple Git executables, then only the ++latest executable is used. + +These commands use `git for-each-repo --config=maintenance.repo` to run +`git maintenance run --schedule=<frequency>` on each repository listed in +the multi-valued `maintenance.repo` config option. These are typically -+loaded from the user-specific global config located at `~/.gitconfig`. -+The `git maintenance` process then determines which maintenance tasks -+are configured to run on each repository with each `<frequency>` using -+the `maintenance.<task>.schedule` config options. These values are loaded -+from the global or repository config values. ++loaded from the user-specific global config. The `git maintenance` process ++then determines which maintenance tasks are configured to run on each ++repository with each `<frequency>` using the `maintenance.<task>.schedule` ++config options. These values are loaded from the global or repository ++config values. + +If the config values are insufficient to achieve your desired background +maintenance schedule, then you can create your own schedule. If you run +`crontab -e`, then an editor will load with your user-specific `cron` +schedule. In that editor, you can add your own schedule lines. You could +start by adapting the default schedule listed earlier, or you could read -+https://man7.org/linux/man-pages/man5/crontab.5.html[the `crontab` documentation] -+for advanced scheduling techniques. Please do use the full path and -+`--exec-path` techniques from the default schedule to ensure you are -+executing the correct binaries in your schedule. ++the crontab(5) documentation for advanced scheduling techniques. Please ++do use the full path and `--exec-path` techniques from the default ++schedule to ensure you are executing the correct binaries in your ++schedule. + GIT 3: 0fafd75d10 ! 3: 1629bcfcf8 maintenance: use launchctl on macOS @@ Commit message of macOS 10.11, which was released in September 2015. Before that release the 'launchctl load' subcommand was recommended. The best source of information on this transition I have seen is available - at [2]. + at [2]. The current design does not preclude a future version that + detects the available fatures of 'launchctl' to use the older + commands. However, it is best to rely on the newest version since + Apple might completely remove the deprecated version on short + notice. [2] https://babodee.wordpress.com/2016/04/09/launchctl-2-0-syntax/ @@ Commit message Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> ## Documentation/git-maintenance.txt ## -@@ Documentation/git-maintenance.txt: for advanced scheduling techniques. Please do use the full path and - executing the correct binaries in your schedule. +@@ Documentation/git-maintenance.txt: schedule to ensure you are executing the correct binaries in your + schedule. +BACKGROUND MAINTENANCE ON MACOS SYSTEMS +--------------------------------------- + +While macOS technically supports `cron`, using `crontab -e` requires -+elevated privileges and the executed process do not have a full user ++elevated privileges and the executed process does not have a full user +context. Without a full user context, Git and its credential helpers +cannot access stored credentials, so some maintenance tasks are not +functional. + +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]. -+ -+Scheduling maintenance through `git maintenance (start|stop)` requires -+some `launchctl` features available only in macOS 10.11 or later. ++which is the recommended way to schedule timed jobs in macOS. Scheduling ++maintenance through `git maintenance (start|stop)` requires some ++`launchctl` features available only in macOS 10.11 or later. + +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 ++$ ls ~/Library/LaunchAgents/org.git-scm.git* +org.git-scm.git.daily.plist +org.git-scm.git.hourly.plist +org.git-scm.git.weekly.plist @@ Documentation/git-maintenance.txt: for advanced scheduling techniques. Please do +and delete the `.plist` files. + +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. ++launchctl.plist(5) for more information. + + GIT @@ builtin/gc.c: static int maintenance_unregister(void) + return strbuf_detach(&output, NULL); +} + -+static int bootout(const char *filename) ++static int boot_plist(int enable, const char *filename) +{ + int result; -+ struct strvec args = STRVEC_INIT; ++ struct child_process child = CHILD_PROCESS_INIT; + char *uid = get_uid(); + const char *launchctl = getenv("GIT_TEST_CRONTAB"); + if (!launchctl) + launchctl = "/bin/launchctl"; + -+ strvec_split(&args, launchctl); -+ strvec_push(&args, "bootout"); -+ strvec_pushf(&args, "gui/%s", uid); -+ strvec_push(&args, filename); ++ strvec_split(&child.args, launchctl); + -+ result = run_command_v_opt(args.v, 0); ++ if (enable) ++ strvec_push(&child.args, "bootstrap"); ++ else ++ strvec_push(&child.args, "bootout"); ++ strvec_pushf(&child.args, "gui/%s", uid); ++ strvec_push(&child.args, filename); + -+ strvec_clear(&args); -+ free(uid); -+ return result; -+} ++ child.no_stderr = 1; ++ child.no_stdout = 1; + -+static int bootstrap(const char *filename) -+{ -+ int result; -+ struct strvec args = STRVEC_INIT; -+ char *uid = get_uid(); -+ const char *launchctl = getenv("GIT_TEST_CRONTAB"); -+ if (!launchctl) -+ launchctl = "/bin/launchctl"; ++ if (start_command(&child)) ++ die(_("failed to start launchctl")); + -+ strvec_split(&args, launchctl); -+ strvec_push(&args, "bootstrap"); -+ strvec_pushf(&args, "gui/%s", uid); -+ strvec_push(&args, filename); ++ result = finish_command(&child); + -+ result = run_command_v_opt(args.v, 0); -+ -+ strvec_clear(&args); + free(uid); + return result; +} @@ builtin/gc.c: static int maintenance_unregister(void) + const char *frequency = get_frequency(schedule); + char *name = get_service_name(frequency); + char *filename = get_service_filename(name); -+ int result = bootout(filename); ++ int result = boot_plist(0, filename); ++ unlink(filename); + free(filename); + free(name); + return result; @@ builtin/gc.c: static int maintenance_unregister(void) + + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); -+ plist = fopen(filename, "w"); -+ -+ if (!plist) -+ die(_("failed to open '%s'"), filename); ++ plist = xfopen(filename, "w"); + + 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" @@ builtin/gc.c: static int maintenance_unregister(void) + fprintf(plist, "</array>\n</dict>\n</plist>\n"); + + /* bootout might fail if not already running, so ignore */ -+ bootout(filename); -+ if (bootstrap(filename)) ++ boot_plist(0, filename); ++ if (boot_plist(1, filename)) + die(_("failed to bootstrap service %s"), filename); + + fclose(plist); @@ t/t7900-maintenance.sh: test_expect_success 'stop from existing schedule' ' ' +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' -+ echo "#!/bin/sh\necho \$@ >>args" >print-args && -+ chmod a+x print-args && ++ write_script print-args "#!/bin/sh\necho \$* >>args" && + + rm -f args && + GIT_TEST_CRONTAB="./print-args" git maintenance start && @@ t/t7900-maintenance.sh: test_expect_success 'stop from existing schedule' ' + for frequency in hourly daily weekly + do + PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && -+ xmllint "$PLIST" >/dev/null && ++ xmllint --noout "$PLIST" && + grep schedule=$frequency "$PLIST" && + echo "bootout gui/$UID $PLIST" >>expect && + echo "bootstrap gui/$UID $PLIST" >>expect || return 1 @@ t/t7900-maintenance.sh: test_expect_success 'stop from existing schedule' ' + test_cmp expect args && + + rm -f args && -+ GIT_TEST_CRONTAB="./print-args" git maintenance stop && ++ GIT_TEST_CRONTAB="./print-args" git maintenance stop && + + # stop does not unregister the repo + git config --get --global maintenance.repo "$(pwd)" && + -+ # stop does not remove plist files, but boots them out -+ rm expect && -+ for frequency in hourly daily weekly -+ do -+ PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && -+ grep schedule=$frequency "$PLIST" && -+ echo "bootout gui/$UID $PLIST" >>expect || return 1 -+ done && -+ test_cmp expect args ++ 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 +' + test_expect_success 'register preserves existing strategy' ' 4: 84eb44de31 ! 4: ed7a61978f maintenance: use Windows scheduled tasks @@ Commit message Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> ## Documentation/git-maintenance.txt ## -@@ Documentation/git-maintenance.txt: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSy - for more information. +@@ Documentation/git-maintenance.txt: To create more advanced customizations to your background tasks, see + launchctl.plist(5) for more information. +BACKGROUND MAINTENANCE ON WINDOWS SYSTEMS @@ builtin/gc.c: static int platform_update_schedule(int run_maintenance, int fd) +static int schedule_task(const char *exec_path, enum schedule_priority schedule) +{ + int result; -+ struct strvec args = STRVEC_INIT; ++ struct child_process child = CHILD_PROCESS_INIT; + const char *xml, *schtasks; -+ char *xmlpath; ++ char *xmlpath, *tempDir; + FILE *xmlfp; + const char *frequency = get_frequency(schedule); + char *name = get_task_name(frequency); + -+ xmlpath = xstrfmt("%s/schedule-%s.xml", -+ the_repository->objects->odb->path, -+ frequency); -+ xmlfp = fopen(xmlpath, "w"); -+ if (!xmlfp) -+ die(_("failed to open '%s'"), xmlpath); ++ tempDir = xstrfmt("%s/temp", the_repository->objects->odb->path); ++ xmlpath = xstrfmt("%s/schedule-%s.xml", tempDir, frequency); ++ safe_create_leading_directories(xmlpath); ++ xmlfp = xfopen(xmlpath, "w"); + + xml = "<?xml version=\"1.0\" encoding=\"UTF-16\"?>\n" + "<Task version=\"1.4\" xmlns=\"http://schemas.microsoft.com/windows/2004/02/mit/task\";>\n" @@ builtin/gc.c: static int platform_update_schedule(int run_maintenance, int fd) + schtasks = getenv("GIT_TEST_CRONTAB"); + if (!schtasks) + schtasks = "schtasks"; -+ strvec_split(&args, schtasks); -+ strvec_pushl(&args, "/create", "/tn", name, "/f", "/xml", xmlpath, NULL); ++ strvec_split(&child.args, schtasks); ++ strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", xmlpath, NULL); + -+ result = run_command_v_opt(args.v, 0); ++ child.no_stdout = 1; ++ child.no_stderr = 1; ++ ++ if (start_command(&child)) ++ die(_("failed to start schtasks")); ++ result = finish_command(&child); + -+ strvec_clear(&args); + unlink(xmlpath); ++ rmdir(tempDir); + free(xmlpath); + free(name); + return result; @@ t/t7900-maintenance.sh: test_expect_success !MACOS_MAINTENANCE 'stop from existi GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && grep "Important information!" cron.txt @@ t/t7900-maintenance.sh: test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' - test_cmp expect args + test_line_count = 0 actual ' +test_expect_success MINGW 'start and stop Windows maintenance' ' @@ t/t7900-maintenance.sh: test_expect_success MACOS_MAINTENANCE 'start and stop ma + # start registers the repo + git config --get --global maintenance.repo "$(pwd)" && + -+ for frequency in hourly daily weekly -+ do -+ printf "/create /tn Git Maintenance (%s) /f /xml .git/objects/schedule-%s.xml\n" \ -+ $frequency $frequency -+ done >expect && ++ printf "/create /tn Git Maintenance (%s) /f /xml .git/objects/temp/schedule-%s.xml\n" \ ++ hourly hourly daily daily weekly weekly >expect && + test_cmp expect args && + + rm -f args && -- gitgitgadget
