git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 12/18] chainlint.pl: complain about loops lacking explicit failure handling

From
Eric Sunshine via GitGitGadget <gitgitgadget@gmail.com>
Date
Sep 1, 2022, 00:29 UTC
Message-ID
<1049172aaca8e13f64201dc0ebf04ba5c655c0ce.1661992197.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1322.git.git.1661992197.gitgitgadget@gmail.com>
From: Eric Sunshine <sunshine@sunshineco.com>

Shell `for` and `while` loops do not terminate automatically just because a command fails within the loop body. Instead, the loop continues to iterate and eventually returns the exit status of the final command of the final iteration, which may not be the command which failed, thus it is possible for failures to go undetected. Consequently, it is important for test authors to explicitly handle failure within the loop body by terminating the loop manually upon failure. This can be done by returning a non-zero exit code from within the loop body (i.e. `|| return 1`) or exiting (i.e. `|| exit 1`) if the loop is within a subshell, or by manually checking `$?` and taking some appropriate action. Therefore, add logic to detect and complain about loops which lack explicit `return` or `exit`, or `$?` check.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl                                | 11 ++++++
 t/chainlint/complex-if-in-cuddled-loop.expect |  2 +-
 t/chainlint/for-loop.expect                   |  4 +--
 t/chainlint/loop-detect-failure.expect        | 15 ++++++++
 t/chainlint/loop-detect-failure.test          | 17 +++++++++
 t/chainlint/loop-detect-status.expect         | 18 ++++++++++
 t/chainlint/loop-detect-status.test           | 19 ++++++++++
 t/chainlint/loop-in-if.expect                 |  2 +-
 t/chainlint/nested-loop-detect-failure.expect | 31 ++++++++++++++++
 t/chainlint/nested-loop-detect-failure.test   | 35 +++++++++++++++++++
 t/chainlint/semicolon.expect                  |  2 +-
 t/chainlint/while-loop.expect                 |  4 +--
 12 files changed, 153 insertions(+), 7 deletions(-)
 create mode 100644 t/chainlint/loop-detect-failure.expect
 create mode 100644 t/chainlint/loop-detect-failure.test
 create mode 100644 t/chainlint/loop-detect-status.expect
 create mode 100644 t/chainlint/loop-detect-status.test
 create mode 100644 t/chainlint/nested-loop-detect-failure.expect
 create mode 100644 t/chainlint/nested-loop-detect-failure.test
diff --git a/t/chainlint.pl b/t/chainlint.pl
index a76a09ecf5e..674b3ddf696 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -482,6 +482,17 @@ sub match_ending {
 	return undef;
 }
 
+sub parse_loop_body {
+	my $self = shift @_;
+	my @tokens = $self->SUPER::parse_loop_body(@_);
+	# did loop signal failure via "|| return" or "|| exit"?
+	return @tokens if !@tokens || grep(/^(?:return|exit|\$\?)$/, @tokens);
+	# flag missing "return/exit" handling explicit failure in loop body
+	my $n = find_non_nl(\@tokens);
+	splice(@tokens, $n + 1, 0, '?!LOOP?!');
+	return @tokens;
+}
+
 my @safe_endings = (
 	[qr/^(?:&&|\|\||\||&)$/],
 	[qr/^(?:exit|return)$/, qr/^(?:\d+|\$\?)$/],
diff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect
index 2fca1834095..dac2d0fd1d9 100644
--- a/t/chainlint/complex-if-in-cuddled-loop.expect
+++ b/t/chainlint/complex-if-in-cuddled-loop.expect
@@ -4,6 +4,6 @@
      :
    else
      echo >file
-   fi
+   fi ?!LOOP?!
  done) &&
 test ! -f file
diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index 6671b8cd842..a5810c9bddd 100644
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -2,10 +2,10 @@
 	for i in a b c
 	do
 		echo $i ?!AMP?!
-		cat <<-EOF
+		cat <<-EOF ?!LOOP?!
 	done ?!AMP?!
 	for i in a b c; do
 		echo $i &&
-		cat $i
+		cat $i ?!LOOP?!
 	done
 )
