Maintenance IV: Platform-specific background maintenance

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 2, 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 3 describes a plan to get around that.

Thanks, -Stolee

Derrick Stolee (3):
maintenance: extract platform-specific scheduling
maintenance: use launchctl on macOS
maintenance: use Windows scheduled tasks

 builtin/gc.c           | 428 +++++++++++++++++++++++++++++++++++++++--
 t/t7900-maintenance.sh |  86 ++++++++-
 t/test-lib.sh          |   4 +
 3 files changed, 498 insertions(+), 20 deletions(-)

base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-776%2Fderrickstolee%2Fmaintenance%2FmacOS-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-776/derrickstolee/maintenance/macOS-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/776

gitgitgadget

From: Derrick Stolee <[email protected]>

The existing schedule mechanism using 'cron' is supported by POSIX platforms, but not Windows. It also works slightly differently on macOS to significant detriment of the user experience. To allow for new implementations on these platforms, extract a method that performs the platform-specific scheduling mechanism. This will be swapped at compile time with new implementations on specialized platforms.

---
 builtin/gc.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

index e3098ef6a1..c1f7d9bdc2 100644
@@ -1494,7 +1494,7 @@ static int maintenance_unregister(void)
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
 #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
 
-static int update_background_schedule(int run_maintenance)
+static int platform_update_schedule(int run_maintenance, int fd)
 {
 	int result = 0;
 	int in_old_region = 0;

@@ -1503,11 +1503,6 @@ static int update_background_schedule(int run_maintenance)
 	FILE *cron_list, *cron_in;
 	const char *crontab_name;
 	struct strbuf line = STRBUF_INIT;
-	struct lock_file lk;
-	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
-
-	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)
-		return error(_("another process is scheduling background maintenance"));
 
 	crontab_name = getenv("GIT_TEST_CRONTAB");
 	if (!crontab_name)

@@ -1516,12 +1511,11 @@ static int update_background_schedule(int run_maintenance)
 	strvec_split(&crontab_list.args, crontab_name);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
-	crontab_list.out = dup(lk.tempfile->fd);
+	crontab_list.out = dup(fd);
 	crontab_list.git_cmd = 0;
 
 	if (start_command(&crontab_list)) {
-		result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
-		goto cleanup;
+		return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
 	}
 
 	/* Ignore exit code, as an empty crontab will return error. */

@@ -1531,7 +1525,7 @@ static int update_background_schedule(int run_maintenance)
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
 	 */
-	cron_list = fdopen(lk.tempfile->fd, "r");
+	cron_list = fdopen(fd, "r");
 	rewind(cron_list);
 
 	strvec_split(&crontab_edit.args, crontab_name);

@@ -1539,8 +1533,7 @@ static int update_background_schedule(int run_maintenance)
 	crontab_edit.git_cmd = 0;
 
 	if (start_command(&crontab_edit)) {
-		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
-		goto cleanup;
+		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
 	}
 
 	cron_in = fdopen(crontab_edit.in, "w");

@@ -1586,13 +1579,24 @@ static int update_background_schedule(int run_maintenance)
 	close(crontab_edit.in);
 
 done_editing:
-	if (finish_command(&crontab_edit)) {
+	if (finish_command(&crontab_edit))
 		result = error(_("'crontab' died"));
-		goto cleanup;
-	}
-	fclose(cron_list);
+	else
+		fclose(cron_list);
+	return result;
+}
+
+static int update_background_schedule(int run_maintenance)
+{
+	int result;
+	struct lock_file lk;
+	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
+
+	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);
 
-cleanup:
 	rollback_lock_file(&lk);
 	return result;
 }

gitgitgadget

From: Derrick Stolee <[email protected]>

The existing mechanism for scheduling background maintenance is done through cron. The 'crontab -e' command allows updating the schedule while cron itself runs those commands. While this is technically supported by macOS, it has some significant deficiencies:

1. Every run of 'crontab -e' must request elevated privileges through the user interface. When running 'git maintenance start' from the Terminal app, it presents a dialog box saying "Terminal.app would like to administer your computer. Administration can include modifying passwords, networking, and system settings." This is more alarming than what we are hoping to achieve. If this alert had some information about how "git" is trying to run "crontab" then we would have some reason to believe that this dialog might be fine. However, it also doesn't help that some scenarios just leave Git waiting for a response without presenting anything to the user. I experienced this when executing the command from a Bash terminal view inside Visual Studio Code.

2. While cron initializes a user environment enough for "git config --global --show-origin" to show the correct config file information, it does not set up the environment enough for Git Credential Manager Core to load credentials during a 'prefetch' task. My prefetches against private repositories required re-authenticating through UI pop-ups in a way that should not be required.

The solution is to switch from cron to the Apple-recommended [1] 'launchd' tool.

[1] https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html

The basics of this tool is that we need to create XML-formatted "plist" files inside "~/Library/LaunchAgents/" and then use the 'launchctl' tool to make launchd aware of them. The plist files include all of the scheduling information, along with the command-line arguments split across an array of <string> tags.

For example, here is my plist file for the weekly scheduled tasks:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> <plist version="1.0"><dict>
<key>Label</key><string>org.git-scm.git.weekly</string>
<key>ProgramArguments</key>
<array>
<string>/usr/local/libexec/git-core/git</string> <string>--exec-path=/usr/local/libexec/git-core</string> <string>for-each-repo</string>
<string>--config=maintenance.repo</string>
<string>maintenance</string>
<string>run</string>
<string>--schedule=weekly</string>
</array>
<key>StartCalendarInterval</key>
<array>
<dict>
<key>Day</key><integer>0</integer>
<key>Hour</key><integer>0</integer>
<key>Minute</key><integer>0</integer>
</dict>
</array>
</dict>
</plist>

The schedules for the daily and hourly tasks are more complicated since we need to use an array for the StartCalendarInterval with an entry for each of the six days other than the 0th day (to avoid colliding with the weekly task), and each of the 23 hours other than the 0th hour (to avoid colliding with the daily task).

The "Label" value is currently filled with "org.git-scm.git.X" where X is the frequency. We need a different plist file for each frequency.

The launchctl command needs to be aligned with a user id in order to initialize the command environment. This must be done using the 'launchctl bootstrap' subcommand. This subcommand is new as 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].

[2] https://babodee.wordpress.com/2016/04/09/launchctl-2-0-syntax/

To remove a schedule, we must run 'launchctl bootout' with a valid plist file. We also need to 'bootout' a task before the 'bootstrap' subcommand will succeed, if such a task already exists.

We can verify the commands that were run by 'git maintenance start' and 'git maintenance stop' by injecting a script that writes the command-line arguments into GIT_TEST_CRONTAB.

---
 builtin/gc.c           | 209 +++++++++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh |  52 +++++++++-
 t/test-lib.sh          |   4 +
 3 files changed, 262 insertions(+), 3 deletions(-)

index c1f7d9bdc2..fa0ae63a80 100644
@@ -1491,6 +1491,214 @@ static int maintenance_unregister(void)
 	return run_command(&config_unset);
 }
 
