tests: fix broken &&-chains & abort loops on error

This series fixes some broken &&-chains in tests and adds missing || return 1 (or || exit 1) to loops to ensure they exit early upon error. It is a followup to an earlier series which fixed many more such problems.^1
[https://lore.kernel.org/git/[email protected]/]

Eric Sunshine (3):
t2407: fix broken &&-chains in compound statement
t1092: fix buggy sparse "blame" test
t: detect and signal failure within loop

 t/perf/p7527-builtin-fsmonitor.sh        |  2 +-
 t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----
 t/t2407-worktree-heads.sh                |  4 ++--
 t/t5329-pack-objects-cruft.sh            |  8 ++++----
 t/t6429-merge-sequence-rename-caching.sh |  2 +-
 5 files changed, 13 insertions(+), 13 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1312%2Fsunshineco%2Fchainmore-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1312/sunshineco/chainmore-v1
Pull-Request: https://github.com/git/git/pull/1312

gitgitgadget

From: Eric Sunshine <[email protected]>

The breaks in the &&-chain in this test went unnoticed because the "magic exit code 117" &&-chain checker built into test-lib.sh only recognizes broken &&-chains at the top-level; it does not work within {...} groups, (...) subshells, $(...) substitutions, or within bodies of compound statements, such as if, for, while, case, etc. Furthermore, chainlint.sed detects broken &&-chains only in (...) subshells. Thus, the &&-chain breaks in this test fall into the blind spots of the &&-chain linters.

---
 t/t2407-worktree-heads.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 50815acd3e8..019a40df2ca 100755
@@ -41,10 +41,10 @@ test_expect_success 'setup' '
 test_expect_success 'refuse to overwrite: checked out in worktree' '
 	for i in 1 2 3 4
 	do
-		test_must_fail git branch -f wt-$i HEAD 2>err
+		test_must_fail git branch -f wt-$i HEAD 2>err &&
 		grep "cannot force update the branch" err &&
 
-		test_must_fail git branch -D wt-$i 2>err
+		test_must_fail git branch -D wt-$i 2>err &&
 		grep "Cannot delete branch" err || return 1
 	done
 '

gitgitgadget

From: Eric Sunshine <[email protected]>

This test wants to verify that git blame errors out when asked to blame a file _not_ in the sparse checkout. However, the very first file it asks to blame _is_ present in the checkout, thus test_must_fail git blame $file gives an unexpected result (the "blame" succeeds). This problem went unnoticed because the test invokes test_must_fail git blame $file in loop but forgets to break out of the loop early upon failure, thus the failure gets swallowed.

Fix the test by having it not ask to blame a file present in the sparse checkout, and instead only blame files not present, as intended. While at it, also add the missing || return 1 which allowed this bug to go unnoticed.

---
 t/t1092-sparse-checkout-compatibility.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index a6a14c8a21f..e13368861ce 100755
@@ -567,7 +567,7 @@ test_expect_success 'blame with pathspec outside sparse definition' '
 	init_repos &&
 	test_sparse_match git sparse-checkout set &&
 
-	for file in a \
+	for file in \
 			deep/a \
 			deep/deeper1/a \
 			deep/deeper1/deepest/a

@@ -579,7 +579,7 @@ test_expect_success 'blame with pathspec outside sparse definition' '
 		# We compare sparse-checkout-err and sparse-index-err in
 		# `test_sparse_match`. Given we know they are the same, we
 		# only check the content of sparse-index-err here.
-		test_cmp expect sparse-index-err
+		test_cmp expect sparse-index-err || return 1
 	done
 '
 

gitgitgadget

From: Eric Sunshine <[email protected]>

Failures within for and while loops can go unnoticed if not detected and signaled manually since the loop itself does not abort when a contained command fails, nor will a failure necessarily be detected when the loop finishes since the loop returns the exit code of the last command it ran on the final iteration, which may not be the command which failed. Therefore, detect and signal failures manually within loops using the idiom || return 1 (or || exit 1 within subshells).

---
 t/perf/p7527-builtin-fsmonitor.sh        | 2 +-
 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 t/t5329-pack-objects-cruft.sh            | 8 ++++----
 t/t6429-merge-sequence-rename-caching.sh | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

index 9338b9ea008..c3f9a4caa4c 100755
@@ -249,7 +249,7 @@ test_expect_success "Cleanup temp and matrix branches" "
 	do
 		for fsm_val in $fsm_values
 		do
-			cleanup $uc_val $fsm_val
+			cleanup $uc_val $fsm_val || return 1
 		done
 	done
 "

index e13368861ce..0302e36fd66 100755
@@ -556,7 +556,7 @@ test_expect_success 'blame with pathspec inside sparse definition' '
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
 	do
-		test_all_match git blame $file
+		test_all_match git blame $file || return 1
 	done
 '
 

@@ -1571,7 +1571,7 @@ test_expect_success 'sparse index is not expanded: blame' '
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
 	do
-		ensure_not_expanded blame $file
+		ensure_not_expanded blame $file || return 1
 	done
 '
 

@@ -1907,7 +1907,7 @@ test_expect_success 'rm pathspec outside sparse definition' '
 		test_sparse_match test_must_fail git rm $file &&
 		test_sparse_match test_must_fail git rm --cached $file &&
 		test_sparse_match git rm --sparse $file &&
-		test_sparse_match git status --porcelain=v2
+		test_sparse_match git status --porcelain=v2 || return 1
 	done &&
 
 	cat >folder1-full <<-EOF &&

index 8968f7a08d8..6049e2c1d78 100755
@@ -29,7 +29,7 @@ basic_cruft_pack_tests () {
 				while read oid
 				do
 					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
 				done |
 				sort -k1
 			) >expect &&

@@ -232,7 +232,7 @@ test_expect_success 'cruft tags rescue tagged objects' '
 		while read oid
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $oid)"
+				"$objdir/$(test_oid_to_path $oid)" || exit 1
 		done <objects &&
 
 		test-tool chmtime -500 \

