Re: [PATCH v3 4/4] maintenance: use Windows scheduled tasks

[Eric Sunshine <sunshine@sunshineco.com>]

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