+#if defined(__APPLE__)
+
+static char *get_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)
+{
+	char *expanded;
+	struct strbuf filename = STRBUF_INIT;
+	strbuf_addf(&filename, "~/Library/LaunchAgents/%s.plist", name);
+
+	expanded = expand_user_path(filename.buf, 1);
+	if (!expanded)
+		die(_("failed to expand path '%s'"), filename.buf);
+
+	strbuf_release(&filename);
+	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)
+{
+	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);
+}
+
+static int bootout(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";
+
+	strvec_split(&args, launchctl);
+	strvec_push(&args, "bootout");
+	strvec_pushf(&args, "gui/%s", uid);
+	strvec_push(&args, filename);
+
+	result = run_command_v_opt(args.v, 0);
+
+	strvec_clear(&args);
+	free(uid);
+	return result;
+}
+
+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";
+
+	strvec_split(&args, launchctl);
+	strvec_push(&args, "bootstrap");
+	strvec_pushf(&args, "gui/%s", uid);
+	strvec_push(&args, filename);
+
+	result = run_command_v_opt(args.v, 0);
+
+	strvec_clear(&args);
+	free(uid);
+	return result;
+}
+
+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);
+}
+
+static int schedule_plist(const char *exec_path, enum schedule_priority schedule)
+{
+	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);
+
+	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);
+
+	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);
+
+	switch (schedule) {
+	case SCHEDULE_HOURLY:
+		repeat = "<dict>\n"
+			 "<key>Hour</key><integer>%d</integer>\n"
+			 "<key>Minute</key><integer>0</integer>\n"
+			 "<dict>\n";
+		for (i = 1; i <= 23; i++)
+			fprintf(plist, repeat, i);
+		break;
+
+	case SCHEDULE_DAILY:
+		repeat = "<dict>\n"
+			 "<key>Day</key><integer>%d</integer>\n"
+			 "<key>Hour</key><integer>0</integer>\n"
+			 "<key>Minute</key><integer>0</integer>\n"
+			 "</dict>\n";
+		for (i = 1; i <= 6; i++)
+			fprintf(plist, repeat, i);
+		break;
+
+	case SCHEDULE_WEEKLY:
+		fprintf(plist,
+			"<dict>\n"
+			"<key>Day</key><integer>0</integer>\n"
+			"<key>Hour</key><integer>0</integer>\n"
+			"<key>Minute</key><integer>0</integer>\n"
+			"</dict>\n");
+		break;
+
+	default:
+		/* unreachable */
+		break;
+	}
+	fprintf(plist, "</array>\n</dict>\n</plist>\n");
+
+	/* bootout might fail if not already running, so ignore */
+	bootout(filename);
+	if (bootstrap(filename))
+		die(_("failed to bootstrap service %s"), filename);
+
+	fclose(plist);
+	free(filename);
+	free(name);
+	return 0;
+}
+
+static int add_plists(void)
+{
+	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);
+}
+
+static int platform_update_schedule(int run_maintenance, int fd)
+{
+	if (run_maintenance)
+		return add_plists();
+	else
+		return remove_plists();
+}
+#else
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
 #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
 

@@ -1585,6 +1793,7 @@ static int platform_update_schedule(int run_maintenance, int fd)
 		fclose(cron_list);
 	return result;
 }
+#endif
 
 static int update_background_schedule(int run_maintenance)
 {

index 4a60d1ed76..620ffbf3af 100644
@@ -1703,6 +1703,10 @@ 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

gitgitgadget

A few comments below (not necessarily worth a re-roll)...

It's not clear whether or not this is saying that git-maintenance will dynamically adapt to work with modern and older 'launchctl'. A glance at the actual code reveals that it knows only about modern 'bootstrap'. Perhaps this could be a bit clearer by saying that it only supports modern versions, and that support for older versions can be added later if needed. (For those of us who are stuck with 10-20 year old hardware and OS versions, 2015 isn't that long ago.)

Yes, this is a strange place to be. How far do we go back to support as many users as possible? How many users will be simultaneously stuck on an old version of macOS _and_ interested in updating to this latest version of Git? Is that worth the extra functionality to detect the the OS version and change commands?

The good news is that this patch doesn't lock us in to the boot(strap|out) subcommands too much. We could add in load/unload subcommands for systems that are too old. However, I did think it was prudent to take the currently-recommended option for fear that Apple will completely _delete_ the load/unload options in an upcoming release.

This makes me realize that I should update the documentation to give pointers for how to view the schedules for each platform:

- Windows: Open "Task Scheduler"
- macOS: 'launchctl list | grep org.git-scm.git'
- Others: 'crontab -l'

Good point. Or a simple boolean value for "add" or "remove".

You're right. In an earlier version I thought I needed to add the name in the XML, but it turns out I did not.

Thanks! I'll use that in several places and try to remember next time.

You're right, it does. It also returns with a non-zero exit code. Along with your later suggestion to clear the .plist files, we will want to have several conditions to not error out during a case where the task is not scheduled.

TIL. thanks.

Yeah, you're right. Thanks!

Yes, let's clear up these .plist files.

Thanks!
-Stolee

I don't think this patch series needs to answer these questions provided that it doesn't close the door to someone adding older-version support down the road. My review comment was more about the commit message being clearer about the choice -- supporting only recent 'launchctl' -- being made by this series. (And perhaps the documentation could mention that it requires a reasonably modern 'launchctl'.)

Good idea.

I haven't looked at the documentation, but if it doesn't already, I wonder if it should give examples of how to set these up by hand or how to customize the ones created by git-maintenance itself. I was also wondering if git-maintenance could have a mode in which it generates the template file(s) for you but doesn't actually activate/install it, instead providing instructions for activation/installation. That way, people could modify the scheduling file before actually activating it. However, this may all be outside the scope of the patch series, and could be done later if desired.

Thanks. I'll be sure to make the commit message more clear.

Outside of the technical details, the biggest questions I've tried to handle with the background maintenance feature has been to balance customization with ease-of-use. My philosophy is that users fall into a few expertise buckets, and have different expectations:

* Beginners will never know about background maintenance and so will never run "git maintenance start" or set the config values.

* Intermediate users might discover "git maintenance start" and will appreciate that they don't need to learn cron to set up a good default schedule.

* Advanced users will read the documentation and use Git config settings to customize their maintenance tasks and schedule.

* Expert users might decide the task schedule available by "git maintenance start" is too restrictive, so they create their own background schedule with custom tasks. They might not even run the maintenance builtin and opt instead for 'git gc' or 'git repack' directly!

My main target has been these "intermediate" users who might run the command and forget about it. However, I've also tried to keep the advanced users in mind with the advanced config options available.

Your comment about documentation demonstrates a way to serve the advanced and expert users by providing a clear framework for discovering what Git is doing under the hood and how to modify or adapt that to their needs. It is also important to demonstrate how to set up schedules in a similar way without having them be overwritten by a later "git maintenance start" command.

I will give this a shot in v2. Thanks.

-Stolee

As I go through the effort to remove get_service_name() I find that actually the name is used in one place in the XML file:

+ "<key>Label</key><string>%s</string>\n"

This "Label" should match the filename, I believe. I can still be more careful about how often this name is actually required.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

Git's background maintenance uses cron by default, but this is not available on Windows. Instead, integrate with Task Scheduler.

Tasks can be scheduled using the 'schtasks' command. There are several command-line options that can allow for some advanced scheduling, but unfortunately these seem to all require authenticating using a password.

Instead, use the "/xml" option to pass an XML file that contains the configuration for the necessary schedule. These XML files are based on some that I exported after constructing a schedule in the Task Scheduler GUI. These options only run background maintenance when the user is logged in, and more fields are populated with the current username and SID at run-time by 'schtasks'.

There is a deficiency in the current design. Windows has two kinds of
applications: GUI applications that start by "winmain()" and console applications that start by "main()". Console applications are attached to a new Console window if they are not already associated with a GUI application. This means that every hour the scheudled task launches a command window for the scheduled tasks. Not only is this visually obtrusive, but it also takes focus from whatever else the user is doing!

A simple fix would be to insert a GUI application that acts as a shim between the scheduled task and Git. This is currently possible in Git for Windows by setting the <Command> tag equal to

C:\Program Files\Git\git-bash.exe

with options "--hide --no-needs-console --command=cmd\git.exe" followed by the arguments currently used. Since git-bash.exe is not included in Windows builds of core Git, I chose to leave out this feature. My plan is to submit a small patch to Git for Windows that converts the use of git.exe with this use of git-bash.exe in the short term. In the long term, we can consider creating this GUI shim application within core Git, perhaps in contrib/.

---
 builtin/gc.c           | 181 +++++++++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh |  40 ++++++++-
 2 files changed, 218 insertions(+), 3 deletions(-)

index fa0ae63a80..24511fec2e 100644
@@ -1698,6 +1698,187 @@ static int platform_update_schedule(int run_maintenance, int fd)
 	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);
