chainlint.pl: don't require `return|exit|continue` to end with `&&`
In order to check for &&-chain breakage, each time TestParser encounters
a new command, it checks whether the previous command ends with `&&`,
and -- with a couple exceptions -- signals breakage if it does not. The
first exception is that a command may validly end with `||`, which is
commonly employed as `command || return 1` at the very end of a loop
body to terminate the loop early. The second is that piping one
command's output with `|` to another command does not constitute a
&&-chain break (the exit status of the pipe is the exit status of the
final command in the pipe).
However, it turns out that there are a few additional cases found in the
wild in which it is likely safe for `&&` to be missing even when other
commands follow. For instance:
while {condition-1}
do
test {condition-2} || return 1 # or `exit 1` within a subshell
more-commands
done
while {condition-1}
do
test {condition-2} || continue
more-commands
done
Such cases indicate deliberate thought about failure modes by the test
author, thus flagging them as breaking the &&-chain is not helpful.
Therefore, take these special cases into consideration when checking for
&&-chain breakage.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine committedSep 1, 2022 at 00:29 UTC35ebb1e37b25b9d799d1064d36a2ce668ad20264
7 files changed+63-2
t/chainlint.pl
+18-2
index 898573a910..31c444067c 100755--- a/t/chainlint.pl+++ b/t/chainlint.pl@@ -473,13 +473,29 @@ sub ends_with { return 1; }+sub match_ending {+ my ($tokens, $endings) = @_;+ for my $needles (@$endings) {+ next if @$tokens < scalar(grep {$_ ne "\n"} @$needles);+ return 1 if ends_with($tokens, $needles);+ }+ return undef;+}++my @safe_endings = (+ [qr/^(?:&&|\|\||\|)$/],+ [qr/^(?:exit|return)$/, qr/^(?:\d+|\$\?)$/],+ [qr/^(?:exit|return)$/, qr/^(?:\d+|\$\?)$/, qr/^;$/],+ [qr/^(?:exit|return|continue)$/],+ [qr/^(?:exit|return|continue)$/, qr/^;$/]);+ sub accumulate { my ($self, $tokens, $cmd) = @_; goto DONE unless @$tokens; goto DONE if @$cmd == 1 && $$cmd[0] eq "\n";- # did previous command end with "&&", "||", "|"?- goto DONE if ends_with($tokens, [qr/^(?:&&|\|\||\|)$/]);+ # did previous command end with "&&", "|", "|| return" or similar?+ goto DONE if match_ending($tokens, \@safe_endings); # flag missing "&&" at end of previous command my $n = find_non_nl($tokens);
t/chainlint/chain-break-continue.expect
+12
new file mode 100644index 0000000000..47a3457710--- /dev/null+++ b/t/chainlint/chain-break-continue.expect@@ -0,0 +1,12 @@+git ls-tree --name-only -r refs/notes/many_notes |+while read path+do+ test "$path" = "foobar/non-note.txt" && continue+ test "$path" = "deadbeef" && continue+ test "$path" = "de/adbeef" && continue++ if test $(expr length "$path") -ne $hexsz+ then+ return 1+ fi+done
t/chainlint/chain-break-continue.test
+13
new file mode 100644index 0000000000..f0af71d8bd--- /dev/null+++ b/t/chainlint/chain-break-continue.test@@ -0,0 +1,13 @@+git ls-tree --name-only -r refs/notes/many_notes |+while read path+do+# LINT: broken &&-chain okay if explicit "continue"+ test "$path" = "foobar/non-note.txt" && continue+ test "$path" = "deadbeef" && continue+ test "$path" = "de/adbeef" && continue++ if test $(expr length "$path") -ne $hexsz+ then+ return 1+ fi+done
t/chainlint/chain-break-return-exit.expect
+4
new file mode 100644index 0000000000..dba292ee89--- /dev/null+++ b/t/chainlint/chain-break-return-exit.expect@@ -0,0 +1,4 @@+for i in 1 2 3 4 ; do+ git checkout main -b $i || return $?+ test_commit $i $i $i tag$i || return $?+done
t/chainlint/chain-break-return-exit.test
+5
new file mode 100644index 0000000000..e2b059933a--- /dev/null+++ b/t/chainlint/chain-break-return-exit.test@@ -0,0 +1,5 @@+for i in 1 2 3 4 ; do+# LINT: broken &&-chain okay if explicit "return $?" signals failure+ git checkout main -b $i || return $?+ test_commit $i $i $i tag$i || return $?+done
t/chainlint/return-loop.expect
+5
new file mode 100644index 0000000000..cfc0549bef--- /dev/null+++ b/t/chainlint/return-loop.expect@@ -0,0 +1,5 @@+while test $i -lt $((num - 5))+do+ git notes add -m "notes for commit$i" HEAD~$i || return 1+ i=$((i + 1))+done