[PATCH 00/15] tests: don't ignore "git" exit codes

[PATCH 00/15] tests: don't ignore "git" exit codes

[Ævar Arnfjörð Bjarmason]

This series fixes issues where we ignored the exit code of "git" due
to it being on the LHS of a pipe, or because we interpolated its
output with $() in a "test" construct, or had missing &&- chains in
helper functions etc.

This series is not made by string-replacing things in our test suite,
if it was it would be much larger. These are all tests I've seen real
hide real failures under SANITIZE=leak, either on current "master", or
in combination with various local leak fixes I've got unsubmitted.

In cases where I was starting to fix a pattern in a file I'd fix the
rest of the file if it was easy, but otherwise these are all cases
where I ran SANITIZE=leak, had a test pass, but having ASAN_OPTIONS
log to a file revealed that we had memory leaks within that test.

As an aside we still have various potential issues with hidden
segfaults etc. in the test suite after this that are tricked to solve,
because:

 * Our tests will (mostly) catch segfaults and abort(), but if we
   invoke a command that invokes another command it needs to ferry the
   exit code up to us.

 * run-command.c notably does not do that, so for e.g. "git push"
   tests where we expect a failure and an underlying "git" command
   fails we won't ferry up the segfault or abort exit code.

 * We have gitweb.perl and some other perl code ignoring return values
   from close(), i.e. ignoring exit codes from "git rev-parse" et al.

 * We have in-tree shellscripts like "git-merge-one-file.sh" invoking
   git commands, and if they fail returning "1", not ferrying up the
   segfault or abort() exit code.

Ævar Arnfjörð Bjarmason (15):
  tests: change some 'test $(git) = "x"' to test_cmp
  tests: use "test_stdout_line_count", not "test $(git [...] | wc -l)"
  read-tree tests: check "diff-files" exit code on failure
  diff tests: don't ignore "git diff" exit code
  diff tests: don't ignore "git diff" exit code in "read" loop
  apply tests: use "test_must_fail" instead of ad-hoc pattern
  merge tests: use "test_must_fail" instead of ad-hoc pattern
  rev-parse tests: don't ignore "git reflog" exit code
  notes tests: don't ignore "git" exit code
  diff tests: don't ignore "git rev-list" exit code
  rev-list tests: don't hide abort() in "test_expect_failure"
  gettext tests: don't ignore "test-tool regex" exit code
  apply tests: don't ignore "git ls-files" exit code, drop sub-shell
  checkout tests: don't ignore "git <cmd>" exit code
  rev-list simplify tests: don't ignore "git" exit code

 t/t0002-gitfile.sh                     |   6 +-
 t/t1001-read-tree-m-2way.sh            |   6 +-
 t/t1002-read-tree-m-u-2way.sh          |   6 +-
 t/t1503-rev-parse-verify.sh            |   5 +-
 t/t2012-checkout-last.sh               |  51 ++++++---
 t/t2200-add-update.sh                  |  33 ++++--
 t/t3302-notes-index-expensive.sh       |   6 +-
 t/t3303-notes-subtrees.sh              |   9 +-
 t/t3305-notes-fanout.sh                |  14 +--
 t/t4020-diff-external.sh               | 153 ++++++++++++-------------
 t/t4027-diff-submodule.sh              |   7 +-
 t/t4123-apply-shrink.sh                |  18 +--
 t/t4128-apply-root.sh                  |  36 +++---
 t/t6005-rev-list-count.sh              |  43 ++++---
 t/t6012-rev-list-simplify.sh           |  12 +-
 t/t6102-rev-list-unexpected-objects.sh |  13 ++-
 t/t6407-merge-binary.sh                |  22 +---
 t/t7103-reset-bare.sh                  |   7 +-
 t/t7812-grep-icase-non-ascii.sh        |  11 +-
 19 files changed, 240 insertions(+), 218 deletions(-)

-- 
2.35.1.1226.g8b497615d32

[PATCH 01/15] tests: change some 'test $(git) = "x"' to test_cmp

[Ævar Arnfjörð Bjarmason]

Change some of the patterns in the test suite where we were hiding the
exit code from "git" by invoking it in a sub-shell within a "test"
expression to use temporary files and test_cmp instead.

These are not all the occurrences of this anti-pattern, but these in
particular hid issues where LSAN was dying, and I'd thus marked these
tests as passing under the linux-leaks CI job in past commits with
"TEST_PASSES_SANITIZE_LEAK=true". Let's deal with that by either
removing that marking, or skipping specific tests under
!SANITIZE_LEAK.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0002-gitfile.sh    |  6 ++++--
 t/t2200-add-update.sh | 33 +++++++++++++++++++++------------
 t/t4128-apply-root.sh | 33 ++++++++++++++++++++-------------
 t/t7103-reset-bare.sh |  7 +++++--
 4 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 76052cb5620..f6356db183b 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -65,9 +65,11 @@ test_expect_success 'check commit-tree' '
 	test_path_is_file "$REAL/objects/$(objpath $SHA)"
 '
 
-test_expect_success 'check rev-list' '
+test_expect_success !SANITIZE_LEAK 'check rev-list' '
 	git update-ref "HEAD" "$SHA" &&
-	test "$SHA" = "$(git rev-list HEAD)"
+	git rev-list HEAD >actual &&
+	echo $SHA >expected &&
+	test_cmp expected actual
 '
 
 test_expect_success 'setup_git_dir twice in subdir' '
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index acd3650d3c0..0c38f8e3569 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -14,7 +14,6 @@ only the updates to dir/sub.
 Also tested are "git add -u" without limiting, and "git add -u"
 without contents changes, and other conditions'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
@@ -41,20 +40,28 @@ test_expect_success update '
 '
 
 test_expect_success 'update noticed a removal' '
-	test "$(git ls-files dir1/sub1)" = ""
+	git ls-files dir1/sub1 >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'update touched correct path' '
-	test "$(git diff-files --name-status dir2/sub3)" = ""
+	git diff-files --name-status dir2/sub3 >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'update did not touch other tracked files' '
-	test "$(git diff-files --name-status check)" = "M	check" &&
-	test "$(git diff-files --name-status top)" = "M	top"
+	echo "M	check" >expect &&
+	git diff-files --name-status check >actual &&
+	test_cmp expect actual &&
+
+	echo "M	top" >expect &&
+	git diff-files --name-status top >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'update did not touch untracked files' '
-	test "$(git ls-files dir2/other)" = ""
+	git ls-files dir2/other >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'cache tree has not been corrupted' '
@@ -76,9 +83,8 @@ test_expect_success 'update from a subdirectory' '
 '
 
 test_expect_success 'change gets noticed' '
-
-	test "$(git diff-files --name-status dir1)" = ""
-
+	git diff-files --name-status dir1 >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'non-qualified update in subdir updates from the root' '
@@ -103,7 +109,8 @@ test_expect_success 'replace a file with a symlink' '
 test_expect_success 'add everything changed' '
 
 	git add -u &&
-	test -z "$(git diff-files)"
+	git diff-files >out &&
+	test_must_be_empty out
 
 '
 
@@ -111,7 +118,8 @@ test_expect_success 'touch and then add -u' '
 
 	touch check &&
 	git add -u &&
-	test -z "$(git diff-files)"
+	git diff-files >out &&
+	test_must_be_empty out
 
 '
 
@@ -119,7 +127,8 @@ test_expect_success 'touch and then add explicitly' '
 
 	touch check &&
 	git add check &&
-	test -z "$(git diff-files)"
+	git diff-files >out &&
+	test_must_be_empty out
 
 '
 
diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh
index cb3181e8b71..ba89a2f2d73 100755
--- a/t/t4128-apply-root.sh
+++ b/t/t4128-apply-root.sh
@@ -2,8 +2,6 @@
 
 test_description='apply same filename'
 
-
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
@@ -26,10 +24,11 @@ diff a/bla/blub/dir/file b/bla/blub/dir/file
 EOF
 
 test_expect_success 'apply --directory -p (1)' '
-
 	git apply --directory=some/sub -p3 --index patch &&
-	test Bello = $(git show :some/sub/dir/file) &&
-	test Bello = $(cat some/sub/dir/file)
+	echo Bello >expect &&
+	git show :some/sub/dir/file >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/file
 
 '
 
@@ -37,8 +36,10 @@ test_expect_success 'apply --directory -p (2) ' '
 
 	git reset --hard initial &&
 	git apply --directory=some/sub/ -p3 --index patch &&
-	test Bello = $(git show :some/sub/dir/file) &&
-	test Bello = $(cat some/sub/dir/file)
+	echo Bello >expect &&
+	git show :some/sub/dir/file >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/file
 
 '
 
@@ -55,8 +56,10 @@ EOF
 test_expect_success 'apply --directory (new file)' '
 	git reset --hard initial &&
 	git apply --directory=some/sub/dir/ --index patch &&
-	test content = $(git show :some/sub/dir/newfile) &&
-	test content = $(cat some/sub/dir/newfile)
+	echo content >expect &&
+	git show :some/sub/dir/newfile >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/newfile
 '
 
 cat > patch << EOF
@@ -72,8 +75,10 @@ EOF
 test_expect_success 'apply --directory -p (new file)' '
 	git reset --hard initial &&
 	git apply -p2 --directory=some/sub/dir/ --index patch &&
-	test content = $(git show :some/sub/dir/newfile2) &&
-	test content = $(cat some/sub/dir/newfile2)
+	echo content >expect &&
+	git show :some/sub/dir/newfile2 >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/newfile2
 '
 
 cat > patch << EOF
@@ -107,8 +112,10 @@ EOF
 test_expect_success 'apply --directory (quoted filename)' '
 	git reset --hard initial &&
 	git apply --directory=some/sub/dir/ --index patch &&
-	test content = $(git show :some/sub/dir/quotefile) &&
-	test content = $(cat some/sub/dir/quotefile)
+	echo content >expect &&
+	git show :some/sub/dir/quotefile >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/quotefile
 '
 
 test_done
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index 0de83e36199..a60153f9f32 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -63,9 +63,12 @@ test_expect_success '"mixed" reset is not allowed in bare' '
 	test_must_fail git reset --mixed HEAD^
 '
 
-test_expect_success '"soft" reset is allowed in bare' '
+test_expect_success !SANITIZE_LEAK '"soft" reset is allowed in bare' '
 	git reset --soft HEAD^ &&
-	test "$(git show --pretty=format:%s | head -n 1)" = "one"
+	git show --pretty=format:%s >out &&
+	echo one >expect &&
+	head -n 1 out >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.35.1.1226.g8b497615d32

[PATCH 02/15] tests: use "test_stdout_line_count", not "test $(git [...] | wc -l)"

[Ævar Arnfjörð Bjarmason]

Use the test_stdout_line_count helper added in
cdff1bb5a3d (test-lib-functions: introduce test_stdout_line_count,
2021-07-04) so that we'll spot if git itself dies, segfaults etc in
these expressions.

Because we didn't distinguish these failure conditions before I'd
mistakenly marked these tests as passing under SANITIZE=leak in
dd9cede9136 (leak tests: mark some rev-list tests as passing with
SANITIZE=leak, 2021-10-31).

While we're at it let's re-indent these lines to match our usual
style, as we're having to change all of them anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6005-rev-list-count.sh | 43 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/t/t6005-rev-list-count.sh b/t/t6005-rev-list-count.sh
index 86542c650e2..e960049f647 100755
--- a/t/t6005-rev-list-count.sh
+++ b/t/t6005-rev-list-count.sh
@@ -2,7 +2,6 @@
 
 test_description='git rev-list --max-count and --skip test'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
@@ -14,39 +13,39 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'no options' '
-    test $(git rev-list HEAD | wc -l) = 5
+	test_stdout_line_count = 5 git rev-list HEAD
 '
 
 test_expect_success '--max-count' '
-    test $(git rev-list HEAD --max-count=0 | wc -l) = 0 &&
-    test $(git rev-list HEAD --max-count=3 | wc -l) = 3 &&
-    test $(git rev-list HEAD --max-count=5 | wc -l) = 5 &&
-    test $(git rev-list HEAD --max-count=10 | wc -l) = 5
+	test_stdout_line_count = 0 git rev-list HEAD --max-count=0 &&
+	test_stdout_line_count = 3 git rev-list HEAD --max-count=3 &&
+	test_stdout_line_count = 5 git rev-list HEAD --max-count=5 &&
+	test_stdout_line_count = 5 git rev-list HEAD --max-count=10
 '
 
 test_expect_success '--max-count all forms' '
-    test $(git rev-list HEAD --max-count=1 | wc -l) = 1 &&
-    test $(git rev-list HEAD -1 | wc -l) = 1 &&
-    test $(git rev-list HEAD -n1 | wc -l) = 1 &&
-    test $(git rev-list HEAD -n 1 | wc -l) = 1
+	test_stdout_line_count = 1 git rev-list HEAD --max-count=1 &&
+	test_stdout_line_count = 1 git rev-list HEAD -1 &&
+	test_stdout_line_count = 1 git rev-list HEAD -n1 &&
+	test_stdout_line_count = 1 git rev-list HEAD -n 1
 '
 
 test_expect_success '--skip' '
-    test $(git rev-list HEAD --skip=0 | wc -l) = 5 &&
-    test $(git rev-list HEAD --skip=3 | wc -l) = 2 &&
-    test $(git rev-list HEAD --skip=5 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=10 | wc -l) = 0
+	test_stdout_line_count = 5 git rev-list HEAD --skip=0 &&
+	test_stdout_line_count = 2 git rev-list HEAD --skip=3 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=5 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=10
 '
 
 test_expect_success '--skip --max-count' '
