From: Eric Sunshine <sunshine@sunshineco.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
"Jonathan Nieder" <jrnieder@gmail.com>,
"Jonathan Tan" <jonathantanmy@google.com>,
"Son Luong Ngoc" <sluongng@gmail.com>,
"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
"Martin Ågren" <martin.agren@gmail.com>,
"Derrick Stolee" <stolee@gmail.com>,
"Derrick Stolee" <derrickstolee@github.com>,
"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v5 4/4] maintenance: use Windows scheduled tasks
Date: Fri, 27 Nov 2020 04:08:10 -0500 [thread overview]
Message-ID: <CAPig+cQ6vZzbb36t5Kn=NM9wXC8i1MpcGXfK=QwnNRoTQsQP0A@mail.gmail.com> (raw)
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:
> 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.
next prev parent reply other threads:[~2020-11-27 9:12 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-03 14:03 [PATCH 0/3] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-11-03 14:03 ` [PATCH 1/3] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-03 14:03 ` [PATCH 2/3] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-03 18:45 ` Eric Sunshine
2020-11-03 21:21 ` Derrick Stolee
2020-11-03 22:27 ` Eric Sunshine
2020-11-04 13:33 ` Derrick Stolee
2020-11-04 14:17 ` Derrick Stolee
2020-11-03 14:03 ` [PATCH 3/3] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-03 19:06 ` Eric Sunshine
2020-11-03 21:23 ` Derrick Stolee
2020-11-03 20:18 ` [PATCH 0/3] Maintenance IV: Platform-specific background maintenance Junio C Hamano
2020-11-03 20:21 ` Junio C Hamano
2020-11-03 21:09 ` Derrick Stolee
2020-11-03 22:30 ` Junio C Hamano
2020-11-04 13:02 ` Derrick Stolee
2020-11-04 17:00 ` Junio C Hamano
2020-11-04 18:43 ` Derrick Stolee
2020-11-04 20:06 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2020-11-04 20:06 ` [PATCH v2 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-04 20:06 ` [PATCH v2 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-11 7:10 ` Eric Sunshine
2020-11-04 20:06 ` [PATCH v2 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-11 8:12 ` Eric Sunshine
2020-11-12 13:42 ` Derrick Stolee
2020-11-12 16:43 ` Eric Sunshine
2020-11-04 20:06 ` [PATCH v2 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-11 8:59 ` Eric Sunshine
2020-11-12 13:56 ` Derrick Stolee
2020-11-13 14:00 ` [PATCH v3 0/4] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-11-13 14:00 ` [PATCH v3 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-13 14:00 ` [PATCH v3 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-13 14:00 ` [PATCH v3 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-13 20:19 ` Eric Sunshine
2020-11-13 20:42 ` Derrick Stolee
2020-11-13 20:53 ` Eric Sunshine
2020-11-13 20:56 ` Eric Sunshine
2020-11-13 14:00 ` [PATCH v3 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-13 20:44 ` Eric Sunshine
2020-11-13 21:32 ` Derrick Stolee
2020-11-13 21:40 ` Eric Sunshine
2020-11-16 13:13 ` Derrick Stolee
2020-11-13 20:47 ` [PATCH v3 0/4] Maintenance IV: Platform-specific background maintenance Eric Sunshine
2020-11-14 9:23 ` Eric Sunshine
2020-11-16 13:17 ` Derrick Stolee
2020-11-17 21:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget
2020-11-17 21:13 ` [PATCH v4 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-17 21:13 ` [PATCH v4 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-18 0:34 ` Eric Sunshine
2020-11-18 18:30 ` Derrick Stolee
2020-11-17 21:13 ` [PATCH v4 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-18 6:45 ` Eric Sunshine
2020-11-18 18:22 ` Derrick Stolee
2020-11-17 21:13 ` [PATCH v4 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-18 7:15 ` Eric Sunshine
2020-11-18 18:30 ` Derrick Stolee
2020-11-18 20:54 ` Eric Sunshine
2020-11-18 21:16 ` Derrick Stolee
2020-11-17 23:36 ` [PATCH v4 0/4] Maintenance IV: Platform-specific background maintenance Eric Sunshine
2020-11-24 2:20 ` Derrick Stolee
2020-11-24 2:59 ` Eric Sunshine
2020-11-17 23:54 ` Eric Sunshine
2020-11-24 4:16 ` [PATCH v5 " Derrick Stolee via GitGitGadget
2020-11-24 4:16 ` [PATCH v5 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-24 4:16 ` [PATCH v5 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-24 4:16 ` [PATCH v5 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-24 4:16 ` [PATCH v5 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-27 9:08 ` Eric Sunshine [this message]
2020-12-09 19:28 ` [PATCH v6 0/4] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-12-09 19:28 ` [PATCH v6 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-12-09 19:29 ` [PATCH v6 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-12-09 19:29 ` [PATCH v6 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-12-09 19:29 ` [PATCH v6 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-12-10 0:32 ` [PATCH v6 0/4] Maintenance IV: Platform-specific background maintenance Junio C Hamano
2020-12-10 0:49 ` Eric Sunshine
2020-12-10 1:04 ` Junio C Hamano
2021-01-05 12:17 ` Derrick Stolee
2021-01-05 13:08 ` [PATCH v7 " Derrick Stolee via GitGitGadget
2021-01-05 13:08 ` [PATCH v7 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2021-01-05 13:08 ` [PATCH v7 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2021-01-05 13:08 ` [PATCH v7 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2021-01-10 6:34 ` Eric Sunshine
2021-01-05 13:08 ` [PATCH v7 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cQ6vZzbb36t5Kn=NM9wXC8i1MpcGXfK=QwnNRoTQsQP0A@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=congdanhqx@gmail.com \
--cc=derrickstolee@github.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.com \
--cc=martin.agren@gmail.com \
--cc=sluongng@gmail.com \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).