diff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect
new file mode 100644
index 00000000000..a66025c39d4
--- /dev/null
+++ b/t/chainlint/loop-detect-failure.expect
@@ -0,0 +1,15 @@
+git init r1 &&
+for n in 1 2 3 4 5
+do
+	echo "This is file: $n" > r1/file.$n &&
+	git -C r1 add file.$n &&
+	git -C r1 commit -m "$n" || return 1
+done &&
+
+git init r2 &&
+for n in 1000 10000
+do
+	printf "%"$n"s" X > r2/large.$n &&
+	git -C r2 add large.$n &&
+	git -C r2 commit -m "$n" ?!LOOP?!
+done
diff --git a/t/chainlint/loop-detect-failure.test b/t/chainlint/loop-detect-failure.test
new file mode 100644
index 00000000000..b9791cc802e
--- /dev/null
+++ b/t/chainlint/loop-detect-failure.test
@@ -0,0 +1,17 @@
+git init r1 &&
+# LINT: loop handles failure explicitly with "|| return 1"
+for n in 1 2 3 4 5
+do
+	echo "This is file: $n" > r1/file.$n &&
+	git -C r1 add file.$n &&
+	git -C r1 commit -m "$n" || return 1
+done &&
+
+git init r2 &&
+# LINT: loop fails to handle failure explicitly with "|| return 1"
+for n in 1000 10000
+do
+	printf "%"$n"s" X > r2/large.$n &&
+	git -C r2 add large.$n &&
+	git -C r2 commit -m "$n"
+done
diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
new file mode 100644
index 00000000000..0ad23bb35e4
--- /dev/null
+++ b/t/chainlint/loop-detect-status.expect
@@ -0,0 +1,18 @@
+( while test $i -le $blobcount
+do
+	printf "Generating blob $i/$blobcount\r" >& 2 &&
+	printf "blob\nmark :$i\ndata $blobsize\n" &&
+
+	printf "%-${blobsize}s" $i &&
+	echo "M 100644 :$i $i" >> commit &&
+	i=$(($i+1)) ||
+	echo $? > exit-status
+done &&
+echo "commit refs/heads/main" &&
+echo "author A U Thor <author@email.com> 123456789 +0000" &&
+echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+echo "data 5" &&
+echo ">2gb" &&
+cat commit ) |
+git fast-import --big-file-threshold=2 &&
+test ! -f exit-status
diff --git a/t/chainlint/loop-detect-status.test b/t/chainlint/loop-detect-status.test
new file mode 100644
index 00000000000..1c6c23cfc9e
--- /dev/null
+++ b/t/chainlint/loop-detect-status.test
@@ -0,0 +1,19 @@
+# LINT: "$?" handled explicitly within loop body
+(while test $i -le $blobcount
+ do
+	printf "Generating blob $i/$blobcount\r" >&2 &&
+	printf "blob\nmark :$i\ndata $blobsize\n" &&
+	#test-tool genrandom $i $blobsize &&
+	printf "%-${blobsize}s" $i &&
+	echo "M 100644 :$i $i" >> commit &&
+	i=$(($i+1)) ||
+	echo $? > exit-status
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
+git fast-import --big-file-threshold=2 &&
+test ! -f exit-status
diff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect
index e1be42376c5..6c5d6e5b243 100644
--- a/t/chainlint/loop-in-if.expect
+++ b/t/chainlint/loop-in-if.expect
@@ -4,7 +4,7 @@
 		while true
 		do
 			echo "pop" ?!AMP?!
-			echo "glup"
+			echo "glup" ?!LOOP?!
 		done ?!AMP?!
 		foo
 	fi ?!AMP?!
diff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect
new file mode 100644
index 00000000000..4793a0e8e12
--- /dev/null
+++ b/t/chainlint/nested-loop-detect-failure.expect
@@ -0,0 +1,31 @@
+for i in 0 1 2 3 4 5 6 7 8 9 ;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	do
+		echo "$i$j" > "path$i$j" ?!LOOP?!
+	done ?!LOOP?!
+done &&
+
+for i in 0 1 2 3 4 5 6 7 8 9 ;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	do
+		echo "$i$j" > "path$i$j" || return 1
+	done
+done &&
+
+for i in 0 1 2 3 4 5 6 7 8 9 ;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	do
+		echo "$i$j" > "path$i$j" ?!LOOP?!
+	done || return 1
+done &&
+
+for i in 0 1 2 3 4 5 6 7 8 9 ;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	do
+		echo "$i$j" > "path$i$j" || return 1
+	done || return 1
+done
diff --git a/t/chainlint/nested-loop-detect-failure.test b/t/chainlint/nested-loop-detect-failure.test
new file mode 100644
index 00000000000..e6f0c1acfb8
--- /dev/null
+++ b/t/chainlint/nested-loop-detect-failure.test
@@ -0,0 +1,35 @@
+# LINT: neither loop handles failure explicitly with "|| return 1"
+for i in 0 1 2 3 4 5 6 7 8 9;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9;
+	do
+		echo "$i$j" >"path$i$j"
+	done
+done &&
+
+# LINT: inner loop handles failure explicitly with "|| return 1"
+for i in 0 1 2 3 4 5 6 7 8 9;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9;
+	do
+		echo "$i$j" >"path$i$j" || return 1
+	done
+done &&
+
+# LINT: outer loop handles failure explicitly with "|| return 1"
+for i in 0 1 2 3 4 5 6 7 8 9;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9;
+	do
+		echo "$i$j" >"path$i$j"
+	done || return 1
+done &&
+
+# LINT: inner & outer loops handles failure explicitly with "|| return 1"
+for i in 0 1 2 3 4 5 6 7 8 9;
+do
+	for j in 0 1 2 3 4 5 6 7 8 9;
+	do
+		echo "$i$j" >"path$i$j" || return 1
+	done || return 1
+done
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index ed0b3707ae9..3aa2259f36c 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -15,5 +15,5 @@
 ) &&
 (cd foo &&
 	for i in a b c; do
-		echo;
+		echo; ?!LOOP?!
 	done)
diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index 0d3a9b3d128..f272aa21fee 100644
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -2,10 +2,10 @@
 	while true
 	do
 		echo foo ?!AMP?!
-		cat <<-EOF
+		cat <<-EOF ?!LOOP?!
 	done ?!AMP?!
 	while true; do
 		echo foo &&
-		cat bar
+		cat bar ?!LOOP?!
 	done
 )

-- 
gitgitgadget
Previous: Eric Sunshine via GitGitGadgetNext: Eric Sunshine via GitGitGadget
Message 31 of 51 in “make test "linting" more comprehensive”
  1. 00/18 make test "linting" more comprehensiveEric Sunshine via GitGitGadget, Sep 1, 2022
  2. 01/18 t: add skeleton chainlint.plEric Sunshine via GitGitGadget, Sep 1, 2022
  3. Ævar Arnfjörð BjarmasonSep 1, 2022
  4. Eric SunshineSep 2, 2022
  5. 02/18 chainlint.pl: add POSIX shell lexical analyzerEric Sunshine via GitGitGadget, Sep 1, 2022
  6. Ævar Arnfjörð BjarmasonSep 1, 2022
  7. Eric SunshineSep 3, 2022
  8. 04/18 chainlint.pl: add parser to validate testsEric Sunshine via GitGitGadget, Sep 1, 2022
  9. 03/18 chainlint.pl: add POSIX shell parserEric Sunshine via GitGitGadget, Sep 1, 2022
  10. 06/18 chainlint.pl: validate test scripts in parallelEric Sunshine via GitGitGadget, Sep 1, 2022
  11. Ævar Arnfjörð BjarmasonSep 1, 2022
  12. Eric SunshineSep 3, 2022
  13. Eric WongSep 6, 2022
  14. Eric SunshineSep 6, 2022
  15. Jeff KingSep 6, 2022
  16. Eric SunshineNov 21, 2022
  17. Ævar Arnfjörð BjarmasonNov 21, 2022
  18. Eric SunshineNov 21, 2022
  19. Ævar Arnfjörð BjarmasonNov 21, 2022
  20. Eric SunshineNov 21, 2022
  21. Jeff KingNov 21, 2022
  22. Eric SunshineNov 21, 2022
  23. Eric SunshineNov 21, 2022
  24. Jeff KingNov 21, 2022
  25. Eric SunshineNov 21, 2022
  26. Jeff KingNov 21, 2022
  27. Ævar Arnfjörð BjarmasonNov 22, 2022
  28. 05/18 chainlint.pl: add parser to identify test definitionsEric Sunshine via GitGitGadget, Sep 1, 2022
  29. 07/18 chainlint.pl: don't require `return|exit|continue` to end with `&&`Eric Sunshine via GitGitGadget, Sep 1, 2022
  30. 10/18 chainlint.pl: don't flag broken &&-chain if `$?` handled explicitlyEric Sunshine via GitGitGadget, Sep 1, 2022
  31. 12/18 chainlint.pl: complain about loops lacking explicit failure handlingEric Sunshine via GitGitGadget, Sep 1, 2022
  32. 09/18 chainlint.pl: don't require `&` background command to end with `&&`Eric Sunshine via GitGitGadget, Sep 1, 2022
  33. 08/18 t/Makefile: apply chainlint.pl to existing self-testsEric Sunshine via GitGitGadget, Sep 1, 2022
  34. 11/18 chainlint.pl: don't flag broken &&-chain if failure indicated explicitlyEric Sunshine via GitGitGadget, Sep 1, 2022
  35. 13/18 chainlint.pl: allow `|| echo` to signal failure upstream of a pipeEric Sunshine via GitGitGadget, Sep 1, 2022
  36. 14/18 t/chainlint: add more chainlint.pl self-testsEric Sunshine via GitGitGadget, Sep 1, 2022
  37. 15/18 test-lib: retire "lint harder" optimization hackEric Sunshine via GitGitGadget, Sep 1, 2022
  38. 16/18 test-lib: replace chainlint.sed with chainlint.plEric Sunshine via GitGitGadget, Sep 1, 2022
  39. Elijah NewrenSep 3, 2022
  40. Eric SunshineSep 3, 2022
  41. 18/18 t: retire unused chainlint.sedEric Sunshine via GitGitGadget, Sep 1, 2022
  42. Johannes SchindelinSep 2, 2022
  43. Eric SunshineSep 2, 2022
  44. Jeff KingSep 2, 2022
  45. Junio C HamanoSep 2, 2022
  46. 17/18 t/Makefile: teach `make test` and `make prove` to run chainlint.plEric Sunshine via GitGitGadget, Sep 1, 2022
  47. Jeff KingSep 11, 2022
  48. Eric SunshineSep 11, 2022
  49. Jeff KingSep 11, 2022
  50. Eric SunshineSep 12, 2022
  51. Jeff KingSep 13, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.