-    test $(git rev-list HEAD --skip=0 --max-count=0 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=0 --max-count=10 | wc -l) = 5 &&
-    test $(git rev-list HEAD --skip=3 --max-count=0 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=3 --max-count=1 | wc -l) = 1 &&
-    test $(git rev-list HEAD --skip=3 --max-count=2 | wc -l) = 2 &&
-    test $(git rev-list HEAD --skip=3 --max-count=10 | wc -l) = 2 &&
-    test $(git rev-list HEAD --skip=5 --max-count=10 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=10 --max-count=10 | wc -l) = 0
+	test_stdout_line_count = 0 git rev-list HEAD --skip=0 --max-count=0 &&
+	test_stdout_line_count = 5 git rev-list HEAD --skip=0 --max-count=10 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=3 --max-count=0 &&
+	test_stdout_line_count = 1 git rev-list HEAD --skip=3 --max-count=1 &&
+	test_stdout_line_count = 2 git rev-list HEAD --skip=3 --max-count=2 &&
+	test_stdout_line_count = 2 git rev-list HEAD --skip=3 --max-count=10 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=5 --max-count=10 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=10 --max-count=10
 '
 
 test_done
-- 
2.35.1.1226.g8b497615d32

[PATCH 03/15] read-tree tests: check "diff-files" exit code on failure

[Ævar Arnfjörð Bjarmason]

Fix an issue with the exit code of "diff-files" being ignored, which
has been ignored ever since these tests were originally added in
c859600954d ([PATCH] read-tree: save more user hassles during
fast-forward., 2005-06-07).

Since the exit code was ignored we'd hide errors here under
SANITIZE=leak, which resulted in me mistakenly marking these tests as
passing under SANITIZE=leak in e5a917fcf42 (unpack-trees: don't leak
memory in verify_clean_subdirectory(), 2021-10-07) and
4ea08416b8e (leak tests: mark a read-tree test as passing
SANITIZE=leak, 2021-10-31).

As it would be non-trivial to fix these tests (the leak is in
revision.c) let's un-mark them as passing under SANITIZE=leak in
addition to fixing the issue of ignoring the exit code.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1001-read-tree-m-2way.sh   | 6 +++---
 t/t1002-read-tree-m-u-2way.sh | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index d1115528cb9..0710b1fb1e9 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -21,7 +21,6 @@ In the test, these paths are used:
 	yomin   - not in H or M
 '
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
@@ -38,11 +37,12 @@ compare_change () {
 }
 
 check_cache_at () {
-	clean_if_empty=$(git diff-files -- "$1")
+	git diff-files -- "$1" >out &&
+	clean_if_empty=$(cat out) &&
 	case "$clean_if_empty" in
 	'')  echo "$1: clean" ;;
 	?*)  echo "$1: dirty" ;;
-	esac
+	esac &&
 	case "$2,$clean_if_empty" in
 	clean,)		:     ;;
 	clean,?*)	false ;;
diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
index ca5c5510c73..46cbd5514a6 100755
--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh
@@ -9,7 +9,6 @@ This is identical to t1001, but uses -u to update the work tree as well.
 
 '
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
@@ -23,11 +22,12 @@ compare_change () {
 }
 
 check_cache_at () {
-	clean_if_empty=$(git diff-files -- "$1")
+	git diff-files -- "$1" >out &&
+	clean_if_empty=$(cat out) &&
 	case "$clean_if_empty" in
 	'')  echo "$1: clean" ;;
 	?*)  echo "$1: dirty" ;;
-	esac
+	esac &&
 	case "$2,$clean_if_empty" in
 	clean,)		:     ;;
 	clean,?*)	false ;;
-- 
2.35.1.1226.g8b497615d32

Re: [PATCH 03/15] read-tree tests: check "diff-files" exit code on failure

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  check_cache_at () {
> -	clean_if_empty=$(git diff-files -- "$1")
> +	git diff-files -- "$1" >out &&
> +	clean_if_empty=$(cat out) &&
>  	case "$clean_if_empty" in
>  	'')  echo "$1: clean" ;;
>  	?*)  echo "$1: dirty" ;;
> -	esac
> +	esac &&

Good to see such an attention to the detail.

>  	case "$2,$clean_if_empty" in
>  	clean,)		:     ;;
>  	clean,?*)	false ;;
> diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
> index ca5c5510c73..46cbd5514a6 100755
> --- a/t/t1002-read-tree-m-u-2way.sh
> +++ b/t/t1002-read-tree-m-u-2way.sh
> @@ -9,7 +9,6 @@ This is identical to t1001, but uses -u to update the work tree as well.
>  
>  '
>  
> -TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-read-tree.sh
>  
> @@ -23,11 +22,12 @@ compare_change () {
>  }
>  
>  check_cache_at () {
> -	clean_if_empty=$(git diff-files -- "$1")
> +	git diff-files -- "$1" >out &&
> +	clean_if_empty=$(cat out) &&
>  	case "$clean_if_empty" in
>  	'')  echo "$1: clean" ;;
>  	?*)  echo "$1: dirty" ;;
> -	esac
> +	esac &&
>  	case "$2,$clean_if_empty" in
>  	clean,)		:     ;;
>  	clean,?*)	false ;;

[PATCH 04/15] diff tests: don't ignore "git diff" exit code

[Ævar Arnfjörð Bjarmason]

Fix a test pattern that originated in f1af60bdba4 (Support 'diff=pgm'
attribute, 2007-04-22) so that we'll stop using "git diff" on the
left-hand-side of a pipe, and thus ignoring its exit code.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4020-diff-external.sh | 49 ++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 54bb8ef27e7..879ee04d291 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -37,17 +37,15 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment' '
 
 '
 
-test_expect_success 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
-
-	GIT_EXTERNAL_DIFF=echo git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+test_expect_success !SANITIZE_LEAK 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
+	GIT_EXTERNAL_DIFF=echo git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
 test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
-
-	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
@@ -83,16 +81,16 @@ test_expect_success 'diff.external' '
 	}
 '
 
-test_expect_success 'diff.external should apply only to diff' '
+test_expect_success !SANITIZE_LEAK 'diff.external should apply only to diff' '
 	test_config diff.external echo &&
-	git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+	git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 '
 
 test_expect_success 'diff.external and --no-ext-diff' '
 	test_config diff.external echo &&
-	git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 '
 
 test_expect_success 'diff attribute' '
@@ -115,17 +113,15 @@ test_expect_success 'diff attribute' '
 
 '
 
-test_expect_success 'diff attribute should apply only to diff' '
-
-	git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
+	git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
 test_expect_success 'diff attribute and --no-ext-diff' '
-
-	git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
@@ -148,17 +144,15 @@ test_expect_success 'diff attribute' '
 
 '
 
-test_expect_success 'diff attribute should apply only to diff' '
-
-	git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
+	git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
 test_expect_success 'diff attribute and --no-ext-diff' '
-
-	git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
@@ -177,7 +171,8 @@ test_expect_success 'attributes trump GIT_EXTERNAL_DIFF and diff.external' '
 
 test_expect_success 'no diff with -diff' '
 	echo >.gitattributes "file -diff" &&
-	git diff | grep Binary
+	git diff >out &&
+	grep Binary out
 '
 
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file
-- 
2.35.1.1226.g8b497615d32

[PATCH 05/15] diff tests: don't ignore "git diff" exit code in "read" loop

[Ævar Arnfjörð Bjarmason]

Fix a test pattern that originated in f1af60bdba4 (Support 'diff=pgm'
attribute, 2007-04-22) so that we'll stop using "git diff" on the
left-hand-side of a pipe, and thus ignoring its exit code.

Rather than use intermediate files let's rewrite these tests to a much
simpler but more exhaustive "test_tmp" where we'll ignore certain
fields in the output.

Note that this is not a faithful conversion of the previous
"read/test" in some cases, as we were ignoring more fields there than
we strictly needed to. Now we'll "test_cmp" everything we can, and
only ignore the likes of paths to $TEMPDIR etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4020-diff-external.sh | 104 ++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 51 deletions(-)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 879ee04d291..1219f8bd4c0 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -24,16 +24,12 @@ test_expect_success setup '
 '
 
 test_expect_success 'GIT_EXTERNAL_DIFF environment' '
-
-	GIT_EXTERNAL_DIFF=echo git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff >out &&
+	cut -d" " -f1,3- <out >actual &&
+	test_cmp expect actual
 
 '
 
@@ -52,15 +48,14 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
 test_expect_success SYMLINKS 'typechange diff' '
 	rm -f file &&
 	ln -s elif file &&
-	GIT_EXTERNAL_DIFF=echo git diff  | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z120000 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	} &&
+
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 120000
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual &&
+
 	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff >actual &&
 	git diff >expect &&
 	test_cmp expect actual
@@ -70,15 +65,13 @@ test_expect_success 'diff.external' '
 	git reset --hard &&
 	echo third >file &&
 	test_config diff.external echo &&
-	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
+
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 100644
+	EOF
+	git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success !SANITIZE_LEAK 'diff.external should apply only to diff' '
@@ -101,16 +94,12 @@ test_expect_success 'diff attribute' '
 
 	echo >.gitattributes "file diff=parrot" &&
 
-	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
-
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 100644
+	EOF
+	git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
@@ -132,16 +121,12 @@ test_expect_success 'diff attribute' '
 
 	echo >.gitattributes "file diff=color" &&
 
-	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
-
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 100644
+	EOF
+	git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
@@ -159,14 +144,26 @@ test_expect_success 'diff attribute and --no-ext-diff' '
 test_expect_success 'GIT_EXTERNAL_DIFF trumps diff.external' '
 	>.gitattributes &&
 	test_config diff.external "echo ext-global" &&
-	GIT_EXTERNAL_DIFF="echo ext-env" git diff | grep ext-env
+
+	cat >expect <<-EOF &&
+	ext-env file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF="echo ext-env" git diff >out &&
+	cut -d" " -f1-2,4- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'attributes trump GIT_EXTERNAL_DIFF and diff.external' '
 	test_config diff.foo.command "echo ext-attribute" &&
 	test_config diff.external "echo ext-global" &&
 	echo "file diff=foo" >.gitattributes &&
-	GIT_EXTERNAL_DIFF="echo ext-env" git diff | grep ext-attribute
+
+	cat >expect <<-EOF &&
+	ext-attribute file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF="echo ext-env" git diff >out &&
+	cut -d" " -f1-2,4- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'no diff with -diff' '
@@ -212,7 +209,12 @@ test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '
 	touch file.ext &&
 	git add file.ext &&
 	echo with extension > file.ext &&
-	GIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\.ext &&
+
+	cat >expect <<-EOF &&
+	file.ext file $(git rev-parse --verify HEAD:file) 100644 file.ext $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff file.ext >out &&
+	cut -d" " -f1,3- <out >actual &&
 	git update-index --force-remove file.ext &&
 	rm file.ext
 '
-- 
2.35.1.1226.g8b497615d32

[PATCH 06/15] apply tests: use "test_must_fail" instead of ad-hoc pattern

[Ævar Arnfjörð Bjarmason]

Change a fragile test pattern introduced in 6b763c424e4 (git-apply: do
not read past the end of buffer, 2007-09-05). Before this we wouldn't
distinguish normal "git apply" failures from segfaults or abort().

I'd previously marked this test as passing under SANITIZE=leak in
f54f48fc074 (leak tests: mark some apply tests as passing with
SANITIZE=leak, 2021-10-31). Let's remove that annotation as this test
will no longer pass.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4123-apply-shrink.sh | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh
index dfa053ff28e..3ef84619f53 100755
--- a/t/t4123-apply-shrink.sh
+++ b/t/t4123-apply-shrink.sh
@@ -2,8 +2,6 @@
 
 test_description='apply a patch that is larger than the preimage'
 
-
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >F  <<\EOF
@@ -41,20 +39,8 @@ test_expect_success setup '
 '
 
 test_expect_success 'apply should fail gracefully' '
-
-	if git apply --index patch
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		status=$? &&
-		echo "Status was $status" &&
-		if test -f .git/index.lock
-		then
-			echo Oops, should not have crashed
-			false
-		fi
-	fi
+	test_must_fail git apply --index patch &&
+	test_path_is_missing .git/index.lock
 '
 
 test_done
-- 
2.35.1.1226.g8b497615d32

Re: [PATCH 06/15] apply tests: use "test_must_fail" instead of ad-hoc pattern

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  test_expect_success 'apply should fail gracefully' '
> -
> -	if git apply --index patch
> -	then
> -		echo Oops, should not have succeeded
> -		false
> -	else
> -		status=$? &&
> -		echo "Status was $status" &&
> -		if test -f .git/index.lock
> -		then
> -			echo Oops, should not have crashed
> -			false
> -		fi
> -	fi
> +	test_must_fail git apply --index patch &&
> +	test_path_is_missing .git/index.lock
>  '

Wow, that is very old fashioned.  Thanks for cleaning up the
leftover mess from the days before test_must_fail was prevalent.

[PATCH 07/15] merge tests: use "test_must_fail" instead of ad-hoc pattern

[Ævar Arnfjörð Bjarmason]

As in the preceding commit change a similar fragile test pattern
introduced in b798671fa93 (merge-recursive: do not rudely die on
binary merge, 2007-08-14) to use a "test_must_fail" instead.

Before this we wouldn't distinguish normal "git merge" failures from
segfaults or abort(). Unlike the preceding commit we didn't end up
hiding any SANITIZE=leak failures in this case, but let's
correspondingly change these anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6407-merge-binary.sh | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh
index 8e6241f92e6..0753fc95f45 100755
--- a/t/t6407-merge-binary.sh
+++ b/t/t6407-merge-binary.sh
@@ -43,14 +43,9 @@ test_expect_success resolve '
 	rm -f a* m* &&
 	git reset --hard anchor &&
 
-	if git merge -s resolve main
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		git ls-files -s >current &&
-		test_cmp expect current
-	fi
+	test_must_fail git merge -s resolve main &&
+	git ls-files -s >current &&
+	test_cmp expect current
 '
 
 test_expect_success recursive '
@@ -58,14 +53,9 @@ test_expect_success recursive '
 	rm -f a* m* &&
 	git reset --hard anchor &&
 
-	if git merge -s recursive main
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		git ls-files -s >current &&
-		test_cmp expect current
-	fi
+	test_must_fail git merge -s recursive main &&
+	git ls-files -s >current &&
+	test_cmp expect current
 '
 
 test_done
