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>

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 898573a910030be6b883c2831cfeba233a453640..31c444067cee8c261c34848c40b3ee29bdbc6c2d 100755 (executable)
--- 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 (file)
index 0000000..47a3457
--- /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 (file)
index 0000000..f0af71d
--- /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 (file)
index 0000000..dba292e
--- /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 (file)
index 0000000..e2b0599
--- /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 (file)
index 0000000..cfc0549
--- /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 (file)
index 0000000..f90b171
--- /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