chainlint: annotate original test definition rather than token stream

When chainlint detects problems in a test, such as a broken &&-chain, it
prints out the test with "?!FOO?!" annotations inserted at each problem
location. However, rather than annotating the original test definition,
it instead dumps out a parsed token representation of the test. Since it
lacks comments, indentations, here-doc bodies, and so forth, this
tokenized representation can be difficult for the test author to digest
and relate back to the original test definition.

However, now that each parsed token carries positional information, the
location of a detected problem can be pinpointed precisely in the
original test definition. Therefore, take advantage of this information
to annotate the test definition itself rather than annotating the parsed
token stream, thus making it easier for a test author to relate a
problem back to the source.

Maintaining the positional meta-information associated with each
detected problem requires a slight change in how the problems are
managed internally. In particular, shell syntax such as:

    msg="total: $(cd data; wc -w *.txt) words"

requires the lexical analyzer to recursively invoke the parser in order
to detect problems within the $(...) expression inside the double-quoted
string. In this case, the recursive parse context will detect the broken
&&-chain between the `cd` and `wc` commands, returning the token stream:

    cd data ; ?!AMP?! wc -w *.txt

However, the parent parse context will see everything inside the
double-quotes as a single string token:

    "total: $(cd data ; ?!AMP?! wc -w *.txt) words"

losing whatever positional information was attached to the ";" token
where the problem was detected.

One way to preserve the positional information of a detected problem in
a recursive parse context within a string would be to attach the
positional information to the annotation textually; for instance:

    "total: $(cd data ; ?!AMP:21:22?! wc -w *.txt) words"

and then extract the positional information when annotating the original
test definition.

However, a cleaner and much simpler approach is to maintain the list of
detected problems separately rather than embedding the problems as
annotations directly in the parsed token stream. Not only does this
ensure that positional information within recursive parse contexts is
not lost, but it keeps the token stream free from non-token pollution,
which may simplify implementation of validations added in the future
since they won't have to handle non-token "?!FOO!?" items specially.