@@ -272,7 +272,7 @@ test_expect_success 'cruft commits rescue parents, trees' '
 		while read object
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $object)"
+				"$objdir/$(test_oid_to_path $object)" || exit 1
 		done <objects &&
 		test-tool chmtime +500 "$objdir/$(test_oid_to_path \
 			$(git rev-parse HEAD))" &&

@@ -345,7 +345,7 @@ test_expect_success 'expired objects are pruned' '
 		while read object
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $object)"
+				"$objdir/$(test_oid_to_path $object)" || exit 1
 		done <objects &&
 
 		keep="$(basename "$(ls $packdir/pack-*.pack)")" &&

index e1ce9199164..650b3cd14ff 100755
@@ -725,7 +725,7 @@ test_expect_success 'avoid assuming we detected renames' '
 		mkdir unrelated &&
 		for i in $(test_seq 1 10)
 		do
-			>unrelated/$i
+			>unrelated/$i || exit 1
 		done &&
 		test_seq  2 10 >numbers &&
 		test_seq 12 20 >values &&

gitgitgadget

Thank you for catching this!

-Stolee

And the answer is "no". Without use of rhetorical question:

The loop is on the upstream side of a pipe, so "exit 1" will be lost. "sort -k1" will get a shortened output, unless the failure happens at the last iteration, so it is likely that the test may fail, but relying on the "expect" (what is supposed to have the _right_ answer) file not being right to get our breakage noticed does not sound right.

As to the questionable one, we could probably do something like the attached patch if we really wanted to. We can guarantee that this "expect" will never match any "actual", which is output from pack-mtimes test tool command. Whatever "tricky/ugly" approach we choose to take, I think this one deserves to be done in a single patch on its own with an explanation.

----- >8 --------- >8 --------- >8 --------- >8 ----
t5329: notice a failure within a loop

We try to write "|| return 1" at the end of a sequence of &&-chained command in a loop of our tests, so that a failure of any step during the earlier iteration of the loop can properly be caught.

There is one loop in this test script that is used to compute the expected result, that will be later compared with an actual output produced by the "test-tool pack-mtimes" command. This particular loop, however, is placed on the upstream side of a pipe, whose non-zero exit code does not get noticed.

Emit a line that will never be produced by the "test-tool pack-mtimes" to cause the later comparison to fail. As we use test_cmp to compare this "expected output" file with the "actual output", the "error message" we are emitting into the expected output stream will stand out and shown to the tester.

---
 t/t5329-pack-objects-cruft.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 6049e2c1d7..43d752acc7 100755
@@ -29,7 +29,8 @@ basic_cruft_pack_tests () {
 				while read oid
 				do
 					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" ||
+					echo "object list generation failed for $obj"
 				done |
 				sort -k1
 			) >expect &&

[...]

That's not something I'm likely ever going to remember to think of as capable of failing and needing this special care. Is this a preliminary series before you send chainlint improvements that finds this kind of thing for us? Or did you notice this some other way?

Change is fine, of course, I'm just curious how it was found (and how I can avoid adding more of these that you'll need to later fix up).

Am 22.08.22 um 22:59 schrieb Junio C Hamano:

This could fail due to lack of space on the filesystem or "inode" exhaustion (indeed, I've seen out-of-space failures when I've underallocated the ramdisk on which I run tests). But there are some cases of added || return or || exit in the original series[1] which are just churn because the code inside the loop wouldn't / couldn't / shouldn't fail. I wasn't happy about adding those simply to pacify a not-smart-enough linter, but I eventually convinced myself that the small amount of inconvenience of those pointless cases was greatly outweighed by the vast number of cases in which adding || return or || exit was the correct thing to do since those cases could genuinely have allowed errors in Git or in the tests themselves to go unnoticed.

Yes, this is another preliminary series before sending the new chainlint series, and it was the new linter which found these cases.

[1]: https://lore.kernel.org/git/[email protected]/

Although the implementation of the new linter has been complete for over a year, I finally found time to work on polishing the patch series itself. I had hoped to get it submitted this past week, but time constraints prevented it. So, the answer is that the new linter (once I submit it and once Junio accepts it -- if he does) should help you avoid introducing more such cases.