From: Todd Zullinger <tmz@pobox.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Santiago Torres <santiago@nyu.edu>
Subject: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Date: Mon, 13 Nov 2017 16:07:45 -0500 [thread overview]
Message-ID: <20171113210745.24638-1-tmz@pobox.com> (raw)
In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a
call to gpgconf was added to kill the gpg-agent. The intention was to
ignore all output from the call, but the order of the redirection needs
to be switched to ensure that both stdout and stderr are redirected to
/dev/null. Without this, gpgconf from gnupg-2.0 releases would output
'gpgconf: invalid option "--kill"' each time it was called.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
I noticed that gpgconf produced error output for a number of tests on
CentOS/RHEL. As an example:
*** t5534-push-signed.sh ***
gpgconf: invalid option "--kill"
Looking at the code in lib-gpg.sh, it appeared the intention was to ignore this
output. Reading through the review of the patch confirmed that feeling[1]. The
current code gets caught by the subtleties of output redirection. (Who hasn't
been burned at some point by the difference between '2>&1 >/dev/null' and
'>/dev/null 2>&1' ? ;)
[1] https://public-inbox.org/git/xmqq379qlvzi.fsf@gitster.mtv.corp.google.com/
Of course, beyond getting stderr to /dev/null, there is the fact that on
versions of gnupg < 2.1, gpgconf --kill is not available. I noticed this with
gnupg-2.0.14 on CentOS 6. It also occurs on CentOS 7, which provides
gnupg-2.0.22.
I don't know if there's much value in trying to better handle older gnupg-2.0
systems. Using gpgconf --reload might be sufficient to work on gnupg-2.0 and
newer systems. That might solve the issues with gpg-agent caching stale file
handles that motivated the initial patch. If not, finding what works well with
both gnupg-2.0 and newer seems mildly painful. This method works for me on
2.0, 2.1, and 2.2:
pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}')
And people say crypto tools aren't intuitive. Pfff. :/
A fairly gross way to use that in lib-gpg.sh might be:
diff --git c/t/lib-gpg.sh w/t/lib-gpg.sh
index 43679a4c64..c91d9b334f 100755
--- c/t/lib-gpg.sh
+++ w/t/lib-gpg.sh
@@ -1,5 +1,10 @@
#!/bin/sh
+gpg_killagent() {
+ pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}')
+ test -n "$pid" && kill "$pid"
+}
+
gpg_version=$(gpg --version 2>&1)
if test $? != 127
then
@@ -31,7 +36,7 @@ then
chmod 0700 ./gpghome &&
GNUPGHOME="$(pwd)/gpghome" &&
export GNUPGHOME &&
- (gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) &&
+ (gpg_killagent >/dev/null 2>&1 || : ) &&
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
I have my doubts that doing something like the above is worthwhile. It's
probably good enough to simply fix up the gpgconf stderr redirection and call
it a day.
I haven't seen any gpg failures in the test runs I've done, so I haven't had a
need to re-run those tests. (I also have not run the test suite with the ugly
gpg_killagent() diff above. I have run it with the change to fix stderr
redirection and confirmed it succeeds without the gpgconf error messages.)
Lastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null. I
suspect it's similarly not intentional:
$ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh
apply_autostash () {
if test -f "$state_dir/autostash"
then
stash_sha1=$(cat "$state_dir/autostash")
if git stash apply $stash_sha1 2>&1 >/dev/null
then
echo "$(gettext 'Applied autostash.')" >&2
else
git stash store -m "autostash" -q $stash_sha1 ||
I'll send a separate patch to adjust that code as well.
t/lib-gpg.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 43679a4c64..a5d3b2cbaa 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -31,7 +31,7 @@ then
chmod 0700 ./gpghome &&
GNUPGHOME="$(pwd)/gpghome" &&
export GNUPGHOME &&
- (gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) &&
+ (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
--
2.15.0
next reply other threads:[~2017-11-13 21:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-13 21:07 Todd Zullinger [this message]
2017-11-13 22:18 ` [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null Santiago Torres
2017-11-13 22:43 ` Todd Zullinger
2017-11-13 23:02 ` Santiago Torres
2017-11-13 23:06 ` Santiago Torres
2017-11-14 3:10 ` Todd Zullinger
2017-11-14 15:35 ` Santiago Torres
2017-11-14 2:49 ` Junio C Hamano
2017-11-14 3:03 ` Todd Zullinger
2017-11-14 3:30 ` Junio C Hamano
2017-11-14 5:15 ` Todd Zullinger
2017-11-14 9:31 ` Johan Herland
2017-11-14 14:52 ` Junio C Hamano
2017-11-14 16:17 ` [PATCH] notes: send "Automatic notes merge failed" messages to stderr Todd Zullinger
2017-11-15 13:13 ` Johan Herland
2017-11-15 21:09 ` Todd Zullinger
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=20171113210745.24638-1-tmz@pobox.com \
--to=tmz@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=santiago@nyu.edu \
/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).