-- 
2.35.1.1226.g8b497615d32

[PATCH 08/15] rev-parse tests: don't ignore "git reflog" exit code

[Ævar Arnfjörð Bjarmason]

Amend a test added in 9c46c054ae4 (rev-parse: tests git rev-parse
--verify master@{n}, for various n, 2010-08-24) so that we'll stop
ignoring the exit code of "git reflog" by having it on the
left-hand-side of a pipe.

Because of this I'd marked this test as passing under SANITIZE=leak in
f442c94638d (leak tests: mark some rev-parse tests as passing with
SANITIZE=leak, 2021-10-31). As all of it except this specific test
will now pass, let's skip it under the !SANITIZE_LEAK prerequisite.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1503-rev-parse-verify.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 94fe413ee37..ba43168d123 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -132,8 +132,9 @@ test_expect_success 'use --default' '
 	test_must_fail git rev-parse --verify --default bar
 '
 
-test_expect_success 'main@{n} for various n' '
-	N=$(git reflog | wc -l) &&
+test_expect_success !SANITIZE_LEAK 'main@{n} for various n' '
+	git reflog >out &&
+	N=$(wc -l <out) &&
 	Nm1=$(($N-1)) &&
 	Np1=$(($N+1)) &&
 	git rev-parse --verify main@{0} &&
-- 
2.35.1.1226.g8b497615d32

[PATCH 09/15] notes tests: don't ignore "git" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile test pattern that's been with us ever since these
tests were introduced in [1], [2] and [3] to properly return the exit
code of the failing command on failure.

Because of this I'd marked this test as passing under SANITIZE=leak in
[4] and [5]. We need to remove those annotations as these tests will
no longer pass.

1. 9081a421a6d (checkout: fix "branch info" memory leaks, 2021-11-16)
2. 0057c0917d3 (Add selftests verifying that we can parse notes trees
   with various fanouts, 2009-10-09)
3. 048cdd4665e (t3305: Verify that adding many notes with git-notes
   triggers increased fanout, 2010-02-13)
4. ca089724952 (leak tests: mark some notes tests as passing with
   SANITIZE=leak, 2021-10-31)
5. 9081a421a6d (checkout: fix "branch info" memory leaks, 2021-11-16)
---
 t/t3302-notes-index-expensive.sh |  6 +++---
 t/t3303-notes-subtrees.sh        |  9 ++++-----
 t/t3305-notes-fanout.sh          | 14 +++++++-------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index bc9d8ee1e6a..bb5fea02a03 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -8,7 +8,6 @@ test_description='Test commit notes index (expensive!)'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_repo () {
@@ -65,7 +64,8 @@ create_repo () {
 test_notes () {
 	count=$1 &&
 	git config core.notesRef refs/notes/commits &&
-	git log | grep "^    " >output &&
+	git log >tmp &&
+	grep "^    " tmp >output &&
 	i=$count &&
 	while test $i -gt 0
 	do
@@ -90,7 +90,7 @@ write_script time_notes <<\EOF
 			unset GIT_NOTES_REF
 			;;
 		esac
-		git log
+		git log || exit $?
 		i=$(($i+1))
 	done >/dev/null
 EOF
diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh
index 7e0a8960af8..eac193757bf 100755
--- a/t/t3303-notes-subtrees.sh
+++ b/t/t3303-notes-subtrees.sh
@@ -5,7 +5,6 @@ test_description='Test commit notes organized in subtrees'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 number_of_commits=100
@@ -79,7 +78,7 @@ test_sha1_based () {
 	(
 		start_note_commit &&
 		nr=$number_of_commits &&
-		git rev-list refs/heads/main |
+		git rev-list refs/heads/main >out &&
 		while read sha1; do
 			note_path=$(echo "$sha1" | sed "$1")
 			cat <<INPUT_END &&
@@ -91,9 +90,9 @@ EOF
 INPUT_END
 
 			nr=$(($nr-1))
-		done
-	) |
-	git fast-import --quiet
+		done <out
+	) >gfi &&
+	git fast-import --quiet <gfi
 }
 
 test_expect_success 'test notes in 2/38-fanout' 'test_sha1_based "s|^..|&/|"'
diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh
index 1f5964865ae..9976d787f47 100755
--- a/t/t3305-notes-fanout.sh
+++ b/t/t3305-notes-fanout.sh
@@ -2,7 +2,6 @@
 
 test_description='Test that adding/removing many notes triggers automatic fanout restructuring'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 path_has_fanout() {
@@ -24,7 +23,7 @@ touched_one_note_with_fanout() {
 all_notes_have_fanout() {
 	notes_commit=$1 &&
 	fanout=$2 &&
-	git ls-tree -r --name-only $notes_commit 2>/dev/null |
+	git ls-tree -r --name-only $notes_commit |
 	while read path
 	do
 		path_has_fanout $path $fanout || return 1
@@ -51,8 +50,9 @@ test_expect_success 'creating many notes with git-notes' '
 	done
 '
 
-test_expect_success 'many notes created correctly with git-notes' '
-	git log | grep "^    " > output &&
+test_expect_success !SANITIZE_LEAK 'many notes created correctly with git-notes' '
+	git log >output.raw &&
+	grep "^    " output.raw >output &&
 	i=$num_notes &&
 	while test $i -gt 0
 	do
@@ -91,13 +91,13 @@ test_expect_success 'stable fanout 0 is followed by stable fanout 1' '
 test_expect_success 'deleting most notes with git-notes' '
 	remove_notes=285 &&
 	i=0 &&
-	git rev-list HEAD |
+	git rev-list HEAD >revs &&
 	while test $i -lt $remove_notes && read sha1
 	do
 		i=$(($i + 1)) &&
 		test_tick &&
-		git notes remove "$sha1" 2>/dev/null || return 1
-	done
+		git notes remove "$sha1" || return 1
+	done <revs
 '
 
 test_expect_success 'most notes deleted correctly with git-notes' '
-- 
2.35.1.1226.g8b497615d32

[PATCH 10/15] diff tests: don't ignore "git rev-list" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile pattern introduced in 2b459b483cb (diff: make sure
work tree side is shown as 0{40} when different, 2008-03-02) to check
the exit code of "git rev-list", while we're at it let's get rid of
the needless sub-shell for invoking it in favor of the "-C" option.

Because of this I'd marked these tests as passing under SANITIZE=leak
in 16d4bd4f14e (leak tests: mark some diff tests as passing with
SANITIZE=leak, 2021-10-31), let's remove the
"TEST_PASSES_SANITIZE_LEAK=true" annotation as they no longer do.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4027-diff-submodule.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 6cef0da982f..295da987cce 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -2,7 +2,6 @@
 
 test_description='difference in submodules'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
@@ -28,10 +27,8 @@ test_expect_success setup '
 		git commit -m "submodule #2"
 	) &&
 
-	set x $(
-		cd sub &&
-		git rev-list HEAD
-	) &&
+	git -C sub rev-list HEAD >revs &&
+	set x $(cat revs) &&
 	echo ":160000 160000 $3 $ZERO_OID M	sub" >expect &&
 	subtip=$3 subprev=$2
 '
-- 
2.35.1.1226.g8b497615d32

[PATCH 11/15] rev-list tests: don't hide abort() in "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

Change a couple of uses of "test_expect_failure" to use a
"test_expect_success" to positively assert the current behavior, and
replace the intent of "test_expect_failure" with a "TODO" comment int
the description.

As noted in [1] the "test_expect_failure" feature is overly eager to
accept any failure as OK, and thus by design hides segfaults, abort()
etc. Because of that I didn't notice in dd9cede9136 (leak tests: mark
some rev-list tests as passing with SANITIZE=leak, 2021-10-31) that
this test leaks memory under SANITIZE=leak.

I have some larger local changes to add a better
"test_expect_failure", which would work just like
"test_expect_success", but would allow us say "test_todo" here (and
"success" would emit a "not ok [...] # TODO", not "ok [...]".

In lieu of those larger changes let's more narrowly fix the problem at
hand here and stop conflating the current "success" with actual
SANITIZE=leak failures.

1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6102-rev-list-unexpected-objects.sh | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
index 6f0902b8638..cf0195e8263 100755
--- a/t/t6102-rev-list-unexpected-objects.sh
+++ b/t/t6102-rev-list-unexpected-objects.sh
@@ -17,8 +17,13 @@ test_expect_success 'setup unexpected non-blob entry' '
 	broken_tree="$(git hash-object -w --literally -t tree broken-tree)"
 '
 
-test_expect_failure 'traverse unexpected non-blob entry (lone)' '
-	test_must_fail git rev-list --objects $broken_tree
+test_expect_success !SANITIZE_LEAK 'TODO (should fail!): traverse unexpected non-blob entry (lone)' '
+	sed "s/Z$//" >expect <<-EOF &&
+	$broken_tree Z
+	$tree foo
+	EOF
+	git rev-list --objects $broken_tree >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'traverse unexpected non-blob entry (seen)' '
@@ -116,8 +121,8 @@ test_expect_success 'setup unexpected non-blob tag' '
 	tag=$(git hash-object -w --literally -t tag broken-tag)
 '
 
-test_expect_failure 'traverse unexpected non-blob tag (lone)' '
-	test_must_fail git rev-list --objects $tag
+test_expect_success !SANITIZE_LEAK 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '
+	git rev-list --objects $tag
 '
 
 test_expect_success 'traverse unexpected non-blob tag (seen)' '
-- 
2.35.1.1226.g8b497615d32

Re: [PATCH 11/15] rev-list tests: don't hide abort() in "test_expect_failure"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change a couple of uses of "test_expect_failure" to use a
> "test_expect_success" to positively assert the current behavior, and
> replace the intent of "test_expect_failure" with a "TODO" comment int
> the description.
>
> As noted in [1] the "test_expect_failure" feature is overly eager to

And noted in [2], it is not a good idea to abuse "test_expect_success"
for this purpose, either, though.

[2] https://lore.kernel.org/git/xmqq4k9kj15p.fsf@gitster.g/

Re: [PATCH 11/15] rev-list tests: don't hide abort() in "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change a couple of uses of "test_expect_failure" to use a
>> "test_expect_success" to positively assert the current behavior, and
>> replace the intent of "test_expect_failure" with a "TODO" comment int
>> the description.
>>
>> As noted in [1] the "test_expect_failure" feature is overly eager to
>
> And noted in [2], it is not a good idea to abuse "test_expect_success"
> for this purpose, either, though.
>
> [2] https://lore.kernel.org/git/xmqq4k9kj15p.fsf@gitster.g/

As noted I do have a "test_todo" (or "test_expect_todo") replacement for
"test_expect_failure" which I think I think will address your concern
there.

But do you mind if this is left like this for now? Due to the semantics
of "test_expect_failure" we can't use it in conjunction with
"test_must_fail" currently and not hide segfaults or abort().

So having it marked as "ok ... # TODO" v.s. "not ok ... # TODO" isn't
ideal, but certainly better than silently hiding abort() and segfaults.

[PATCH 12/15] gettext tests: don't ignore "test-tool regex" exit code

[Ævar Arnfjörð Bjarmason]

Amend a prerequisite check added in 5c1ebcca4d1 (grep/icase: avoid
kwsset on literal non-ascii strings, 2016-06-25) to do invoke
'test-tool regex' in such a way that we'll notice if it dies under
SANITIZE=leak due to having a memory leak, as opposed to us not having
the "ICASE" support we're checking for.

Because we weren't making a distinction between the two I'd marked
these tests as passing under SANITIZE=leak in 03d85e21951 (leak tests:
mark remaining leak-free tests as such, 2021-12-17).

Doing this is tricky. Ideally "test_lazy_prereq" would materialize as
a "real" test that we could check the exit code of with the same
signal matching that "test_must_fail" does.

However lazy prerequisites aren't real tests, and are instead lazily
materialized in the guts of "test_have_prereq" when we've already
started another test.

We could detect the abort() (or similar) there and pass that exit code
down, and fail the test that caused the prerequisites to be
materialized.

But that would require extensive changes to test-lib.sh and
test-lib-functions.sh. Let's instead simply check if the exit code of
"test-tool regex" is zero, and if so set the prerequisites. If it's
non-zero let's run it again with "test_must_fail". We'll thus make a
distinction between "bad" non-zero (segv etc) and "good" (exit 1 etc.).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7812-grep-icase-non-ascii.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
index ca3f24f8079..347bf4a12f3 100755
--- a/t/t7812-grep-icase-non-ascii.sh
+++ b/t/t7812-grep-icase-non-ascii.sh
@@ -11,9 +11,14 @@ test_expect_success GETTEXT_LOCALE 'setup' '
 	export LC_ALL
 '
 
-test_have_prereq GETTEXT_LOCALE &&
-test-tool regex "HALLÓ" "Halló" ICASE &&
-test_set_prereq REGEX_LOCALE
+test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' '
+	if test-tool regex "HALLÓ" "Halló" ICASE
+	then
+		test_set_prereq REGEX_LOCALE
+	else
+		test_must_fail test-tool regex "HALLÓ" "Halló" ICASE
+	fi
+'
 
 test_expect_success REGEX_LOCALE 'grep literal string, no -F' '
 	git grep -i "TILRAUN: Halló Heimur!" &&
-- 
2.35.1.1226.g8b497615d32

Re: [PATCH 12/15] gettext tests: don't ignore "test-tool regex" exit code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -test_have_prereq GETTEXT_LOCALE &&
> -test-tool regex "HALLÓ" "Halló" ICASE &&
> -test_set_prereq REGEX_LOCALE
> +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' '
> +	if test-tool regex "HALLÓ" "Halló" ICASE
> +	then
> +		test_set_prereq REGEX_LOCALE

This looks sensible but

> +	else
> +		test_must_fail test-tool regex "HALLÓ" "Halló" ICASE

this side looks puzzling.  I think this way to avoid counting abort
etc as passing "must fail" test would be the least bad that we can
do.

Nicely done.



> +	fi
> +'

Re: [PATCH 12/15] gettext tests: don't ignore "test-tool regex" exit code

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -test_have_prereq GETTEXT_LOCALE &&
>> -test-tool regex "HALLÓ" "Halló" ICASE &&
>> -test_set_prereq REGEX_LOCALE
>> +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' '
>> +	if test-tool regex "HALLÓ" "Halló" ICASE
>> +	then
>> +		test_set_prereq REGEX_LOCALE
>
> This looks sensible but
>
>> +	else
>> +		test_must_fail test-tool regex "HALLÓ" "Halló" ICASE
>
> this side looks puzzling.  I think this way to avoid counting abort
> etc as passing "must fail" test would be the least bad that we can
> do.
>
> Nicely done.

Thanks. For the purposes of a re-roll I'll note this as a "nothing to
change", since the commit message explains why we're doing this, unless
you have comments on that explanation (the last paragraph of the commit
message).

Re: [PATCH 12/15] gettext tests: don't ignore "test-tool regex" exit code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Mar 02 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> -test_have_prereq GETTEXT_LOCALE &&
>>> -test-tool regex "HALLÓ" "Halló" ICASE &&
>>> -test_set_prereq REGEX_LOCALE
>>> +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' '
>>> +	if test-tool regex "HALLÓ" "Halló" ICASE
>>> +	then
>>> +		test_set_prereq REGEX_LOCALE
>>
>> This looks sensible but
>>
>>> +	else
>>> +		test_must_fail test-tool regex "HALLÓ" "Halló" ICASE
>>
>> this side looks puzzling.  I think this way to avoid counting abort
>> etc as passing "must fail" test would be the least bad that we can
>> do.
>>
>> Nicely done.
>
> Thanks. For the purposes of a re-roll I'll note this as a "nothing to
> change", since the commit message explains why we're doing this, unless
> you have comments on that explanation (the last paragraph of the commit
> message).

The comment was mostly if it is more appropriate to explain the
puzzling code with an in-code comment, rather than the log message.

In-code comment is for those who may find the current code strange.
The log message is for those who wonder why the current code came to
be in today's shape.

[PATCH 13/15] apply tests: don't ignore "git ls-files" exit code, drop sub-shell

[Ævar Arnfjörð Bjarmason]

Fix code added in 969c877506c (git apply --directory broken for new
files, 2008-10-12) so that it doesn't invoke "git ls-files" on the
left-hand-side of a pipe, instead let's use an intermediate file.

Since we're doing that we can also drop the sub-shell that was here to
group the two.

There are a lot of these sorts of patterns in the test suite, and
there's no particular reason to fix this one other than in a preceding
commit all similar patterns except this one were fixed in
"t/t4128-apply-root.sh", so let's fix this one straggler as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4128-apply-root.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh
index ba89a2f2d73..f6db5a79dd9 100755
--- a/t/t4128-apply-root.sh
+++ b/t/t4128-apply-root.sh
@@ -96,7 +96,8 @@ test_expect_success 'apply --directory (delete file)' '
 	echo content >some/sub/dir/delfile &&
 	git add some/sub/dir/delfile &&
 	git apply --directory=some/sub/dir/ --index patch &&
-	! (git ls-files | grep delfile)
+	git ls-files >out &&
+	! grep delfile out
 '
 
 cat > patch << 'EOF'
-- 
2.35.1.1226.g8b497615d32

[PATCH 14/15] checkout tests: don't ignore "git <cmd>" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile pattern introduced in 696acf45f96 (checkout:
implement "-" abbreviation, add docs and tests, 2009-01-17) to check
the exit code of both "git symbolic-ref" and "git rev-parse".

Without this change this test will become flaky e.g. under
SANITIZE=leak if some (but not all) memory leaks revealed by these
commands are fixed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t2012-checkout-last.sh | 51 +++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index 42601d5a310..1f6c4ed0428 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -21,14 +21,20 @@ test_expect_success 'first branch switch' '
 	git checkout other
 '
 
+test_cmp_symbolic_HEAD_ref () {
+	echo refs/heads/"$1" >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success '"checkout -" switches back' '
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" switches forth' '
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/other"
+	test_cmp_symbolic_HEAD_ref other
 '
 
 test_expect_success 'detach HEAD' '
@@ -37,12 +43,16 @@ test_expect_success 'detach HEAD' '
 
 test_expect_success '"checkout -" attaches again' '
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/other"
+	test_cmp_symbolic_HEAD_ref other
 '
 
 test_expect_success '"checkout -" detaches again' '
 	git checkout - &&
-	test "z$(git rev-parse HEAD)" = "z$(git rev-parse other)" &&
+
+	git rev-parse other >expect &&
+	git rev-parse HEAD >actual &&
+	test_cmp expect actual &&
+
 	test_must_fail git symbolic-ref HEAD
 '
 
@@ -63,31 +73,31 @@ more_switches () {
 test_expect_success 'switch to the last' '
 	more_switches &&
 	git checkout @{-1} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch2"
+	test_cmp_symbolic_HEAD_ref branch2
 '
 
 test_expect_success 'switch to second from the last' '
 	more_switches &&
 	git checkout @{-2} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch3"
+	test_cmp_symbolic_HEAD_ref branch3
 '
 
 test_expect_success 'switch to third from the last' '
 	more_switches &&
 	git checkout @{-3} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch4"
+	test_cmp_symbolic_HEAD_ref branch4
 '
 
 test_expect_success 'switch to fourth from the last' '
 	more_switches &&
 	git checkout @{-4} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch5"
+	test_cmp_symbolic_HEAD_ref branch5
 '
 
 test_expect_success 'switch to twelfth from the last' '
 	more_switches &&
 	git checkout @{-12} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch13"
+	test_cmp_symbolic_HEAD_ref branch13
 '
 
 test_expect_success 'merge base test setup' '
@@ -98,19 +108,28 @@ test_expect_success 'merge base test setup' '
 test_expect_success 'another...main' '
 	git checkout another &&
 	git checkout another...main &&
-	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify main^)"
+
+	git rev-parse --verify main^ >expect &&
+	git rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '...main' '
 	git checkout another &&
 	git checkout ...main &&
-	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify main^)"
+
+	git rev-parse --verify main^ >expect &&
+	git rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'main...' '
 	git checkout another &&
 	git checkout main... &&
-	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify main^)"
+
+	git rev-parse --verify main^ >expect &&
+	git rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '"checkout -" works after a rebase A' '
@@ -118,7 +137,7 @@ test_expect_success '"checkout -" works after a rebase A' '
 	git checkout other &&
 	git rebase main &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" works after a rebase A B' '
@@ -127,7 +146,7 @@ test_expect_success '"checkout -" works after a rebase A B' '
 	git checkout other &&
 	git rebase main moodle &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" works after a rebase -i A' '
@@ -135,7 +154,7 @@ test_expect_success '"checkout -" works after a rebase -i A' '
 	git checkout other &&
 	git rebase -i main &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" works after a rebase -i A B' '
@@ -144,7 +163,7 @@ test_expect_success '"checkout -" works after a rebase -i A B' '
 	git checkout other &&
 	git rebase main foodle &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_done
-- 
2.35.1.1226.g8b497615d32

[PATCH 15/15] rev-list simplify tests: don't ignore "git" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile test pattern introduced in 65347030590 (Topo-sort
before --simplify-merges, 2008-08-03) to check the exit code of both
"git name-rev" and "git log".

