Re: [PATCH v5 4/4] maintenance: use Windows scheduled tasks
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Nov 27, 2020, 09:08 UTC
- Message-ID
- <CAPig+cQ6vZzbb36t5Kn=NM9wXC8i1MpcGXfK=QwnNRoTQsQP0A@mail.gmail.com>
- In-Reply-To
- <ac9a28bea39b4d1897ad6a41331e52770c8c7531.1606191405.git.gitgitgadget@gmail.com>
On Mon, Nov 23, 2020 at 11:16 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 14 quoted lines
> 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), ...);
Show 13 quoted lines
> 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.