+	}
+}
+
+static char *get_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)
+{
+	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";
+
+	strvec_split(&args, schtasks);
+	strvec_pushl(&args, "/delete", "/tn", name, "/f", NULL);
+
+	result = run_command_v_opt(args.v, 0);
+
+	strvec_clear(&args);
+	free(name);
+	return result;
+}
+
+static int remove_scheduled_tasks(void)
+{
+	return remove_task(SCHEDULE_HOURLY) ||
+		remove_task(SCHEDULE_DAILY) ||
+		remove_task(SCHEDULE_WEEKLY);
+}
+
+static int schedule_task(const char *exec_path, enum schedule_priority schedule)
+{
+	int result;
+	struct strvec args = STRVEC_INIT;
+	const char *xml, *schtasks;
+	char *xmlpath;
+	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);
+
+	xml = "<?xml version=\"1.0\" encoding=\"UTF-16\"?>\n"
+	      "<Task version=\"1.4\" xmlns=\"http://schemas.microsoft.com/windows/2004/02/mit/task\">\n"
+	      "<Triggers>\n"
+	      "<CalendarTrigger>\n";
+	fprintf(xmlfp, xml);
+
+	switch (schedule) {
+	case SCHEDULE_HOURLY:
+		fprintf(xmlfp,
+			"<StartBoundary>2020-01-01T01:00:00</StartBoundary>\n"
+			"<Enabled>true</Enabled>\n"
+			"<ScheduleByDay>\n"
+			"<DaysInterval>1</DaysInterval>\n"
+			"</ScheduleByDay>\n"
+			"<Repetition>\n"
+			"<Interval>PT1H</Interval>\n"
+			"<Duration>PT23H</Duration>\n"
+			"<StopAtDurationEnd>false</StopAtDurationEnd>\n"
+			"</Repetition>\n");
+		break;
+
+	case SCHEDULE_DAILY:
+		fprintf(xmlfp,
+			"<StartBoundary>2020-01-01T00:00:00</StartBoundary>\n"
+			"<Enabled>true</Enabled>\n"
+			"<ScheduleByWeek>\n"
+			"<DaysOfWeek>\n"
+			"<Monday />\n"
+			"<Tuesday />\n"
+			"<Wednesday />\n"
+			"<Thursday />\n"
+			"<Friday />\n"
+			"<Saturday />\n"
+			"</DaysOfWeek>\n"
+			"<WeeksInterval>1</WeeksInterval>\n"
+			"</ScheduleByWeek>\n");
+		break;
+
+	case SCHEDULE_WEEKLY:
+		fprintf(xmlfp,
+			"<StartBoundary>2020-01-01T00:00:00</StartBoundary>\n"
+			"<Enabled>true</Enabled>\n"
+			"<ScheduleByWeek>\n"
+			"<DaysOfWeek>\n"
+			"<Sunday />\n"
+			"</DaysOfWeek>\n"
+			"<WeeksInterval>1</WeeksInterval>\n"
+			"</ScheduleByWeek>\n");
+		break;
+
+	default:
+		break;
+	}
+
+	xml=  "</CalendarTrigger>\n"
+	      "</Triggers>\n"
+	      "<Principals>\n"
+	      "<Principal id=\"Author\">\n"
+	      "<LogonType>InteractiveToken</LogonType>\n"
+	      "<RunLevel>LeastPrivilege</RunLevel>\n"
+	      "</Principal>\n"
+	      "</Principals>\n"
+	      "<Settings>\n"
+	      "<MultipleInstancesPolicy>IgnoreNew</MultipleInstancesPolicy>\n"
+	      "<Enabled>true</Enabled>\n"
+	      "<Hidden>true</Hidden>\n"
+	      "<UseUnifiedSchedulingEngine>true</UseUnifiedSchedulingEngine>\n"
+	      "<WakeToRun>false</WakeToRun>\n"
+	      "<ExecutionTimeLimit>PT72H</ExecutionTimeLimit>\n"
+	      "<Priority>7</Priority>\n"
+	      "</Settings>\n"
+	      "<Actions Context=\"Author\">\n"
+	      "<Exec>\n"
+	      "<Command>\"%s\\git.exe\"</Command>\n"
+	      "<Arguments>--exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
+	      "</Exec>\n"
+	      "</Actions>\n"
+	      "</Task>\n";
+	fprintf(xmlfp, xml, exec_path, exec_path, frequency);
+	fclose(xmlfp);
+
+	schtasks = getenv("GIT_TEST_CRONTAB");
+	if (!schtasks)
+		schtasks = "schtasks";
+	strvec_split(&args, schtasks);
+	strvec_pushl(&args, "/create", "/tn", name, "/f", "/xml", xmlpath, NULL);
+
+	result = run_command_v_opt(args.v, 0);
+
+	strvec_clear(&args);
+	unlink(xmlpath);
+	free(xmlpath);
+	free(name);
+	return result;
+}
+
+static int add_scheduled_tasks(void)
+{
+	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);
+}
+
+static int platform_update_schedule(int run_maintenance, int fd)
+{
+	if (run_maintenance)
+		return add_scheduled_tasks();
+	else
+		return remove_scheduled_tasks();
+}
+
 #else
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
 #define END_LINE "# END GIT MAINTENANCE SCHEDULE"

index f0210aa206..73dc0078da 100755
@@ -367,7 +367,7 @@ test_expect_success 'register and unregister' '
 	test_cmp before actual
 '
 
-test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'start from empty cron table' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 
 	# start registers the repo

@@ -378,7 +378,7 @@ test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' '
 	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
 '
 
-test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'stop from existing schedule' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
 
 	# stop does not unregister the repo

@@ -389,7 +389,7 @@ test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' '
 	test_must_be_empty cron.txt
 '
 
-test_expect_success !MACOS_MAINTENANCE 'start preserves existing schedule' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'start preserves existing schedule' '
 	echo "Important information!" >cron.txt &&
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 	grep "Important information!" cron.txt

@@ -441,6 +441,40 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
 	test_cmp expect args
 '
 
+test_expect_success MINGW 'start and stop Windows maintenance' '
+	echo "echo \$@ >>args" >print-args &&
+	chmod a+x print-args &&
+
+	rm -f args &&
+	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
+	cat args &&
+
+	# start registers the repo
+	git config --get --global maintenance.repo "$(pwd)" &&
+
+	rm expect &&
+	for frequency in hourly daily weekly
+	do
+		echo "/create /tn Git Maintenance ($frequency) /f /xml .git/objects/schedule-$frequency.xml" >>expect \
+			|| return 1
+	done &&
+	test_cmp expect args &&
+
+	rm -f args &&
+	GIT_TEST_CRONTAB="/bin/sh print-args"  git maintenance stop &&
+
+	# stop does not unregister the repo
+	git config --get --global maintenance.repo "$(pwd)" &&
+
+	rm expect &&
+	for frequency in hourly daily weekly
+	do
+		echo "/delete /tn Git Maintenance ($frequency) /f" >>expect \
+			|| return 1
+	done &&
+	test_cmp expect args
+'
+
 test_expect_success 'register preserves existing strategy' '
 	git config maintenance.strategy none &&
 	git maintenance register &&

gitgitgadget

I wonder if you could use the technique explained in [1] to prevent the console window from popping up.

[1]: https://pureinfotech.com/prevent-command-window-appearing-scheduled-tasks-windows-10/

    for frequency in hourly daily weekly
    do
        echo "/create ..." || return 1
    done >expect &&

    printf "/create /tn ... .git/objects/schedule-%s.xml\n" \
        hourly daily weekly >expect &&

The critical part of that strategy is the "Run whether the user is logged in or not". The resulting option that triggers causes the schtasks command to require a password prompt (or a password passed as a command-line argument). I found that interaction to be too disruptive.

Noted!

Yes. Thanks.

Excellent.

Thanks!
-Stolee

Nicely done.

Ah, I forgot to ask those on CC list how carefully they read the part 3 of the series, as it's been left on 'seen' for some time, and I do not know if it is ready to start cooking in 'next'.

Thanks.

It has been a while since anyone has commented, and I've been running the patches locally for quite a while. I'd be very happy to see them cook in next. I wasn't quite to the place to be pushy about it.

I'm hoping that parts 3 and 4 can make it in time for 2.30, so the feature is universally available for all platforms. I realize that's not entirely up to just me.

Thanks,
-Stolee

After writing this entry in the What's cooking report:

* ds/maintenance-part-4 (2020-11-03) 3 commits
- maintenance: use Windows scheduled tasks
- maintenance: use launchctl on macOS
- maintenance: extract platform-specific scheduling (this branch uses ds/maintenance-part-3.)

Follow-up on the "maintenance part-3" which introduced scheduled maintenance tasks to support platforms whose native scheduling methods are not 'cron'.