This test as a whole would fail under SANITIZE=leak, but we'd pass
several "failing" tests due to hiding these exit codes before we'd
spot git dying with abort(). Now we'll instead spot all of the
failures.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6012-rev-list-simplify.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
index 63fcccec32e..de1e87f1621 100755
--- a/t/t6012-rev-list-simplify.sh
+++ b/t/t6012-rev-list-simplify.sh
@@ -12,7 +12,9 @@ note () {
 }
 
 unnote () {
-	git name-rev --tags --annotate-stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
+	test_when_finished "rm -f tmp" &&
+	git name-rev --tags --annotate-stdin >tmp &&
+	sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g" <tmp
 }
 
 #
@@ -111,8 +113,8 @@ check_outcome () {
 	shift &&
 	param="$*" &&
 	test_expect_$outcome "log $param" '
-		git log --pretty="$FMT" --parents $param |
-		unnote >actual &&
+		git log --pretty="$FMT" --parents $param >out &&
+		unnote >actual <out &&
 		sed -e "s/^.*	\([^ ]*\) .*/\1/" >check <actual &&
 		test_cmp expect check
 	'
@@ -151,8 +153,8 @@ check_result 'L K I H G B' --exclude-first-parent-only --first-parent L ^F
 check_result 'E C B A' --full-history E -- lost
 test_expect_success 'full history simplification without parent' '
 	printf "%s\n" E C B A >expect &&
-	git log --pretty="$FMT" --full-history E -- lost |
-	unnote >actual &&
+	git log --pretty="$FMT" --full-history E -- lost >out &&
+	unnote >actual <out &&
 	sed -e "s/^.*	\([^ ]*\) .*/\1/" >check <actual &&
 	test_cmp expect check
 '
-- 
2.35.1.1226.g8b497615d32

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This series fixes issues where we ignored the exit code of "git" due
> to it being on the LHS of a pipe, or because we interpolated its
> output with $() in a "test" construct, or had missing &&- chains in
> helper functions etc.

Thanks.  I've looked at all the steps, left some comments, and it
was a pleasant read overall.  Writing and reviewing all these
changes, both of us must have too much time on our hands ;-)

Will queue.

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Derrick Stolee]

On 3/2/2022 12:27 PM, Ævar Arnfjörð Bjarmason wrote:
> This series fixes issues where we ignored the exit code of "git" due
> to it being on the LHS of a pipe, or because we interpolated its
> output with $() in a "test" construct, or had missing &&- chains in
> helper functions etc.
> 
> This series is not made by string-replacing things in our test suite,
> if it was it would be much larger. These are all tests I've seen real
> hide real failures under SANITIZE=leak, either on current "master", or
> in combination with various local leak fixes I've got unsubmitted.

My first reaction was to check that Subham was on the CC line (yes)
because they have been working in this space, too. Your focus on
examples that break SANITIZE=leak is appreciated so there is room
for that project.

> In cases where I was starting to fix a pattern in a file I'd fix the
> rest of the file if it was easy, but otherwise these are all cases
> where I ran SANITIZE=leak, had a test pass, but having ASAN_OPTIONS
> log to a file revealed that we had memory leaks within that test.

Neat trick.

The patches in this series clearly do the right transformations to
expose these errors at the appropriate time. The only time I got
hung up was patch 11 where test_expect_failure was swapped for
test_expect_success (while also dropping a test_must_fail inside
the test). The double-negative confused me at first, but in the end
the patch works as-is.

> As an aside we still have various potential issues with hidden
> segfaults etc. in the test suite after this that are tricked to solve,
> because:
> 
>  * Our tests will (mostly) catch segfaults and abort(), but if we
>    invoke a command that invokes another command it needs to ferry the
>    exit code up to us.
> 
>  * run-command.c notably does not do that, so for e.g. "git push"
>    tests where we expect a failure and an underlying "git" command
>    fails we won't ferry up the segfault or abort exit code.

Perhaps run-command.c could auto-exit for certain well-known error
codes that could only happen on certain kinds of failures (segfault,
for example). A simple die() might be something that is expected to
be handled by the top-level command in some cases.

>  * We have gitweb.perl and some other perl code ignoring return values
>    from close(), i.e. ignoring exit codes from "git rev-parse" et al.
> 
>  * We have in-tree shellscripts like "git-merge-one-file.sh" invoking
>    git commands, and if they fail returning "1", not ferrying up the
>    segfault or abort() exit code.

These are more involved and harder to evaluate. Add them to the pile
of projects for new contributors?

Thanks,
-Stolee

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Derrick Stolee wrote:

> On 3/2/2022 12:27 PM, Ævar Arnfjörð Bjarmason wrote:
>> This series fixes issues where we ignored the exit code of "git" due
>> to it being on the LHS of a pipe, or because we interpolated its
>> output with $() in a "test" construct, or had missing &&- chains in
>> helper functions etc.
>> 
>> This series is not made by string-replacing things in our test suite,
>> if it was it would be much larger. These are all tests I've seen real
>> hide real failures under SANITIZE=leak, either on current "master", or
>> in combination with various local leak fixes I've got unsubmitted.
>
> My first reaction was to check that Subham was on the CC line (yes)
> because they have been working in this space, too. Your focus on
> examples that break SANITIZE=leak is appreciated so there is room
> for that project.

Yeah, I'm certainly not meaning to steal this whole "hide exit code"
microproject, and there's *a lot* more to chew on there :) (and I have
no intention of doing the rest).

But hopefully this series also serves as a nice tour-de-force through
the test suite showing a wide variety of exit-code-hiding cases for any
future work in the area.

I.e. the "git on theon LHS of a pipe" is an overly narrow definition of
it.

But then again I think these should also all be covered in passing by
t/README's "don'ts" list.

>> In cases where I was starting to fix a pattern in a file I'd fix the
>> rest of the file if it was easy, but otherwise these are all cases
>> where I ran SANITIZE=leak, had a test pass, but having ASAN_OPTIONS
>> log to a file revealed that we had memory leaks within that test.
>
> Neat trick.
>
> The patches in this series clearly do the right transformations to
> expose these errors at the appropriate time. The only time I got
> hung up was patch 11 where test_expect_failure was swapped for
> test_expect_success (while also dropping a test_must_fail inside
> the test). The double-negative confused me at first, but in the end
> the patch works as-is.
>
>> As an aside we still have various potential issues with hidden
>> segfaults etc. in the test suite after this that are tricked to solve,
>> because:
>> 
>>  * Our tests will (mostly) catch segfaults and abort(), but if we
>>    invoke a command that invokes another command it needs to ferry the
>>    exit code up to us.
>> 
>>  * run-command.c notably does not do that, so for e.g. "git push"
>>    tests where we expect a failure and an underlying "git" command
>>    fails we won't ferry up the segfault or abort exit code.
>
> Perhaps run-command.c could auto-exit for certain well-known error
> codes that could only happen on certain kinds of failures (segfault,
> for example). A simple die() might be something that is expected to
> be handled by the top-level command in some cases.

