Subject: [PATCH v2 11/11] tests: add a "set -o pipefail" for a patched bash

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 16 Jan 2021 16:35:54 +0100

Add a "set -o pipefail" test mode to the test suite to detect failures
in "git" its output is fed directly to a pipe. Doing so is a pattern
we discourage[1] in the test suite, but we've got plenty of tests like
that. Now we can reliably detect those failures.

There was a previous attempt in [2] to add such a test mode, but as
noted by Jeff King in [3] adding it is a matter of peeing against the
wind with current bash semantics of failing on SIGPIPE.

This series relies on a patch of mine to bash, which I'm submitting
upstream, while not breaking anything for vanilla bash users. They
won't have GIT_TEST_PIPEFAIL turned on for them, and will only get
breakages if they turn it on explicitly with "GIT_TEST_PIPEFAIL=true".

Vanilla bash ignores SIGPIPE under "set -e" since version 3.1. It's
only under "set -o pipefail" (added in 3.2) that it doesn't take
account of SIGPIPE, in a seeming omission nobody bothered to fix yet.

Patching bash[4] with:

    diff --git a/jobs.c b/jobs.c
    index a581f305..fa5de82a 100644
    --- a/jobs.c
    +++ b/jobs.c
    @@ -2851,8 +2851,14 @@ raw_job_exit_status (job)
           p = jobs[job]->pipe;
           do
     	{
    -	  if (WSTATUS (p->status) != EXECUTION_SUCCESS)
    -	    fail = WSTATUS(p->status);
    +	  if (WSTATUS (p->status) != EXECUTION_SUCCESS
    +#if defined (DONT_REPORT_SIGPIPE)
    +              && WTERMSIG (p->status) != SIGPIPE
    +#endif
    +              )
    +            {
    +              fail = WSTATUS(p->status);
    +            }
     	  p = p->next;
     	}
           while (p != jobs[job]->pipe);

Makes it useful for something like the git test suite.

Under this test mode we only tests we need to skip those tests which
are explicitly testing that a piped command returned SIGPIPE. Those
tests will now return 0 instead of an exit code indicating SIGPIPE.

Forcing the mode to run under vanilla bash with
"GIT_TEST_PIPEFAIL=true" doesn't fail any tests for me, except the
test in t0000-basic.sh which explicitly checks for the desired
pipefail semantics. However, as Jeff noted in [3] that absence of
failure isn't reliable. I might not see some of the failures due to
the racy nature of how vanilla "set -o pipefail" interacts with *nix
pipe semantics.

1. a378fee5b0 (Documentation: add shell guidelines, 2018-10-05)
2. https://lore.kernel.org/git/[email-masked]/
3. https://lore.kernel.org/git/[email-masked]/
4. https://github.com/bminor/bash/compare/master...avar:avar/ignore-sigterm-and-sigpipe-on-pipe-fail

Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
---
 t/README              |  5 +++++
 t/t0000-basic.sh      |  4 ++++
 t/t0005-signals.sh    |  4 ++--
 t/t3600-rm.sh         |  5 ++++-
 t/t5000-tar-tree.sh   |  2 +-
 t/t9902-completion.sh |  5 +++++
 t/test-lib.sh         | 29 +++++++++++++++++++++++++++++
 7 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index c730a70770..cecc00442d 100644
--- a/t/README
+++ b/t/README
@@ -439,6 +439,11 @@ GIT_TEST_DEFAULT_HASH=<hash-algo> specifies which hash algorithm to
 use in the test scripts. Recognized values for <hash-algo> are "sha1"
 and "sha256".
 
+GIT_TEST_PIPEFAIL=<boolean>, when true, run 'set -o pipefail' to catch
+failures in commands that aren't the last in a pipe. Defaults to true
+on bash versions which know how to ignore SIGPIPE failures under the
+'set -o pipefail' mode.
+
 Naming Tests
 ------------
 
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f4ba2e8c85..38e04b9ce0 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1326,4 +1326,8 @@ test_expect_success 'test_must_fail rejects a non-git command with env' '
 	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
 '
 
+test_expect_success BASH_SET_O_PIPEFAIL 'our bash under "set -o pipefail" mode ignores SIGPIPE failures' '
+	yes | head -n 1 | true
+'
+
 test_done
diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 4c214bd11c..cc5784a274 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -40,12 +40,12 @@ test_expect_success 'create blob' '
 	git add file
 '
 
-test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
+test_expect_success !MINGW,!BASH_SET_O_PIPEFAIL 'a constipated git dies with SIGPIPE' '
 	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 
-test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '
+test_expect_success !MINGW,!BASH_SET_O_PIPEFAIL 'a constipated git dies with SIGPIPE even if parent ignores it' '
 	OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 4f7e62d05c..7b5d92add5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -251,7 +251,10 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) &&
-	test_match_signal 13 "$OUT" &&
+	if ! test_have_prereq BASH_SET_O_PIPEFAIL
+	then
+		test_match_signal 13 "$OUT"
+	fi &&
 	test_path_is_missing .git/index.lock
 '
 
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 3ebb0d3b65..3adcbce84c 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -416,7 +416,7 @@ test_expect_success LONG_IS_64BIT 'set up repository with huge blob' '
 
 # We expect git to die with SIGPIPE here (otherwise we
 # would generate the whole 64GB).
-test_expect_success LONG_IS_64BIT 'generate tar with huge size' '
+test_expect_success LONG_IS_64BIT,!BASH_SET_O_PIPEFAIL 'generate tar with huge size' '
 	{
 		git archive HEAD
 		echo $? >exit-code
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index c4a7758409..947294bebb 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -7,6 +7,11 @@ test_description='test bash completion'
 
 . ./lib-bash.sh
 
+if test -n "$GIT_TEST_PIPEFAIL_TRUE"
+then
+	set +o pipefail
+fi
+
 complete ()
 {
 	# do nothing
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9fa7c1d0f6..118dc80ffc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -36,6 +36,31 @@ then
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
+# Does "set -o pipefail" on this bash version handle SIGPIPE? Use it!
+. "$TEST_DIRECTORY/lib-bash-detection.sh"
+GIT_TEST_PIPEFAIL_TRUE=
+GIT_TEST_PIPEFAIL_DEFAULT=false
+if test -n "$TEST_SH_IS_BIN_BASH" &&
+       $BASH -c 'set -eo pipefail; yes | head -n 1 >/dev/null'
+then
+	GIT_TEST_PIPEFAIL_DEFAULT=true
+fi
+# We're too early for test_bool_env
+if git env--helper --type=bool --default="$GIT_TEST_PIPEFAIL_DEFAULT" \
+       --exit-code GIT_TEST_PIPEFAIL
+then
+	set -o pipefail
+
+	# Only "set -o pipefail" in the main test scripts, not any
+	# sub-programs we spawn.
+	GIT_TEST_PIPEFAIL=
+	export GIT_TEST_PIPEFAIL
+
+	# For the convenience of the prereq for it.
+	GIT_TEST_PIPEFAIL_TRUE=true
+	export GIT_TEST_PIPEFAIL_TRUE
+fi
+
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
 # the noise level. This needs to happen at the start of the script,
@@ -1552,6 +1577,10 @@ test_lazy_prereq PIPE '
 	rm -f testfifo && mkfifo testfifo
 '
 
+test_lazy_prereq BASH_SET_O_PIPEFAIL '
+	test -n "$GIT_TEST_PIPEFAIL_TRUE"
+'
+
 test_lazy_prereq SYMLINKS '
 	# test whether the filesystem supports symbolic links
 	ln -s x y && test -h y
-- 
2.29.2.222.g5d2a92d10f8