I was wondering if I should propose making these two parts into one, so we may be pretty much on the same page.

This is a good summary of this series.

I'm happy to have the two topics be merged into one series, but I'd prefer to only re-roll these three patches during review. Of course, by keeping them together we have the ability to re- roll all of the patches, but part-3 has a decent length and has been stable for a while.

I defer to what is easiest for you.

Thanks,
-Stolee

Two topics, one on top of the other, is fine, as long as I can remember (and you help me by reminding) to keep them together, and the bottom one is reasonably solid that I do not have to do the rebasing myself ;-)

Sounds good to me. If I need to re-roll part 3, then I'll consider replacing both parts with a new series.

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

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 | 119 +++++++++
 builtin/gc.c                      | 428 ++++++++++++++++++++++++++++--
 t/t7900-maintenance.sh            |  83 +++++-
 t/test-lib.sh                     |   4 +
 4 files changed, 614 insertions(+), 20 deletions(-)

base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-776%2Fderrickstolee%2Fmaintenance%2FmacOS-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-776/derrickstolee/maintenance/macOS-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/776

Range-diff vs v1:

 1:  d35f1aa162 = 1:  d35f1aa162 maintenance: extract platform-specific scheduling
 -:  ---------- > 2:  709a173720 maintenance: include 'cron' details in docs
 2:  832fdf1687 ! 3:  0fafd75d10 maintenance: use launchctl on macOS

@@ Commit message
and 'git maintenance stop' by injecting a script that writes the command-line arguments into GIT_TEST_CRONTAB.

+ An earlier version of this patch accidentally had an opening
+ "<dict>" tag when it should have had a closing "</dict>" tag. This
+ was caught during manual testing with actual 'launchctl' commands,
+ but we do not want to update developers' tasks when running tests.
+ It appears that macOS includes the "xmllint" tool which can verify
+ the XML format, so call it from the macOS-specific tests to ensure
+ the .plist files are well-formatted.
+
+ Helped-by: Eric Sunshine <[email protected]>

+ ## 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.
+
+
++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
++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.
++
++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
++org.git-scm.git.daily.plist
++org.git-scm.git.hourly.plist
++org.git-scm.git.weekly.plist
++-----------------------------------------------------------------------
++
++One task is registered for each --schedule=<frequency> option. To
++inspect how the XML format describes each schedule, open one of these
++.plist files in an editor and inspect the <array> element following
++the <key>StartCalendarInterval</key> element.
++
++git maintenance start will overwrite these files and register the
++tasks again with launchctl, so any customizations should be done by
++creating your own .plist files with distinct names. Similarly, the
++git maintenance stop command will unregister the tasks with launchctl
++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.
++
++
+ GIT
+ ---
+ Part of the linkgit:git[1] suite
+
## builtin/gc.c ##
@@ builtin/gc.c: static int maintenance_unregister(void)
return run_command(&config_unset);
@@ builtin/gc.c: static int maintenance_unregister(void)
+ repeat = "<dict>\n"
+ "<key>Hour</key><integer>%d</integer>\n"
+ "<key>Minute</key><integer>0</integer>\n"
-+ "<dict>\n";
++ "</dict>\n";
+ for (i = 1; i <= 23; i++)
+ fprintf(plist, repeat, i);
+ break;
@@ 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 &&
+ grep schedule=$frequency "$PLIST" &&
+ echo "bootout gui/$UID $PLIST" >>expect &&
+ echo "bootstrap gui/$UID $PLIST" >>expect || return 1
3: a9221cc4aa ! 4: 84eb44de31 maintenance: use Windows scheduled tasks

     @@ Commit message
          short term. In the long term, we can consider creating this GUI
          shim application within core Git, perhaps in contrib/.
      
     +    Helped-by: Eric Sunshine <[email protected]>
          Signed-off-by: Derrick Stolee <[email protected]>
      
     + ## Documentation/git-maintenance.txt ##
     +@@ Documentation/git-maintenance.txt: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSy
     + for more information.
     + 
     + 
     ++BACKGROUND MAINTENANCE ON WINDOWS SYSTEMS
     ++-----------------------------------------
     ++
     ++Windows does not support `cron` and instead has its own system for
     ++scheduling background tasks. The `git maintenance start` command uses
     ++the `schtasks` command to submit tasks to this system. You can inspect
     ++all background tasks using the Task Scheduler application. The tasks
     ++added by Git have names of the form `Git Maintenance (<frequency>)`.
     ++The Task Scheduler GUI has ways to inspect these tasks, but you can also
     ++export the tasks to XML files and view the details there.
     ++
     ++Note that since Git is a console application, these background tasks
     ++create a console window visible to the current user. This can be changed
     ++manually by selecting the "Run whether user is logged in or not" option
     ++in Task Scheduler. This change requires a password input, which is why
     ++`git maintenance start` does not select it by default.
     ++
     ++If you want to customize the background tasks, please rename the tasks
     ++so future calls to `git maintenance (start|stop)` do not overwrite your
     ++custom tasks.
     ++
     ++
     + GIT
     + ---
     + Part of the linkgit:git[1] suite
     +
       ## builtin/gc.c ##
      @@ builtin/gc.c: static int platform_update_schedule(int run_maintenance, int fd)
       	else
     @@ t/t7900-maintenance.sh: test_expect_success MACOS_MAINTENANCE 'start and stop ma
       '
       
      +test_expect_success MINGW 'start and stop Windows maintenance' '
     -+	echo "echo \$@ >>args" >print-args &&
     -+	chmod a+x print-args &&
     ++	write_script print-args <<-\EOF &&
     ++	echo $* >>args
     ++	EOF
      +
      +	rm -f args &&
      +	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
     -+	cat args &&
      +
      +	# start registers the repo
      +	git config --get --global maintenance.repo "$(pwd)" &&
      +
     -+	rm expect &&
      +	for frequency in hourly daily weekly
      +	do
     -+		echo "/create /tn Git Maintenance ($frequency) /f /xml .git/objects/schedule-$frequency.xml" >>expect \
     -+			|| return 1
     -+	done &&
     ++		printf "/create /tn Git Maintenance (%s) /f /xml .git/objects/schedule-%s.xml\n" \
     ++			$frequency $frequency
     ++	done >expect &&
      +	test_cmp expect args &&
      +
      +	rm -f args &&
     -+	GIT_TEST_CRONTAB="/bin/sh print-args"  git maintenance stop &&
     ++	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance stop &&
      +
      +	# stop does not unregister the repo
      +	git config --get --global maintenance.repo "$(pwd)" &&
      +
      +	rm expect &&
     -+	for frequency in hourly daily weekly
     -+	do
     -+		echo "/delete /tn Git Maintenance ($frequency) /f" >>expect \
     -+			|| return 1
     -+	done &&
     ++	printf "/delete /tn Git Maintenance (%s) /f\n" \
     ++		hourly daily weekly >expect &&
      +	test_cmp expect args
      +'
      +

gitgitgadget

From: Derrick Stolee <[email protected]>

The existing schedule mechanism using 'cron' is supported by POSIX platforms, but not Windows. It also works slightly differently on macOS to significant detriment of the user experience. To allow for new implementations on these platforms, extract a method that performs the platform-specific scheduling mechanism. This will be swapped at compile time with new implementations on specialized platforms.

---
 builtin/gc.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

index e3098ef6a1..c1f7d9bdc2 100644
@@ -1494,7 +1494,7 @@ static int maintenance_unregister(void)
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
 #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
 
-static int update_background_schedule(int run_maintenance)
+static int platform_update_schedule(int run_maintenance, int fd)
 {
 	int result = 0;
 	int in_old_region = 0;

@@ -1503,11 +1503,6 @@ static int update_background_schedule(int run_maintenance)
 	FILE *cron_list, *cron_in;
 	const char *crontab_name;
 	struct strbuf line = STRBUF_INIT;
-	struct lock_file lk;
-	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
-
-	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)
-		return error(_("another process is scheduling background maintenance"));
 
 	crontab_name = getenv("GIT_TEST_CRONTAB");
 	if (!crontab_name)

@@ -1516,12 +1511,11 @@ static int update_background_schedule(int run_maintenance)
 	strvec_split(&crontab_list.args, crontab_name);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
