[3/3] maintenance: use Windows scheduled tasks

Commit Message

Derrick Stolee Nov. 3, 2020, 2:03 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'.

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

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/gc.c           | 181 +++++++++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh |  40 ++++++++-
 2 files changed, 218 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Nov. 3, 2020, 7:06 p.m. UTC | #1

On Tue, Nov 3, 2020 at 9:05 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> 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!

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/

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -441,6 +441,40 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
> +test_expect_success MINGW 'start and stop Windows maintenance' '
> +       echo "echo \$@ >>args" >print-args &&
> +       chmod a+x print-args &&

Same comments as my review of [2/3] regarding $@ and write_script().

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

Is this 'cat' leftover debugging gunk?

> +       # 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 &&

Rather than using >> within the loop, it's often simpler to capture
the output of the for-loop in its entirety:

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

However, in this case 'printf' may be even simpler:

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

> +       GIT_TEST_CRONTAB="/bin/sh print-args"  git maintenance stop &&

Too many spaces before the 'git' command.

Derrick Stolee Nov. 3, 2020, 9:23 p.m. UTC | #2

On 11/3/2020 2:06 PM, Eric Sunshine wrote:
> On Tue, Nov 3, 2020 at 9:05 AM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>> 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!
> 
> 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/

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.

>> Signed-off-by: Derrick Stolee <[e-mail]>
>> ---
>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> @@ -441,6 +441,40 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
>> +test_expect_success MINGW 'start and stop Windows maintenance' '
>> +       echo "echo \$@ >>args" >print-args &&
>> +       chmod a+x print-args &&
> 
> Same comments as my review of [2/3] regarding $@ and write_script().

Noted!

>> +       rm -f args &&
>> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
>> +       cat args &&
> 
> Is this 'cat' leftover debugging gunk?

Yes. Thanks.

>> +       # 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 &&
> 
> Rather than using >> within the loop, it's often simpler to capture
> the output of the for-loop in its entirety:
> 
>     for frequency in hourly daily weekly
>     do
>         echo "/create ..." || return 1
>     done >expect &&
> 
> However, in this case 'printf' may be even simpler:
> 
>     printf "/create /tn ... .git/objects/schedule-%s.xml\n" \
>         hourly daily weekly >expect &&

Excellent.

>> +       GIT_TEST_CRONTAB="/bin/sh print-args"  git maintenance stop &&
> 
> Too many spaces before the 'git' command.

Thanks!
-Stolee

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index fa0ae63a80..24511fec2e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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"
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index f0210aa206..73dc0078da 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
@@ -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 &&