Yes. I have a local WIP patch to make it do that.

Basically just set a a global "uh oh, one of our ran commands segfaulted
or abort()-ed, here's that exit status".

Then when we do the real exit() (which we always intercept due to the
trace2 logging needing to do so) an non-zero exit() of e.g. status 128
will be changed to the appropriate exit status of that segfault or
abort().

We'll thus ferry such exit codes upwards, clobbering whatever the
"desired" exit code of e.g. "git pull" would have been (but that's a
feature in this case).

One thing I got hung up on is that it's relatively straightforward for
the wait_or_whine() case in run-command.c, but I didn't look into how to
do that with the pthread_join() we do for finish_async(), which
e.g. happens if the multiplex'd dialog exits in this way.

>>  * We have gitweb.perl and some other perl code ignoring return values
>>    from close(), i.e. ignoring exit codes from "git rev-parse" et al.
>> 
>>  * We have in-tree shellscripts like "git-merge-one-file.sh" invoking
>>    git commands, and if they fail returning "1", not ferrying up the
>>    segfault or abort() exit code.
>
> These are more involved and harder to evaluate. Add them to the pile
> of projects for new contributors?

FWIW those are relatively easy, for the first:

    use v5.10.1;
    use autodie qw(close);

And for the second just go through the relatively small amount of such
remaining shell code and convert:

	if ! git ...
	then
		exit 1

To:

	git ...
	code=$?
        if test $code -ne 0
	then
		exit $code

Or whatever.
    

    

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Phillip Wood]

On 03/03/2022 02:02, Derrick Stolee wrote:
> On 3/2/2022 12:27 PM, Ævar Arnfjörð Bjarmason wrote:
>> As an aside we still have various potential issues with hidden
>> segfaults etc. in the test suite after this that are tricked to solve,
>> because:
>>
>>   * Our tests will (mostly) catch segfaults and abort(), but if we
>>     invoke a command that invokes another command it needs to ferry the
>>     exit code up to us.
>>
>>   * run-command.c notably does not do that, so for e.g. "git push"

I'm not sure what you mean by this, the return value of run_command() 
already indicates which signal if any killed the child see for example 
run_specified_editor() which re-raises SIGINT and SIGQUIT if the editor 
is killed by those signals.

>>     tests where we expect a failure and an underlying "git" command
>>     fails we won't ferry up the segfault or abort exit code.
 >
> Perhaps run-command.c could auto-exit for certain well-known error
> codes that could only happen on certain kinds of failures (segfault,
> for example). A simple die() might be something that is expected to
> be handled by the top-level command in some cases.

I think we need to be careful that run_command() does not re-raise a 
signal before the caller has a chance to do any cleanup. A caller to 
run_command() can already check the return value and choose to die based 
on that after doing any cleanup. If run_command() starts dying we'll end 
up adding more unsafe signal handlers to do the cleanup.

Best Wishes

Phillip

>>   * We have gitweb.perl and some other perl code ignoring return values
>>     from close(), i.e. ignoring exit codes from "git rev-parse" et al.
>>
>>   * We have in-tree shellscripts like "git-merge-one-file.sh" invoking
>>     git commands, and if they fail returning "1", not ferrying up the
>>     segfault or abort() exit code.
> 
> These are more involved and harder to evaluate. Add them to the pile
> of projects for new contributors?
> 
> Thanks,
> -Stolee

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 03 2022, Phillip Wood wrote:

> On 03/03/2022 02:02, Derrick Stolee wrote:
>> On 3/2/2022 12:27 PM, Ævar Arnfjörð Bjarmason wrote:
>>> As an aside we still have various potential issues with hidden
>>> segfaults etc. in the test suite after this that are tricked to solve,
>>> because:
>>>
>>>   * Our tests will (mostly) catch segfaults and abort(), but if we
>>>     invoke a command that invokes another command it needs to ferry the
>>>     exit code up to us.
>>>
>>>   * run-command.c notably does not do that, so for e.g. "git push"
>
> I'm not sure what you mean by this, the return value of run_command()
> already indicates which signal if any killed the child see for example 
> run_specified_editor() which re-raises SIGINT and SIGQUIT if the
> editor is killed by those signals.

Yes, it returns the correct status code, but that doesn't help with
(pseudo)code like:

	if (run_command("foo")) /* exits with e.g. 123 */
		die("oh no, foo failed"); /* exits with 128 */

I should have said "code using run-command.c does not do that...",
sorry.

I.e. if "pack-objects" or whatever invoked by a "git push" segfaults
we might exit with status 128, not the code that the underlying command
failed with, and thus lose the segfault or abort().

If you add the appropriate "log_path" to LSAN_OPTIONS and run the test
suite you can see where this fails. I'm adding a mode to test-lib.sh
sooner than later (waiting on the outstanding ab/test-leak-diag) to make
it trivial to report on those.

>>>     tests where we expect a failure and an underlying "git" command
>>>     fails we won't ferry up the segfault or abort exit code.
>>
>> Perhaps run-command.c could auto-exit for certain well-known error
>> codes that could only happen on certain kinds of failures (segfault,
>> for example). A simple die() might be something that is expected to
>> be handled by the top-level command in some cases.
>
> I think we need to be careful that run_command() does not re-raise a
> signal before the caller has a chance to do any cleanup. A caller to 
> run_command() can already check the return value and choose to die
> based on that after doing any cleanup. If run_command() starts dying
> we'll end up adding more unsafe signal handlers to do the cleanup.

I think the method I described in
https://lore.kernel.org/git/220303.86fsnz5o9w.gmgdl@evledraar.gmail.com/
in the side-thread doesn't suffer from those problems.

I.e. I think the solution to this is not to interrupt whatever code
calls run-command, we'll let it and whatever else the calling program
wants to do run to completion.

We'll just ignore whatever exit(status) we picked at the very end and
exit instead with the status of our a failing child process we invoked,
if any of them returned a status that "test_must_fail" would count as
"failed but BAD!"

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Yes, it returns the correct status code, but that doesn't help with
> (pseudo)code like:
>
> 	if (run_command("foo")) /* exits with e.g. 123 */
> 		die("oh no, foo failed"); /* exits with 128 */
>
> I should have said "code using run-command.c does not do that...",
> sorry.

Yeah, but even if callers of run_command() can tell "foo"
segfaulted, I do not think it is sensible to exit as if we
segfaulted (or, we _could_ actually die by segfaulting ourselves,
which is worse).

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 03 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Yes, it returns the correct status code, but that doesn't help with
>> (pseudo)code like:
>>
>> 	if (run_command("foo")) /* exits with e.g. 123 */
>> 		die("oh no, foo failed"); /* exits with 128 */
>>
>> I should have said "code using run-command.c does not do that...",
>> sorry.
>
> Yeah, but even if callers of run_command() can tell "foo"
> segfaulted, I do not think it is sensible to exit as if we
> segfaulted (or, we _could_ actually die by segfaulting ourselves,
> which is worse).

I should have made it clear that I'm thinking about this as a test-only
mode, so hidden behind some GIT_TEST_* variable, i.e. as a means to an
end in getting the test suite to spot the failure in the sub-process.

It's not the only way to do it, but it's the simplest and most reliable
given how our tests are run.

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Shubham Mishra]

Thanks Ævar for CCing me. Your patch is insightful for me.

> This series is not made by string-replacing things in our test suite,
> if it was it would be much larger. These are all tests I've seen real
> hide real failures under SANITIZE=leak, either on current "master", or
> in combination with various local leak fixes I've got unsubmitted.

Can you please tell me what "SANITIZE=leak" do?

Thanks,
Shubham

Re: [PATCH 00/15] tests: don't ignore "git" exit codes

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 03 2022, Shubham Mishra wrote:

> Thanks Ævar for CCing me. Your patch is insightful for me.
>
>> This series is not made by string-replacing things in our test suite,
>> if it was it would be much larger. These are all tests I've seen real
>> hide real failures under SANITIZE=leak, either on current "master", or
>> in combination with various local leak fixes I've got unsubmitted.
>
> Can you please tell me what "SANITIZE=leak" do?
>
> Thanks,
> Shubham