-	crontab_list.out = dup(lk.tempfile->fd);
+	crontab_list.out = dup(fd);
 	crontab_list.git_cmd = 0;
 
 	if (start_command(&crontab_list)) {
-		result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
-		goto cleanup;
+		return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
 	}
 
 	/* Ignore exit code, as an empty crontab will return error. */

@@ -1531,7 +1525,7 @@ static int update_background_schedule(int run_maintenance)
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
 	 */
-	cron_list = fdopen(lk.tempfile->fd, "r");
+	cron_list = fdopen(fd, "r");
 	rewind(cron_list);
 
 	strvec_split(&crontab_edit.args, crontab_name);

@@ -1539,8 +1533,7 @@ static int update_background_schedule(int run_maintenance)
 	crontab_edit.git_cmd = 0;
 
 	if (start_command(&crontab_edit)) {
-		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
-		goto cleanup;
+		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
 	}
 
 	cron_in = fdopen(crontab_edit.in, "w");

@@ -1586,13 +1579,24 @@ static int update_background_schedule(int run_maintenance)
 	close(crontab_edit.in);
 
 done_editing:
-	if (finish_command(&crontab_edit)) {
+	if (finish_command(&crontab_edit))
 		result = error(_("'crontab' died"));
-		goto cleanup;
-	}
-	fclose(cron_list);
+	else
+		fclose(cron_list);
+	return result;
+}
+
+static int update_background_schedule(int run_maintenance)
+{
+	int result;
+	struct lock_file lk;
+	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
+
+	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);
 
-cleanup:
 	rollback_lock_file(&lk);
 	return result;
 }

gitgitgadget

From: Derrick Stolee <[email protected]>

Advanced and expert users may want to know how 'git maintenance start' schedules background maintenance in order to customize their own schedules beyond what the maintenance.* config values allow. Start a new set of sections in git-maintenance.txt that describe how 'cron' is used to run these tasks.

This is particularly valuable for users who want to inspect what Git is doing or for users who want to customize the schedule further. Having a baseline can provide a way forward for users who have never worked with cron schedules.

---
 Documentation/git-maintenance.txt | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

index 6fec1eb8dc..4c7aac877d 100644
@@ -218,6 +218,60 @@ Further, the `git gc` command should not be combined with
 but does not take the lock in the same way as `git maintenance run`. If
 possible, use `git maintenance run --task=gc` instead of `git gc`.
 
+The following sections describe the mechanisms put in place to run
+background maintenance by `git maintenance start` and how to customize
+them.
+
+BACKGROUND MAINTENANCE ON POSIX SYSTEMS
+---------------------------------------
+
+The standard mechanism for scheduling background tasks on POSIX systems
+is `cron`. 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:
+
+-----------------------------------------------------------------------
+# BEGIN GIT MAINTENANCE SCHEDULE
+# The following schedule was created by Git
+# Any edits made in this region might be
+# replaced in the future by a Git command.
+
+0 1-23 * * * "/<path>/git" --exec-path="/<path>" for-each-repo --config=maintenance.repo maintenance run --schedule=hourly
+0 0 * * 1-6 "/<path>/git" --exec-path="/<path>" for-each-repo --config=maintenance.repo maintenance run --schedule=daily
+0 0 * * 0 "/<path>/git" --exec-path="/<path>" for-each-repo --config=maintenance.repo maintenance run --schedule=weekly
+
+# END GIT MAINTENANCE SCHEDULE
+-----------------------------------------------------------------------
+
+The comments are used as a region to mark the schedule as written by Git.
+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.
+
+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.
+
+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.
+
 
 GIT
 ---

gitgitgadget

A few comments below, not necessarily worth a re-roll...

From: Derrick Stolee <[email protected]>

The existing mechanism for scheduling background maintenance is done through cron. The 'crontab -e' command allows updating the schedule while cron itself runs those commands. While this is technically supported by macOS, it has some significant deficiencies:

1. Every run of 'crontab -e' must request elevated privileges through the user interface. When running 'git maintenance start' from the Terminal app, it presents a dialog box saying "Terminal.app would like to administer your computer. Administration can include modifying passwords, networking, and system settings." This is more alarming than what we are hoping to achieve. If this alert had some information about how "git" is trying to run "crontab" then we would have some reason to believe that this dialog might be fine. However, it also doesn't help that some scenarios just leave Git waiting for a response without presenting anything to the user. I experienced this when executing the command from a Bash terminal view inside Visual Studio Code.

2. While cron initializes a user environment enough for "git config --global --show-origin" to show the correct config file information, it does not set up the environment enough for Git Credential Manager Core to load credentials during a 'prefetch' task. My prefetches against private repositories required re-authenticating through UI pop-ups in a way that should not be required.

The solution is to switch from cron to the Apple-recommended [1] 'launchd' tool.

[1] https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html

The basics of this tool is that we need to create XML-formatted "plist" files inside "~/Library/LaunchAgents/" and then use the 'launchctl' tool to make launchd aware of them. The plist files include all of the scheduling information, along with the command-line arguments split across an array of <string> tags.

For example, here is my plist file for the weekly scheduled tasks:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> <plist version="1.0"><dict>
<key>Label</key><string>org.git-scm.git.weekly</string>
<key>ProgramArguments</key>
<array>
<string>/usr/local/libexec/git-core/git</string> <string>--exec-path=/usr/local/libexec/git-core</string> <string>for-each-repo</string>
<string>--config=maintenance.repo</string>
<string>maintenance</string>
<string>run</string>
<string>--schedule=weekly</string>
</array>
<key>StartCalendarInterval</key>
<array>
<dict>
<key>Day</key><integer>0</integer>
<key>Hour</key><integer>0</integer>
<key>Minute</key><integer>0</integer>
</dict>
</array>
</dict>
</plist>

The schedules for the daily and hourly tasks are more complicated since we need to use an array for the StartCalendarInterval with an entry for each of the six days other than the 0th day (to avoid colliding with the weekly task), and each of the 23 hours other than the 0th hour (to avoid colliding with the daily task).

The "Label" value is currently filled with "org.git-scm.git.X" where X is the frequency. We need a different plist file for each frequency.

The launchctl command needs to be aligned with a user id in order to initialize the command environment. This must be done using the 'launchctl bootstrap' subcommand. This subcommand is new as 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].

[2] https://babodee.wordpress.com/2016/04/09/launchctl-2-0-syntax/

To remove a schedule, we must run 'launchctl bootout' with a valid plist file. We also need to 'bootout' a task before the 'bootstrap' subcommand will succeed, if such a task already exists.

We can verify the commands that were run by 'git maintenance start' and 'git maintenance stop' by injecting a script that writes the command-line arguments into GIT_TEST_CRONTAB.

An earlier version of this patch accidentally had an opening "<dict>" tag when it should have had a closing "</dict>" tag. This was caught during manual testing with actual 'launchctl' commands, but we do not want to update developers' tasks when running tests. It appears that macOS includes the "xmllint" tool which can verify the XML format, so call it from the macOS-specific tests to ensure the .plist files are well-formatted.

---
 Documentation/git-maintenance.txt |  43 ++++++
 builtin/gc.c                      | 209 ++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            |  53 +++++++-
 t/test-lib.sh                     |   4 +
 4 files changed, 306 insertions(+), 3 deletions(-)

index 4c7aac877d..451ebac131 100644
@@ -273,6 +273,49 @@ for advanced scheduling techniques. Please do use the full path and
 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
+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.
+
+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
+org.git-scm.git.daily.plist
+org.git-scm.git.hourly.plist
+org.git-scm.git.weekly.plist
+-----------------------------------------------------------------------
+
+One task is registered for each `--schedule=<frequency>` option. To
+inspect how the XML format describes each schedule, open one of these
+`.plist` files in an editor and inspect the `<array>` element following
+the `<key>StartCalendarInterval</key>` element.
+
+`git maintenance start` will overwrite these files and register the
+tasks again with `launchctl`, so any customizations should be done by
+creating your own `.plist` files with distinct names. Similarly, the
+`git maintenance stop` command will unregister the tasks with `launchctl`
+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.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 4a60d1ed76..620ffbf3af 100644
@@ -1703,6 +1703,10 @@ 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

gitgitgadget

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.

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

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.

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

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

xmllint's --noout option should do the trick.

From: Derrick Stolee <[email protected]>

