chainlint: colorize problem annotations and test delimiters #1324
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/preview |
|
Preview email sent as pull.1324.git.git.1662972813584.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1324.git.git.1663023803268.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1324.git.git.1663023888412.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
> + $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.
> +# 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};
Sounds quite sensible.
|
|
On the Git mailing list, Eric Sunshine wrote (reply to this): On Mon, Sep 12, 2022 at 7:55 PM Junio C Hamano <gitster@pobox.com> wrote:
> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > + $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.
That's possible, but I'd rather not do that for a couple reasons:
(1) Even with the coloring, I still find it handy to be able to search
for "?!" in the output in order to jump to the next problem (or to
filter to just the problem lines via, say, grep).
(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. As such, having more characters "?!...?!"
may help capture the eye more easily than fewer characters. (In fact,
I briefly considered coloring the entire line red to combat the
eye-gliding problem but wasn't sure if that would be helpful or
hurtful.) |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Sep 12, 2022 at 11:04:48PM +0000, Eric Sunshine via GitGitGadget wrote:
> @@ -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->{bold}$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");
> }
Nice, this ended up much simpler than I feared. I thought we'd have to
touch each spot that added an annotation, and then deal with the
internal text matching (like the one in the hunk above). Being able to
do it centrally on output is much nicer.
> +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`);
> + 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.
-Peff |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Sep 12, 2022 at 04:55:01PM -0700, Junio C Hamano wrote:
> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > + $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.
Having seen the output in the wild (or at least on the example which I
found hard to read originally), I kind of like the "?!" being retained
even in the color version. It really makes things stand out.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Eric Sunshine <sunshine@sunshineco.com> writes:
> (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.
I do not think if configurability like "git diff" has is necessary;
it would be overkill. I personally do not mind more noise "?!"
around the keyword, especially since these are only shown when there
are problems detected. |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): On Mon, Sep 12, 2022 at 8:15 PM Jeff King <peff@peff.net> wrote:
> On Mon, Sep 12, 2022 at 11:04:48PM +0000, Eric Sunshine via GitGitGadget wrote:
> > +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`);
> > + 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!).
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.
> 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.
That might be worth considering at some point.
> 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.
Good to hear. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Sep 12, 2022 at 08:15:33PM -0400, Jeff King wrote:
> 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 timed running the suite with and without TERM=dumb, as that is enough
to get test-lib.sh to skip running tput entirely. It doesn't seem to
make a measurable difference for me. Possibly it could on Windows, but I
don't think it's worth worrying about too much.
(If we did want to worry, "tput -S" is another option; it's not in
POSIX, but probably could be used on Windows).
And of course this was all "gee, I wonder about test-lib.sh"; it is all
orthogonal to your patch.
-Peff |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): On Mon, Sep 12, 2022 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > (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.
Hmm, yes, that might be worth investigating. I also typically work
with black-ink-on-white-paper terminal, and although the problem is
perhaps worse with that color scheme, I nevertheless found that my eye
would sometimes glide over the red annotations even when I tested with
other color schemes (i.e. light-ink-on-dark-paper). |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Sep 12, 2022 at 08:30:02PM -0400, Eric Sunshine wrote:
> 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.
Ah, right, that's even better.
I wondered if we could use the same trick in test-lib.sh, but it does
color some output even on success. But on further thought, the reason
that I couldn't measure any impact of tput in my other message may have
just been because I was running under "prove". So there's no tty and
thus no coloring in the first place. Not to mention that I am using
--verbose-log, which also suppresses color.
So I suspect there is really nothing to speed up at all. Most cases
running all of the tests will end up turning off color anyway. And if
they are not, they are probably bottle-necked on the terminal speed. ;)
-Peff |
sunshineco
force-pushed
the
chainlintcolor
branch
from
d670570 to
d3617c3
Compare
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>
sunshineco
force-pushed
the
chainlintcolor
branch
from
d3617c3 to
acf9183
Compare
|
/preview |
|
Preview email sent as pull.1324.v2.git.git.1663039329708.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via 766c328. |
|
This patch series was integrated into seen via 7a661e8. |
|
This patch series was integrated into next via 577e591. |
|
On the Git mailing list, Jeff King wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
This patch series was integrated into seen via 412d852. |
|
There was a status update in the "Cooking" section about the branch Revamp chainlint script for our tests. Will merge to 'master'. source: <pull.1322.git.git.1661992197.gitgitgadget@gmail.com> source: <pull.1324.git.git.1663023888412.gitgitgadget@gmail.com> |
This comment has been minimized.
This comment has been minimized.
|
This patch series was integrated into seen via ca9ac53. |
|
This patch series was integrated into seen via d579c0d. |
|
There was a status update in the "Cooking" section about the branch Revamp chainlint script for our tests. Will merge to 'master'. source: <pull.1322.git.git.1661992197.gitgitgadget@gmail.com> source: <pull.1324.git.git.1663023888412.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 9d58241. |
|
This patch series was integrated into master via 9d58241. |
|
This patch series was integrated into next via 9d58241. |
|
Closed via 9d58241. |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): 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
|
|
User |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): [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 the Git mailing list, Eric Sunshine wrote (reply to this): 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 the Git mailing list, Eric Sunshine wrote (reply to this): 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 the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/
Cc: Jeff King peff@peff.net
Cc: Junio C Hamano gitster@pobox.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com