Finally, the chainlint self-test "expect" files need a few mechanical
adjustments now that the original test definitions are emitted rather
than the parsed token stream. In particular, the following items missing
from the historic parsed-token output are now preserved verbatim:

    * indentation (and whitespace, in general)

    * comments

    * here-doc bodies

    * here-doc tag quoting (i.e. "\EOF")

    * line-splices (i.e. "\" at the end of a line)

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 59aa79babc2410b03a6012ac873512d191ae1a89..7972c5bbe6f92fbbadeb75cd4992d95a05adbafd 100755 (executable)
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -459,6 +459,13 @@ package TestParser;
 
 use base 'ShellParser';
 
+sub new {
+       my $class = shift @_;
+       my $self = $class->SUPER::new(@_);
+       $self->{problems} = [];
+       return $self;
+}
+
 sub find_non_nl {
        my $tokens = shift @_;
        my $n = shift @_;
@@ -498,7 +505,7 @@ sub parse_loop_body {
        return @tokens if ends_with(\@tokens, [qr/^\|\|$/, "\n", qr/^echo$/, qr/^.+$/]);
        # flag missing "return/exit" handling explicit failure in loop body
        my $n = find_non_nl(\@tokens);
-       splice(@tokens, $n + 1, 0, ['?!LOOP?!', $tokens[$n]->[1], $tokens[$n]->[2]]);
+       push(@{$self->{problems}}, ['LOOP', $tokens[$n]]);
        return @tokens;
 }
 
@@ -511,6 +518,7 @@ my @safe_endings = (
 
 sub accumulate {
        my ($self, $tokens, $cmd) = @_;
+       my $problems = $self->{problems};
 
        # no previous command to check for missing "&&"
        goto DONE unless @$tokens;
@@ -531,13 +539,13 @@ sub accumulate {
        # failure explicitly), then okay for all preceding commands to be
        # missing "&&"
        if ($$cmd[0]->[0] =~ /^(?:false|return|exit)$/) {
-               @$tokens = grep {$_->[0] !~ /^\?!AMP\?!$/} @$tokens;
+               @$problems = grep {$_->[0] ne 'AMP'} @$problems;
                goto DONE;
        }
 
        # flag missing "&&" at end of previous command
        my $n = find_non_nl($tokens);
-       splice(@$tokens, $n + 1, 0, ['?!AMP?!', $$tokens[$n]->[1], $$tokens[$n]->[2]]) unless $n < 0;
+       push(@$problems, ['AMP', $tokens->[$n]]) unless $n < 0;
 
 DONE:
        $self->SUPER::accumulate($tokens, $cmd);
@@ -594,12 +602,21 @@ sub check_test {
        $self->{ntests}++;
        my $parser = TestParser->new(\$body);
        my @tokens = $parser->parse();
-       return unless $emit_all || grep {$_->[0] =~ /\?![^?]+\?!/} @tokens;
+       my $problems = $parser->{problems};
+       return unless $emit_all || @$problems;
        my $c = main::fd_colors(1);
-       my $checked = join(' ', map {$_->[0]} @tokens);
+       my $start = 0;
+       my $checked = '';
+       for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
+               my ($label, $token) = @$_;
+               my $pos = $token->[2];
+               $checked .= substr($body, $start, $pos - $start) . " ?!$label?! ";
+               $start = $pos;
+       }
+       $checked .= substr($body, $start);
        $checked =~ s/^\n//;
-       $checked =~ s/^ //mg;
-       $checked =~ s/ $//mg;
+       $checked =~ s/(\s) \?!/$1?!/mg;
+       $checked =~ s/\?! (\s)/?!$1/mg;
        $checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;
        $checked .= "\n" unless $checked =~ /\n$/;
        push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");

diff --git a/t/chainlint/block-comment.expect b/t/chainlint/block-comment.expect
index d10b2eeaf2754f5d6609f438c523ee26c92e0b05..df2beea8887f3504e5fbab25aef96eb3763ded84 100644 (file)
--- a/t/chainlint/block-comment.expect
+++ b/t/chainlint/block-comment.expect
@@ -1,6 +1,8 @@
 (
        {
+               # show a
                echo a &&
+               # show b
                echo b
        }
 )

diff --git a/t/chainlint/case-comment.expect b/t/chainlint/case-comment.expect
index 1e4b054bda0faa803ecf12c8f4622ed0b1b29023..641c157b98c0af678b15fb2cc25645eac8650602 100644 (file)
--- a/t/chainlint/case-comment.expect
+++ b/t/chainlint/case-comment.expect
@@ -1,7 +1,10 @@
 (
        case "$x" in
+       # found foo
        x) foo ;;
+       # found other
        *)
+               # treat it as bar
                bar
                ;;
        esac

diff --git a/t/chainlint/close-subshell.expect b/t/chainlint/close-subshell.expect
index 0f87db9ae6891f8536c6eec73b71e5f049ca9667..2192a2870a1ae3d098c78126eb06608f74a32437 100644 (file)
--- a/t/chainlint/close-subshell.expect
+++ b/t/chainlint/close-subshell.expect
@@ -15,7 +15,8 @@
 ) | wuzzle &&
 (
        bop
-) | fazz       fozz &&
+) | fazz \
+       fozz &&
 (
        bup
 ) |

diff --git a/t/chainlint/comment.expect b/t/chainlint/comment.expect
index f76fde1ffba91d7becf17c0990c39ac25a7083f0..a68f1f9d7c26745169ab4c45b3e6f5f7c5b3c6b3 100644 (file)
--- a/t/chainlint/comment.expect
+++ b/t/chainlint/comment.expect
@@ -1,4 +1,8 @@
 (
+       # comment 1
        nothing &&
+       # comment 2
        something
+       # comment 3
+       # comment 4
 )

diff --git a/t/chainlint/double-here-doc.expect b/t/chainlint/double-here-doc.expect
index 75477bb1add492288504244af4bd57ec45dd601c..cd584a435730045608f1ce9162274fb6fa53a2c8 100644 (file)
--- a/t/chainlint/double-here-doc.expect
+++ b/t/chainlint/double-here-doc.expect
@@ -1,2 +1,12 @@
-run_sub_test_lib_test_err run-inv-range-start "--run invalid range start" --run="a-5" <<-EOF &&
-check_sub_test_lib_test_err run-inv-range-start <<-EOF_OUT 3 <<-EOF_ERR
+run_sub_test_lib_test_err run-inv-range-start \
+       "--run invalid range start" \
+       --run="a-5" <<-\EOF &&
+test_expect_success "passing test #1" "true"
+test_done
+EOF
+check_sub_test_lib_test_err run-inv-range-start \
+       <<-\EOF_OUT 3<<-EOF_ERR
+> FATAL: Unexpected exit with code 1
+EOF_OUT
+> error: --run: invalid non-numeric in range start: ${SQ}a-5${SQ}
+EOF_ERR

diff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect
index f42f2d41ba8c68398631c256e2abb9705d32c6ba..e8733c97c645afce31a1253e90a69cfb017e4d7d 100644 (file)
--- a/t/chainlint/empty-here-doc.expect
+++ b/t/chainlint/empty-here-doc.expect
@@ -1,3 +1,4 @@
 git ls-tree $tree path > current &&
-cat > expected <<EOF &&
+cat > expected <<\EOF &&
+EOF
 test_output

diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index a5810c9bddd8352c74f3213be1fd54d1986a72f4..d65c82129a68b7c3e2088ba9a95971e03a6952ee 100644 (file)
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -2,7 +2,9 @@
        for i in a b c
        do
                echo $i ?!AMP?!
-               cat <<-EOF ?!LOOP?!
+               cat <<-\EOF ?!LOOP?!
+               bar
+               EOF
        done ?!AMP?!
        for i in a b c; do
                echo $i &&

diff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect
index 2af9ced71cc331414ce22e5d4ef3fc1320b3c15d..7d9c2b560701f66eb854b1b31c8b3820347f7eb1 100644 (file)
--- a/t/chainlint/here-doc-close-subshell.expect
+++ b/t/chainlint/here-doc-close-subshell.expect
@@ -1,2 +1,4 @@
 (
-       cat <<-INPUT)
+       cat <<-\INPUT)
+       fizz
+       INPUT

diff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect
index fb6cf7285d02649a8df406db3e11ca4d59b9eeae..f92a7ce9992420e2e5483ddcd83d3118d5dfd516 100644 (file)
--- a/t/chainlint/here-doc-indent-operator.expect
+++ b/t/chainlint/here-doc-indent-operator.expect
@@ -1,5 +1,11 @@
-cat > expect <<-EOF &&
+cat >expect <<- EOF &&
+header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
+num_commits: $1
+chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data
+EOF
 
-cat > expect <<-EOF ?!AMP?!
+cat >expect << -EOF ?!AMP?!
+this is not indented
+-EOF
 
 cleanup

diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect
index f8b3aa73c4f180be48afff988c0f7cece67e45d4..b7364c82c89feba3baf9a149937f1e43bf91ff23 100644 (file)
--- a/t/chainlint/here-doc-multi-line-command-subst.expect
+++ b/t/chainlint/here-doc-multi-line-command-subst.expect
@@ -1,5 +1,8 @@
 (
-       x=$(bobble <<-END &&
+       x=$(bobble <<-\END &&
+               fossil
+               vegetable
+               END
                wiffle) ?!AMP?!
        echo $x
 )

diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect
index be64b26869ada1f4d7d9715cb754c2aa826c6978..6c13bdcbfb5d12500ffc01915b853d0169abf13e 100644 (file)
--- a/t/chainlint/here-doc-multi-line-string.expect
+++ b/t/chainlint/here-doc-multi-line-string.expect
@@ -1,5 +1,7 @@
 (
-       cat <<-TXT && echo "multi-line
+       cat <<-\TXT && echo "multi-line
        string" ?!AMP?!
+       fizzle
+       TXT
        bap
 )

diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 110059ba58420e5924de64edb0ec44346b43cb34..1df3f782821b6a7221767fbdd76ad2c26b5d67c9 100644 (file)
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,7 +1,25 @@
-boodle wobba        gorgo snoot        wafta snurb <<EOF &&
+boodle wobba \
+       gorgo snoot \
+       wafta snurb <<EOF &&
+quoth the raven,
+nevermore...
+EOF
 
 cat <<-Arbitrary_Tag_42 >foo &&
+snoz
+boz
+woz
+Arbitrary_Tag_42
 
-cat <<zump >boo &&
+cat <<"zump" >boo &&
+snoz
+boz
+woz
+zump
 
-horticulture <<EOF
+horticulture <<\EOF
+gomez
+morticia
+wednesday
+pugsly
+EOF

diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect
index 44d86c35976ce1957aa0b4fb90f6b7e31f230d3c..cbaaf857d47a0bf23813933e768b6a9128670737 100644 (file)
--- a/t/chainlint/if-then-else.expect
+++ b/t/chainlint/if-then-else.expect
@@ -8,7 +8,9 @@
                echo foo
        else
                echo foo &&
-               cat <<-EOF
+               cat <<-\EOF
+               bar
+               EOF
        fi ?!AMP?!
        echo poodle
 ) &&

diff --git a/t/chainlint/incomplete-line.expect b/t/chainlint/incomplete-line.expect
index ffac8f901857eef401cdcfa6d60734c92a96b416..134d3a14f5c0c1efc51e21fe1fa29b52b5f9a0ab 100644 (file)
--- a/t/chainlint/incomplete-line.expect
+++ b/t/chainlint/incomplete-line.expect
@@ -1,4 +1,10 @@
-line 1 line 2 line 3 line 4 &&
+line 1 \
+line 2 \
+line 3 \
+line 4 &&
 (
-       line 5  line 6  line 7  line 8
+       line 5 \
+       line 6 \
+       line 7 \
+       line 8
 )

diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect
index dd0dace077f0e093ccda9dc33b3296830e13c8d2..6bad21853006d38834bf1b4ebc2da46e3c6faebc 100644 (file)
--- a/t/chainlint/inline-comment.expect
+++ b/t/chainlint/inline-comment.expect
@@ -1,6 +1,6 @@
 (
-       foobar &&
-       barfoo ?!AMP?!
+       foobar && # comment 1
+       barfoo ?!AMP?! # wrong position for &&
        flibble "not a # comment"
 ) &&
 

diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
index 0ad23bb35e4fb173eb808f777fef2f2a453c8c1a..24da9e86d596b5d068b149242e213ba0224051d3 100644 (file)
--- a/t/chainlint/loop-detect-status.expect
+++ b/t/chainlint/loop-detect-status.expect
@@ -2,7 +2,7 @@
 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)) ||

diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect
index e3bef63f7548cb0c187ae938280029dd470922bb..29b3832a986af30d7026eef513cf01dc769316a2 100644 (file)
--- a/t/chainlint/nested-here-doc.expect
+++ b/t/chainlint/nested-here-doc.expect
@@ -1,7 +1,30 @@
 cat <<ARBITRARY >foop &&
+naddle
+fub <<EOF
+       nozzle
+       noodle
+EOF
+formp
+ARBITRARY
 
 (
-       cat <<-INPUT_END &&
-       cat <<-EOT ?!AMP?!
+       cat <<-\INPUT_END &&
+       fish are mice
+       but geese go slow
+       data <<EOF
+               perl is lerp
+               and nothing else
+       EOF
+       toink
+       INPUT_END
+
+       cat <<-\EOT ?!AMP?!
+       text goes here
+       data <<EOF
+               data goes here
+       EOF
+       more test here
+       EOT
+
        foobar
 )

diff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect
index be4b27a305bec54678ae4669a1666405ec06f966..9138cf386d359267143945d13c1882b0feb5c6f4 100644 (file)
--- a/t/chainlint/nested-subshell-comment.expect
+++ b/t/chainlint/nested-subshell-comment.expect
@@ -2,6 +2,8 @@
        foo &&
        (
                bar &&
+               # bottles wobble while fiddles gobble
+               # minor numbers of cows (or do they?)
                baz &&
                snaff
        ) ?!AMP?!

diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 029d129299a0a5c68d45661071ba3ae144cd5377..52789278d13b7605a63c0c92c09c518990ab316f 100644 (file)
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,10 +1,30 @@
 (
-       echo wobba             gorgo snoot             wafta snurb <<-EOF &&
+       echo wobba \
+               gorgo snoot \
+               wafta snurb <<-EOF &&
+       quoth the raven,
+       nevermore...
+       EOF
+
        cat <<EOF >bip ?!AMP?!
-       echo <<-EOF >bop
+       fish fly high
+EOF
+
+       echo <<-\EOF >bop
+       gomez
+       morticia
+       wednesday
+       pugsly
+       EOF
 ) &&
 (
-       cat <<-ARBITRARY >bup &&
-       cat <<-ARBITRARY3 >bup3 &&
+       cat <<-\ARBITRARY >bup &&
+       glink
+       FIZZ
+       ARBITRARY
+       cat <<-"ARBITRARY3" >bup3 &&
+       glink
+       FIZZ
+       ARBITRARY3
        meep
 )

diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
index 69167da2f27a3098297191c6cbfc737eacb91e8a..71b3b3bc20ed1d6718f8d0ee6efe35b147557b8c 100644 (file)
--- a/t/chainlint/t7900-subtree.expect
+++ b/t/chainlint/t7900-subtree.expect
@@ -4,12 +4,16 @@ sub2
 sub3
 sub4" &&
        chks_sub=$(cat <<TXT | sed "s,^,sub dir/,"
+$chks
+TXT
 ) &&
        chkms="main-sub1
 main-sub2
 main-sub3
 main-sub4" &&
        chkms_sub=$(cat <<TXT | sed "s,^,sub dir/,"
+$chkms
+TXT
 ) &&
        subfiles=$(git ls-files) &&
        check_equal "$subfiles" "$chkms

diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index f272aa21fee1950a97a9eeddee9436a11c4e1f3f..1f5eaea0fd59757ea78b7dc0b24ec0971c2faa36 100644 (file)
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -2,7 +2,9 @@
        while true
        do
                echo foo ?!AMP?!
-               cat <<-EOF ?!LOOP?!
+               cat <<-\EOF ?!LOOP?!
+               bar
+               EOF
        done ?!AMP?!
        while true; do
                echo foo &&