Git's background maintenance uses cron by default, but this is not available on Windows. Instead, integrate with Task Scheduler.

Tasks can be scheduled using the 'schtasks' command. There are several command-line options that can allow for some advanced scheduling, but unfortunately these seem to all require authenticating using a password.

Instead, use the "/xml" option to pass an XML file that contains the configuration for the necessary schedule. These XML files are based on some that I exported after constructing a schedule in the Task Scheduler GUI. These options only run background maintenance when the user is logged in, and more fields are populated with the current username and SID at run-time by 'schtasks'.

There is a deficiency in the current design. Windows has two kinds of
applications: GUI applications that start by "winmain()" and console applications that start by "main()". Console applications are attached to a new Console window if they are not already associated with a GUI application. This means that every hour the scheudled task launches a command window for the scheduled tasks. Not only is this visually obtrusive, but it also takes focus from whatever else the user is doing!

A simple fix would be to insert a GUI application that acts as a shim between the scheduled task and Git. This is currently possible in Git for Windows by setting the <Command> tag equal to

C:\Program Files\Git\git-bash.exe

with options "--hide --no-needs-console --command=cmd\git.exe" followed by the arguments currently used. Since git-bash.exe is not included in Windows builds of core Git, I chose to leave out this feature. My plan is to submit a small patch to Git for Windows that converts the use of git.exe with this use of git-bash.exe in the short term. In the long term, we can consider creating this GUI shim application within core Git, perhaps in contrib/.

---
 Documentation/git-maintenance.txt |  22 ++++
 builtin/gc.c                      | 181 ++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            |  36 +++++-
 3 files changed, 236 insertions(+), 3 deletions(-)

index 451ebac131..f4f6a4091b 100644
@@ -316,6 +316,28 @@ https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSy
 for more information.
 
 
+BACKGROUND MAINTENANCE ON WINDOWS SYSTEMS
+-----------------------------------------
+
+Windows does not support `cron` and instead has its own system for
+scheduling background tasks. The `git maintenance start` command uses
+the `schtasks` command to submit tasks to this system. You can inspect
+all background tasks using the Task Scheduler application. The tasks
+added by Git have names of the form `Git Maintenance (<frequency>)`.
+The Task Scheduler GUI has ways to inspect these tasks, but you can also
+export the tasks to XML files and view the details there.
+
+Note that since Git is a console application, these background tasks
+create a console window visible to the current user. This can be changed
+manually by selecting the "Run whether user is logged in or not" option
+in Task Scheduler. This change requires a password input, which is why
+`git maintenance start` does not select it by default.
+
+If you want to customize the background tasks, please rename the tasks
+so future calls to `git maintenance (start|stop)` do not overwrite your
+custom tasks.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 1c43b34a93..e7ad130cbc 100755
@@ -367,7 +367,7 @@ test_expect_success 'register and unregister' '
 	test_cmp before actual
 '
 
-test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'start from empty cron table' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 
 	# start registers the repo

@@ -378,7 +378,7 @@ test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' '
 	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
 '
 
-test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'stop from existing schedule' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
 
 	# stop does not unregister the repo

@@ -389,7 +389,7 @@ test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' '
 	test_must_be_empty cron.txt
 '
 
-test_expect_success !MACOS_MAINTENANCE 'start preserves existing schedule' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'start preserves existing schedule' '
 	echo "Important information!" >cron.txt &&
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 	grep "Important information!" cron.txt

@@ -442,6 +442,36 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
 	test_cmp expect args
 '
 
+test_expect_success MINGW 'start and stop Windows maintenance' '
+	write_script print-args <<-\EOF &&
+	echo $* >>args
+	EOF
+
+	rm -f args &&
+	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
+
+	# 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 &&
+	test_cmp expect args &&
+
+	rm -f args &&
+	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance stop &&
+
+	# stop does not unregister the repo
+	git config --get --global maintenance.repo "$(pwd)" &&
+
+	rm expect &&
+	printf "/delete /tn Git Maintenance (%s) /f\n" \
+		hourly daily weekly >expect &&
+	test_cmp expect args
+'
+
 test_expect_success 'register preserves existing strategy' '
 	git config maintenance.strategy none &&
 	git maintenance register &&

gitgitgadget

A temp directory is a good idea.

It's sufficient, and avoids issues with deep tabbing and chaining "|| return 1"".

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: [email protected] [[email protected]], [email protected] [[email protected]], [email protected] [[email protected]]cc:
Derrick Stolee [email protected] [[email protected]]cc: Đoàn Trần Công Danh [email protected] [[email protected]]cc: Martin Ågren [email protected] [[email protected]]cc: Eric Sunshine [email protected] [[email protected]]cc: Derrick Stolee [email protected] [[email protected]]

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 <[email protected]>

## 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 <[email protected]>
      
       ## 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 <[email protected]>
      
       ## 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

From: Derrick Stolee <[email protected]>

The existing schedule mechanism using 'cron' is supported by POSIX platforms, but not Windows. It also works slightly differently on macOS to significant detriment of the user experience. To allow for new implementations on these platforms, extract a method that performs the platform-specific scheduling mechanism. This will be swapped at compile time with new implementations on specialized platforms.

---
 builtin/gc.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

index e3098ef6a1..c1f7d9bdc2 100644
@@ -1494,7 +1494,7 @@ static int maintenance_unregister(void)
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
 #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
 
-static int update_background_schedule(int run_maintenance)
+static int platform_update_schedule(int run_maintenance, int fd)
 {
 	int result = 0;
 	int in_old_region = 0;

@@ -1503,11 +1503,6 @@ static int update_background_schedule(int run_maintenance)
 	FILE *cron_list, *cron_in;
 	const char *crontab_name;
 	struct strbuf line = STRBUF_INIT;
-	struct lock_file lk;
-	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
-
-	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)
-		return error(_("another process is scheduling background maintenance"));
 
 	crontab_name = getenv("GIT_TEST_CRONTAB");
 	if (!crontab_name)

@@ -1516,12 +1511,11 @@ static int update_background_schedule(int run_maintenance)
 	strvec_split(&crontab_list.args, crontab_name);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
-	crontab_list.out = dup(lk.tempfile->fd);
+	crontab_list.out = dup(fd);
 	crontab_list.git_cmd = 0;
 
 	if (start_command(&crontab_list)) {
-		result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
-		goto cleanup;
+		return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
 	}
 
 	/* Ignore exit code, as an empty crontab will return error. */

@@ -1531,7 +1525,7 @@ static int update_background_schedule(int run_maintenance)
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
 	 */
-	cron_list = fdopen(lk.tempfile->fd, "r");
+	cron_list = fdopen(fd, "r");
 	rewind(cron_list);
 
 	strvec_split(&crontab_edit.args, crontab_name);

@@ -1539,8 +1533,7 @@ static int update_background_schedule(int run_maintenance)
 	crontab_edit.git_cmd = 0;
 
 	if (start_command(&crontab_edit)) {
-		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
-		goto cleanup;
+		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
 	}
 
 	cron_in = fdopen(crontab_edit.in, "w");

@@ -1586,13 +1579,24 @@ static int update_background_schedule(int run_maintenance)
 	close(crontab_edit.in);
 
 done_editing:
-	if (finish_command(&crontab_edit)) {
+	if (finish_command(&crontab_edit))
 		result = error(_("'crontab' died"));
-		goto cleanup;
-	}
-	fclose(cron_list);
+	else
+		fclose(cron_list);
+	return result;
+}
+
+static int update_background_schedule(int run_maintenance)
+{
+	int result;
+	struct lock_file lk;
+	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
+
+	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);
 
-cleanup:
 	rollback_lock_file(&lk);
 	return result;
 }

gitgitgadget

From: Derrick Stolee <[email protected]>

Advanced and expert users may want to know how 'git maintenance start' schedules background maintenance in order to customize their own schedules beyond what the maintenance.* config values allow. Start a new set of sections in git-maintenance.txt that describe how 'cron' is used to run these tasks.

This is particularly valuable for users who want to inspect what Git is doing or for users who want to customize the schedule further. Having a baseline can provide a way forward for users who have never worked with cron schedules.

---
 Documentation/git-maintenance.txt | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

