Skip to content

Commit e44f15b

Browse files
sunshinecogitster
sunshineco
authored and
gitster
committed
chainlint: make error messages self-explanatory
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 588ef84 commit e44f15b

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

44 files changed

+104
-90
lines changed

‎t/chainlint.pl

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@
99
# Input arguments are pathnames of shell scripts containing test definitions,
1010
# or globs referencing a collection of scripts. For each problem discovered,
1111
# the pathname of the script containing the test is printed along with the test
12-
# name and the test body with a `?!FOO?!` annotation at the location of each
13-
# detected problem, where "FOO" is a tag such as "AMP" which indicates a broken
14-
# &&-chain. Returns zero if no problems are discovered, otherwise non-zero.
12+
# name and the test body with a `?!LINT: ...?!` annotation at the location of
13+
# each detected problem, where "..." is an explanation of the problem. Returns
14+
# zero if no problems are discovered, otherwise non-zero.
1515

1616
use warnings;
1717
use strict;
@@ -181,7 +181,7 @@ sub swallow_heredocs {
181181
$self->{lineno} += () = $body =~ /\n/sg;
182182
next;
183183
}
184-
push(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);
184+
push(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]);
185185
$$b =~ /(?:\G|\n).*\z/gc; # consume rest of input
186186
my $body = substr($$b, $start, pos($$b) - $start);
187187
$self->{lineno} += () = $body =~ /\n/sg;
@@ -238,6 +238,7 @@ sub new {
238238
stop => [],
239239
output => [],
240240
heredocs => {},
241+
insubshell => 0,
241242
} => $class;
242243
$self->{lexer} = Lexer->new($self, $s);
243244
return $self;
@@ -296,8 +297,11 @@ sub parse_group {
296297

297298
sub parse_subshell {
298299
my $self = shift @_;
299-
return ($self->parse(qr/^\)$/),
300-
$self->expect(')'));
300+
$self->{insubshell}++;
301+
my @tokens = ($self->parse(qr/^\)$/),
302+
$self->expect(')'));
303+
$self->{insubshell}--;
304+
return @tokens;
301305
}
302306

303307
sub parse_case_pattern {
@@ -528,7 +532,7 @@ sub parse_loop_body {
528532
return @tokens if ends_with(\@tokens, [qr/^\|\|$/, "\n", qr/^echo$/, qr/^.+$/]);
529533
# flag missing "return/exit" handling explicit failure in loop body
530534
my $n = find_non_nl(\@tokens);
531-
push(@{$self->{problems}}, ['LOOP', $tokens[$n]]);
535+
push(@{$self->{problems}}, [$self->{insubshell} ? 'LOOPEXIT' : 'LOOPRETURN', $tokens[$n]]);
532536
return @tokens;
533537
}
534538

@@ -620,6 +624,15 @@ sub unwrap {
620624
return $s
621625
}
622626

627+
sub format_problem {
628+
local $_ = shift;
629+
/^AMP$/ && return "missing '&&'";
630+
/^LOOPRETURN$/ && return "missing '|| return 1'";
631+
/^LOOPEXIT$/ && return "missing '|| exit 1'";
632+
/^HEREDOC$/ && return 'unclosed heredoc';
633+
die("unrecognized problem type '$_'\n");
634+
}
635+
623636
sub check_test {
624637
my $self = shift @_;
625638
my $title = unwrap(shift @_);
@@ -643,9 +656,10 @@ sub check_test {
643656
for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
644657
my ($label, $token) = @$_;
645658
my $pos = $token->[2];
659+
my $err = format_problem($label);
646660
$checked .= substr($body, $start, $pos - $start);
647661
$checked .= ' ' unless $checked =~ /\s$/;
648-
$checked .= "$c->{rev}$c->{red}?!$label?!$c->{reset}";
662+
$checked .= "$c->{rev}$c->{red}?!LINT: $err?!$c->{reset}";
649663
$checked .= ' ' unless $pos >= length($body) ||
650664
substr($body, $pos, 1) =~ /^\s/;
651665
$start = $pos;

‎t/chainlint/arithmetic-expansion.expect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
5 baz
55
6 ) &&
66
7 (
7-
8 bar=$((42 + 1)) ?!AMP?!
7+
8 bar=$((42 + 1)) ?!LINT: missing '&&'?!
88
9 baz
99
10 )

‎t/chainlint/block.expect

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
2 (
22
3 foo &&
33
4 {
4-
5 echo a ?!AMP?!
4+
5 echo a ?!LINT: missing '&&'?!
55
6 echo b
66
7 } &&
77
8 bar &&
88
9 {
99
10 echo c
10-
11 } ?!AMP?!
10+
11 } ?!LINT: missing '&&'?!
1111
12 baz
1212
13 ) &&
1313
14
1414
15 {
15-
16 echo a; ?!AMP?! echo b
15+
16 echo a; ?!LINT: missing '&&'?! echo b
1616
17 } &&
17-
18 { echo a; ?!AMP?! echo b; } &&
17+
18 { echo a; ?!LINT: missing '&&'?! echo b; } &&
1818
19
1919
20 {
2020
21 echo "${var}9" &&

‎t/chainlint/broken-chain.expect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
2 (
22
3 foo &&
3-
4 bar ?!AMP?!
3+
4 bar ?!LINT: missing '&&'?!
44
5 baz &&
55
6 wop
66
7 )

‎t/chainlint/case.expect

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@
99
10 case "$x" in
1010
11 x) foo ;;
1111
12 *) bar ;;
12-
13 esac ?!AMP?!
12+
13 esac ?!LINT: missing '&&'?!
1313
14 foobar
1414
15 ) &&
1515
16 (
1616
17 case "$x" in 1) true;; esac &&
17-
18 case "$y" in 2) false;; esac ?!AMP?!
17+
18 case "$y" in 2) false;; esac ?!LINT: missing '&&'?!
1818
19 foobar
1919
20 )

‎t/chainlint/chain-break-false.expect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
5 echo failed!
55
6 false
66
7 else
7-
8 echo it went okay ?!AMP?!
7+
8 echo it went okay ?!LINT: missing '&&'?!
88
9 congratulate user
99
10 fi

‎t/chainlint/chained-block.expect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
2 echo nobody home && {
2-
3 test the doohicky ?!AMP?!
2+
3 test the doohicky ?!LINT: missing '&&'?!
33
4 right now
44
5 } &&
55
6

‎t/chainlint/chained-subshell.expect

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
2 mkdir sub && (
22
3 cd sub &&
3-
4 foo the bar ?!AMP?!
3+
4 foo the bar ?!LINT: missing '&&'?!
44
5 nuff said
55
6 ) &&
66
7
77
8 cut "-d " -f actual | (read s1 s2 s3 &&
8-
9 test -f $s1 ?!AMP?!
8+
9 test -f $s1 ?!LINT: missing '&&'?!
99
10 test $(cat $s2) = tree2path1 &&
1010
11 test $(cat $s3) = tree3path1)

‎t/chainlint/command-substitution.expect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
5 baz
55
6 ) &&
66
7 (
7-
8 bar=$(gobble blocks) ?!AMP?!
7+
8 bar=$(gobble blocks) ?!LINT: missing '&&'?!
88
9 baz
99
10 )

‎t/chainlint/complex-if-in-cuddled-loop.expect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
5 :
55
6 else
66
7 echo >file
7-
8 fi ?!LOOP?!
7+
8 fi ?!LINT: missing '|| exit 1'?!
88
9 done) &&
99
10 test ! -f file

0 commit comments

Comments
 (0)