From: Fabian Stelzer <fs@gigacodes.de>
Date: Fri, 10 Dec 2021 09:38:27 GMT
Subject: Re: [PATCH 12/19] tests: fix broken &&-chains in `{...}` groups
Message-ID: <20211210093827.a5s5mby3if5f7a4u@fs>
In-Reply-To: <20211209051115.52629-13-sunshine@sunshineco.com>

On 09.12.2021 00:11, Eric Sunshine wrote:
>The top-level &&-chain checker built into t/test-lib.sh causes tests to
>magically exit with code 117 if the &&-chain is broken. However, it has
>the shortcoming that the magic does not work within `{...}` groups,
>`(...)` subshells, `$(...)` substitutions, or within bodies of compound
>statements, such as `if`, `for`, `while`, `case`, etc. `chainlint.sed`
>partly fills in the gap by catching broken &&-chains in `(...)`
>subshells, but bugs can still lurk behind broken &&-chains in the other
>cases.
>
>Fix broken &&-chains in `{...}` groups in order to reduce the number of
>possible lurking bugs.
>
>Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>---
>
>diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>index 1a1a69ad92..bb3de2701a 100755
>--- a/t/t0021-conversion.sh
>+++ b/t/t0021-conversion.sh
>@@ -76,13 +76,13 @@ test_expect_success setup '
> 	git config filter.rot13.clean ./rot13.sh &&
>
> 	{
>-	    echo "*.t filter=rot13"
>+	    echo "*.t filter=rot13" &&
> 	    echo "*.i ident"
> 	} >.gitattributes &&
>
> 	{
>-	    echo a b c d e f g h i j k l m
>-	    echo n o p q r s t u v w x y z
>+	    echo a b c d e f g h i j k l m &&
>+	    echo n o p q r s t u v w x y z &&
> 	    echo '\''$Id$'\''
> 	} >test &&
> 	cat test >test.t &&
>@@ -159,7 +159,7 @@ test_expect_success expanded_in_repo '
> 		printf "\$Id: NoTerminatingSymbolAtEOF"
> 	} >expected-output-crlf &&
> 	{
>-		echo "expanded-keywords ident"
>+		echo "expanded-keywords ident" &&
> 		echo "expanded-keywords-crlf ident text eol=crlf"
> 	} >>.gitattributes &&
>

Wouldn't some of these be better off as heredocs as well?
There are a couple more below. I personally don't much mind either way but 
since you changed quite a few in an earlier commit why not these?