index 6fec1eb8dc..1aa1112418 100644
@@ -218,6 +218,60 @@ Further, the `git gc` command should not be combined with
 but does not take the lock in the same way as `git maintenance run`. If
 possible, use `git maintenance run --task=gc` instead of `git gc`.
 
+The following sections describe the mechanisms put in place to run
+background maintenance by `git maintenance start` and how to customize
+them.
+
+BACKGROUND MAINTENANCE ON POSIX SYSTEMS
+---------------------------------------
+
+The standard mechanism for scheduling background tasks on POSIX systems
+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:
+
+-----------------------------------------------------------------------
+# BEGIN GIT MAINTENANCE SCHEDULE
+# The following schedule was created by Git
+# Any edits made in this region might be
+# replaced in the future by a Git command.
+
+0 1-23 * * * "/<path>/git" --exec-path="/<path>" for-each-repo --config=maintenance.repo maintenance run --schedule=hourly
+0 0 * * 1-6 "/<path>/git" --exec-path="/<path>" for-each-repo --config=maintenance.repo maintenance run --schedule=daily
+0 0 * * 0 "/<path>/git" --exec-path="/<path>" for-each-repo --config=maintenance.repo maintenance run --schedule=weekly
+
+# END GIT MAINTENANCE SCHEDULE
+-----------------------------------------------------------------------
+
+The comments are used as a region to mark the schedule as written by Git.
+Any modifications within this region will be completely deleted by
+`git maintenance stop` or overwritten by `git maintenance start`.
+
+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. 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
+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
 ---

gitgitgadget

From: Derrick Stolee <[email protected]>

The existing mechanism for scheduling background maintenance is done through cron. The 'crontab -e' command allows updating the schedule while cron itself runs those commands. While this is technically supported by macOS, it has some significant deficiencies:

1. Every run of 'crontab -e' must request elevated privileges through the user interface. When running 'git maintenance start' from the Terminal app, it presents a dialog box saying "Terminal.app would like to administer your computer. Administration can include modifying passwords, networking, and system settings." This is more alarming than what we are hoping to achieve. If this alert had some information about how "git" is trying to run "crontab" then we would have some reason to believe that this dialog might be fine. However, it also doesn't help that some scenarios just leave Git waiting for a response without presenting anything to the user. I experienced this when executing the command from a Bash terminal view inside Visual Studio Code.

2. While cron initializes a user environment enough for "git config --global --show-origin" to show the correct config file information, it does not set up the environment enough for Git Credential Manager Core to load credentials during a 'prefetch' task. My prefetches against private repositories required re-authenticating through UI pop-ups in a way that should not be required.

The solution is to switch from cron to the Apple-recommended [1] 'launchd' tool.

[1] https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html

The basics of this tool is that we need to create XML-formatted "plist" files inside "~/Library/LaunchAgents/" and then use the 'launchctl' tool to make launchd aware of them. The plist files include all of the scheduling information, along with the command-line arguments split across an array of <string> tags.

For example, here is my plist file for the weekly scheduled tasks:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> <plist version="1.0"><dict>
<key>Label</key><string>org.git-scm.git.weekly</string>
<key>ProgramArguments</key>
<array>
<string>/usr/local/libexec/git-core/git</string> <string>--exec-path=/usr/local/libexec/git-core</string> <string>for-each-repo</string>
<string>--config=maintenance.repo</string>
<string>maintenance</string>
<string>run</string>
<string>--schedule=weekly</string>
</array>
<key>StartCalendarInterval</key>
<array>
<dict>
<key>Day</key><integer>0</integer>
<key>Hour</key><integer>0</integer>
<key>Minute</key><integer>0</integer>
</dict>
</array>
</dict>
</plist>

The schedules for the daily and hourly tasks are more complicated since we need to use an array for the StartCalendarInterval with an entry for each of the six days other than the 0th day (to avoid colliding with the weekly task), and each of the 23 hours other than the 0th hour (to avoid colliding with the daily task).

The "Label" value is currently filled with "org.git-scm.git.X" where X is the frequency. We need a different plist file for each frequency.

The launchctl command needs to be aligned with a user id in order to initialize the command environment. This must be done using the 'launchctl bootstrap' subcommand. This subcommand is new as 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]. 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/

To remove a schedule, we must run 'launchctl bootout' with a valid plist file. We also need to 'bootout' a task before the 'bootstrap' subcommand will succeed, if such a task already exists.

We can verify the commands that were run by 'git maintenance start' and 'git maintenance stop' by injecting a script that writes the command-line arguments into GIT_TEST_CRONTAB.

An earlier version of this patch accidentally had an opening "<dict>" tag when it should have had a closing "</dict>" tag. This was caught during manual testing with actual 'launchctl' commands, but we do not want to update developers' tasks when running tests. It appears that macOS includes the "xmllint" tool which can verify the XML format, so call it from the macOS-specific tests to ensure the .plist files are well-formatted.

---
 Documentation/git-maintenance.txt |  40 ++++++
 builtin/gc.c                      | 195 ++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            |  48 +++++++-
 t/test-lib.sh                     |   4 +
 4 files changed, 284 insertions(+), 3 deletions(-)

index 1aa1112418..5f8f63f098 100644
@@ -273,6 +273,46 @@ 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 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 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/org.git-scm.git*
+org.git-scm.git.daily.plist
+org.git-scm.git.hourly.plist
+org.git-scm.git.weekly.plist
+-----------------------------------------------------------------------
+
+One task is registered for each `--schedule=<frequency>` option. To
+inspect how the XML format describes each schedule, open one of these
+`.plist` files in an editor and inspect the `<array>` element following
+the `<key>StartCalendarInterval</key>` element.
+
+`git maintenance start` will overwrite these files and register the
+tasks again with `launchctl`, so any customizations should be done by
+creating your own `.plist` files with distinct names. Similarly, the
+`git maintenance stop` command will unregister the tasks with `launchctl`
+and delete the `.plist` files.
+
+To create more advanced customizations to your background tasks, see
+launchctl.plist(5) for more information.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 4a60d1ed76..620ffbf3af 100644
@@ -1703,6 +1703,10 @@ 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

gitgitgadget

We would also want to pass a "die_on_failure" into the method, since in the 'git maintenance start' case we don't want to report a failure when 'launchctl bootout' fails before we call 'launchctl bootstrap'.

This die() is only if the process cannot _start_, for example due to launchctl not existing on $PATH. The result from finish_command() would be non-zero when we bootout a plist that doesn't exist.

Ah. Sorry for misunderstanding. That explains why it works this way on macOS but it did _not_ work that way on Windows.

Sorry, I missed this comment from v1 when reapplying the changes for v3.

Thank you for your continued attention and patience. -Stolee

Right. I started writing that we'd also need a die_one_failure flag but deleted the comment since I decided to wait until I got an answer...

... to this question.

Another thought I had was simply checking for the presence of the file and skipping bootout altogether if it doesn't exist. That would, I think, obviate the need for mucking with stdout/stderr oppression.

Sorry on my part too. I missed the args redirect in my example. It should be:

write_script print-args <<-\EOF
echo $* >args
EOF

Erm, s/oppression/suppression/.

From: Derrick Stolee <[email protected]>

Git's background maintenance uses cron by default, but this is not available on Windows. Instead, integrate with Task Scheduler.

Tasks can be scheduled using the 'schtasks' command. There are several command-line options that can allow for some advanced scheduling, but unfortunately these seem to all require authenticating using a password.

Instead, use the "/xml" option to pass an XML file that contains the configuration for the necessary schedule. These XML files are based on some that I exported after constructing a schedule in the Task Scheduler GUI. These options only run background maintenance when the user is logged in, and more fields are populated with the current username and SID at run-time by 'schtasks'.

There is a deficiency in the current design. Windows has two kinds of
applications: GUI applications that start by "winmain()" and console applications that start by "main()". Console applications are attached to a new Console window if they are not already associated with a GUI application. This means that every hour the scheudled task launches a command window for the scheduled tasks. Not only is this visually obtrusive, but it also takes focus from whatever else the user is doing!

A simple fix would be to insert a GUI application that acts as a shim between the scheduled task and Git. This is currently possible in Git for Windows by setting the <Command> tag equal to

C:\Program Files\Git\git-bash.exe

