[PATCH 07/18] chainlint.pl: don't require `return|exit|continue` to end with `&&`
From: Eric Sunshine via GitGitGadget <hidden>
Date: 2022-09-01 00:30:35
Subsystem:
the rest · Maintainer:
Linus Torvalds
From: Eric Sunshine <redacted>
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 <redacted>
---
t/chainlint.pl | 20 ++++++++++++++++++--
t/chainlint/chain-break-continue.expect | 12 ++++++++++++
t/chainlint/chain-break-continue.test | 13 +++++++++++++
t/chainlint/chain-break-return-exit.expect | 4 ++++
t/chainlint/chain-break-return-exit.test | 5 +++++
t/chainlint/return-loop.expect | 5 +++++
t/chainlint/return-loop.test | 6 ++++++
7 files changed, 63 insertions(+), 2 deletions(-)
create mode 100644 t/chainlint/chain-break-continue.expect
create mode 100644 t/chainlint/chain-break-continue.test
create mode 100644 t/chainlint/chain-break-return-exit.expect
create mode 100644 t/chainlint/chain-break-return-exit.test
create mode 100644 t/chainlint/return-loop.expect
create mode 100644 t/chainlint/return-loop.test
diff --git a/t/chainlint.pl b/t/chainlint.pl
index 898573a9100..31c444067ce 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);
diff --git a/t/chainlint/chain-break-continue.expect b/t/chainlint/chain-break-continue.expect
new file mode 100644
index 00000000000..47a34577100
--- /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
diff --git a/t/chainlint/chain-break-continue.test b/t/chainlint/chain-break-continue.test
new file mode 100644
index 00000000000..f0af71d8bd9
--- /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
diff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect
new file mode 100644
index 00000000000..dba292ee89b
--- /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
diff --git a/t/chainlint/chain-break-return-exit.test b/t/chainlint/chain-break-return-exit.test
new file mode 100644
index 00000000000..e2b059933aa
--- /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
diff --git a/t/chainlint/return-loop.expect b/t/chainlint/return-loop.expect
new file mode 100644
index 00000000000..cfc0549befe
--- /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
diff --git a/t/chainlint/return-loop.test b/t/chainlint/return-loop.test
new file mode 100644
index 00000000000..f90b1713005
--- /dev/null
+++ b/t/chainlint/return-loop.test@@ -0,0 +1,6 @@ +while test $i -lt $((num - 5)) +do +# LINT: "|| return {n}" valid loop escape outside subshell; no "&&" needed + git notes add -m "notes for commit$i" HEAD~$i || return 1 + i=$((i + 1)) +done
--
gitgitgadget