chainlint: colorize problem annotations and test delimiters

Quoted 1 line · Eric Sunshine via GitGitGadget

+ $checked =~ s/(\?![^?]+\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;

It may be just me, but coloring the whole "?!LOOP?!" in red feels a bit strange. I would have expected more like

        if ($c->{color_in_use}) {
                $checked =~ s/\?!([^?]+)\?!/$c->{bold}$c->{red}$1$c->{reset}/mg;
        }

IOW, switching between "?!LOOP?!" and "<BOLD><RED>LOOP<RESET>".

But it may be just me.

(2) In practice, I found that even after coloring those annotations in red, it was still easy for the eye to glide right over them in the output without really noticing them. Switching it to bold red helped a bit, but my eye still glided over them sometimes. One possible reason that the eye was able to glide over them may be because the "?!FOO?!" annotations are very short bits of text buried in the much larger and textually noisy test body.

Quoted 7 lines · Eric Sunshine

(2) In practice, I found that even after coloring those annotations in red, it was still easy for the eye to glide right over them in the output without really noticing them. Switching it to bold red helped a bit, but my eye still glided over them sometimes. One possible reason that the eye was able to glide over them may be because the "?!FOO?!" annotations are very short bits of text buried in the much larger and textually noisy test body.

Maybe partly because I work with black-ink-on-white-paper terminal setting, and maybe partly because my color perception is suboptimal, I learned to use "[diff.color] old = red reverse", because non-bold red letters do not stand out enough. Perhaps you may want to try reverse output to see how well it makes them stand out for you.

t/chainlint.pl sub check_test { · line 593

Quoted 1 line · Eric Sunshine via GitGitGadget

t/chainlint.pl +1 -0

+	$checked =~ s/(\?![^?]+\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;

It may be just me, but coloring the whole "?!LOOP?!" in red feels a bit strange. I would have expected more like

	if ($c->{color_in_use}) {
		$checked =~ s/\?!([^?]+)\?!/$c->{bold}$c->{red}$1$c->{reset}/mg;
	}

IOW, switching between "?!LOOP?!" and "<BOLD><RED>LOOP<RESET>".

But it may be just me.

t/chainlint.pl +0 -0

@@ -585,12 +585,14 @@ sub check_test {
 	my $parser = TestParser->new(\$body);
 	my @tokens = $parser->parse();
 	return unless $emit_all || grep(/\?![^?]+\?!/, @tokens);

t/chainlint.pl +9 -0

+my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');
+my %COLORS = ();
+sub get_colors {
+	return \%COLORS if %COLORS;
+	if (exists($ENV{NO_COLOR}) ||
+	    system("tput sgr0 >/dev/null 2>&1") != 0 ||
+	    system("tput bold >/dev/null 2>&1") != 0 ||
+	    system("tput setaf 1 >/dev/null 2>&1") != 0) {
+		%COLORS = @NOCOLORS;

[...]

Quoted 16 lines · Eric Sunshine via GitGitGadget

t/chainlint.pl +16 -0

+my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');
+my %COLORS = ();
+sub get_colors {
+     return \%COLORS if %COLORS;
+     if (exists($ENV{NO_COLOR}) ||
+         system("tput sgr0 >/dev/null 2>&1") != 0 ||
+         system("tput bold >/dev/null 2>&1") != 0 ||
+         system("tput setaf 1 >/dev/null 2>&1") != 0) {
+             %COLORS = @NOCOLORS;
+             return \%COLORS;
+     }
+     %COLORS = (bold  => `tput bold`,
+                reset => `tput sgr0`,
+                blue  => `tput setaf 4`,
+                green => `tput setaf 2`,
+                red   => `tput setaf 1`);

t/chainlint.pl if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) { · line 643

Quoted 3 lines

t/chainlint.pl +3 -0

+     chomp(%COLORS);
+     return \%COLORS;
+}

This is a lot of new processes. Should be OK in the run-once-for-all-tests mode. It does make me wonder how much time regular test-lib.sh spends doing these tput checks for every script (at least it's not every snippet!).

It feels like we could build a color.sh snippet once and then include it in each script. But maybe that is dumb, since you could in theory build in one terminal and then run in another. Unlikely, but it shows that file dependencies are a mismatch. I guess a better match would be stuffing it into the environment before starting all of the tests.

I ran this on my pre-fixup state where I had a half-dozen linter checks. It's _so_ much more readable. Thanks for working on it.

This is indeed a lot of new processes, but this color interrogation is done lazily, only if a problem is detected, so it should be zero-cost in the (hopefully) normal case of a lint-clean script.

I had the exact same thought about the cost being paid by test-lib.sh making all those tput invocations.

This is a lot of new processes. Should be OK in the run-once-for-all-tests mode. It does make me wonder how much time regular test-lib.sh spends doing these tput checks for every script (at least it's not every snippet!).

It feels like we could build a color.sh snippet once and then include it in each script. But maybe that is dumb, since you could in theory build in one terminal and then run in another. Unlikely, but it shows that file dependencies are a mismatch. I guess a better match would be stuffing it into the environment before starting all of the tests.

Reverse video certainly makes the "?!FOO?!" annotations pop out and draw the reader's attention. I find that I don't have a strong preference between this version and v1 which merely used bold-red, but I suspect that v2 with its reverse video is probably the better approach.

Quoted 4 lines · Eric Sunshine via GitGitGadget

Reverse video certainly makes the "?!FOO?!" annotations pop out and draw the reader's attention. I find that I don't have a strong preference between this version and v1 which merely used bold-red, but I suspect that v2 with its reverse video is probably the better approach.

I find this one slightly uglier, but they are equally attention-grabbing. And as I hope to rarely see them in the first place, I am fine either way. :)

Thanks again for adding this.

From: Eric Sunshine <[email protected]>

When chainlint.pl detects problems in a test definition, it emits the test definition with "?!FOO?!" annotations highlighting the problems it discovered. For instance, given this problematic test:

test_expect_success 'discombobulate frobnitz' '
git frob babble &&
(echo balderdash; echo gnabgib) >expect && for i in three two one
do
git nitfol $i
done >actual
test_cmp expect actual
'

chainlint.pl will output:

# chainlint: t1234-confusing.sh
# chainlint: discombobulate frobnitz
git frob babble &&
(echo balderdash ; ?!AMP?! echo gnabgib) >expect && for i in three two one
do
git nitfol $i ?!LOOP?!
done >actual ?!AMP?!
test_cmp expect actual

in which it may be difficult to spot the "?!FOO?!" annotations. The problem is compounded when multiple tests, possibly in multiple scripts, fail "linting", in which case it may be difficult to spot the "# chainlint:" lines which delimit one problematic test from another.

To ameliorate this potential problem, colorize the "?!FOO?!" annotations in order to quickly draw the test author's attention to the problem spots, and colorize the "# chainlint:" lines to help the author identify the name of each script and each problematic test.

Colorization is disabled automatically if output is not directed to a terminal or if NO_COLOR environment variable is set. The implementation is specific to Unix (it employs tput if available) but works equally well in the Git for Windows development environment which emulates Unix sufficiently.

Signed-off-by Eric Sunshine <[email protected]>

---
chainlint: colorize problem annotations and test delimiters

This is a re-roll of [1] which colorizes the output of "chainlint.pl" when it detects problems in Git test definitions. During discussion, it was noted that the eye could sometimes glide right over[2] the bold-red "?!FOO?!" annotations, so Junio suggested using reverse video, which is what v2 does.

Reverse video certainly makes the "?!FOO?!" annotations pop out and draw the reader's attention. I find that I don't have a strong preference between this version and v1 which merely used bold-red, but I suspect that v2 with its reverse video is probably the better approach.

[1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/CAPig+cTq3j5M7cz3T14h9U6e+H5PAu8JJ_Svq87W3WviwS6_qA@mail.gmail.com/
[3] https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1324%2Fsunshineco%2Fchainlintcolor-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1324/sunshineco/chainlintcolor-v2
Pull-Request: https://github.com/git/git/pull/1324

Range-diff vs v1:

1: d670570e81f ! 1: acf9183ccc6 chainlint: colorize problem annotations and test delimiters

     @@ t/chainlint.pl: sub check_test {
       	$checked =~ s/^\n//;
       	$checked =~ s/^ //mg;
       	$checked =~ s/ $//mg;
     -+	$checked =~ s/(\?![^?]+\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;
     ++	$checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;
       	$checked .= "\n" unless $checked =~ /\n$/;
      -	push(@{$self->{output}}, "# chainlint: $title\n$checked");
      +	push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
     @@ t/chainlint.pl: if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {
      +# thread and ignore %ENV changes in subthreads.
      +$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};
      +
     -+my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');
     ++my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '', green => '', red => '');
      +my %COLORS = ();
      +sub get_colors {
      +	return \%COLORS if %COLORS;
      +	if (exists($ENV{NO_COLOR}) ||
      +	    system("tput sgr0 >/dev/null 2>&1") != 0 ||
      +	    system("tput bold >/dev/null 2>&1") != 0 ||
     ++	    system("tput rev  >/dev/null 2>&1") != 0 ||
      +	    system("tput setaf 1 >/dev/null 2>&1") != 0) {
      +		%COLORS = @NOCOLORS;
      +		return \%COLORS;
      +	}
      +	%COLORS = (bold  => `tput bold`,
     ++		   rev   => `tput rev`,
      +		   reset => `tput sgr0`,
      +		   blue  => `tput setaf 4`,
      +		   green => `tput setaf 2`,

 t/chainlint.pl | 46 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

t/chainlint.pl +43 -3

index 386999ce65d..976db4b8a01 100755
@@ -585,12 +585,14 @@ sub check_test {
 	my $parser = TestParser->new(\$body);
 	my @tokens = $parser->parse();
 	return unless $emit_all || grep(/\?![^?]+\?!/, @tokens);
+	my $c = main::fd_colors(1);
 	my $checked = join(' ', @tokens);
 	$checked =~ s/^\n//;
 	$checked =~ s/^ //mg;
 	$checked =~ s/ $//mg;
+	$checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;
 	$checked .= "\n" unless $checked =~ /\n$/;
-	push(@{$self->{output}}, "# chainlint: $title\n$checked");
+	push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
 }
 
 sub parse_cmd {

@@ -615,6 +617,41 @@ if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {
 	$interval = sub { return Time::HiRes::tv_interval(shift); };
 }
 
+# Restore TERM if test framework set it to "dumb" so 'tput' will work; do this
+# outside of get_colors() since under 'ithreads' all threads use %ENV of main
+# thread and ignore %ENV changes in subthreads.
+$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};
+
+my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '', green => '', red => '');
+my %COLORS = ();
+sub get_colors {
+	return \%COLORS if %COLORS;
+	if (exists($ENV{NO_COLOR}) ||
+	    system("tput sgr0 >/dev/null 2>&1") != 0 ||
+	    system("tput bold >/dev/null 2>&1") != 0 ||
+	    system("tput rev  >/dev/null 2>&1") != 0 ||
+	    system("tput setaf 1 >/dev/null 2>&1") != 0) {
+		%COLORS = @NOCOLORS;
+		return \%COLORS;
+	}
+	%COLORS = (bold  => `tput bold`,
+		   rev   => `tput rev`,
+		   reset => `tput sgr0`,
+		   blue  => `tput setaf 4`,
+		   green => `tput setaf 2`,
+		   red   => `tput setaf 1`);
+	chomp(%COLORS);
+	return \%COLORS;
+}
+
+my %FD_COLORS = ();
+sub fd_colors {
+	my $fd = shift;
+	return $FD_COLORS{$fd} if exists($FD_COLORS{$fd});
+	$FD_COLORS{$fd} = -t $fd ? get_colors() : {@NOCOLORS};
+	return $FD_COLORS{$fd};
+}
+
 sub ncores {
 	# Windows
 	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});

@@ -630,6 +667,8 @@ sub show_stats {
 	my $walltime = $interval->($start_time);
 	my ($usertime) = times();
 	my ($total_workers, $total_scripts, $total_tests, $total_errs) = (0, 0, 0, 0);
+	my $c = fd_colors(2);
+	print(STDERR $c->{green});
 	for (@$stats) {
 		my ($worker, $nscripts, $ntests, $nerrs) = @$_;
 		print(STDERR "worker $worker: $nscripts scripts, $ntests tests, $nerrs errors\n");

@@ -638,7 +677,7 @@ sub show_stats {
 		$total_tests += $ntests;
 		$total_errs += $nerrs;
 	}
-	printf(STDERR "total: %d workers, %d scripts, %d tests, %d errors, %.2fs/%.2fs (wall/user)\n", $total_workers, $total_scripts, $total_tests, $total_errs, $walltime, $usertime);
+	printf(STDERR "total: %d workers, %d scripts, %d tests, %d errors, %.2fs/%.2fs (wall/user)$c->{reset}\n", $total_workers, $total_scripts, $total_tests, $total_errs, $walltime, $usertime);
 }
 
 sub check_script {

@@ -656,8 +695,9 @@ sub check_script {
 		my $parser = ScriptParser->new(\$s);
 		1 while $parser->parse_cmd();
 		if (@{$parser->{output}}) {
+			my $c = fd_colors(1);
 			my $s = join('', @{$parser->{output}});
-			$emit->("# chainlint: $path\n" . $s);
+			$emit->("$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\n" . $s);
 			$nerrs += () = $s =~ /\?![^?]+\?!/g;
 		}
 		$ntests += $parser->{ntests};

base-commit: 76d57e004b0391503ca7719c932df2a0bd617d0a

From: Eric Sunshine <[email protected]>

When chainlint.pl detects problems in a test definition, it emits the test definition with "?!FOO?!" annotations highlighting the problems it discovered. For instance, given this problematic test:

test_expect_success 'discombobulate frobnitz' '
git frob babble &&
(echo balderdash; echo gnabgib) >expect && for i in three two one
do
git nitfol $i
done >actual
test_cmp expect actual
'

chainlint.pl will output:

# chainlint: t1234-confusing.sh
# chainlint: discombobulate frobnitz
git frob babble &&
(echo balderdash ; ?!AMP?! echo gnabgib) >expect && for i in three two one
do
git nitfol $i ?!LOOP?!
done >actual ?!AMP?!
test_cmp expect actual

Quoted 10 lines · Eric Sunshine via GitGitGadget

When chainlint.pl detects problems in a test definition, it emits the test definition with "?!FOO?!" annotations highlighting the problems it discovered. For instance, given this problematic test:

    test_expect_success 'discombobulate frobnitz' '
        (echo balderdash; echo gnabgib) >expect &&
    '

chainlint.pl will output:

Quoted 3 lines

    # chainlint: t1234-confusing.sh
    # chainlint: discombobulate frobnitz
    (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&

I've noticed that chainlint.pl is better in some ways, but that the "deparse" output tends to be quite jarring. but I can't find version of it that emitted this "will output" here.

Before this patch, or fb41727b7ed (t: retire unused chainlint.sed, 2022-09-01) we'd emit this instead:

        ( echo balderdash ; ?!AMP?! echo gnabgib ) > expect &&

The difference is in whitespace, e.g. "( ", not "(", "> " not ">". This is just because it's emitting "raw" tokenizer tokens.

Was there maybe some local version where the whitespace munging you're doing against $checked was different & this commit message was left over?

Anyway, that sort of an aside, but I did go hunting for the version with slightly better whitespace output.

But to get to the actual point: I've found the new chainlint.pl output harder to read sometimes, because it goes through this parse & deparse state, so you're preserving "\n"''s.

Whereas the old "sed" output also sucked because we couldn't note where the issue was, but we spewed out the test source verbatim.

But it seem to me that we could get much better output if the ShellParser/Lexer etc. just kept enough state to emit "startcol", "endcol" and "linenum" along with every token, or something like that (you might want offsets from the beginning of the parsed source instead).

Then when it has errors it could emit the actual source passed in, and even do gcc/clang-style underlining.

I poked at getting that working for a few minutes, but quickly saw that someone more familiar with the code could do it much quicker, so consider the above a feature request :)

Another thing: When a test *ends* in a "&&" (common when you copy/paste e.g. "test_cmp expect actual &&\n" from another test) it doesn't spot it, but instead we get all the way to the eval/117, i.e. "broken &&-chain or run-away HERE-DOC".

More feature requests (because for some reason you've got infinite time, but I don't :): This software is really close to being able to also change the tests on the fly. If you could define callbacks where you could change subsets of the parse stream, say a single command like:

grep some.*rx file

So we could rewrite that into:

if ! grep some.*rx foo
then
cat foo &&
false
fi

And other interesting auto-fixups and borderline coccinelle transformations, e.g. changing our various:

        test "$(git ...) = "" &&

Into:

        git ... >out &&
        test_must_be_empty out

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Another thing: When a test *ends* in a "&&" (common when you copy/paste e.g. "test_cmp expect actual &&\n" from another test) it doesn't spot it, but instead we get all the way to the eval/117, i.e. "broken &&-chain or run-away HERE-DOC".

Yes, I recall considering that case and others, but decided that that's probably outside the scope of the linter. [...]

It is unfortunate, though, that the shell's "syntax error" output gets swallowed by the eval/117 checker in test-lib.sh and turned into a somewhat less useful message. I'm not quite sure how we can fix the eval/117 checker to not swallow genuine syntax errors like that, unless we perhaps specially recognize exit code 2 and, um, do something...

Quoted 11 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Eric Sunshine via GitGitGadget

When chainlint.pl detects problems in a test definition, it emits the test definition with "?!FOO?!" annotations highlighting the problems it discovered. For instance, given this problematic test:

    test_expect_success 'discombobulate frobnitz' '
        (echo balderdash; echo gnabgib) >expect &&
    '

chainlint.pl will output:

Quoted 7 lines

Quoted 3 lines

    # chainlint: t1234-confusing.sh
    # chainlint: discombobulate frobnitz
    (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&

I've noticed that chainlint.pl is better in some ways, but that the "deparse" output tends to be quite jarring. but I can't find version of it that emitted this "will output" here.

There is no such version.
[...]
No, I botched the commit message. I typed the example test in by hand and then, also by hand, typed in the example output, forgetting to insert the spaces which you correctly noted are missing from the example output. I should have run the example test through chainlint.pl and copy/pasted its output into the commit message. (I did, in fact, run the sample test through chanlint.pl _after_ hand-typing the example output, and compared them by eye but missed most of the whitespace differences.)

Quoted 1 line

Anyway, that sort of an aside, but I did go hunting for the version with slightly better whitespace output.

Sorry, my fault for a faulty commit message.

Quoted 6 lines

But to get to the actual point: I've found the new chainlint.pl output harder to read sometimes, because it goes through this parse & deparse state, so you're preserving "\n"''s.

Whereas the old "sed" output also sucked because we couldn't note where the issue was, but we spewed out the test source verbatim.

Somewhat verbatim. chainlint.sed did swallow blank lines and comment lines, and it folded multi-line strings into one-line strings.

Quoted 6 lines

But it seem to me that we could get much better output if the ShellParser/Lexer etc. just kept enough state to emit "startcol", "endcol" and "linenum" along with every token, or something like that (you might want offsets from the beginning of the parsed source instead).

Quoted 6 lines

Then when it has errors it could emit the actual source passed in, and even do gcc/clang-style underlining.

I poked at getting that working for a few minutes, but quickly saw that someone more familiar with the code could do it much quicker, so consider the above a feature request :)

Yes, there should be better integration between the lexer and parser for emitting errors. Unfortunately, it didn't occur to me during implementation, and I only thought about it when Peff mentioned the difficult-to-read output in a different part of this discussion.

An alternative, somewhat hacky approach, might be to simply retain whitespace as tokens in the token stream. That would require less retrofitting of the lexer, though perhaps more complexity/ugliness in the parser. It wouldn't give you gcc/clang-level underlining, etc., but would more or less preserve whitespace in the test definition. Definitely not a proper solution, but perhaps "good enough".

Quoted 4 lines

Another thing: When a test *ends* in a "&&" (common when you copy/paste e.g. "test_cmp expect actual &&\n" from another test) it doesn't spot it, but instead we get all the way to the eval/117, i.e. "broken &&-chain or run-away HERE-DOC".

Yes, I recall considering that case and others, but decided that that's probably outside the scope of the linter. In particular, a trailing "&&" is a plain old syntax error, and the shell itself is perfectly capable of diagnosing that problem along with all other syntax errors, and you'll find out about syntax errors in your code when the shell tries running it. The linter, on the other hand, is meant to catch semantic problems (per the project's best-practices) in what is assumed to be syntactically valid shell code. I suppose the linter could be made to complain about this syntax error and others, but it seems unnecessary to bloat it by duplicating behavior already provided by the shell itself.

It is unfortunate, though, that the shell's "syntax error" output gets swallowed by the eval/117 checker in test-lib.sh and turned into a somewhat less useful message. I'm not quite sure how we can fix the eval/117 checker to not swallow genuine syntax errors like that, unless we perhaps specially recognize exit code 2 and, um, do