[v4,4/4] maintenance: use Windows scheduled tasks

Commit Message

Derrick Stolee Nov. 17, 2020, 9:13 p.m. UTC

From: Derrick Stolee <[e-mail]>

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

Since the GIT_TEST_MAINT_SCHEDULER environment variable allows us to
specify 'schtasks' as the scheduler, we can test the Windows-specific
logic on a macOS platform. Thus, add a check that the XML file written
by Git is valid when xmllint exists on the system.

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

Co-authored-by: Eric Sunshine <[e-mail]>
Signed-off-by: Eric Sunshine <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/git-maintenance.txt |  22 ++++
 builtin/gc.c                      | 165 ++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            |  40 ++++++++
 3 files changed, 227 insertions(+)

Comments

Eric Sunshine Nov. 18, 2020, 7:15 a.m. UTC | #1

On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> [...]
> Since the GIT_TEST_MAINT_SCHEDULER environment variable allows us to
> specify 'schtasks' as the scheduler, we can test the Windows-specific
> logic on a macOS platform. Thus, add a check that the XML file written
> by Git is valid when xmllint exists on the system.

Nit: xmllint can be installed on Linux (and likely other platforms),
as well, so it's not clear why this calls out macOS specially. More
generally, it may not be important to call out xmllint at all in the
commit message; it's just _one_ thing being checked by a test which is
checking several other things not called out individually by the
commit message. Anyhow, this is minor; not worth a re-roll.

> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -1671,6 +1671,167 @@ static int launchctl_update_schedule(int run_maintenance, int fd, const char *cm
> +static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule, const char *cmd)
> +{
> +       xmlpath =  xstrfmt("%s/schedule-%s.xml",
> +                          the_repository->objects->odb->path,
> +                          frequency);

I missed this in the earlier rounds since I wasn't paying close enough
attention, but placing this XML file within the object database
directory (.git/objects/) feels rather odd, even if it is just a
temporary file. Using the .git/ directory itself might be better,
perhaps like this:

    struct strbuf xmlpath = STRBUF_INIT;
    strbuf_git_common_path(&xmlpath, the_repository,
        "schtasks-%s.xml", frequency);
    ...
    strbuf_release(&xmlpath);

Derrick Stolee Nov. 18, 2020, 6:30 p.m. UTC | #2

On 11/18/2020 2:15 AM, Eric Sunshine wrote:
> On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>> [...]
>> Since the GIT_TEST_MAINT_SCHEDULER environment variable allows us to
>> specify 'schtasks' as the scheduler, we can test the Windows-specific
>> logic on a macOS platform. Thus, add a check that the XML file written
>> by Git is valid when xmllint exists on the system.
> 
> Nit: xmllint can be installed on Linux (and likely other platforms),
> as well, so it's not clear why this calls out macOS specially. More
> generally, it may not be important to call out xmllint at all in the
> commit message; it's just _one_ thing being checked by a test which is
> checking several other things not called out individually by the
> commit message. Anyhow, this is minor; not worth a re-roll.

Sorry, it should just say "other platforms"

>> diff --git a/builtin/gc.c b/builtin/gc.c
>> @@ -1671,6 +1671,167 @@ static int launchctl_update_schedule(int run_maintenance, int fd, const char *cm
>> +static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule, const char *cmd)
>> +{
>> +       xmlpath =  xstrfmt("%s/schedule-%s.xml",
>> +                          the_repository->objects->odb->path,
>> +                          frequency);
> 
> I missed this in the earlier rounds since I wasn't paying close enough
> attention, but placing this XML file within the object database
> directory (.git/objects/) feels rather odd, even if it is just a
> temporary file. Using the .git/ directory itself might be better,
> perhaps like this:
> 
>     struct strbuf xmlpath = STRBUF_INIT;
>     strbuf_git_common_path(&xmlpath, the_repository,
>         "schtasks-%s.xml", frequency);
>     ...
>     strbuf_release(&xmlpath);

It does look odd, and in this case we could use the .git directory
instead. I specifically use the objects directory for the maintenance
lock in 'git maintenance run' to allow maintenance to run when
GIT_OBJECT_DIRECTORY points to an alternate, allowing us to maintain
object databases that don't have a full .git directory around them.

Thanks,
-Stolee

Eric Sunshine Nov. 18, 2020, 8:54 p.m. UTC | #3

On Wed, Nov 18, 2020 at 1:30 PM Derrick Stolee <[e-mail]> wrote:
> On 11/18/2020 2:15 AM, Eric Sunshine wrote:
> > On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
> > <[e-mail]> wrote:
> >> +       xmlpath =  xstrfmt("%s/schedule-%s.xml",
> >> +                          the_repository->objects->odb->path,
> >> +                          frequency);
> >
> > I missed this in the earlier rounds since I wasn't paying close enough
> > attention, but placing this XML file within the object database
> > directory (.git/objects/) feels rather odd, even if it is just a
> > temporary file. Using the .git/ directory itself might be better,
> > perhaps like this:
>
> It does look odd, and in this case we could use the .git directory
> instead. I specifically use the objects directory for the maintenance
> lock in 'git maintenance run' to allow maintenance to run when
> GIT_OBJECT_DIRECTORY points to an alternate, allowing us to maintain
> object databases that don't have a full .git directory around them.

I guess I'm confused. Won't a Git "common" directory exist even for
such a case when GIT_OBJECT_DIRECTORY is pointing elsewhere, whether
the "common" directory is .git/ or a bare repository, or whatnot?

Anyhow, this brings us back to my original suggestion of creating
these temporary files in a genuine temporary directory (/tmp or
$TMPDIR or $TEMP) instead of arbitrarily choosing a path within the
repository itself. An important reason for using a genuine temporary
directory for these temporary XML files is that it makes it less
confusing for those who come along later and try to understand this
code; they won't have to puzzle out why it is using a repository
location for a file which is clearly temporary.

To make this really simple, you could use one of the
x?mks_tempfile_t*() functions from tempfile.h which will automatically
place the file in $TMPDIR, thus relieving this code from having to
make the choice. Doing so would simplify this code even further since
you would replace create_tempfile() with x?mks_tempfile_t*(), and
wouldn't have to maintain (or free) `xmlpath` manually.

As for the test script, the `print-args` is already picking up the
pathname of the temporary file specified by the /xml option, so it
should be possible to make the rest of the test work with the
generated temporary filename.

Derrick Stolee Nov. 18, 2020, 9:16 p.m. UTC | #4

On 11/18/2020 3:54 PM, Eric Sunshine wrote:
> On Wed, Nov 18, 2020 at 1:30 PM Derrick Stolee <[e-mail]> wrote:
>> On 11/18/2020 2:15 AM, Eric Sunshine wrote:
>>> On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
>>> <[e-mail]> wrote:
>>>> +       xmlpath =  xstrfmt("%s/schedule-%s.xml",
>>>> +                          the_repository->objects->odb->path,
>>>> +                          frequency);
>>>
>>> I missed this in the earlier rounds since I wasn't paying close enough
>>> attention, but placing this XML file within the object database
>>> directory (.git/objects/) feels rather odd, even if it is just a
>>> temporary file. Using the .git/ directory itself might be better,
>>> perhaps like this:
>>
>> It does look odd, and in this case we could use the .git directory
>> instead. I specifically use the objects directory for the maintenance
>> lock in 'git maintenance run' to allow maintenance to run when
>> GIT_OBJECT_DIRECTORY points to an alternate, allowing us to maintain
>> object databases that don't have a full .git directory around them.
> 
> I guess I'm confused. Won't a Git "common" directory exist even for
> such a case when GIT_OBJECT_DIRECTORY is pointing elsewhere, whether
> the "common" directory is .git/ or a bare repository, or whatnot?

The reason to use the object dir for the 'git maintenance run' lock
is to avoid multiple enlistments pointing at a common alternate from
running concurrent maintenance on the same object directory.

That doesn't really apply to the temp files in this patch.

> Anyhow, this brings us back to my original suggestion of creating
> these temporary files in a genuine temporary directory (/tmp or
> $TMPDIR or $TEMP) instead of arbitrarily choosing a path within the
> repository itself. An important reason for using a genuine temporary
> directory for these temporary XML files is that it makes it less
> confusing for those who come along later and try to understand this
> code; they won't have to puzzle out why it is using a repository
> location for a file which is clearly temporary.
> 
> To make this really simple, you could use one of the
> x?mks_tempfile_t*() functions from tempfile.h which will automatically
> place the file in $TMPDIR, thus relieving this code from having to
> make the choice. Doing so would simplify this code even further since
> you would replace create_tempfile() with x?mks_tempfile_t*(), and
> wouldn't have to maintain (or free) `xmlpath` manually.
> 
> As for the test script, the `print-args` is already picking up the
> pathname of the temporary file specified by the /xml option, so it
> should be possible to make the rest of the test work with the
> generated temporary filename.
 
I'll adopt your recommendations here. Thanks.
-Stolee

Patch

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index f2d59f2bcc..e1adfff6db 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -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
diff --git a/builtin/gc.c b/builtin/gc.c
index 782769f243..f6c42f96c1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1671,6 +1671,167 @@  static int launchctl_update_schedule(int run_maintenance, int fd, const char *cm
 		return launchctl_remove_plists(cmd);
 }
 
+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 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 = schtasks_task_name(frequency);
+
+	strvec_split(&args, cmd);
+	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 schtasks_remove_tasks(const char *cmd)
+{
+	return schtasks_remove_task(SCHEDULE_HOURLY, cmd) ||
+		schtasks_remove_task(SCHEDULE_DAILY, cmd) ||
+		schtasks_remove_task(SCHEDULE_WEEKLY, cmd);
+}
+
+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;
+	char *xmlpath;
+	struct tempfile *tfile;
+	const char *frequency = get_frequency(schedule);
+	char *name = schtasks_task_name(frequency);
+
+	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=\"US-ASCII\"?>\n"
+	      "<Task version=\"1.4\" xmlns=\"http://schemas.microsoft.com/windows/2004/02/mit/task\">\n"
+	      "<Triggers>\n"
+	      "<CalendarTrigger>\n";
+	fputs(xml, tfile->fp);
+
+	switch (schedule) {
+	case SCHEDULE_HOURLY:
+		fprintf(tfile->fp,
+			"<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(tfile->fp,
+			"<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(tfile->fp,
+			"<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(tfile->fp, xml, exec_path, exec_path, frequency);
+	strvec_split(&child.args, cmd);
+	strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", xmlpath, NULL);
+	close_tempfile_gently(tfile);
+
+	child.no_stdout = 1;
+	child.no_stderr = 1;
+
+	if (start_command(&child))
+		die(_("failed to start schtasks"));
+	result = finish_command(&child);
+
+	delete_tempfile(&tfile);
+	free(xmlpath);
+	free(name);
+	return result;
+}
+
+static int schtasks_schedule_tasks(const char *cmd)
+{
+	const char *exec_path = git_exec_path();
+
+	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 schtasks_update_schedule(int run_maintenance, int fd, const char *cmd)
+{
+	if (run_maintenance)
+		return schtasks_schedule_tasks(cmd);
+	else
+		return schtasks_remove_tasks(cmd);
+}
+
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
 #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
 
@@ -1761,6 +1922,8 @@  static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)
 
 #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
@@ -1789,6 +1952,8 @@  static int update_background_schedule(int enable)
 
 	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
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 6d37312901..a26ff22541 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -453,6 +453,46 @@  test_expect_success !MINGW 'start and stop macOS maintenance' '
 	test_line_count = 0 actual
 '
 
+test_expect_success 'start and stop Windows maintenance' '
+	write_script print-args <<-\EOF &&
+	echo $* >>args
+	while test $# -gt 0
+	do
+		case "$1" in
+		/xml) shift; xmlfile=$1; break ;;
+		*) shift ;;
+		esac
+	done
+	test -z "$xmlfile" || cp "$xmlfile" .
+	EOF
+
+	rm -f args &&
+	GIT_TEST_MAINT_SCHEDULER="schtasks:./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/schedule-%s.xml\n" \
+		hourly hourly daily daily weekly weekly >expect &&
+	test_cmp expect args &&
+
+	for frequency in hourly daily weekly
+	do
+		test_xmllint "schedule-$frequency.xml"
+	done &&
+
+	rm -f args &&
+	GIT_TEST_MAINT_SCHEDULER="schtasks:./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 &&