SANITIZE=address and SANITIZE=leak are the flags we use to turn on the
AddressSanitizer
(https://github.com/google/sanitizers/wiki/AddressSanitizer) and
LeakSanitizer
(https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer).

As the latter URL shows it's a way to instrument a program to die on
exit if unreferenced malloc'd memory wasn't free'd.

[PATCH v2 00/15] tests: don't ignore "git" exit codes

[Ævar Arnfjörð Bjarmason]

This series fixes issues where we ignored the exit code of "git" due
to it being on the LHS of a pipe, or because we interpolated its
output with $() in a "test" construct, or had missing &&- chains in
helper functions etc.

As noted in v1
(https://lore.kernel.org/git/cover-00.15-00000000000-20220302T171755Z-avarab@gmail.com/)
this is not a general search/replacement, but fixes real issues we run
into with SANITIZE=leak.

Changes since v1:

 * Minor commit message improvement for 11/15
 * Added a comment for some non-obvious prereq setup code, as
   suggested by Junio.

Ævar Arnfjörð Bjarmason (15):
  tests: change some 'test $(git) = "x"' to test_cmp
  tests: use "test_stdout_line_count", not "test $(git [...] | wc -l)"
  read-tree tests: check "diff-files" exit code on failure
  diff tests: don't ignore "git diff" exit code
  diff tests: don't ignore "git diff" exit code in "read" loop
  apply tests: use "test_must_fail" instead of ad-hoc pattern
  merge tests: use "test_must_fail" instead of ad-hoc pattern
  rev-parse tests: don't ignore "git reflog" exit code
  notes tests: don't ignore "git" exit code
  diff tests: don't ignore "git rev-list" exit code
  rev-list tests: don't hide abort() in "test_expect_failure"
  gettext tests: don't ignore "test-tool regex" exit code
  apply tests: don't ignore "git ls-files" exit code, drop sub-shell
  checkout tests: don't ignore "git <cmd>" exit code
  rev-list simplify tests: don't ignore "git" exit code

 t/t0002-gitfile.sh                     |   6 +-
 t/t1001-read-tree-m-2way.sh            |   6 +-
 t/t1002-read-tree-m-u-2way.sh          |   6 +-
 t/t1503-rev-parse-verify.sh            |   5 +-
 t/t2012-checkout-last.sh               |  51 ++++++---
 t/t2200-add-update.sh                  |  33 ++++--
 t/t3302-notes-index-expensive.sh       |   6 +-
 t/t3303-notes-subtrees.sh              |   9 +-
 t/t3305-notes-fanout.sh                |  14 +--
 t/t4020-diff-external.sh               | 153 ++++++++++++-------------
 t/t4027-diff-submodule.sh              |   7 +-
 t/t4123-apply-shrink.sh                |  18 +--
 t/t4128-apply-root.sh                  |  36 +++---
 t/t6005-rev-list-count.sh              |  43 ++++---
 t/t6012-rev-list-simplify.sh           |  12 +-
 t/t6102-rev-list-unexpected-objects.sh |  13 ++-
 t/t6407-merge-binary.sh                |  22 +---
 t/t7103-reset-bare.sh                  |   7 +-
 t/t7812-grep-icase-non-ascii.sh        |  16 ++-
 19 files changed, 245 insertions(+), 218 deletions(-)

Range-diff against v1:
 1:  78b9c52551f =  1:  78b9c52551f tests: change some 'test $(git) = "x"' to test_cmp
 2:  e1105b029d6 =  2:  e1105b029d6 tests: use "test_stdout_line_count", not "test $(git [...] | wc -l)"
 3:  5f02e30d1ab =  3:  5f02e30d1ab read-tree tests: check "diff-files" exit code on failure
 4:  a425ced5609 =  4:  a425ced5609 diff tests: don't ignore "git diff" exit code
 5:  b1aeac3f68e =  5:  b1aeac3f68e diff tests: don't ignore "git diff" exit code in "read" loop
 6:  7952ae1f3b5 =  6:  7952ae1f3b5 apply tests: use "test_must_fail" instead of ad-hoc pattern
 7:  276be19e35e =  7:  276be19e35e merge tests: use "test_must_fail" instead of ad-hoc pattern
 8:  dca2ac3a171 =  8:  dca2ac3a171 rev-parse tests: don't ignore "git reflog" exit code
 9:  ca9e12f2bac =  9:  ca9e12f2bac notes tests: don't ignore "git" exit code
10:  946397033d4 = 10:  946397033d4 diff tests: don't ignore "git rev-list" exit code
11:  26c838f0560 ! 11:  52397b3575a rev-list tests: don't hide abort() in "test_expect_failure"
    @@ Commit message
         "test_expect_success", but would allow us say "test_todo" here (and
         "success" would emit a "not ok [...] # TODO", not "ok [...]".
     
    -    In lieu of those larger changes let's more narrowly fix the problem at
    +    So even though using "test_expect_success" here comes with its own
    +    problems[2], let's use it as a narrow change to fix the problem at
         hand here and stop conflating the current "success" with actual
         SANITIZE=leak failures.
     
         1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/
    +    2. https://lore.kernel.org/git/xmqq4k9kj15p.fsf@gitster.g/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
12:  f3cc5bc7eb9 ! 12:  552dcac705d gettext tests: don't ignore "test-tool regex" exit code
    @@ t/t7812-grep-icase-non-ascii.sh: test_expect_success GETTEXT_LOCALE 'setup' '
     -test-tool regex "HALLÓ" "Halló" ICASE &&
     -test_set_prereq REGEX_LOCALE
     +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' '
    ++	# This "test-tool" invocation is identical...
     +	if test-tool regex "HALLÓ" "Halló" ICASE
     +	then
     +		test_set_prereq REGEX_LOCALE
     +	else
    ++
    ++		# ... to this one, but this way "test_must_fail" will
    ++		# tell a segfault or abort() from the regexec() test
    ++		# itself
     +		test_must_fail test-tool regex "HALLÓ" "Halló" ICASE
     +	fi
     +'
13:  834809b1b8a = 13:  dbe8d168401 apply tests: don't ignore "git ls-files" exit code, drop sub-shell
14:  34cada14fec = 14:  22b81d7f93a checkout tests: don't ignore "git <cmd>" exit code
15:  4ee216711cf = 15:  16889ed154f rev-list simplify tests: don't ignore "git" exit code
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 01/15] tests: change some 'test $(git) = "x"' to test_cmp

[Ævar Arnfjörð Bjarmason]

Change some of the patterns in the test suite where we were hiding the
exit code from "git" by invoking it in a sub-shell within a "test"
expression to use temporary files and test_cmp instead.

These are not all the occurrences of this anti-pattern, but these in
particular hid issues where LSAN was dying, and I'd thus marked these
tests as passing under the linux-leaks CI job in past commits with
"TEST_PASSES_SANITIZE_LEAK=true". Let's deal with that by either
removing that marking, or skipping specific tests under
!SANITIZE_LEAK.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0002-gitfile.sh    |  6 ++++--
 t/t2200-add-update.sh | 33 +++++++++++++++++++++------------
 t/t4128-apply-root.sh | 33 ++++++++++++++++++++-------------
 t/t7103-reset-bare.sh |  7 +++++--
 4 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 76052cb5620..f6356db183b 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -65,9 +65,11 @@ test_expect_success 'check commit-tree' '
 	test_path_is_file "$REAL/objects/$(objpath $SHA)"
 '
 
-test_expect_success 'check rev-list' '
+test_expect_success !SANITIZE_LEAK 'check rev-list' '
 	git update-ref "HEAD" "$SHA" &&
-	test "$SHA" = "$(git rev-list HEAD)"
+	git rev-list HEAD >actual &&
+	echo $SHA >expected &&
+	test_cmp expected actual
 '
 
 test_expect_success 'setup_git_dir twice in subdir' '
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index acd3650d3c0..0c38f8e3569 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -14,7 +14,6 @@ only the updates to dir/sub.
 Also tested are "git add -u" without limiting, and "git add -u"
 without contents changes, and other conditions'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
@@ -41,20 +40,28 @@ test_expect_success update '
 '
 
 test_expect_success 'update noticed a removal' '
-	test "$(git ls-files dir1/sub1)" = ""
+	git ls-files dir1/sub1 >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'update touched correct path' '
-	test "$(git diff-files --name-status dir2/sub3)" = ""
+	git diff-files --name-status dir2/sub3 >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'update did not touch other tracked files' '
-	test "$(git diff-files --name-status check)" = "M	check" &&
-	test "$(git diff-files --name-status top)" = "M	top"
+	echo "M	check" >expect &&
+	git diff-files --name-status check >actual &&
+	test_cmp expect actual &&
+
+	echo "M	top" >expect &&
+	git diff-files --name-status top >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'update did not touch untracked files' '
-	test "$(git ls-files dir2/other)" = ""
+	git ls-files dir2/other >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'cache tree has not been corrupted' '
@@ -76,9 +83,8 @@ test_expect_success 'update from a subdirectory' '
 '
 
 test_expect_success 'change gets noticed' '
-
-	test "$(git diff-files --name-status dir1)" = ""
-
+	git diff-files --name-status dir1 >out &&
+	test_must_be_empty out
 '
 
 test_expect_success 'non-qualified update in subdir updates from the root' '
@@ -103,7 +109,8 @@ test_expect_success 'replace a file with a symlink' '
 test_expect_success 'add everything changed' '
 
 	git add -u &&
-	test -z "$(git diff-files)"
+	git diff-files >out &&
+	test_must_be_empty out
 
 '
 
@@ -111,7 +118,8 @@ test_expect_success 'touch and then add -u' '
 
 	touch check &&
 	git add -u &&
-	test -z "$(git diff-files)"
+	git diff-files >out &&
+	test_must_be_empty out
 
 '
 
@@ -119,7 +127,8 @@ test_expect_success 'touch and then add explicitly' '
 
 	touch check &&
 	git add check &&
-	test -z "$(git diff-files)"
+	git diff-files >out &&
+	test_must_be_empty out
 
 '
 
diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh
index cb3181e8b71..ba89a2f2d73 100755
--- a/t/t4128-apply-root.sh
+++ b/t/t4128-apply-root.sh
@@ -2,8 +2,6 @@
 
 test_description='apply same filename'
 
-
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
@@ -26,10 +24,11 @@ diff a/bla/blub/dir/file b/bla/blub/dir/file
 EOF
 
 test_expect_success 'apply --directory -p (1)' '
-
 	git apply --directory=some/sub -p3 --index patch &&
-	test Bello = $(git show :some/sub/dir/file) &&
-	test Bello = $(cat some/sub/dir/file)
+	echo Bello >expect &&
+	git show :some/sub/dir/file >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/file
 
 '
 
@@ -37,8 +36,10 @@ test_expect_success 'apply --directory -p (2) ' '
 
 	git reset --hard initial &&
 	git apply --directory=some/sub/ -p3 --index patch &&
-	test Bello = $(git show :some/sub/dir/file) &&
-	test Bello = $(cat some/sub/dir/file)
+	echo Bello >expect &&
+	git show :some/sub/dir/file >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/file
 
 '
 
@@ -55,8 +56,10 @@ EOF
 test_expect_success 'apply --directory (new file)' '
 	git reset --hard initial &&
 	git apply --directory=some/sub/dir/ --index patch &&
-	test content = $(git show :some/sub/dir/newfile) &&
-	test content = $(cat some/sub/dir/newfile)
+	echo content >expect &&
+	git show :some/sub/dir/newfile >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/newfile
 '
 
 cat > patch << EOF
@@ -72,8 +75,10 @@ EOF
 test_expect_success 'apply --directory -p (new file)' '
 	git reset --hard initial &&
 	git apply -p2 --directory=some/sub/dir/ --index patch &&
-	test content = $(git show :some/sub/dir/newfile2) &&
-	test content = $(cat some/sub/dir/newfile2)
+	echo content >expect &&
+	git show :some/sub/dir/newfile2 >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/newfile2
 '
 
 cat > patch << EOF
@@ -107,8 +112,10 @@ EOF
 test_expect_success 'apply --directory (quoted filename)' '
 	git reset --hard initial &&
 	git apply --directory=some/sub/dir/ --index patch &&
-	test content = $(git show :some/sub/dir/quotefile) &&
-	test content = $(cat some/sub/dir/quotefile)
+	echo content >expect &&
+	git show :some/sub/dir/quotefile >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/quotefile
 '
 
 test_done
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index 0de83e36199..a60153f9f32 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -63,9 +63,12 @@ test_expect_success '"mixed" reset is not allowed in bare' '
 	test_must_fail git reset --mixed HEAD^
 '
 
-test_expect_success '"soft" reset is allowed in bare' '
+test_expect_success !SANITIZE_LEAK '"soft" reset is allowed in bare' '
 	git reset --soft HEAD^ &&
-	test "$(git show --pretty=format:%s | head -n 1)" = "one"
+	git show --pretty=format:%s >out &&
+	echo one >expect &&
+	head -n 1 out >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 02/15] tests: use "test_stdout_line_count", not "test $(git [...] | wc -l)"

[Ævar Arnfjörð Bjarmason]

Use the test_stdout_line_count helper added in
cdff1bb5a3d (test-lib-functions: introduce test_stdout_line_count,
2021-07-04) so that we'll spot if git itself dies, segfaults etc in
these expressions.

Because we didn't distinguish these failure conditions before I'd
mistakenly marked these tests as passing under SANITIZE=leak in
dd9cede9136 (leak tests: mark some rev-list tests as passing with
SANITIZE=leak, 2021-10-31).

While we're at it let's re-indent these lines to match our usual
style, as we're having to change all of them anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6005-rev-list-count.sh | 43 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/t/t6005-rev-list-count.sh b/t/t6005-rev-list-count.sh
index 86542c650e2..e960049f647 100755
--- a/t/t6005-rev-list-count.sh
+++ b/t/t6005-rev-list-count.sh
@@ -2,7 +2,6 @@
 
 test_description='git rev-list --max-count and --skip test'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
@@ -14,39 +13,39 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'no options' '
-    test $(git rev-list HEAD | wc -l) = 5
+	test_stdout_line_count = 5 git rev-list HEAD
 '
 
 test_expect_success '--max-count' '
-    test $(git rev-list HEAD --max-count=0 | wc -l) = 0 &&
-    test $(git rev-list HEAD --max-count=3 | wc -l) = 3 &&
-    test $(git rev-list HEAD --max-count=5 | wc -l) = 5 &&
-    test $(git rev-list HEAD --max-count=10 | wc -l) = 5
+	test_stdout_line_count = 0 git rev-list HEAD --max-count=0 &&
+	test_stdout_line_count = 3 git rev-list HEAD --max-count=3 &&
+	test_stdout_line_count = 5 git rev-list HEAD --max-count=5 &&
+	test_stdout_line_count = 5 git rev-list HEAD --max-count=10
 '
 
 test_expect_success '--max-count all forms' '
-    test $(git rev-list HEAD --max-count=1 | wc -l) = 1 &&
-    test $(git rev-list HEAD -1 | wc -l) = 1 &&
-    test $(git rev-list HEAD -n1 | wc -l) = 1 &&
-    test $(git rev-list HEAD -n 1 | wc -l) = 1
+	test_stdout_line_count = 1 git rev-list HEAD --max-count=1 &&
+	test_stdout_line_count = 1 git rev-list HEAD -1 &&
+	test_stdout_line_count = 1 git rev-list HEAD -n1 &&
+	test_stdout_line_count = 1 git rev-list HEAD -n 1
 '
 
 test_expect_success '--skip' '
-    test $(git rev-list HEAD --skip=0 | wc -l) = 5 &&
-    test $(git rev-list HEAD --skip=3 | wc -l) = 2 &&
-    test $(git rev-list HEAD --skip=5 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=10 | wc -l) = 0
+	test_stdout_line_count = 5 git rev-list HEAD --skip=0 &&
+	test_stdout_line_count = 2 git rev-list HEAD --skip=3 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=5 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=10
 '
 
 test_expect_success '--skip --max-count' '
-    test $(git rev-list HEAD --skip=0 --max-count=0 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=0 --max-count=10 | wc -l) = 5 &&
-    test $(git rev-list HEAD --skip=3 --max-count=0 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=3 --max-count=1 | wc -l) = 1 &&
-    test $(git rev-list HEAD --skip=3 --max-count=2 | wc -l) = 2 &&
-    test $(git rev-list HEAD --skip=3 --max-count=10 | wc -l) = 2 &&
-    test $(git rev-list HEAD --skip=5 --max-count=10 | wc -l) = 0 &&
-    test $(git rev-list HEAD --skip=10 --max-count=10 | wc -l) = 0
+	test_stdout_line_count = 0 git rev-list HEAD --skip=0 --max-count=0 &&
+	test_stdout_line_count = 5 git rev-list HEAD --skip=0 --max-count=10 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=3 --max-count=0 &&
+	test_stdout_line_count = 1 git rev-list HEAD --skip=3 --max-count=1 &&
+	test_stdout_line_count = 2 git rev-list HEAD --skip=3 --max-count=2 &&
+	test_stdout_line_count = 2 git rev-list HEAD --skip=3 --max-count=10 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=5 --max-count=10 &&
+	test_stdout_line_count = 0 git rev-list HEAD --skip=10 --max-count=10
 '
 
 test_done
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 03/15] read-tree tests: check "diff-files" exit code on failure

[Ævar Arnfjörð Bjarmason]

Fix an issue with the exit code of "diff-files" being ignored, which
has been ignored ever since these tests were originally added in
c859600954d ([PATCH] read-tree: save more user hassles during
fast-forward., 2005-06-07).

Since the exit code was ignored we'd hide errors here under
SANITIZE=leak, which resulted in me mistakenly marking these tests as
passing under SANITIZE=leak in e5a917fcf42 (unpack-trees: don't leak
memory in verify_clean_subdirectory(), 2021-10-07) and
4ea08416b8e (leak tests: mark a read-tree test as passing
SANITIZE=leak, 2021-10-31).

As it would be non-trivial to fix these tests (the leak is in
revision.c) let's un-mark them as passing under SANITIZE=leak in
addition to fixing the issue of ignoring the exit code.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1001-read-tree-m-2way.sh   | 6 +++---
 t/t1002-read-tree-m-u-2way.sh | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index d1115528cb9..0710b1fb1e9 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -21,7 +21,6 @@ In the test, these paths are used:
 	yomin   - not in H or M
 '
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
@@ -38,11 +37,12 @@ compare_change () {
 }
 
 check_cache_at () {
-	clean_if_empty=$(git diff-files -- "$1")
+	git diff-files -- "$1" >out &&
+	clean_if_empty=$(cat out) &&
 	case "$clean_if_empty" in
 	'')  echo "$1: clean" ;;
 	?*)  echo "$1: dirty" ;;
-	esac
+	esac &&
 	case "$2,$clean_if_empty" in
 	clean,)		:     ;;
 	clean,?*)	false ;;
diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
index ca5c5510c73..46cbd5514a6 100755
--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh
@@ -9,7 +9,6 @@ This is identical to t1001, but uses -u to update the work tree as well.
 
 '
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
@@ -23,11 +22,12 @@ compare_change () {
 }
 
 check_cache_at () {
-	clean_if_empty=$(git diff-files -- "$1")
+	git diff-files -- "$1" >out &&
+	clean_if_empty=$(cat out) &&
 	case "$clean_if_empty" in
 	'')  echo "$1: clean" ;;
 	?*)  echo "$1: dirty" ;;
-	esac
+	esac &&
 	case "$2,$clean_if_empty" in
 	clean,)		:     ;;
 	clean,?*)	false ;;
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 04/15] diff tests: don't ignore "git diff" exit code

