On Mon, Nov 23, 2020 at 11:16 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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.
