chainlint.pl: complain about loops lacking explicit failure handling
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine committed
Sep 1, 2022 at 00:29 UTC
fd4094c3cad7c62adb0b7080e0dca37f66bf0c6e
12 files changed
+153
-7
t/chainlint.pl
+11
index a76a09ecf5..674b3ddf69 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+|\$\?)$/],
t/chainlint/complex-if-in-cuddled-loop.expect
+1
-1
index 2fca183409..dac2d0fd1d 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
t/chainlint/for-loop.expect
+2
-2
index 6671b8cd84..a5810c9bdd 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
)
t/chainlint/loop-detect-failure.expect
+15
new file mode 100644
index 0000000000..a66025c39d
--- /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
t/chainlint/loop-detect-failure.test
+17
new file mode 100644
index 0000000000..b9791cc802
--- /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
t/chainlint/loop-detect-status.expect
+18
new file mode 100644
index 0000000000..0ad23bb35e
--- /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
t/chainlint/loop-detect-status.test
+19
new file mode 100644
index 0000000000..1c6c23cfc9
--- /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
t/chainlint/loop-in-if.expect
+1
-1
index e1be42376c..6c5d6e5b24 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?!
t/chainlint/nested-loop-detect-failure.expect
+31
new file mode 100644
index 0000000000..4793a0e8e1
--- /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
t/chainlint/nested-loop-detect-failure.test
+35
new file mode 100644
index 0000000000..e6f0c1acfb
--- /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
t/chainlint/semicolon.expect
+1
-1
index ed0b3707ae..3aa2259f36 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)
t/chainlint/while-loop.expect
+2
-2
index 0d3a9b3d12..f272aa21fe 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
)