[Ævar Arnfjörð Bjarmason]

Fix a test pattern that originated in f1af60bdba4 (Support 'diff=pgm'
attribute, 2007-04-22) so that we'll stop using "git diff" on the
left-hand-side of a pipe, and thus ignoring its exit code.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4020-diff-external.sh | 49 ++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 54bb8ef27e7..879ee04d291 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -37,17 +37,15 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment' '
 
 '
 
-test_expect_success 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
-
-	GIT_EXTERNAL_DIFF=echo git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+test_expect_success !SANITIZE_LEAK 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
+	GIT_EXTERNAL_DIFF=echo git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
 test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
-
-	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
@@ -83,16 +81,16 @@ test_expect_success 'diff.external' '
 	}
 '
 
-test_expect_success 'diff.external should apply only to diff' '
+test_expect_success !SANITIZE_LEAK 'diff.external should apply only to diff' '
 	test_config diff.external echo &&
-	git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+	git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 '
 
 test_expect_success 'diff.external and --no-ext-diff' '
 	test_config diff.external echo &&
-	git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 '
 
 test_expect_success 'diff attribute' '
@@ -115,17 +113,15 @@ test_expect_success 'diff attribute' '
 
 '
 
-test_expect_success 'diff attribute should apply only to diff' '
-
-	git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
+	git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
 test_expect_success 'diff attribute and --no-ext-diff' '
-
-	git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
@@ -148,17 +144,15 @@ test_expect_success 'diff attribute' '
 
 '
 
-test_expect_success 'diff attribute should apply only to diff' '
-
-	git log -p -1 HEAD |
-	grep "^diff --git a/file b/file"
+test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
+	git log -p -1 HEAD >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
 test_expect_success 'diff attribute and --no-ext-diff' '
-
-	git diff --no-ext-diff |
-	grep "^diff --git a/file b/file"
+	git diff --no-ext-diff >out &&
+	grep "^diff --git a/file b/file" out
 
 '
 
@@ -177,7 +171,8 @@ test_expect_success 'attributes trump GIT_EXTERNAL_DIFF and diff.external' '
 
 test_expect_success 'no diff with -diff' '
 	echo >.gitattributes "file -diff" &&
-	git diff | grep Binary
+	git diff >out &&
+	grep Binary out
 '
 
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 05/15] diff tests: don't ignore "git diff" exit code in "read" loop

[Ævar Arnfjörð Bjarmason]

Fix a test pattern that originated in f1af60bdba4 (Support 'diff=pgm'
attribute, 2007-04-22) so that we'll stop using "git diff" on the
left-hand-side of a pipe, and thus ignoring its exit code.

Rather than use intermediate files let's rewrite these tests to a much
simpler but more exhaustive "test_tmp" where we'll ignore certain
fields in the output.

Note that this is not a faithful conversion of the previous
"read/test" in some cases, as we were ignoring more fields there than
we strictly needed to. Now we'll "test_cmp" everything we can, and
only ignore the likes of paths to $TEMPDIR etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4020-diff-external.sh | 104 ++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 51 deletions(-)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 879ee04d291..1219f8bd4c0 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -24,16 +24,12 @@ test_expect_success setup '
 '
 
 test_expect_success 'GIT_EXTERNAL_DIFF environment' '
-
-	GIT_EXTERNAL_DIFF=echo git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff >out &&
+	cut -d" " -f1,3- <out >actual &&
+	test_cmp expect actual
 
 '
 
@@ -52,15 +48,14 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
 test_expect_success SYMLINKS 'typechange diff' '
 	rm -f file &&
 	ln -s elif file &&
-	GIT_EXTERNAL_DIFF=echo git diff  | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z120000 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	} &&
+
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 120000
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual &&
+
 	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff >actual &&
 	git diff >expect &&
 	test_cmp expect actual
@@ -70,15 +65,13 @@ test_expect_success 'diff.external' '
 	git reset --hard &&
 	echo third >file &&
 	test_config diff.external echo &&
-	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
+
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 100644
+	EOF
+	git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success !SANITIZE_LEAK 'diff.external should apply only to diff' '
@@ -101,16 +94,12 @@ test_expect_success 'diff attribute' '
 
 	echo >.gitattributes "file diff=parrot" &&
 
-	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
-
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 100644
+	EOF
+	git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
@@ -132,16 +121,12 @@ test_expect_success 'diff attribute' '
 
 	echo >.gitattributes "file diff=color" &&
 
-	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
-		test "z$path" = zfile &&
-		test "z$oldmode" = z100644 &&
-		test "z$newhex" = "z$ZERO_OID" &&
-		test "z$newmode" = z100644 &&
-		oh=$(git rev-parse --verify HEAD:file) &&
-		test "z$oh" = "z$oldhex"
-	}
-
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 $(test_oid zero) 100644
+	EOF
+	git diff >out &&
+	cut -d" " -f1,3-4,6- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success !SANITIZE_LEAK 'diff attribute should apply only to diff' '
@@ -159,14 +144,26 @@ test_expect_success 'diff attribute and --no-ext-diff' '
 test_expect_success 'GIT_EXTERNAL_DIFF trumps diff.external' '
 	>.gitattributes &&
 	test_config diff.external "echo ext-global" &&
-	GIT_EXTERNAL_DIFF="echo ext-env" git diff | grep ext-env
+
+	cat >expect <<-EOF &&
+	ext-env file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF="echo ext-env" git diff >out &&
+	cut -d" " -f1-2,4- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'attributes trump GIT_EXTERNAL_DIFF and diff.external' '
 	test_config diff.foo.command "echo ext-attribute" &&
 	test_config diff.external "echo ext-global" &&
 	echo "file diff=foo" >.gitattributes &&
-	GIT_EXTERNAL_DIFF="echo ext-env" git diff | grep ext-attribute
+
+	cat >expect <<-EOF &&
+	ext-attribute file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF="echo ext-env" git diff >out &&
+	cut -d" " -f1-2,4- <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'no diff with -diff' '
@@ -212,7 +209,12 @@ test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '
 	touch file.ext &&
 	git add file.ext &&
 	echo with extension > file.ext &&
-	GIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\.ext &&
+
+	cat >expect <<-EOF &&
+	file.ext file $(git rev-parse --verify HEAD:file) 100644 file.ext $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff file.ext >out &&
+	cut -d" " -f1,3- <out >actual &&
 	git update-index --force-remove file.ext &&
 	rm file.ext
 '
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 06/15] apply tests: use "test_must_fail" instead of ad-hoc pattern

[Ævar Arnfjörð Bjarmason]

Change a fragile test pattern introduced in 6b763c424e4 (git-apply: do
not read past the end of buffer, 2007-09-05). Before this we wouldn't
distinguish normal "git apply" failures from segfaults or abort().

I'd previously marked this test as passing under SANITIZE=leak in
f54f48fc074 (leak tests: mark some apply tests as passing with
SANITIZE=leak, 2021-10-31). Let's remove that annotation as this test
will no longer pass.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4123-apply-shrink.sh | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh
index dfa053ff28e..3ef84619f53 100755
--- a/t/t4123-apply-shrink.sh
+++ b/t/t4123-apply-shrink.sh
@@ -2,8 +2,6 @@
 
 test_description='apply a patch that is larger than the preimage'
 
-
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >F  <<\EOF
@@ -41,20 +39,8 @@ test_expect_success setup '
 '
 
 test_expect_success 'apply should fail gracefully' '
-
-	if git apply --index patch
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		status=$? &&
-		echo "Status was $status" &&
-		if test -f .git/index.lock
-		then
-			echo Oops, should not have crashed
-			false
-		fi
-	fi
+	test_must_fail git apply --index patch &&
+	test_path_is_missing .git/index.lock
 '
 
 test_done
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 07/15] merge tests: use "test_must_fail" instead of ad-hoc pattern

[Ævar Arnfjörð Bjarmason]

As in the preceding commit change a similar fragile test pattern
introduced in b798671fa93 (merge-recursive: do not rudely die on
binary merge, 2007-08-14) to use a "test_must_fail" instead.

Before this we wouldn't distinguish normal "git merge" failures from
segfaults or abort(). Unlike the preceding commit we didn't end up
hiding any SANITIZE=leak failures in this case, but let's
correspondingly change these anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6407-merge-binary.sh | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh
index 8e6241f92e6..0753fc95f45 100755
--- a/t/t6407-merge-binary.sh
+++ b/t/t6407-merge-binary.sh
@@ -43,14 +43,9 @@ test_expect_success resolve '
 	rm -f a* m* &&
 	git reset --hard anchor &&
 
-	if git merge -s resolve main
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		git ls-files -s >current &&
-		test_cmp expect current
-	fi
+	test_must_fail git merge -s resolve main &&
+	git ls-files -s >current &&
+	test_cmp expect current
 '
 
 test_expect_success recursive '
@@ -58,14 +53,9 @@ test_expect_success recursive '
 	rm -f a* m* &&
 	git reset --hard anchor &&
 
-	if git merge -s recursive main
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		git ls-files -s >current &&
-		test_cmp expect current
-	fi
+	test_must_fail git merge -s recursive main &&
+	git ls-files -s >current &&
+	test_cmp expect current
 '
 
 test_done
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 08/15] rev-parse tests: don't ignore "git reflog" exit code

[Ævar Arnfjörð Bjarmason]

Amend a test added in 9c46c054ae4 (rev-parse: tests git rev-parse
--verify master@{n}, for various n, 2010-08-24) so that we'll stop
ignoring the exit code of "git reflog" by having it on the
left-hand-side of a pipe.

Because of this I'd marked this test as passing under SANITIZE=leak in
f442c94638d (leak tests: mark some rev-parse tests as passing with
SANITIZE=leak, 2021-10-31). As all of it except this specific test
will now pass, let's skip it under the !SANITIZE_LEAK prerequisite.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1503-rev-parse-verify.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 94fe413ee37..ba43168d123 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -132,8 +132,9 @@ test_expect_success 'use --default' '
 	test_must_fail git rev-parse --verify --default bar
 '
 
-test_expect_success 'main@{n} for various n' '
-	N=$(git reflog | wc -l) &&
+test_expect_success !SANITIZE_LEAK 'main@{n} for various n' '
+	git reflog >out &&
+	N=$(wc -l <out) &&
 	Nm1=$(($N-1)) &&
 	Np1=$(($N+1)) &&
 	git rev-parse --verify main@{0} &&
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 09/15] notes tests: don't ignore "git" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile test pattern that's been with us ever since these
tests were introduced in [1], [2] and [3] to properly return the exit
code of the failing command on failure.

Because of this I'd marked this test as passing under SANITIZE=leak in
[4] and [5]. We need to remove those annotations as these tests will
no longer pass.

1. 9081a421a6d (checkout: fix "branch info" memory leaks, 2021-11-16)
2. 0057c0917d3 (Add selftests verifying that we can parse notes trees
   with various fanouts, 2009-10-09)
3. 048cdd4665e (t3305: Verify that adding many notes with git-notes
   triggers increased fanout, 2010-02-13)
4. ca089724952 (leak tests: mark some notes tests as passing with
   SANITIZE=leak, 2021-10-31)
5. 9081a421a6d (checkout: fix "branch info" memory leaks, 2021-11-16)
---
 t/t3302-notes-index-expensive.sh |  6 +++---
 t/t3303-notes-subtrees.sh        |  9 ++++-----
 t/t3305-notes-fanout.sh          | 14 +++++++-------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index bc9d8ee1e6a..bb5fea02a03 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -8,7 +8,6 @@ test_description='Test commit notes index (expensive!)'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_repo () {
@@ -65,7 +64,8 @@ create_repo () {
 test_notes () {
 	count=$1 &&
 	git config core.notesRef refs/notes/commits &&
-	git log | grep "^    " >output &&
+	git log >tmp &&
+	grep "^    " tmp >output &&
 	i=$count &&
 	while test $i -gt 0
 	do
@@ -90,7 +90,7 @@ write_script time_notes <<\EOF
 			unset GIT_NOTES_REF
 			;;
 		esac
-		git log
+		git log || exit $?
 		i=$(($i+1))
 	done >/dev/null
 EOF
diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh
index 7e0a8960af8..eac193757bf 100755
--- a/t/t3303-notes-subtrees.sh
+++ b/t/t3303-notes-subtrees.sh
@@ -5,7 +5,6 @@ test_description='Test commit notes organized in subtrees'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 number_of_commits=100
@@ -79,7 +78,7 @@ test_sha1_based () {
 	(
 		start_note_commit &&
 		nr=$number_of_commits &&
-		git rev-list refs/heads/main |
+		git rev-list refs/heads/main >out &&
 		while read sha1; do
 			note_path=$(echo "$sha1" | sed "$1")
 			cat <<INPUT_END &&
@@ -91,9 +90,9 @@ EOF
 INPUT_END
 
 			nr=$(($nr-1))
-		done
-	) |
-	git fast-import --quiet
+		done <out
+	) >gfi &&
+	git fast-import --quiet <gfi
 }
 
 test_expect_success 'test notes in 2/38-fanout' 'test_sha1_based "s|^..|&/|"'
diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh
index 1f5964865ae..9976d787f47 100755
--- a/t/t3305-notes-fanout.sh
+++ b/t/t3305-notes-fanout.sh
@@ -2,7 +2,6 @@
 
 test_description='Test that adding/removing many notes triggers automatic fanout restructuring'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 path_has_fanout() {
@@ -24,7 +23,7 @@ touched_one_note_with_fanout() {
 all_notes_have_fanout() {
 	notes_commit=$1 &&
 	fanout=$2 &&
-	git ls-tree -r --name-only $notes_commit 2>/dev/null |
+	git ls-tree -r --name-only $notes_commit |
 	while read path
 	do
 		path_has_fanout $path $fanout || return 1
@@ -51,8 +50,9 @@ test_expect_success 'creating many notes with git-notes' '
 	done
 '
 
-test_expect_success 'many notes created correctly with git-notes' '
-	git log | grep "^    " > output &&
+test_expect_success !SANITIZE_LEAK 'many notes created correctly with git-notes' '
+	git log >output.raw &&
+	grep "^    " output.raw >output &&
 	i=$num_notes &&
 	while test $i -gt 0
 	do
@@ -91,13 +91,13 @@ test_expect_success 'stable fanout 0 is followed by stable fanout 1' '
 test_expect_success 'deleting most notes with git-notes' '
 	remove_notes=285 &&
 	i=0 &&
-	git rev-list HEAD |
+	git rev-list HEAD >revs &&
 	while test $i -lt $remove_notes && read sha1
 	do
 		i=$(($i + 1)) &&
 		test_tick &&
-		git notes remove "$sha1" 2>/dev/null || return 1
-	done
+		git notes remove "$sha1" || return 1
+	done <revs
 '
 
 test_expect_success 'most notes deleted correctly with git-notes' '
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 10/15] diff tests: don't ignore "git rev-list" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile pattern introduced in 2b459b483cb (diff: make sure
work tree side is shown as 0{40} when different, 2008-03-02) to check
the exit code of "git rev-list", while we're at it let's get rid of
the needless sub-shell for invoking it in favor of the "-C" option.

Because of this I'd marked these tests as passing under SANITIZE=leak
in 16d4bd4f14e (leak tests: mark some diff tests as passing with
SANITIZE=leak, 2021-10-31), let's remove the
"TEST_PASSES_SANITIZE_LEAK=true" annotation as they no longer do.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4027-diff-submodule.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 6cef0da982f..295da987cce 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -2,7 +2,6 @@
 
 test_description='difference in submodules'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
@@ -28,10 +27,8 @@ test_expect_success setup '
 		git commit -m "submodule #2"
 	) &&
 
-	set x $(
-		cd sub &&
-		git rev-list HEAD
-	) &&
+	git -C sub rev-list HEAD >revs &&
+	set x $(cat revs) &&
 	echo ":160000 160000 $3 $ZERO_OID M	sub" >expect &&
 	subtip=$3 subprev=$2
 '
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 11/15] rev-list tests: don't hide abort() in "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

Change a couple of uses of "test_expect_failure" to use a
"test_expect_success" to positively assert the current behavior, and
replace the intent of "test_expect_failure" with a "TODO" comment int
the description.

As noted in [1] the "test_expect_failure" feature is overly eager to
accept any failure as OK, and thus by design hides segfaults, abort()
etc. Because of that I didn't notice in dd9cede9136 (leak tests: mark
some rev-list tests as passing with SANITIZE=leak, 2021-10-31) that
this test leaks memory under SANITIZE=leak.

