diff mbox series

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

Message ID ed7a61978fe9dce26ca459b5a86490c15e470698.1605276024.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Maintenance IV: Platform-specific background maintenance | expand

Commit Message

Derrick Stolee Nov. 13, 2020, 2 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

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

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-maintenance.txt |  22 ++++
 builtin/gc.c                      | 184 ++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh            |  33 +++++-
 3 files changed, 236 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Nov. 13, 2020, 8:44 p.m. UTC | #1 
On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Git's background maintenance uses cron by default, but this is not
> available on Windows. Instead, integrate with Task Scheduler.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -1684,6 +1684,190 @@ static int platform_update_schedule(int run_maintenance, int fd)
> +static int schedule_task(const char *exec_path, enum schedule_priority schedule)
> +{
> +       char *xmlpath, *tempDir;
> +       tempDir = xstrfmt("%s/temp", the_repository->objects->odb->path);
> +       xmlpath =  xstrfmt("%s/schedule-%s.xml", tempDir, frequency);

When I wondered aloud in my previous review whether writing these
throwaway files to a temporary directory would make sense, I was
thinking more along the lines of /tmp or $TEMP. More specifically, we
have xmkstemp() in wrapper.c which is good for this sort of thing (or
one of the other temporary-file-making functions in there). We also
have a more full-featured temporary-file API in tempfile.h which would
ensure that these throwaway files actually get thrown away when the
command finishes.

This is not necessarily worth a re-roll.

> +       if (start_command(&child))
> +               die(_("failed to start schtasks"));
> +       result = finish_command(&child);
> +
> +       unlink(xmlpath);
> +       rmdir(tempDir);

Neither xmlpath and tempDir get cleaned up from the filesystem if the
preceding die() is triggered (which may or may not make sense --
perhaps you want to keep them around if it helps with the diagnosis of
the failure). The functions in tempfile.h would ensure the temporary
file is cleaned up even if the program die()s, or you could manually
remove the temporary file before die()ing.

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -437,6 +437,33 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
> +test_expect_success MINGW 'start and stop Windows maintenance' '
> +       write_script print-args <<-\EOF &&
> +       echo $* >>args
> +       EOF

Using `>>` here makes it harder to reason about the test than using
`>` would, especially since `>>` seems to be unnecessary in this case.

> +       rm -f args &&
> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&

Is it a requirement on Windows to mention /bin/sh here? Specifically,
I'm wondering why a simple ./print-args doesn't work. (It's especially
unclear since write_script() is used heavily in the test suite and it
seems to work well enough on Windows without specifying /bin/sh.)
Derrick Stolee Nov. 13, 2020, 9:32 p.m. UTC | #2 
On 11/13/2020 3:44 PM, Eric Sunshine wrote:
> On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> Git's background maintenance uses cron by default, but this is not
>> available on Windows. Instead, integrate with Task Scheduler.
>> [...]
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> @@ -1684,6 +1684,190 @@ static int platform_update_schedule(int run_maintenance, int fd)
>> +static int schedule_task(const char *exec_path, enum schedule_priority schedule)
>> +{
>> +       char *xmlpath, *tempDir;
>> +       tempDir = xstrfmt("%s/temp", the_repository->objects->odb->path);
>> +       xmlpath =  xstrfmt("%s/schedule-%s.xml", tempDir, frequency);
> 
> When I wondered aloud in my previous review whether writing these
> throwaway files to a temporary directory would make sense, I was
> thinking more along the lines of /tmp or $TEMP. More specifically, we
> have xmkstemp() in wrapper.c which is good for this sort of thing (or
> one of the other temporary-file-making functions in there). We also
> have a more full-featured temporary-file API in tempfile.h which would
> ensure that these throwaway files actually get thrown away when the
> command finishes.
> 
> This is not necessarily worth a re-roll.
> 
>> +       if (start_command(&child))
>> +               die(_("failed to start schtasks"));
>> +       result = finish_command(&child);
>> +
>> +       unlink(xmlpath);
>> +       rmdir(tempDir);
> 
> Neither xmlpath and tempDir get cleaned up from the filesystem if the
> preceding die() is triggered (which may or may not make sense --
> perhaps you want to keep them around if it helps with the diagnosis of
> the failure). The functions in tempfile.h would ensure the temporary
> file is cleaned up even if the program die()s, or you could manually
> remove the temporary file before die()ing.

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.

>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> @@ -437,6 +437,33 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
>> +test_expect_success MINGW 'start and stop Windows maintenance' '
>> +       write_script print-args <<-\EOF &&
>> +       echo $* >>args
>> +       EOF
> 
> Using `>>` here makes it harder to reason about the test than using
> `>` would, especially since `>>` seems to be unnecessary in this case.

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

>> +       rm -f args &&
>> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
> 
> Is it a requirement on Windows to mention /bin/sh here? Specifically,
> I'm wondering why a simple ./print-args doesn't work. (It's especially
> unclear since write_script() is used heavily in the test suite and it
> seems to work well enough on Windows without specifying /bin/sh.)

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
Eric Sunshine Nov. 13, 2020, 9:40 p.m. UTC | #3 
On Fri, Nov 13, 2020 at 4:32 PM Derrick Stolee <stolee@gmail.com> wrote:
> On 11/13/2020 3:44 PM, Eric Sunshine wrote:
> > On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +test_expect_success MINGW 'start and stop Windows maintenance' '
> >> +       write_script print-args <<-\EOF &&
> >> +       echo $* >>args
> >> +       EOF
> >
> > Using `>>` here makes it harder to reason about the test than using
> > `>` would, especially since `>>` seems to be unnecessary in this case.
>
> 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).

Makes sense. Thanks.

> >> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
> >
> > Is it a requirement on Windows to mention /bin/sh here? Specifically,
> > I'm wondering why a simple ./print-args doesn't work. (It's especially
> > unclear since write_script() is used heavily in the test suite and it
> > seems to work well enough on Windows without specifying /bin/sh.)
>
> 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

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?
Derrick Stolee Nov. 16, 2020, 1:13 p.m. UTC | #4 
On 11/13/2020 4:40 PM, Eric Sunshine wrote:
> On Fri, Nov 13, 2020 at 4:32 PM Derrick Stolee <stolee@gmail.com> wrote:
>> On 11/13/2020 3:44 PM, Eric Sunshine wrote:
>>> On Fri, Nov 13, 2020 at 9:00 AM Derrick Stolee via GitGitGadget
>>>> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
>>>
>>> Is it a requirement on Windows to mention /bin/sh here? Specifically,
>>> I'm wondering why a simple ./print-args doesn't work. (It's especially
>>> unclear since write_script() is used heavily in the test suite and it
>>> seems to work well enough on Windows without specifying /bin/sh.)
>>
>> 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
> 
> 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
diff mbox series

Patch

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 5f8f63f098..6970f2b898 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 da2c892f68..76a3afa20a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1684,6 +1684,190 @@  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 child_process child = CHILD_PROCESS_INIT;
+	const char *xml, *schtasks;
+	char *xmlpath, *tempDir;
+	FILE *xmlfp;
+	const char *frequency = get_frequency(schedule);
+	char *name = get_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"
+	      "<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(&child.args, schtasks);
+	strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", xmlpath, NULL);
+
+	child.no_stdout = 1;
+	child.no_stderr = 1;
+
+	if (start_command(&child))
+		die(_("failed to start schtasks"));
+	result = finish_command(&child);
+
+	unlink(xmlpath);
+	rmdir(tempDir);
+	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"
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 29d340a828..0dc2479117 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -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 &&