with options "--hide --no-needs-console --command=cmd\git.exe" followed by the arguments currently used. Since git-bash.exe is not included in Windows builds of core Git, I chose to leave out this feature. My plan is to submit a small patch to Git for Windows that converts the use of git.exe with this use of git-bash.exe in the short term. In the long term, we can consider creating this GUI shim application within core Git, perhaps in contrib/.

---
 Documentation/git-maintenance.txt |  22 ++++
 builtin/gc.c                      | 184 ++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            |  33 +++++-
 3 files changed, 236 insertions(+), 3 deletions(-)

index 5f8f63f098..6970f2b898 100644
@@ -313,6 +313,28 @@ To create more advanced customizations to your background tasks, see
 launchctl.plist(5) for more information.
 
 
+BACKGROUND MAINTENANCE ON WINDOWS SYSTEMS
+-----------------------------------------
+
+Windows does not support `cron` and instead has its own system for
+scheduling background tasks. The `git maintenance start` command uses
+the `schtasks` command to submit tasks to this system. You can inspect
+all background tasks using the Task Scheduler application. The tasks
+added by Git have names of the form `Git Maintenance (<frequency>)`.
+The Task Scheduler GUI has ways to inspect these tasks, but you can also
+export the tasks to XML files and view the details there.
+
+Note that since Git is a console application, these background tasks
+create a console window visible to the current user. This can be changed
+manually by selecting the "Run whether user is logged in or not" option
+in Task Scheduler. This change requires a password input, which is why
+`git maintenance start` does not select it by default.
+
+If you want to customize the background tasks, please rename the tasks
+so future calls to `git maintenance (start|stop)` do not overwrite your
+custom tasks.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 29d340a828..0dc2479117 100755
@@ -367,7 +367,7 @@ test_expect_success 'register and unregister' '
 	test_cmp before actual
 '
 
-test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'start from empty cron table' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 
 	# start registers the repo

@@ -378,7 +378,7 @@ test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' '
 	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
 '
 
-test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'stop from existing schedule' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
 
 	# stop does not unregister the repo

@@ -389,7 +389,7 @@ test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' '
 	test_must_be_empty cron.txt
 '
 
-test_expect_success !MACOS_MAINTENANCE 'start preserves existing schedule' '
+test_expect_success !MACOS_MAINTENANCE,!MINGW 'start preserves existing schedule' '
 	echo "Important information!" >cron.txt &&
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 	grep "Important information!" cron.txt

@@ -437,6 +437,33 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
 	test_line_count = 0 actual
 '
 
+test_expect_success MINGW 'start and stop Windows maintenance' '
+	write_script print-args <<-\EOF &&
+	echo $* >>args
+	EOF
+
+	rm -f args &&
+	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
+
+	# start registers the repo
+	git config --get --global maintenance.repo "$(pwd)" &&
+
+	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 &&
+	GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance stop &&
+
+	# stop does not unregister the repo
+	git config --get --global maintenance.repo "$(pwd)" &&
+
+	rm expect &&
+	printf "/delete /tn Git Maintenance (%s) /f\n" \
+		hourly daily weekly >expect &&
+	test_cmp expect args
+'
+
 test_expect_success 'register preserves existing strategy' '
 	git config maintenance.strategy none &&
 	git maintenance register &&

gitgitgadget

While I do like to have access to the data when trying to resolve an issue, it's probably better to use the tempfile library.

Since we execute the GIT_TEST_CRONTAB executable multiple times, we need to use >> to log all three instances (and their order). Using ">args" would only capture the final call for the weekly schedule.

On macOS, there are as many as six calls (three bootouts, three bootstraps).

I landed on this after trying several attempts to get this to work, including "$(pwd)/print-args" and I'm not sure why it doesn't work in the Windows case. It is something to do with how I am executing the subcommand from within Git. I'm pretty sure this idea of "mocking" an executable through Git is relatively new, or at least rare

Thanks,
-Stolee

Makes sense. Thanks.

Just for clarification... You mentioned in response to my [3/4] review that your accidentally-working write_script() only worked as expected on Mac but not on Windows. When you arrived at this solution of GIT_TEST_CRONTAB="/bin/sh ..." here, was that before or after you fixed write_script() to take the script body from stdin?

You're right. That was necessary only for the old way that I was creating the script. The correct way works with GIT_TEST_CRONTAB equal to ./print-args.

-Stolee

I forgot to mention a couple things when reviewing the patches individually, so I'll point them out here...

s/fatures/features/

These days, we usually say:

test_must_be_empty actual

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 <[email protected]>" 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 <[email protected]>
Date: Sat, 14 Nov 2020 02:39:05 -0500
Subject: [PATCH] maintenance: test start/stop on all platforms from any
platform

---
 builtin/gc.c           | 204 +++++++++++++++++++----------------------
 t/t7900-maintenance.sh |  66 +++++++++----
 t/test-lib.sh          |   4 -
 3 files changed, 143 insertions(+), 131 deletions(-)

index 76a3afa20a..955d4b3baf 100644
@@ -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;
 }
 

index 620ffbf3af..4a60d1ed76 100644
@@ -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

2.29.2.454.gaff20da3a2

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 V4
=============

* Eric did an excellent job providing a patch that cleans up several parts of my series. The most impressive is his mechanism for testing the platform-specific Git logic in a way that is (mostly) platform-agnostic.

* Windows doesn't have the 'id' command, so we cannot run the macOS platform test on Windows.

* I noticed far too late that while my example XML files had been edited with UTF-8 encoding, Git is actually writing them as US-ASCII. Somehow xmllint and launchd are not complaining, but schtasks does complain. Unfortunately, I cannot find a way to catch this problem other than to install my tip version on all three platforms and go through the entire git maintenance start process, and double-check that the processes are running on the hour.

Here is a diff from the tip of v3 + Eric's patch to the tip of v4:

index 955d4b3baf..1a3725429c 100644
@@ -1642,13 +1642,13 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
         break;
     }
     fprintf(plist, "</array>\n</dict>\n</plist>\n");
+    fclose(plist);
 
     /* bootout might fail if not already running, so ignore */
     launchctl_boot_plist(0, filename, cmd);
     if (launchctl_boot_plist(1, filename, cmd))
         die(_("failed to bootstrap service %s"), filename);
 
-    fclose(plist);
     free(filename);
     free(name);
     return 0;

@@ -1707,25 +1707,27 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
     int result;
     struct child_process child = CHILD_PROCESS_INIT;
     const char *xml;
-    char *xmlpath, *tempDir;
-    FILE *xmlfp;
+    char *xmlpath;
+    struct tempfile *tfile;
     const char *frequency = get_frequency(schedule);
     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");
+    xmlpath =  xstrfmt("%s/schedule-%s.xml",
+               the_repository->objects->odb->path,
+               frequency);
+    tfile = create_tempfile(xmlpath);
+    if (!tfile || !fdopen_tempfile(tfile, "w"))
+        die(_("failed to create '%s'"), xmlpath);
 
-    xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+    xml = "<?xml version=\"1.0\" encoding=\"US-ASCII\"?>\n"
           "<Task version=\"1.4\" xmlns=\"http://schemas.microsoft.com/windows/2004/02/mit/task\">\n"
           "<Triggers>\n"
           "<CalendarTrigger>\n";
-    fputs(xml, xmlfp);
+    fputs(xml, tfile->fp);
 
     switch (schedule) {
     case SCHEDULE_HOURLY:
-        fprintf(xmlfp,
+        fprintf(tfile->fp,
             "<StartBoundary>2020-01-01T01:00:00</StartBoundary>\n"
             "<Enabled>true</Enabled>\n"
             "<ScheduleByDay>\n"

@@ -1739,7 +1741,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
         break;
 
     case SCHEDULE_DAILY:
-        fprintf(xmlfp,
+        fprintf(tfile->fp,
             "<StartBoundary>2020-01-01T00:00:00</StartBoundary>\n"
             "<Enabled>true</Enabled>\n"
             "<ScheduleByWeek>\n"

@@ -1756,7 +1758,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
         break;
 
     case SCHEDULE_WEEKLY:
-        fprintf(xmlfp,
+        fprintf(tfile->fp,
             "<StartBoundary>2020-01-01T00:00:00</StartBoundary>\n"
             "<Enabled>true</Enabled>\n"
             "<ScheduleByWeek>\n"