| Message ID | pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7c04aa73906b9186c9d46010227d4437fd534d93 |
| Headers | show |
| Series | [v2] chainlint: colorize problem annotations and test delimiters | expand |
On Tue, Sep 13, 2022 at 04:01:47AM +0000, Eric Sunshine via GitGitGadget wrote: > 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. -Peff
Jeff King <peff@peff.net> writes: > On Tue, Sep 13, 2022 at 04:01:47AM +0000, Eric Sunshine via GitGitGadget wrote: > >> 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. :) Yup, I tend to think that reverse red is uglier and is more attention grabbing than bold red. Let's stop here for now and let others paint it in other colors by introducing configuration knob or whatnot but outside the topic. > Thanks again for adding this. That too.
On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote: > From: Eric Sunshine <sunshine@sunshineco.com> > > 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 <sunshine@sunshineco.com> > --- > 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/pull.1324.git.git.1663023888412.gitgitgadget@gmail.com/ > [2] > https://lore.kernel.org/git/CAPig+cTq3j5M7cz3T14h9U6e+H5PAu8JJ_Svq87W3WviwS6_qA@mail.gmail.com/ > [3] https://lore.kernel.org/git/xmqqo7vkazuh.fsf@gitster.g/ > > 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(-) > > diff --git a/t/chainlint.pl b/t/chainlint.pl > index 386999ce65d..976db4b8a01 100755 > --- a/t/chainlint.pl > +++ b/t/chainlint.pl > @@ -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
[Please ignore the just-sent empty https://lore.kernel.org/git/221024.86a65lee8i.gmgdl@evledraar.gmail.com/; local PBCAK problem :)] On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote: > From: Eric Sunshine <sunshine@sunshineco.com> > > 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 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: 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 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 Tokenized as: ["grep", "some.*rx" "file"] If you could define an interface to have a callback function e.g. as: sub munge_line_tokens { my $t = shift; return unless $t->[0] eq "grep"; # no changes my @t = @$t; return [qw(if ! grep), @t[1..$#t], qw(then cat), $t[-1], qw(&& false fi)]; } 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
On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote: > > 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: > > > > # 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. > 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? 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.) > 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. > 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. > 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 :) 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". > 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 something... > 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 The lexer/parser implemented for chainlint.pl might indeed be useful for such transformations. I could imagine a tool which someone runs on an old-style test script to help update it to modern conventions, after which the person would, of course, carefully check all the applied transformations. That's not something we'd necessarily want to do project-wide, but might be handy when already working on a test script for some other reason.
On Tue, Oct 25, 2022 at 12:05 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > 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... Another "fix" would be to drop the eval/117 checker altogether. I retained it as a final safeguard in case something slipped past chainlint.pl, however, I'm not sure how much value the eval/117 checker really has since it misses so many real-world cases, such as any &&-chain break in the body of a compound context (if/fi, case/esac, for/done, while/done, (...), {...}, $(...), etc.). Moreover, we see now that it's also obscuring useful error messages (such as "syntax error") from the shell itself. So, dropping it may be an option(?).
On Tue, Oct 25 2022, Eric Sunshine wrote: > On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote: >> > 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: >> > >> > # 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.) > >> 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. No worries! >> 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. Yeah, it had a lot of edge cases, the new one's much better overall. I just sometimes found it jarring to look at code that's not /quite/ my version now, but anyway... :) >> 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 :) > > 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". Yeah, maybe. >> 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. FWIW I thought it would be nice because it sometimes takes 10s or whatever to get to the syntax error by running the test, but the linter can find it right away. > 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
diff --git a/t/chainlint.pl b/t/chainlint.pl index 386999ce65d..976db4b8a01 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -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};