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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Nov 23, 2020 at 11:16 PM 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
> @@ -1671,6 +1671,162 @@ 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)
> +{
> +       const char *frequency = get_frequency(schedule);
> +
> +       tfile = xmks_tempfile("schedule_XXXXXX");
> +       if (!tfile || !fdopen_tempfile(tfile, "w"))
> +               die(_("failed to create temp xml file"));

Several comments:

The "x" prefix on xmks_tempfile() means that it will die() if it can't
open the tempfile, so the `!tfile` condition is pointless, thus it
could be written:

    if (!fdopen_tempfile(tfile, "w"))

The mks_tempfile_t*() functions with a trailing "t" will place the
temporary file in TMPDIR, whereas xmks_tempfile() used here places it
in the worktree directory, which is not as desirable. Ideally, this
would be using xmks_tempfile_t(), however, that function doesn't exist
yet in tempfile.h, so the best you can do (without the extra work of
also adding the missing function) is to use mks_tempfile_t(). That
doesn't die(), so `!tfile` would still be needed in the conditional.

In earlier versions, you incorporated `frequency` into the temporary
filename which was nice because it made the test easier to understand.
It's not hard to do so here, as well, nor to extract a useful filename
in the test (as I'll show below). For instance:

    struct strbuf tpath = STRBUF_INIT;
    strbuf_addf(&tpath, "schedule-%s-XXXXXX", frequency);
    tfile = mks_tempfile_t(tpath.buf);
    strbuf_release(&tpath);
    if (!tfile || !fdopen(tempfile(tfile, "w"))
        die(...);

> +       strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", tfile->filename.buf, NULL);

Alternately, use the getter:

    strvec_pushl(..., get_tempfile_path(&tfile), ...);

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -453,6 +453,50 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
> +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
> +       lines=$(wc -l args | awk "{print \$1;}")

You're using `awk` to pluck out the line count and ignore the
filename, but the same can be accomplished by feeding the file as
stdin to `wc` rather than naming it as an argument, thus this is
equivalent:

    lines=$(wc -l <args)

However, this idea of constructing stable names for the files by
assigning them numerically incrementing values is unnecessary and
makes the test harder to understand.

> +       test -z "$xmlfile" || cp "$xmlfile" "schedule-$lines.xml"

If you take the suggestion earlier in this review of naming the file
"schedule-${frequency}-XXXXXX.xml", then you can strip it down to just
"schedule-${frequency}.xml" using the expression `${xmlfile%-*}.xml`.
There is no need for `$lines`. Thus, copying the file would become:

    test -z "$xmlfile" || cp "$xmlfile" "${xmlfile%-*}.xml"

> +       for i in 1 2 3
> +       do

Which means that you can use the more easily understood `hourly daily
weekly` enumeration here rather than `1 2 3`.

Having said all this, I'm not sure it's worth a re-roll. These sort of
tweaks can be done atop the current series if someone wants to tackle
it.