The annotations emitted by chainlint to indicate detected problems are
overly terse, so much so that developers new to the project -- those who
should most benefit from the linting -- may find them baffling. For
instance, although the author of chainlint and seasoned Git developers
may understand that "?!AMP?!" is an abbreviation of "ampersand" and
indicates a break in the &&-chain, this may not be obvious to newcomers.
The "?!LOOP?!" case is particularly serious because that terse single
word does nothing to convey that the loop body should end with
"|| return 1" (or "|| exit 1" in a subshell) to ensure that a failing
command in the body aborts the loop immediately. Moreover, unlike
&&-chaining which is ubiquitous in Git tests, the "|| return 1" idiom is
relatively infrequent, thus may be harder for a newcomer to discover by
consulting nearby code.
Address these shortcomings by emitting human-readable messages which
both explain the problem and give a strong hint about how to correct it.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine committedSep 10, 2024 at 00:10 UTCe44f15ba3ee873b5df5e8e5d8cc018df288472ef
44 files changed+104-90
t/chainlint.pl
+22-8
index 1a7611ad43..ad26499478 100755--- a/t/chainlint.pl+++ b/t/chainlint.pl@@ -9,9 +9,9 @@ # Input arguments are pathnames of shell scripts containing test definitions, # or globs referencing a collection of scripts. For each problem discovered, # the pathname of the script containing the test is printed along with the test-# name and the test body with a `?!FOO?!` annotation at the location of each-# detected problem, where "FOO" is a tag such as "AMP" which indicates a broken-# &&-chain. Returns zero if no problems are discovered, otherwise non-zero.+# name and the test body with a `?!LINT: ...?!` annotation at the location of+# each detected problem, where "..." is an explanation of the problem. Returns+# zero if no problems are discovered, otherwise non-zero. use warnings; use strict;@@ -181,7 +181,7 @@ sub swallow_heredocs { $self->{lineno} += () = $body =~ /\n/sg; next; }- push(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);+ push(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]); $$b =~ /(?:\G|\n).*\z/gc; # consume rest of input my $body = substr($$b, $start, pos($$b) - $start); $self->{lineno} += () = $body =~ /\n/sg;@@ -238,6 +238,7 @@ sub new { stop => [], output => [], heredocs => {},+ insubshell => 0, } => $class; $self->{lexer} = Lexer->new($self, $s); return $self;@@ -296,8 +297,11 @@ sub parse_group { sub parse_subshell { my $self = shift @_;- return ($self->parse(qr/^\)$/),- $self->expect(')'));+ $self->{insubshell}++;+ my @tokens = ($self->parse(qr/^\)$/),+ $self->expect(')'));+ $self->{insubshell}--;+ return @tokens; } sub parse_case_pattern {@@ -528,7 +532,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);- push(@{$self->{problems}}, ['LOOP', $tokens[$n]]);+ push(@{$self->{problems}}, [$self->{insubshell} ? 'LOOPEXIT' : 'LOOPRETURN', $tokens[$n]]); return @tokens; }@@ -620,6 +624,15 @@ sub unwrap { return $s }+sub format_problem {+ local $_ = shift;+ /^AMP$/ && return "missing '&&'";+ /^LOOPRETURN$/ && return "missing '|| return 1'";+ /^LOOPEXIT$/ && return "missing '|| exit 1'";+ /^HEREDOC$/ && return 'unclosed heredoc';+ die("unrecognized problem type '$_'\n");+}+ sub check_test { my $self = shift @_; my $title = unwrap(shift @_);@@ -643,9 +656,10 @@ sub check_test { for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) { my ($label, $token) = @$_; my $pos = $token->[2];+ my $err = format_problem($label); $checked .= substr($body, $start, $pos - $start); $checked .= ' ' unless $checked =~ /\s$/;- $checked .= "$c->{rev}$c->{red}?!$label?!$c->{reset}";+ $checked .= "$c->{rev}$c->{red}?!LINT: $err?!$c->{reset}"; $checked .= ' ' unless $pos >= length($body) || substr($body, $pos, 1) =~ /^\s/; $start = $pos;
index 9a1838736f..b7b1ce8509 100644--- a/t/chainlint/broken-chain.expect+++ b/t/chainlint/broken-chain.expect@@ -1,6 +1,6 @@ 2 ( 3 foo &&-4 bar ?!AMP?!+4 bar ?!LINT: missing '&&'?! 5 baz && 6 wop 7 )
t/chainlint/case.expect
+2-2
index c04c61ff36..0a3b09e470 100644--- a/t/chainlint/case.expect+++ b/t/chainlint/case.expect@@ -9,11 +9,11 @@ 10 case "$x" in 11 x) foo ;; 12 *) bar ;;-13 esac ?!AMP?!+13 esac ?!LINT: missing '&&'?! 14 foobar 15 ) && 16 ( 17 case "$x" in 1) true;; esac &&-18 case "$y" in 2) false;; esac ?!AMP?!+18 case "$y" in 2) false;; esac ?!LINT: missing '&&'?! 19 foobar 20 )
t/chainlint/chain-break-false.expect
+1-1
index 4f815f8e14..f6a0a301e9 100644--- a/t/chainlint/chain-break-false.expect+++ b/t/chainlint/chain-break-false.expect@@ -4,6 +4,6 @@ 5 echo failed! 6 false 7 else-8 echo it went okay ?!AMP?!+8 echo it went okay ?!LINT: missing '&&'?! 9 congratulate user 10 fi
t/chainlint/chained-block.expect
+1-1
index a546b714a6..f2501bba90 100644--- a/t/chainlint/chained-block.expect+++ b/t/chainlint/chained-block.expect@@ -1,5 +1,5 @@ 2 echo nobody home && {-3 test the doohicky ?!AMP?!+3 test the doohicky ?!LINT: missing '&&'?! 4 right now 5 } && 6
t/chainlint/chained-subshell.expect
+2-2
index f78b268291..93fb1a6578 100644--- a/t/chainlint/chained-subshell.expect+++ b/t/chainlint/chained-subshell.expect@@ -1,10 +1,10 @@ 2 mkdir sub && ( 3 cd sub &&-4 foo the bar ?!AMP?!+4 foo the bar ?!LINT: missing '&&'?! 5 nuff said 6 ) && 7 8 cut "-d " -f actual | (read s1 s2 s3 &&-9 test -f $s1 ?!AMP?!+9 test -f $s1 ?!LINT: missing '&&'?! 10 test $(cat $s2) = tree2path1 && 11 test $(cat $s3) = tree3path1)
index 4323acc93d..4306faee86 100644--- a/t/chainlint/here-doc-body-indent.expect+++ b/t/chainlint/here-doc-body-indent.expect@@ -1,2 +1,2 @@-2 echo "we should find this" ?!AMP?!+2 echo "we should find this" ?!LINT: missing '&&'?! 3 echo "even though our heredoc has its indent stripped"
t/chainlint/here-doc-body-pathological.expect
+2-2
index a93a1fa3aa..2f8ea03a47 100644--- a/t/chainlint/here-doc-body-pathological.expect+++ b/t/chainlint/here-doc-body-pathological.expect@@ -1,7 +1,7 @@-2 echo "outer here-doc does not allow indented end-tag" ?!AMP?!+2 echo "outer here-doc does not allow indented end-tag" ?!LINT: missing '&&'?! 3 cat >file <<-\EOF && 4 but this inner here-doc 5 does allow indented EOF 6 EOF-7 echo "missing chain after" ?!AMP?!+7 echo "missing chain after" ?!LINT: missing '&&'?! 8 echo "but this line is OK because it's the end"
t/chainlint/here-doc-body.expect
+2-2
index ddf1c412af..df8d79bc0a 100644--- a/t/chainlint/here-doc-body.expect+++ b/t/chainlint/here-doc-body.expect@@ -1,7 +1,7 @@-2 echo "missing chain before" ?!AMP?!+2 echo "missing chain before" ?!LINT: missing '&&'?! 3 cat >file <<-\EOF && 4 inside inner here-doc 5 these are not shell commands 6 EOF-7 echo "missing chain after" ?!AMP?!+7 echo "missing chain after" ?!LINT: missing '&&'?! 8 echo "but this line is OK because it's the end"
t/chainlint/here-doc-double.expect
+1-1
index 20dba4b452..e5e981889f 100644--- a/t/chainlint/here-doc-double.expect+++ b/t/chainlint/here-doc-double.expect@@ -1,2 +1,2 @@-8 echo "actual test commands" ?!AMP?!+8 echo "actual test commands" ?!LINT: missing '&&'?! 9 echo "that should be checked"
t/chainlint/here-doc-indent-operator.expect
+1-1
index 277a11202d..ec0e61505b 100644--- a/t/chainlint/here-doc-indent-operator.expect+++ b/t/chainlint/here-doc-indent-operator.expect@@ -4,7 +4,7 @@ 5 chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data 6 EOF 7 -8 cat >expect << -EOF ?!AMP?!+8 cat >expect << -EOF ?!LINT: missing '&&'?! 9 this is not indented 10 -EOF 11
index 64f3235d26..387189b6de 100644--- a/t/chainlint/token-pasting.expect+++ b/t/chainlint/token-pasting.expect@@ -2,13 +2,13 @@ 3 git config filter.rot13.clean ./rot13.sh && 4 5 {-6 echo "*.t filter=rot13" ?!AMP?!+6 echo "*.t filter=rot13" ?!LINT: missing '&&'?! 7 echo "*.i ident" 8 } >.gitattributes && 9 10 {-11 echo a b c d e f g h i j k l m ?!AMP?!-12 echo n o p q r s t u v w x y z ?!AMP?!+11 echo a b c d e f g h i j k l m ?!LINT: missing '&&'?!+12 echo n o p q r s t u v w x y z ?!LINT: missing '&&'?! 13 echo '$Id$' 14 } >test && 15 cat test >test.t &&@@ -19,7 +19,7 @@ 20 git checkout -- test test.t test.i && 21 22 echo "content-test2" >test2.o &&-23 echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!AMP?!+23 echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!LINT: missing '&&'?! 24 25 downstream_url_for_sed=$( 26 printf "%sn" "$downstream_url" |
t/chainlint/unclosed-here-doc-indent.expect
+1-1
index f78e23cb63..156906c85a 100644--- a/t/chainlint/unclosed-here-doc-indent.expect+++ b/t/chainlint/unclosed-here-doc-indent.expect@@ -1,4 +1,4 @@ 2 command_which_is_run &&-3 cat >expect <<-\EOF ?!UNCLOSED-HEREDOC?! &&+3 cat >expect <<-\EOF ?!LINT: unclosed heredoc?! && 4 we forget to end the here-doc 5 command_which_is_gobbled
t/chainlint/unclosed-here-doc.expect
+1-1
index 51304672cf..752c608862 100644--- a/t/chainlint/unclosed-here-doc.expect+++ b/t/chainlint/unclosed-here-doc.expect@@ -1,5 +1,5 @@ 2 command_which_is_run &&-3 cat >expect <<\EOF ?!UNCLOSED-HEREDOC?! &&+3 cat >expect <<\EOF ?!LINT: unclosed heredoc?! && 4 we try to end the here-doc below, 5 but the indentation throws us off 6 since the operator is not "<<-".
t/chainlint/while-loop.expect
+4-4
index 5ffabd5a93..2ba5582165 100644--- a/t/chainlint/while-loop.expect+++ b/t/chainlint/while-loop.expect@@ -1,14 +1,14 @@ 2 ( 3 while true 4 do-5 echo foo ?!AMP?!-6 cat <<-\EOF ?!LOOP?!+5 echo foo ?!LINT: missing '&&'?!+6 cat <<-\EOF ?!LINT: missing '|| exit 1'?! 7 bar 8 EOF-9 done ?!AMP?!+9 done ?!LINT: missing '&&'?! 10 11 while true; do 12 echo foo &&-13 cat bar ?!LOOP?!+13 cat bar ?!LINT: missing '|| exit 1'?! 14 done 15 )