I have some larger local changes to add a better
"test_expect_failure", which would work just like
"test_expect_success", but would allow us say "test_todo" here (and
"success" would emit a "not ok [...] # TODO", not "ok [...]".

So even though using "test_expect_success" here comes with its own
problems[2], let's use it as a narrow change to fix the problem at
hand here and stop conflating the current "success" with actual
SANITIZE=leak failures.

1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/xmqq4k9kj15p.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6102-rev-list-unexpected-objects.sh | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
index 6f0902b8638..cf0195e8263 100755
--- a/t/t6102-rev-list-unexpected-objects.sh
+++ b/t/t6102-rev-list-unexpected-objects.sh
@@ -17,8 +17,13 @@ test_expect_success 'setup unexpected non-blob entry' '
 	broken_tree="$(git hash-object -w --literally -t tree broken-tree)"
 '
 
-test_expect_failure 'traverse unexpected non-blob entry (lone)' '
-	test_must_fail git rev-list --objects $broken_tree
+test_expect_success !SANITIZE_LEAK 'TODO (should fail!): traverse unexpected non-blob entry (lone)' '
+	sed "s/Z$//" >expect <<-EOF &&
+	$broken_tree Z
+	$tree foo
+	EOF
+	git rev-list --objects $broken_tree >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'traverse unexpected non-blob entry (seen)' '
@@ -116,8 +121,8 @@ test_expect_success 'setup unexpected non-blob tag' '
 	tag=$(git hash-object -w --literally -t tag broken-tag)
 '
 
-test_expect_failure 'traverse unexpected non-blob tag (lone)' '
-	test_must_fail git rev-list --objects $tag
+test_expect_success !SANITIZE_LEAK 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '
+	git rev-list --objects $tag
 '
 
 test_expect_success 'traverse unexpected non-blob tag (seen)' '
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 12/15] gettext tests: don't ignore "test-tool regex" exit code

[Ævar Arnfjörð Bjarmason]

Amend a prerequisite check added in 5c1ebcca4d1 (grep/icase: avoid
kwsset on literal non-ascii strings, 2016-06-25) to do invoke
'test-tool regex' in such a way that we'll notice if it dies under
SANITIZE=leak due to having a memory leak, as opposed to us not having
the "ICASE" support we're checking for.

Because we weren't making a distinction between the two I'd marked
these tests as passing under SANITIZE=leak in 03d85e21951 (leak tests:
mark remaining leak-free tests as such, 2021-12-17).

Doing this is tricky. Ideally "test_lazy_prereq" would materialize as
a "real" test that we could check the exit code of with the same
signal matching that "test_must_fail" does.

However lazy prerequisites aren't real tests, and are instead lazily
materialized in the guts of "test_have_prereq" when we've already
started another test.

We could detect the abort() (or similar) there and pass that exit code
down, and fail the test that caused the prerequisites to be
materialized.

But that would require extensive changes to test-lib.sh and
test-lib-functions.sh. Let's instead simply check if the exit code of
"test-tool regex" is zero, and if so set the prerequisites. If it's
non-zero let's run it again with "test_must_fail". We'll thus make a
distinction between "bad" non-zero (segv etc) and "good" (exit 1 etc.).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7812-grep-icase-non-ascii.sh | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
index ca3f24f8079..9047d665a10 100755
--- a/t/t7812-grep-icase-non-ascii.sh
+++ b/t/t7812-grep-icase-non-ascii.sh
@@ -11,9 +11,19 @@ test_expect_success GETTEXT_LOCALE 'setup' '
 	export LC_ALL
 '
 
-test_have_prereq GETTEXT_LOCALE &&
-test-tool regex "HALLÓ" "Halló" ICASE &&
-test_set_prereq REGEX_LOCALE
+test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' '
+	# This "test-tool" invocation is identical...
+	if test-tool regex "HALLÓ" "Halló" ICASE
+	then
+		test_set_prereq REGEX_LOCALE
+	else
+
+		# ... to this one, but this way "test_must_fail" will
+		# tell a segfault or abort() from the regexec() test
+		# itself
+		test_must_fail test-tool regex "HALLÓ" "Halló" ICASE
+	fi
+'
 
 test_expect_success REGEX_LOCALE 'grep literal string, no -F' '
 	git grep -i "TILRAUN: Halló Heimur!" &&
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 13/15] apply tests: don't ignore "git ls-files" exit code, drop sub-shell

[Ævar Arnfjörð Bjarmason]

Fix code added in 969c877506c (git apply --directory broken for new
files, 2008-10-12) so that it doesn't invoke "git ls-files" on the
left-hand-side of a pipe, instead let's use an intermediate file.

Since we're doing that we can also drop the sub-shell that was here to
group the two.

There are a lot of these sorts of patterns in the test suite, and
there's no particular reason to fix this one other than in a preceding
commit all similar patterns except this one were fixed in
"t/t4128-apply-root.sh", so let's fix this one straggler as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4128-apply-root.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh
index ba89a2f2d73..f6db5a79dd9 100755
--- a/t/t4128-apply-root.sh
+++ b/t/t4128-apply-root.sh
@@ -96,7 +96,8 @@ test_expect_success 'apply --directory (delete file)' '
 	echo content >some/sub/dir/delfile &&
 	git add some/sub/dir/delfile &&
 	git apply --directory=some/sub/dir/ --index patch &&
-	! (git ls-files | grep delfile)
+	git ls-files >out &&
+	! grep delfile out
 '
 
 cat > patch << 'EOF'
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 14/15] checkout tests: don't ignore "git <cmd>" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile pattern introduced in 696acf45f96 (checkout:
implement "-" abbreviation, add docs and tests, 2009-01-17) to check
the exit code of both "git symbolic-ref" and "git rev-parse".

Without this change this test will become flaky e.g. under
SANITIZE=leak if some (but not all) memory leaks revealed by these
commands are fixed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t2012-checkout-last.sh | 51 +++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index 42601d5a310..1f6c4ed0428 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -21,14 +21,20 @@ test_expect_success 'first branch switch' '
 	git checkout other
 '
 
+test_cmp_symbolic_HEAD_ref () {
+	echo refs/heads/"$1" >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success '"checkout -" switches back' '
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" switches forth' '
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/other"
+	test_cmp_symbolic_HEAD_ref other
 '
 
 test_expect_success 'detach HEAD' '
@@ -37,12 +43,16 @@ test_expect_success 'detach HEAD' '
 
 test_expect_success '"checkout -" attaches again' '
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/other"
+	test_cmp_symbolic_HEAD_ref other
 '
 
 test_expect_success '"checkout -" detaches again' '
 	git checkout - &&
-	test "z$(git rev-parse HEAD)" = "z$(git rev-parse other)" &&
+
+	git rev-parse other >expect &&
+	git rev-parse HEAD >actual &&
+	test_cmp expect actual &&
+
 	test_must_fail git symbolic-ref HEAD
 '
 
@@ -63,31 +73,31 @@ more_switches () {
 test_expect_success 'switch to the last' '
 	more_switches &&
 	git checkout @{-1} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch2"
+	test_cmp_symbolic_HEAD_ref branch2
 '
 
 test_expect_success 'switch to second from the last' '
 	more_switches &&
 	git checkout @{-2} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch3"
+	test_cmp_symbolic_HEAD_ref branch3
 '
 
 test_expect_success 'switch to third from the last' '
 	more_switches &&
 	git checkout @{-3} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch4"
+	test_cmp_symbolic_HEAD_ref branch4
 '
 
 test_expect_success 'switch to fourth from the last' '
 	more_switches &&
 	git checkout @{-4} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch5"
+	test_cmp_symbolic_HEAD_ref branch5
 '
 
 test_expect_success 'switch to twelfth from the last' '
 	more_switches &&
 	git checkout @{-12} &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/branch13"
+	test_cmp_symbolic_HEAD_ref branch13
 '
 
 test_expect_success 'merge base test setup' '
@@ -98,19 +108,28 @@ test_expect_success 'merge base test setup' '
 test_expect_success 'another...main' '
 	git checkout another &&
 	git checkout another...main &&
-	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify main^)"
+
+	git rev-parse --verify main^ >expect &&
+	git rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '...main' '
 	git checkout another &&
 	git checkout ...main &&
-	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify main^)"
+
+	git rev-parse --verify main^ >expect &&
+	git rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'main...' '
 	git checkout another &&
 	git checkout main... &&
-	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify main^)"
+
+	git rev-parse --verify main^ >expect &&
+	git rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '"checkout -" works after a rebase A' '
@@ -118,7 +137,7 @@ test_expect_success '"checkout -" works after a rebase A' '
 	git checkout other &&
 	git rebase main &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" works after a rebase A B' '
@@ -127,7 +146,7 @@ test_expect_success '"checkout -" works after a rebase A B' '
 	git checkout other &&
 	git rebase main moodle &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" works after a rebase -i A' '
@@ -135,7 +154,7 @@ test_expect_success '"checkout -" works after a rebase -i A' '
 	git checkout other &&
 	git rebase -i main &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_expect_success '"checkout -" works after a rebase -i A B' '
@@ -144,7 +163,7 @@ test_expect_success '"checkout -" works after a rebase -i A B' '
 	git checkout other &&
 	git rebase main foodle &&
 	git checkout - &&
-	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/main"
+	test_cmp_symbolic_HEAD_ref main
 '
 
 test_done
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 15/15] rev-list simplify tests: don't ignore "git" exit code

[Ævar Arnfjörð Bjarmason]

Change a fragile test pattern introduced in 65347030590 (Topo-sort
before --simplify-merges, 2008-08-03) to check the exit code of both
"git name-rev" and "git log".

This test as a whole would fail under SANITIZE=leak, but we'd pass
several "failing" tests due to hiding these exit codes before we'd
spot git dying with abort(). Now we'll instead spot all of the
failures.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6012-rev-list-simplify.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
index 63fcccec32e..de1e87f1621 100755
--- a/t/t6012-rev-list-simplify.sh
+++ b/t/t6012-rev-list-simplify.sh
@@ -12,7 +12,9 @@ note () {
 }
 
 unnote () {
-	git name-rev --tags --annotate-stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
+	test_when_finished "rm -f tmp" &&
+	git name-rev --tags --annotate-stdin >tmp &&
+	sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g" <tmp
 }
 
 #
@@ -111,8 +113,8 @@ check_outcome () {
 	shift &&
 	param="$*" &&
 	test_expect_$outcome "log $param" '
-		git log --pretty="$FMT" --parents $param |
-		unnote >actual &&
+		git log --pretty="$FMT" --parents $param >out &&
+		unnote >actual <out &&
 		sed -e "s/^.*	\([^ ]*\) .*/\1/" >check <actual &&
 		test_cmp expect check
 	'
@@ -151,8 +153,8 @@ check_result 'L K I H G B' --exclude-first-parent-only --first-parent L ^F
 check_result 'E C B A' --full-history E -- lost
 test_expect_success 'full history simplification without parent' '
 	printf "%s\n" E C B A >expect &&
-	git log --pretty="$FMT" --full-history E -- lost |
-	unnote >actual &&
+	git log --pretty="$FMT" --full-history E -- lost >out &&
+	unnote >actual <out &&
 	sed -e "s/^.*	\([^ ]*\) .*/\1/" >check <actual &&
 	test_cmp expect check
 '
-- 
2.35.1.1242.gfeba0eae32b