| Message ID | pull.776.v3.git.1605276024.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Maintenance IV: Platform-specific background maintenance | expand |
On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > * This actually includes the feedback responses I had intended for v2. > Sorry about that! I forgot to mention a couple things when reviewing the patches individually, so I'll point them out here... > + at [2]. The current design does not preclude a future version that > + detects the available fatures of 'launchctl' to use the older s/fatures/features/ > -+ test_cmp expect args > ++ test_line_count = 0 actual These days, we usually say: test_must_be_empty actual
On Fri, Nov 13, 2020 at 03:47:15PM -0500, Eric Sunshine wrote: > I forgot to mention a couple things when reviewing the patches > individually, so I'll point them out here... In v2, you added an `xmllint` check on MacOS after discovering that gc.c was generating a malformed .plist file on that platform. That got me thinking that it would have been nice to have caught the problem earlier, if possible, even without having access to MacOS. Since none of the code added to gc.c has a hard platform dependency, it should be possible to perform all the tests on any platform rather than restricting them to specific platforms via test prerequisites. The patch below, which is built atop v3, does just that. It removes the conditional compilation directives from gc.c and the prerequisites from the test script so that all scheduler-specific code in gc.c is tested on all platform. The changes made by the patch are intended to be folded into each of your patches where appropriate (rather than existing atop your series, which, though possible, would be ugly). If you're interested in incorporating any of these improvements into v4, you can have my "Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>" in addition to the Helped-by: you already added. A few more notes... In addition to making it possible to test all platform-specific schedulers on each platform, I also made a few other changes/enhancements: * simplified UID retrieval and eliminated platform-specific dependencies (though this may need some additional tweaking on Windows, for which I did not test); also fixed the $UID issue mentioned in review * extended xmllint testing to the XML files generated for `schtasks` on Windows too; this required a small modification to the XML header boilerplate to specify the correct file encoding since `xmllint` complains when the file is UTF-8 but claims to be UTF-16; now that the test script captures the generated `schtasks` XML file for checking against `xmllint`, you have the opportunity to perform other sorts of validation checks on the XML too, such as you do in the MacOS `launchctl` test (though I did not add any additional checks) * fixed a potentially crashable `fprintf(xmlfp, xml)` by changing it to `fputs(xml, xmlfp)` since the compiler complains about the former because it can crash if `xml` contains a "%" * fixed the malformed write_script() issue for the MacOS test mentioned in review --- >8 --- From 016887b9fa4269bd4df46bea1d7849c08aba6ad6 Mon Sep 17 00:00:00 2001 From: Eric Sunshine <sunshine@sunshineco.com> Date: Sat, 14 Nov 2020 02:39:05 -0500 Subject: [PATCH] maintenance: test start/stop on all platforms from any platform Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- builtin/gc.c | 204 +++++++++++++++++++---------------------- t/t7900-maintenance.sh | 66 +++++++++---- t/test-lib.sh | 4 - 3 files changed, 143 insertions(+), 131 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 76a3afa20a..955d4b3baf 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1491,16 +1491,28 @@ static int maintenance_unregister(void) return run_command(&config_unset); } -#if defined(__APPLE__) +static const char *get_frequency(enum schedule_priority schedule) +{ + switch (schedule) { + case SCHEDULE_HOURLY: + return "hourly"; + case SCHEDULE_DAILY: + return "daily"; + case SCHEDULE_WEEKLY: + return "weekly"; + default: + BUG("invalid schedule %d", schedule); + } +} -static char *get_service_name(const char *frequency) +static char *launchctl_service_name(const char *frequency) { struct strbuf label = STRBUF_INIT; strbuf_addf(&label, "org.git-scm.git.%s", frequency); return strbuf_detach(&label, NULL); } -static char *get_service_filename(const char *name) +static char *launchctl_service_filename(const char *name) { char *expanded; struct strbuf filename = STRBUF_INIT; @@ -1514,49 +1526,23 @@ static char *get_service_filename(const char *name) return expanded; } -static const char *get_frequency(enum schedule_priority schedule) -{ - switch (schedule) { - case SCHEDULE_HOURLY: - return "hourly"; - case SCHEDULE_DAILY: - return "daily"; - case SCHEDULE_WEEKLY: - return "weekly"; - default: - BUG("invalid schedule %d", schedule); - } -} - -static char *get_uid(void) +static char *launchctl_get_uid(void) { - struct strbuf output = STRBUF_INIT; - struct child_process id = CHILD_PROCESS_INIT; - - strvec_pushl(&id.args, "/usr/bin/id", "-u", NULL); - if (capture_command(&id, &output, 0)) - die(_("failed to discover user id")); - - strbuf_trim_trailing_newline(&output); - return strbuf_detach(&output, NULL); + return xstrfmt("gui/%d", getuid()); } -static int boot_plist(int enable, const char *filename) +static int launchctl_boot_plist(int enable, const char *filename, const char *cmd) { int result; 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(&child.args, launchctl); + char *uid = launchctl_get_uid(); + strvec_split(&child.args, cmd); if (enable) strvec_push(&child.args, "bootstrap"); else strvec_push(&child.args, "bootout"); - strvec_pushf(&child.args, "gui/%s", uid); + strvec_push(&child.args, uid); strvec_push(&child.args, filename); child.no_stderr = 1; @@ -1571,33 +1557,33 @@ static int boot_plist(int enable, const char *filename) return result; } -static int remove_plist(enum schedule_priority schedule) +static int launchctl_remove_plist(enum schedule_priority schedule, const char *cmd) { const char *frequency = get_frequency(schedule); - char *name = get_service_name(frequency); - char *filename = get_service_filename(name); - int result = boot_plist(0, filename); + char *name = launchctl_service_name(frequency); + char *filename = launchctl_service_filename(name); + int result = launchctl_boot_plist(0, filename, cmd); unlink(filename); free(filename); free(name); return result; } -static int remove_plists(void) +static int launchctl_remove_plists(const char *cmd) { - return remove_plist(SCHEDULE_HOURLY) || - remove_plist(SCHEDULE_DAILY) || - remove_plist(SCHEDULE_WEEKLY); + return launchctl_remove_plist(SCHEDULE_HOURLY, cmd) || + launchctl_remove_plist(SCHEDULE_DAILY, cmd) || + launchctl_remove_plist(SCHEDULE_WEEKLY, cmd); } -static int schedule_plist(const char *exec_path, enum schedule_priority schedule) +static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd) { FILE *plist; int i; const char *preamble, *repeat; const char *frequency = get_frequency(schedule); - char *name = get_service_name(frequency); - char *filename = get_service_filename(name); + char *name = launchctl_service_name(frequency); + char *filename = launchctl_service_filename(name); if (safe_create_leading_directories(filename)) die(_("failed to create directories for '%s'"), filename); @@ -1658,8 +1644,8 @@ static int schedule_plist(const char *exec_path, enum schedule_priority schedule fprintf(plist, "</array>\n</dict>\n</plist>\n"); /* bootout might fail if not already running, so ignore */ - boot_plist(0, filename); - if (boot_plist(1, filename)) + launchctl_boot_plist(0, filename, cmd); + if (launchctl_boot_plist(1, filename, cmd)) die(_("failed to bootstrap service %s"), filename); fclose(plist); @@ -1668,57 +1654,38 @@ static int schedule_plist(const char *exec_path, enum schedule_priority schedule return 0; } -static int add_plists(void) +static int launchctl_add_plists(const char *cmd) { const char *exec_path = git_exec_path(); - return schedule_plist(exec_path, SCHEDULE_HOURLY) || - schedule_plist(exec_path, SCHEDULE_DAILY) || - schedule_plist(exec_path, SCHEDULE_WEEKLY); + return launchctl_schedule_plist(exec_path, SCHEDULE_HOURLY, cmd) || + launchctl_schedule_plist(exec_path, SCHEDULE_DAILY, cmd) || + launchctl_schedule_plist(exec_path, SCHEDULE_WEEKLY, cmd); } -static int platform_update_schedule(int run_maintenance, int fd) +static int launchctl_update_schedule(int run_maintenance, int fd, const char *cmd) { if (run_maintenance) - return add_plists(); + return launchctl_add_plists(cmd); else - return remove_plists(); -} - -#elif defined(GIT_WINDOWS_NATIVE) - -static const char *get_frequency(enum schedule_priority schedule) -{ - switch (schedule) { - case SCHEDULE_HOURLY: - return "hourly"; - case SCHEDULE_DAILY: - return "daily"; - case SCHEDULE_WEEKLY: - return "weekly"; - default: - BUG("invalid schedule %d", schedule); - } + return launchctl_remove_plists(cmd); } -static char *get_task_name(const char *frequency) +static char *schtasks_task_name(const char *frequency) { struct strbuf label = STRBUF_INIT; strbuf_addf(&label, "Git Maintenance (%s)", frequency); return strbuf_detach(&label, NULL); } -static int remove_task(enum schedule_priority schedule) +static int schtasks_remove_task(enum schedule_priority schedule, const char *cmd) { int result; struct strvec args = STRVEC_INIT; const char *frequency = get_frequency(schedule); - char *name = get_task_name(frequency); - const char *schtasks = getenv("GIT_TEST_CRONTAB"); - if (!schtasks) - schtasks = "schtasks"; + char *name = schtasks_task_name(frequency); - strvec_split(&args, schtasks); + strvec_split(&args, cmd); strvec_pushl(&args, "/delete", "/tn", name, "/f", NULL); result = run_command_v_opt(args.v, 0); @@ -1728,33 +1695,33 @@ static int remove_task(enum schedule_priority schedule) return result; } -static int remove_scheduled_tasks(void) +static int schtasks_remove_tasks(const char *cmd) { - return remove_task(SCHEDULE_HOURLY) || - remove_task(SCHEDULE_DAILY) || - remove_task(SCHEDULE_WEEKLY); + return schtasks_remove_task(SCHEDULE_HOURLY, cmd) || + schtasks_remove_task(SCHEDULE_DAILY, cmd) || + schtasks_remove_task(SCHEDULE_WEEKLY, cmd); } -static int schedule_task(const char *exec_path, enum schedule_priority schedule) +static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule, const char *cmd) { int result; struct child_process child = CHILD_PROCESS_INIT; - const char *xml, *schtasks; + const char *xml; char *xmlpath, *tempDir; FILE *xmlfp; const char *frequency = get_frequency(schedule); - char *name = get_task_name(frequency); + char *name = schtasks_task_name(frequency); 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" + xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" "<Task version=\"1.4\" xmlns=\"http://schemas.microsoft.com/windows/2004/02/mit/task\">\n" "<Triggers>\n" "<CalendarTrigger>\n"; - fprintf(xmlfp, xml); + fputs(xml, xmlfp); switch (schedule) { case SCHEDULE_HOURLY: @@ -1831,10 +1798,7 @@ static int schedule_task(const char *exec_path, enum schedule_priority schedule) fprintf(xmlfp, xml, exec_path, exec_path, frequency); fclose(xmlfp); - schtasks = getenv("GIT_TEST_CRONTAB"); - if (!schtasks) - schtasks = "schtasks"; - strvec_split(&child.args, schtasks); + strvec_split(&child.args, cmd); strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", xmlpath, NULL); child.no_stdout = 1; @@ -1851,42 +1815,36 @@ static int schedule_task(const char *exec_path, enum schedule_priority schedule) return result; } -static int add_scheduled_tasks(void) +static int schtasks_schedule_tasks(const char *cmd) { const char *exec_path = git_exec_path(); - return schedule_task(exec_path, SCHEDULE_HOURLY) || - schedule_task(exec_path, SCHEDULE_DAILY) || - schedule_task(exec_path, SCHEDULE_WEEKLY); + return schtasks_schedule_task(exec_path, SCHEDULE_HOURLY, cmd) || + schtasks_schedule_task(exec_path, SCHEDULE_DAILY, cmd) || + schtasks_schedule_task(exec_path, SCHEDULE_WEEKLY, cmd); } -static int platform_update_schedule(int run_maintenance, int fd) +static int schtasks_update_schedule(int run_maintenance, int fd, const char *cmd) { if (run_maintenance) - return add_scheduled_tasks(); + return schtasks_schedule_tasks(cmd); else - return remove_scheduled_tasks(); + return schtasks_remove_tasks(cmd); } -#else #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE" #define END_LINE "# END GIT MAINTENANCE SCHEDULE" -static int platform_update_schedule(int run_maintenance, int fd) +static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd) { int result = 0; int in_old_region = 0; struct child_process crontab_list = CHILD_PROCESS_INIT; struct child_process crontab_edit = CHILD_PROCESS_INIT; FILE *cron_list, *cron_in; - const char *crontab_name; struct strbuf line = STRBUF_INIT; - crontab_name = getenv("GIT_TEST_CRONTAB"); - if (!crontab_name) - crontab_name = "crontab"; - - strvec_split(&crontab_list.args, crontab_name); + strvec_split(&crontab_list.args, cmd); strvec_push(&crontab_list.args, "-l"); crontab_list.in = -1; crontab_list.out = dup(fd); @@ -1906,7 +1864,7 @@ static int platform_update_schedule(int run_maintenance, int fd) cron_list = fdopen(fd, "r"); rewind(cron_list); - strvec_split(&crontab_edit.args, crontab_name); + strvec_split(&crontab_edit.args, cmd); crontab_edit.in = -1; crontab_edit.git_cmd = 0; @@ -1963,20 +1921,48 @@ static int platform_update_schedule(int run_maintenance, int fd) fclose(cron_list); return result; } + +#if defined(__APPLE__) +static const char platform_scheduler[] = "launchctl"; +#elif defined(GIT_WINDOWS_NATIVE) +static const char platform_scheduler[] = "schtasks"; +#else +static const char platform_scheduler[] = "crontab"; #endif -static int update_background_schedule(int run_maintenance) +static int update_background_schedule(int enable) { int result; + const char *scheduler = platform_scheduler; + const char *cmd = scheduler; + char *testing; struct lock_file lk; char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); + testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER")); + if (testing) { + char *sep = strchr(testing, ':'); + if (!sep) + die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing); + *sep = '\0'; + scheduler = testing; + cmd = sep + 1; + } + if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) return error(_("another process is scheduling background maintenance")); - result = platform_update_schedule(run_maintenance, lk.tempfile->fd); + if (!strcmp(scheduler, "launchctl")) + result = launchctl_update_schedule(enable, lk.tempfile->fd, cmd); + else if (!strcmp(scheduler, "schtasks")) + result = schtasks_update_schedule(enable, lk.tempfile->fd, cmd); + else if (!strcmp(scheduler, "crontab")) + result = crontab_update_schedule(enable, lk.tempfile->fd, cmd); + else + die("unknown background scheduler: %s", scheduler); rollback_lock_file(&lk); + free(testing); return result; } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 0dc2479117..e92946c10a 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -7,6 +7,19 @@ test_description='git maintenance builtin' GIT_TEST_COMMIT_GRAPH=0 GIT_TEST_MULTI_PACK_INDEX=0 +test_lazy_prereq XMLLINT ' + xmllint --version +' + +test_xmllint () { + if test_have_prereq XMLLINT + then + xmllint --noout "$@" + else + true + fi +} + test_expect_success 'help text' ' test_expect_code 129 git maintenance -h 2>err && test_i18ngrep "usage: git maintenance <subcommand>" err && @@ -367,8 +380,8 @@ test_expect_success 'register and unregister' ' test_cmp before actual ' -test_expect_success !MACOS_MAINTENANCE,!MINGW 'start from empty cron table' ' - GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && +test_expect_success 'start from empty cron table' ' + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start && # start registers the repo git config --get --global maintenance.repo "$(pwd)" && @@ -378,28 +391,32 @@ test_expect_success !MACOS_MAINTENANCE,!MINGW 'start from empty cron table' ' grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt ' -test_expect_success !MACOS_MAINTENANCE,!MINGW 'stop from existing schedule' ' - GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && +test_expect_success 'stop from existing schedule' ' + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance stop && # stop does not unregister the repo git config --get --global maintenance.repo "$(pwd)" && # Operation is idempotent - GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance stop && test_must_be_empty cron.txt ' -test_expect_success !MACOS_MAINTENANCE,!MINGW 'start preserves existing schedule' ' +test_expect_success 'start preserves existing schedule' ' echo "Important information!" >cron.txt && - GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start && grep "Important information!" cron.txt ' -test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' - write_script print-args "#!/bin/sh\necho \$* >>args" && +test_expect_success 'start and stop macOS maintenance' ' + uid=$(id -u) && + + write_script print-args <<-\EOF && + echo $* >>args + EOF rm -f args && - GIT_TEST_CRONTAB="./print-args" git maintenance start && + GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start && # start registers the repo git config --get --global maintenance.repo "$(pwd)" && @@ -417,33 +434,41 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' for frequency in hourly daily weekly do PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && - xmllint --noout "$PLIST" && + test_xmllint "$PLIST" && grep schedule=$frequency "$PLIST" && - echo "bootout gui/$UID $PLIST" >>expect && - echo "bootstrap gui/$UID $PLIST" >>expect || return 1 + 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 && + GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance stop && # stop does not unregister the repo git config --get --global maintenance.repo "$(pwd)" && - printf "bootout gui/$UID $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ + 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 MINGW 'start and stop Windows maintenance' ' +test_expect_success 'start and stop Windows maintenance' ' write_script print-args <<-\EOF && echo $* >>args + while test $# -gt 0 + do + case "$1" in + /xml) shift; xmlfile=$1; break ;; + *) shift ;; + esac + done + test -z "$xmlfile" || cp "$xmlfile" . EOF rm -f args && - GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start && + GIT_TEST_MAINT_SCHEDULER="schtasks:/bin/sh print-args" git maintenance start && # start registers the repo git config --get --global maintenance.repo "$(pwd)" && @@ -452,8 +477,13 @@ test_expect_success MINGW 'start and stop Windows maintenance' ' hourly hourly daily daily weekly weekly >expect && test_cmp expect args && + for frequency in hourly daily weekly + do + test_xmllint "schedule-$frequency.xml" + done && + rm -f args && - GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance stop && + GIT_TEST_MAINT_SCHEDULER="schtasks:/bin/sh print-args" git maintenance stop && # stop does not unregister the repo git config --get --global maintenance.repo "$(pwd)" && diff --git a/t/test-lib.sh b/t/test-lib.sh index 620ffbf3af..4a60d1ed76 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1703,10 +1703,6 @@ test_lazy_prereq REBASE_P ' test -z "$GIT_TEST_SKIP_REBASE_P" ' -test_lazy_prereq MACOS_MAINTENANCE ' - launchctl list -' - # Ensure that no test accidentally triggers a Git command # that runs 'crontab', affecting a user's cron schedule. # Tests that verify the cron integration must set this locally
On 11/14/2020 4:23 AM, Eric Sunshine wrote: > On Fri, Nov 13, 2020 at 03:47:15PM -0500, Eric Sunshine wrote: >> I forgot to mention a couple things when reviewing the patches >> individually, so I'll point them out here... > > In v2, you added an `xmllint` check on MacOS after discovering that > gc.c was generating a malformed .plist file on that platform. That got > me thinking that it would have been nice to have caught the problem > earlier, if possible, even without having access to MacOS. Since none > of the code added to gc.c has a hard platform dependency, it should be > possible to perform all the tests on any platform rather than > restricting them to specific platforms via test prerequisites. The > patch below, which is built atop v3, does just that. It removes the > conditional compilation directives from gc.c and the prerequisites > from the test script so that all scheduler-specific code in gc.c is > tested on all platform. > > The changes made by the patch are intended to be folded into each of > your patches where appropriate (rather than existing atop your series, > which, though possible, would be ugly). If you're interested in > incorporating any of these improvements into v4, you can have my > "Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>" in addition > to the Helped-by: you already added. This approach is fascinating. I will tease it apart to appropriately incorporate it into my series. Thank you for your sign-off, since this elevates the patches from "Helped-by" to "Co-authored by". Thanks, -Stolee
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@gmail.com [jrnieder@gmail.com], jonathantanmy@google.com [jonathantanmy@google.com], sluongng@gmail.com [sluongng@gmail.com]cc: Derrick Stolee stolee@gmail.com [stolee@gmail.com]cc: Đoàn Trần Công Danh congdanhqx@gmail.com [congdanhqx@gmail.com]cc: Martin Ågren martin.agren@gmail.com [martin.agren@gmail.com]cc: Eric Sunshine sunshine@sunshineco.com [sunshine@sunshineco.com]cc: Derrick Stolee stolee@gmail.com [stolee@gmail.com] 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@sunshineco.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> ## 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@microsoft.com> ## 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@microsoft.com> ## 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 &&