Merge branch 'es/chainlint-message-updates'

The error messages from the test script checker have been improved.

* es/chainlint-message-updates:
  chainlint: reduce annotation noise-factor
  chainlint: make error messages self-explanatory
  chainlint: don't be fooled by "?!...?!" in test body

Author: gitster

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;
 }
 
@@ -587,6 +591,7 @@ sub new {
 	my $class = shift @_;
 	my $self = $class->SUPER::new(@_);
 	$self->{ntests} = 0;
+	$self->{nerrs} = 0;
 	return $self;
 }
 
@@ -619,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 @_);
@@ -634,22 +648,26 @@ sub check_test {
 	my $parser = TestParser->new(\$body);
 	my @tokens = $parser->parse();
 	my $problems = $parser->{problems};
+	$self->{nerrs} += @$problems;
 	return unless $emit_all || @$problems;
 	my $c = main::fd_colors(1);
+	my ($erropen, $errclose) = -t 1 ? ("$c->{rev}$c->{red}", $c->{reset}) : ('?!', '?!');
 	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?! ";
+		my $err = format_problem($label);
+		$checked .= substr($body, $start, $pos - $start);
+		$checked .= ' ' unless $checked =~ /\s$/;
+		$checked .= "${erropen}LINT: $err$errclose";
+		$checked .= ' ' unless $pos >= length($body) ||
+		    substr($body, $pos, 1) =~ /^\s/;
 		$start = $pos;
 	}
 	$checked .= substr($body, $start);
 	$checked =~ s/^/$lineno++ . ' '/mge;
 	$checked =~ s/^\d+ \n//;
-	$checked =~ s/(\s) \?!/$1?!/mg;
-	$checked =~ s/\?! (\s)/?!$1/mg;
-	$checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;
 	$checked =~ s/^\d+/$c->{dim}$&$c->{reset}/mg;
 	$checked .= "\n" unless $checked =~ /\n$/;
 	push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
@@ -791,9 +809,9 @@ sub check_script {
 			my $c = fd_colors(1);
 			my $s = join('', @{$parser->{output}});
 			$emit->("$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\n" . $s);
-			$nerrs += () = $s =~ /\?![^?]+\?!/g;
 		}
 		$ntests += $parser->{ntests};
+		$nerrs += $parser->{nerrs};
 	}
 	return [$id, $nscripts, $ntests, $nerrs];
 }

t/chainlint/arithmetic-expansion.expect

@@ -4,6 +4,6 @@
 5 	baz
 6 ) &&
 7 (
-8 	bar=$((42 + 1)) ?!AMP?!
+8 	bar=$((42 + 1)) ?!LINT: missing '&&'?!
 9 	baz
 10 )

t/chainlint/block.expect

@@ -1,20 +1,20 @@
 2 (
 3 	foo &&
 4 	{
-5 		echo a ?!AMP?!
+5 		echo a ?!LINT: missing '&&'?!
 6 		echo b
 7 	} &&
 8 	bar &&
 9 	{
 10 		echo c
-11 	} ?!AMP?!
+11 	} ?!LINT: missing '&&'?!
 12 	baz
 13 ) &&
 14 
 15 {
-16 	echo a; ?!AMP?! echo b
+16 	echo a; ?!LINT: missing '&&'?! echo b
 17 } &&
-18 { echo a; ?!AMP?! echo b; } &&
+18 { echo a; ?!LINT: missing '&&'?! echo b; } &&
 19 
 20 {
 21 	echo "${var}9" &&

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

@@ -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

@@ -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,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

@@ -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)

t/chainlint/command-substitution.expect

@@ -4,6 +4,6 @@
 5 	baz
 6 ) &&
 7 (
-8 	bar=$(gobble blocks) ?!AMP?!
+8 	bar=$(gobble blocks) ?!LINT: missing '&&'?!
 9 	baz
 10 )

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

@@ -4,6 +4,6 @@
 5      :
 6    else
 7      echo >file
-8    fi ?!LOOP?!
+8    fi ?!LINT: missing '|| exit 1'?!
 9  done) &&
 10 test ! -f file