>diff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh
>index 74cc59bf8a..889db50818 100755
>--- a/t/t0069-oidtree.sh
>+++ b/t/t0069-oidtree.sh
>@@ -28,7 +28,7 @@ test_expect_success 'oidtree insert and contains' '
> 	EOF
> 	{
> 		echoid insert 444 1 2 3 4 5 a b c d e &&
>-		echoid contains 44 441 440 444 4440 4444
>+		echoid contains 44 441 440 444 4440 4444 &&
> 		echo clear
> 	} | test-tool oidtree >actual &&
> 	test_cmp expect actual
>@@ -37,11 +37,11 @@ test_expect_success 'oidtree insert and contains' '
> test_expect_success 'oidtree each' '
> 	echoid "" 123 321 321 >expect &&
> 	{
>-		echoid insert f 9 8 123 321 a b c d e
>-		echo each 12300
>-		echo each 3211
>-		echo each 3210
>-		echo each 32100
>+		echoid insert f 9 8 123 321 a b c d e &&
>+		echo each 12300 &&
>+		echo each 3211 &&
>+		echo each 3210 &&
>+		echo each 32100 &&
> 		echo clear
> 	} | test-tool oidtree >actual &&
> 	test_cmp expect actual
>diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>index 67a3f64c2d..f6f00c7039 100755
>--- a/t/t1006-cat-file.sh
>+++ b/t/t1006-cat-file.sh
>@@ -283,7 +283,7 @@ test_expect_success "--batch-check with multiple sha1s gives correct format" '
>
> test_expect_success 'setup blobs which are likely to delta' '
> 	test-tool genrandom foo 10240 >foo &&
>-	{ cat foo; echo plus; } >foo-plus &&
>+	{ cat foo && echo plus; } >foo-plus &&
> 	git add foo foo-plus &&
> 	git commit -m foo &&
> 	cat >blobs <<-\EOF
>diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>index 9571649c42..516dd8bfa8 100755
>--- a/t/t1300-config.sh
>+++ b/t/t1300-config.sh
>@@ -901,7 +901,7 @@ test_expect_success 'get --expiry-date' '
> 	EOF
> 	: "work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)" &&
> 	{
>-		echo "$rel_out $(git config --expiry-date date.valid1)"
>+		echo "$rel_out $(git config --expiry-date date.valid1)" &&
> 		git config --expiry-date date.valid2 &&
> 		git config --expiry-date date.valid3 &&
> 		git config --expiry-date date.valid4 &&
>diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
>index 17d3cc1405..bbc01aae34 100755
>--- a/t/t1403-show-ref.sh
>+++ b/t/t1403-show-ref.sh
>@@ -78,7 +78,7 @@ test_expect_success 'show-ref --verify -q' '
> test_expect_success 'show-ref -d' '
> 	{
> 		echo $(git rev-parse refs/tags/A) refs/tags/A &&
>-		echo $(git rev-parse refs/tags/A^0) "refs/tags/A^{}"
>+		echo $(git rev-parse refs/tags/A^0) "refs/tags/A^{}" &&
> 		echo $(git rev-parse refs/tags/C) refs/tags/C
> 	} >expect &&
> 	git show-ref -d A C >actual &&
>@@ -148,7 +148,7 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
>
> 	{
> 		echo $(git rev-parse HEAD) HEAD &&
>-		echo $(git rev-parse refs/heads/B) refs/heads/B
>+		echo $(git rev-parse refs/heads/B) refs/heads/B &&
> 		echo $(git rev-parse refs/tags/B) refs/tags/B
> 	} >expect &&
> 	git show-ref --head B >actual &&
>@@ -156,8 +156,8 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
>
> 	{
> 		echo $(git rev-parse HEAD) HEAD &&
>-		echo $(git rev-parse refs/heads/B) refs/heads/B
>-		echo $(git rev-parse refs/tags/B) refs/tags/B
>+		echo $(git rev-parse refs/heads/B) refs/heads/B &&
>+		echo $(git rev-parse refs/tags/B) refs/tags/B &&
> 		echo $(git rev-parse refs/tags/B^0) "refs/tags/B^{}"
> 	} >expect &&
> 	git show-ref --head -d B >actual &&
>diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
>index 67b9cc752f..d2ef0041f9 100755
>--- a/t/t2200-add-update.sh
>+++ b/t/t2200-add-update.sh
>@@ -153,10 +153,10 @@ test_expect_success 'add -u resolves unmerged paths' '
> 			echo "100644 $one 1	$path" &&
> 			echo "100644 $two 2	$path" &&
> 			echo "100644 $three 3	$path"
>-		done
>-		echo "100644 $one 1	path3"
>-		echo "100644 $one 1	path4"
>-		echo "100644 $one 3	path5"
>+		done &&
>+		echo "100644 $one 1	path3" &&
>+		echo "100644 $one 1	path4" &&
>+		echo "100644 $one 3	path5" &&
> 		echo "100644 $one 3	path6"
> 	} |
> 	git update-index --index-info &&
>@@ -173,8 +173,8 @@ test_expect_success 'add -u resolves unmerged paths' '
> 	git add -u &&
> 	git ls-files -s path1 path2 path3 path4 path5 path6 >actual &&
> 	{
>-		echo "100644 $three 0	path1"
>-		echo "100644 $two 0	path3"
>+		echo "100644 $three 0	path1" &&
>+		echo "100644 $two 0	path3" &&
> 		echo "100644 $two 0	path5"
> 	} >expect &&
> 	test_cmp expect actual
>diff --git a/t/t2201-add-update-typechange.sh b/t/t2201-add-update-typechange.sh
>index a4eec0a346..687be974d4 100755
>--- a/t/t2201-add-update-typechange.sh
>+++ b/t/t2201-add-update-typechange.sh
>@@ -97,17 +97,17 @@ test_expect_success modify '
> 		"
> 	} >expect &&
> 	{
>-		cat expect
>-		echo ":100644 160000 $_empty $ZERO_OID T	yonk"
>+		cat expect &&
>+		echo ":100644 160000 $_empty $ZERO_OID T	yonk" &&
> 		echo ":100644 000000 $_empty $ZERO_OID D	zifmia"
> 	} >expect-files &&
> 	{
>-		cat expect
>+		cat expect &&
> 		echo ":000000 160000 $ZERO_OID $ZERO_OID A	yonk"
> 	} >expect-index &&
> 	{
>-		echo "100644 $_empty 0	nitfol"
>-		echo "160000 $yomin 0	yomin"
>+		echo "100644 $_empty 0	nitfol" &&
>+		echo "160000 $yomin 0	yomin" &&
> 		echo "160000 $yonk 0	yonk"
> 	} >expect-final
> '
>diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
>index 47d6f35dcc..7cb9909293 100755
>--- a/t/t4023-diff-rename-typechange.sh
>+++ b/t/t4023-diff-rename-typechange.sh
>@@ -55,7 +55,7 @@ test_expect_success 'cross renames to be detected for regular files' '
>
> 	git diff-tree five six -r --name-status -B -M | sort >actual &&
> 	{
>-		echo "R100	foo	bar"
>+		echo "R100	foo	bar" &&
> 		echo "R100	bar	foo"
> 	} | sort >expect &&
> 	test_cmp expect actual
>@@ -66,7 +66,7 @@ test_expect_success 'cross renames to be detected for typechange' '
>
> 	git diff-tree one two -r --name-status -B -M | sort >actual &&
> 	{
>-		echo "R100	foo	bar"
>+		echo "R100	foo	bar" &&
> 		echo "R100	bar	foo"
> 	} | sort >expect &&
> 	test_cmp expect actual
>@@ -78,7 +78,7 @@ test_expect_success 'moves and renames' '
> 	git diff-tree three four -r --name-status -B -M | sort >actual &&
> 	{
> 		# see -B -M (#6) in t4008
>-		echo "C100	foo	bar"
>+		echo "C100	foo	bar" &&
> 		echo "T100	foo"
> 	} | sort >expect &&
> 	test_cmp expect actual
>diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
>index ebff6c6883..ec5c10d2a0 100755
>--- a/t/t4124-apply-ws-rule.sh
>+++ b/t/t4124-apply-ws-rule.sh
>@@ -233,7 +233,7 @@ test_expect_success 'blank at EOF with --whitespace=fix (1)' '
> 	test_write_lines a b c >one &&
> 	git add one &&
> 	test_write_lines a b c >expect &&
>-	{ cat expect; echo; } >one &&
>+	{ cat expect && echo; } >one &&
> 	git diff -- one >patch &&
>
> 	git checkout one &&
>diff --git a/t/t4150-am.sh b/t/t4150-am.sh
>index 2aaaa0d7de..103cd39148 100755
>--- a/t/t4150-am.sh
>+++ b/t/t4150-am.sh
>@@ -116,7 +116,7 @@ test_expect_success setup '
> 		git format-patch --stdout first | sed -e "1d"
> 	} | append_cr >patch1-crlf.eml &&
> 	{
>-		printf "%255s\\n" ""
>+		printf "%255s\\n" "" &&
> 		echo "X-Fake-Field: Line One" &&
> 		echo "X-Fake-Field: Line Two" &&
> 		echo "X-Fake-Field: Line Three" &&
>diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
>index 03b952c90d..0244888a5a 100755
>--- a/t/t4212-log-corrupt.sh
>+++ b/t/t4212-log-corrupt.sh
>@@ -20,10 +20,10 @@ test_expect_success 'fsck notices broken commit' '
>
> test_expect_success 'git log with broken author email' '
> 	{
>-		echo commit $(cat broken_email.hash)
>-		echo "Author: A U Thor <author@example.com>"
>-		echo "Date:   Thu Apr 7 15:13:13 2005 -0700"
>-		echo
>+		echo commit $(cat broken_email.hash) &&
>+		echo "Author: A U Thor <author@example.com>" &&
>+		echo "Date:   Thu Apr 7 15:13:13 2005 -0700" &&
>+		echo &&
> 		echo "    foo"
> 	} >expect.out &&
>
>diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
>index 759169d074..df524f7b6d 100755
>--- a/t/t5316-pack-delta-depth.sh
>+++ b/t/t5316-pack-delta-depth.sh
>@@ -57,8 +57,11 @@ test_expect_success 'create series of packs' '
> 		git commit -m $i &&
> 		cur=$(git rev-parse HEAD^{tree}) &&
> 		{
>-			test -n "$prev" && echo "-$prev"
>-			echo $cur
>+			if test -n "$prev"
>+			then
>+				echo "-$prev"
>+			fi &&
>+			echo $cur &&
> 			echo "$(git rev-parse :file) file"
> 		} | git pack-objects --stdout >tmp &&
> 		git index-pack --stdin --fix-thin <tmp || return 1
>diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>index 1892d6615a..01468ce6d8 100755
>--- a/t/t5510-fetch.sh
>+++ b/t/t5510-fetch.sh
>@@ -71,7 +71,7 @@ test_expect_success "fetch test for-merge" '
> 	main_in_two=$(cd ../two && git rev-parse main) &&
> 	one_in_two=$(cd ../two && git rev-parse one) &&
> 	{
>-		echo "$one_in_two	"
>+		echo "$one_in_two	" &&
> 		echo "$main_in_two	not-for-merge"
> 	} >expected &&
> 	cut -f -2 .git/FETCH_HEAD >actual &&
>diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
>index 9d440e2821..c69cfd5c64 100755
>--- a/t/t5515-fetch-merge-logic.sh
>+++ b/t/t5515-fetch-merge-logic.sh
>@@ -191,17 +191,17 @@ do
> 		cp "$expect_r" expect_r &&
> 		convert_expected expect_r sed_script &&
> 		{
>-			echo "# $cmd"
>-			set x $cmd; shift
>-			git symbolic-ref HEAD refs/heads/$1 ; shift
>-			rm -f .git/FETCH_HEAD
>+			echo "# $cmd" &&
>+			set x $cmd && shift &&
>+			git symbolic-ref HEAD refs/heads/$1 && shift &&
>+			rm -f .git/FETCH_HEAD &&
> 			git for-each-ref \
> 				refs/heads refs/remotes/rem refs/tags |
> 			while read val type refname
> 			do
> 				git update-ref -d "$refname" "$val"
>-			done
>-			git fetch "$@" >/dev/null
>+			done &&
>+			git fetch "$@" >/dev/null &&
> 			cat .git/FETCH_HEAD
> 		} >"$actual_f" &&
> 		git show-ref >"$actual_r" &&
>diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
>index 05a58069b0..b68ec22d3f 100755
>--- a/t/t5562-http-backend-content-length.sh
>+++ b/t/t5562-http-backend-content-length.sh
>@@ -63,7 +63,7 @@ test_expect_success 'setup' '
> 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
> 	{
> 		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
>-			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw
>+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw &&
> 		printf 0000 &&
> 		echo "$hash_next" | git pack-objects --stdout
> 	} >push_body &&
>diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
>index b87ca06a58..1131503b76 100755
>--- a/t/t5570-git-daemon.sh
>+++ b/t/t5570-git-daemon.sh
>@@ -194,7 +194,7 @@ test_expect_success 'hostname cannot break out of directory' '
>
> test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
> 	{
>-		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw
>+		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw &&
> 		printf "0000"
> 	} >input &&
> 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
>diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
>index b043a279f1..80e86d8284 100755
>--- a/t/t5571-pre-push-hook.sh
>+++ b/t/t5571-pre-push-hook.sh
>@@ -114,7 +114,7 @@ test_expect_success 'push to URL' '
>
> test_expect_success 'set up many-ref tests' '
> 	{
>-		nr=1000
>+		nr=1000 &&
> 		while test $nr -lt 2000
> 		do
> 			nr=$(( $nr + 1 )) &&
>diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
>index 04885d0a5e..97f10905d2 100755
>--- a/t/t7513-interpret-trailers.sh
>+++ b/t/t7513-interpret-trailers.sh
>@@ -156,7 +156,7 @@ test_expect_success 'with config option on the command line' '
> 		Acked-by: Johan
> 		Reviewed-by: Peff
> 	EOF
>-	{ echo; echo "Acked-by: Johan"; } |
>+	{ echo && echo "Acked-by: Johan"; } |
> 	git -c "trailer.Acked-by.ifexists=addifdifferent" interpret-trailers \
> 		--trailer "Reviewed-by: Peff" --trailer "Acked-by: Johan" >actual &&
> 	test_cmp expected actual
>diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
>index 5bb302b1ba..ee4fdd8f18 100755
>--- a/t/t8002-blame.sh
>+++ b/t/t8002-blame.sh
>@@ -97,7 +97,7 @@ test_expect_success 'set up abbrev tests' '
> 	test_commit abbrev &&
> 	sha1=$(git rev-parse --verify HEAD) &&
> 	check_abbrev () {
>-		expect=$1; shift
>+		expect=$1 && shift &&
> 		echo $sha1 | cut -c 1-$expect >expect &&
> 		git blame "$@" abbrev.t >actual &&
> 		perl -lne "/[0-9a-f]+/ and print \$&" <actual >actual.sha &&
>-- 
>2.34.1.307.g9b7440fafd
>