{"thread":{"id":"58421","subject":"[PATCH] chainlint: colorize problem annotations and test delimiters","startedAt":"2022-09-12T23:04:55Z","lastAt":"2022-10-25T10:21:12Z","messageCount":18,"participants":["Eric Sunshine via GitGitGadget","Junio C Hamano","Eric Sunshine","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"462951","messageId":"pull.1324.git.git.1663023888412.gitgitgadget@gmail.com","threadId":"58421","inReplyTo":null,"subject":"[PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-12T23:04:48Z","receivedAt":"2022-09-12T23:04:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nWhen `chainlint.pl` detects problems in a test definition, it emits the\ntest definition with \"?!FOO?!\" annotations highlighting the problems it\ndiscovered. For instance, given this problematic test:\n\n test_expect_success 'discombobulate frobnitz' '\n git frob babble &&\n (echo balderdash; echo gnabgib) >expect &&\n for i in three two one\n do\n git nitfol $i\n done >actual\n test_cmp expect actual\n '\n\nchainlint.pl will output:\n\n # chainlint: t1234-confusing.sh\n # chainlint: discombobulate frobnitz\n git frob babble &&\n (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&\n for i in three two one\n do\n git nitfol $i ?!LOOP?!\n done >actual ?!AMP?!\n test_cmp expect actual\n\nin which it may be difficult to spot the \"?!FOO?!\" annotations. The\nproblem is compounded when multiple tests, possibly in multiple\nscripts, fail \"linting\", in which case it may be difficult to spot the\n\"# chainlint:\" lines which delimit one problematic test from another.\n\nTo ameliorate this potential problem, colorize the \"?!FOO?!\" annotations\nin order to quickly draw the test author's attention to the problem\nspots, and colorize the \"# chainlint:\" lines to help the author identify\nthe name of each script and each problematic test.\n\nColorization is disabled automatically if output is not directed to a\nterminal or if NO_COLOR environment variable is set. The implementation\nis specific to Unix (it employs `tput` if available) but works equally\nwell in the Git for Windows development environment which emulates Unix\nsufficiently.\n\nSigned-off-by: Eric Sunshine \n---\n chainlint: colorize problem annotations and test delimiters\n \n Peff nerd-sniped me yet again[1,2,3,4].\n \n This is atop \"es/chainlint\" (eab3357b05b2) in \"next\".\n \n [1]\n https://lore.kernel.org/git/YJzGcZpZ+E9R0gYd@coredump.intra.peff.net/\n [2]\n https://lore.kernel.org/git/Yx1x5lme2SGBjfia@coredump.intra.peff.net/\n [3]\n https://lore.kernel.org/git/CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com/\n [4]\n https://lore.kernel.org/git/Yx4pg2t6JXR+lsd4@coredump.intra.peff.net/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1324%2Fsunshineco%2Fchainlintcolor-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1324/sunshineco/chainlintcolor-v1\nPull-Request: https://github.com/git/git/pull/1324\n\n t/chainlint.pl | 44 +++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 41 insertions(+), 3 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 386999ce65d..3a6d85ecfdd 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -585,12 +585,14 @@ sub check_test {\n \tmy $parser = TestParser->new(\\$body);\n \tmy @tokens = $parser->parse();\n \treturn unless $emit_all || grep(/\\?![^?]+\\?!/, @tokens);\n+\tmy $c = main::fd_colors(1);\n \tmy $checked = join(' ', @tokens);\n \t$checked =~ s/^\\n//;\n \t$checked =~ s/^ //mg;\n \t$checked =~ s/ $//mg;\n+\t$checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n-\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n+\tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n }\n \n sub parse_cmd {\n@@ -615,6 +617,39 @@ if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n \t$interval = sub { return Time::HiRes::tv_interval(shift); };\n }\n \n+# Restore TERM if test framework set it to \"dumb\" so 'tput' will work; do this\n+# outside of get_colors() since under 'ithreads' all threads use %ENV of main\n+# thread and ignore %ENV changes in subthreads.\n+$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};\n+\n+my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');\n+my %COLORS = ();\n+sub get_colors {\n+\treturn \\%COLORS if %COLORS;\n+\tif (exists($ENV{NO_COLOR}) ||\n+\t system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n+\t system(\"tput bold >/dev/null 2>&1\") != 0 ||\n+\t system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n+\t\t%COLORS = @NOCOLORS;\n+\t\treturn \\%COLORS;\n+\t}\n+\t%COLORS = (bold => `tput bold`,\n+\t\t reset => `tput sgr0`,\n+\t\t blue => `tput setaf 4`,\n+\t\t green => `tput setaf 2`,\n+\t\t red => `tput setaf 1`);\n+\tchomp(%COLORS);\n+\treturn \\%COLORS;\n+}\n+\n+my %FD_COLORS = ();\n+sub fd_colors {\n+\tmy $fd = shift;\n+\treturn $FD_COLORS{$fd} if exists($FD_COLORS{$fd});\n+\t$FD_COLORS{$fd} = -t $fd ? get_colors() : {@NOCOLORS};\n+\treturn $FD_COLORS{$fd};\n+}\n+\n sub ncores {\n \t# Windows\n \treturn $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});\n@@ -630,6 +665,8 @@ sub show_stats {\n \tmy $walltime = $interval->($start_time);\n \tmy ($usertime) = times();\n \tmy ($total_workers, $total_scripts, $total_tests, $total_errs) = (0, 0, 0, 0);\n+\tmy $c = fd_colors(2);\n+\tprint(STDERR $c->{green});\n \tfor (@$stats) {\n \t\tmy ($worker, $nscripts, $ntests, $nerrs) = @$_;\n \t\tprint(STDERR \"worker $worker: $nscripts scripts, $ntests tests, $nerrs errors\\n\");\n@@ -638,7 +675,7 @@ sub show_stats {\n \t\t$total_tests += $ntests;\n \t\t$total_errs += $nerrs;\n \t}\n-\tprintf(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);\n+\tprintf(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);\n }\n \n sub check_script {\n@@ -656,8 +693,9 @@ sub check_script {\n \t\tmy $parser = ScriptParser->new(\\$s);\n \t\t1 while $parser->parse_cmd();\n \t\tif (@{$parser->{output}}) {\n+\t\t\tmy $c = fd_colors(1);\n \t\t\tmy $s = join('', @{$parser->{output}});\n-\t\t\t$emit->(\"# chainlint: $path\\n\" . $s);\n+\t\t\t$emit->(\"$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\\n\" . $s);\n \t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n \t\t}\n \t\t$ntests += $parser->{ntests};\n\nbase-commit: 50f0e44ec40b5fab5d618dd26ebd776c47e9af13\n-- \ngitgitgadget\n"},{"id":"462953","messageId":"xmqqsfkwb12i.fsf@gitster.g","threadId":"58421","inReplyTo":"pull.1324.git.git.1663023888412.gitgitgadget@gmail.com","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-12T23:55:01Z","receivedAt":"2022-09-12T23:55:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Sunshine via GitGitGadget\" writes:\n\n> +\t$checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n\nIt may be just me, but coloring the whole \"?!LOOP?!\" in red feels a\nbit strange. I would have expected more like\n\n\tif ($c->{color_in_use}) {\n\t\t$checked =~ s/\\?!([^?]+)\\?!/$c->{bold}$c->{red}$1$c->{reset}/mg;\n\t}\n\nIOW, switching between \"?!LOOP?!\" and \"LOOP\".\n\nBut it may be just me.\n\n> +# Restore TERM if test framework set it to \"dumb\" so 'tput' will work; do this\n> +# outside of get_colors() since under 'ithreads' all threads use %ENV of main\n> +# thread and ignore %ENV changes in subthreads.\n> +$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};\n\nSounds quite sensible.\n\n"},{"id":"462955","messageId":"CAPig+cTq3j5M7cz3T14h9U6e+H5PAu8JJ_Svq87W3WviwS6_qA@mail.gmail.com","threadId":"58421","inReplyTo":"xmqqsfkwb12i.fsf@gitster.g","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-13T00:14:27Z","receivedAt":"2022-09-13T00:14:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 12, 2022 at 7:55 PM Junio C Hamano wrote:\n> \"Eric Sunshine via GitGitGadget\" writes:\n> > + $checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n>\n> It may be just me, but coloring the whole \"?!LOOP?!\" in red feels a\n> bit strange. I would have expected more like\n>\n> if ($c->{color_in_use}) {\n> $checked =~ s/\\?!([^?]+)\\?!/$c->{bold}$c->{red}$1$c->{reset}/mg;\n> }\n>\n> IOW, switching between \"?!LOOP?!\" and \"LOOP\".\n>\n> But it may be just me.\n\nThat's possible, but I'd rather not do that for a couple reasons:\n\n(1) Even with the coloring, I still find it handy to be able to search\nfor \"?!\" in the output in order to jump to the next problem (or to\nfilter to just the problem lines via, say, grep).\n\n(2) In practice, I found that even after coloring those annotations in\nred, it was still easy for the eye to glide right over them in the\noutput without really noticing them. Switching it to bold red helped a\nbit, but my eye still glided over them sometimes. One possible reason\nthat the eye was able to glide over them may be because the \"?!FOO?!\"\nannotations are very short bits of text buried in the much larger and\ntextually noisy test body. As such, having more characters \"?!...?!\"\nmay help capture the eye more easily than fewer characters. (In fact,\nI briefly considered coloring the entire line red to combat the\neye-gliding problem but wasn't sure if that would be helpful or\nhurtful.)\n"},{"id":"462956","messageId":"Yx/LpUglpjY5ZNas@coredump.intra.peff.net","threadId":"58421","inReplyTo":"pull.1324.git.git.1663023888412.gitgitgadget@gmail.com","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-13T00:15:33Z","receivedAt":"2022-09-13T00:15:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2022 at 11:04:48PM +0000, Eric Sunshine via GitGitGadget wrote:\n\n> @@ -585,12 +585,14 @@ sub check_test {\n> \tmy $parser = TestParser->new(\\$body);\n> \tmy @tokens = $parser->parse();\n> \treturn unless $emit_all || grep(/\\?![^?]+\\?!/, @tokens);\n> +\tmy $c = main::fd_colors(1);\n> \tmy $checked = join(' ', @tokens);\n> \t$checked =~ s/^\\n//;\n> \t$checked =~ s/^ //mg;\n> \t$checked =~ s/ $//mg;\n> +\t$checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n> \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n> -\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n> +\tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n> }\n\nNice, this ended up much simpler than I feared. I thought we'd have to\ntouch each spot that added an annotation, and then deal with the\ninternal text matching (like the one in the hunk above). Being able to\ndo it centrally on output is much nicer.\n\n> +my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');\n> +my %COLORS = ();\n> +sub get_colors {\n> +\treturn \\%COLORS if %COLORS;\n> +\tif (exists($ENV{NO_COLOR}) ||\n> +\t system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput bold >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n> +\t\t%COLORS = @NOCOLORS;\n> +\t\treturn \\%COLORS;\n> +\t}\n> +\t%COLORS = (bold => `tput bold`,\n> +\t\t reset => `tput sgr0`,\n> +\t\t blue => `tput setaf 4`,\n> +\t\t green => `tput setaf 2`,\n> +\t\t red => `tput setaf 1`);\n> +\tchomp(%COLORS);\n> +\treturn \\%COLORS;\n> +}\n\nThis is a lot of new processes. Should be OK in the run-once-for-all-tests\nmode. It does make me wonder how much time regular test-lib.sh spends\ndoing these tput checks for every script (at least it's not every\nsnippet!).\n\nIt feels like we could build a color.sh snippet once and then include it\nin each script. But maybe that is dumb, since you could in theory build\nin one terminal and then run in another. Unlikely, but it shows that\nfile dependencies are a mismatch. I guess a better match would be\nstuffing it into the environment before starting all of the tests.\n\n> [...]\n\nI ran this on my pre-fixup state where I had a half-dozen linter checks.\nIt's _so_ much more readable. Thanks for working on it.\n\n-Peff\n"},{"id":"462957","messageId":"Yx/L0vWlMOyLhtjJ@coredump.intra.peff.net","threadId":"58421","inReplyTo":"xmqqsfkwb12i.fsf@gitster.g","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-13T00:16:18Z","receivedAt":"2022-09-13T00:16:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2022 at 04:55:01PM -0700, Junio C Hamano wrote:\n\n> \"Eric Sunshine via GitGitGadget\" writes:\n> \n> > +\t$checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n> \n> It may be just me, but coloring the whole \"?!LOOP?!\" in red feels a\n> bit strange. I would have expected more like\n> \n> \tif ($c->{color_in_use}) {\n> \t\t$checked =~ s/\\?!([^?]+)\\?!/$c->{bold}$c->{red}$1$c->{reset}/mg;\n> \t}\n> \n> IOW, switching between \"?!LOOP?!\" and \"LOOP\".\n> \n> But it may be just me.\n\nHaving seen the output in the wild (or at least on the example which I\nfound hard to read originally), I kind of like the \"?!\" being retained\neven in the color version. It really makes things stand out.\n\n-Peff\n"},{"id":"462958","messageId":"xmqqo7vkazuh.fsf@gitster.g","threadId":"58421","inReplyTo":"CAPig+cTq3j5M7cz3T14h9U6e+H5PAu8JJ_Svq87W3WviwS6_qA@mail.gmail.com","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-13T00:21:26Z","receivedAt":"2022-09-13T00:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> (2) In practice, I found that even after coloring those annotations in\n> red, it was still easy for the eye to glide right over them in the\n> output without really noticing them. Switching it to bold red helped a\n> bit, but my eye still glided over them sometimes. One possible reason\n> that the eye was able to glide over them may be because the \"?!FOO?!\"\n> annotations are very short bits of text buried in the much larger and\n> textually noisy test body.\n\nMaybe partly because I work with black-ink-on-white-paper terminal\nsetting, and maybe partly because my color perception is suboptimal,\nI learned to use \"[diff.color] old = red reverse\", because non-bold\nred letters do not stand out enough. Perhaps you may want to try\nreverse output to see how well it makes them stand out for you.\n\nI do not think if configurability like \"git diff\" has is necessary;\nit would be overkill. I personally do not mind more noise \"?!\"\naround the keyword, especially since these are only shown when there\nare problems detected.\n"},{"id":"462960","messageId":"CAPig+cRTatQRS2MyOTfmz56UKqtz_x_Gk6j=rnYR-jTkM-CDdQ@mail.gmail.com","threadId":"58421","inReplyTo":"Yx/LpUglpjY5ZNas@coredump.intra.peff.net","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-13T00:30:02Z","receivedAt":"2022-09-13T00:30:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 12, 2022 at 8:15 PM Jeff King wrote:\n> On Mon, Sep 12, 2022 at 11:04:48PM +0000, Eric Sunshine via GitGitGadget wrote:\n> > +my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');\n> > +my %COLORS = ();\n> > +sub get_colors {\n> > + return \\%COLORS if %COLORS;\n> > + if (exists($ENV{NO_COLOR}) ||\n> > + system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n> > + system(\"tput bold >/dev/null 2>&1\") != 0 ||\n> > + system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n> > + %COLORS = @NOCOLORS;\n> > + return \\%COLORS;\n> > + }\n> > + %COLORS = (bold => `tput bold`,\n> > + reset => `tput sgr0`,\n> > + blue => `tput setaf 4`,\n> > + green => `tput setaf 2`,\n> > + red => `tput setaf 1`);\n> > + chomp(%COLORS);\n> > + return \\%COLORS;\n> > +}\n>\n> This is a lot of new processes. Should be OK in the run-once-for-all-tests\n> mode. It does make me wonder how much time regular test-lib.sh spends\n> doing these tput checks for every script (at least it's not every\n> snippet!).\n\nThis is indeed a lot of new processes, but this color interrogation is\ndone lazily, only if a problem is detected, so it should be zero-cost\nin the (hopefully) normal case of a lint-clean script.\n\nI had the exact same thought about the cost being paid by test-lib.sh\nmaking all those `tput` invocations.\n\n> It feels like we could build a color.sh snippet once and then include it\n> in each script. But maybe that is dumb, since you could in theory build\n> in one terminal and then run in another. Unlikely, but it shows that\n> file dependencies are a mismatch. I guess a better match would be\n> stuffing it into the environment before starting all of the tests.\n\nThat might be worth considering at some point.\n\n> I ran this on my pre-fixup state where I had a half-dozen linter checks.\n> It's _so_ much more readable. Thanks for working on it.\n\nGood to hear.\n"},{"id":"462961","messageId":"Yx/PnWnkYAuWToiz@coredump.intra.peff.net","threadId":"58421","inReplyTo":"Yx/LpUglpjY5ZNas@coredump.intra.peff.net","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-13T00:32:29Z","receivedAt":"2022-09-13T00:32:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2022 at 08:15:33PM -0400, Jeff King wrote:\n\n> This is a lot of new processes. Should be OK in the run-once-for-all-tests\n> mode. It does make me wonder how much time regular test-lib.sh spends\n> doing these tput checks for every script (at least it's not every\n> snippet!).\n> \n> It feels like we could build a color.sh snippet once and then include it\n> in each script. But maybe that is dumb, since you could in theory build\n> in one terminal and then run in another. Unlikely, but it shows that\n> file dependencies are a mismatch. I guess a better match would be\n> stuffing it into the environment before starting all of the tests.\n\nI timed running the suite with and without TERM=dumb, as that is enough\nto get test-lib.sh to skip running tput entirely. It doesn't seem to\nmake a measurable difference for me. Possibly it could on Windows, but I\ndon't think it's worth worrying about too much.\n\n(If we did want to worry, \"tput -S\" is another option; it's not in\nPOSIX, but probably could be used on Windows).\n\nAnd of course this was all \"gee, I wonder about test-lib.sh\"; it is all\northogonal to your patch.\n\n-Peff\n"},{"id":"462962","messageId":"CAPig+cR4neH_FU+tTr3qzyPp=5WJUfFydw7Y4CMJn4k+iSQs6A@mail.gmail.com","threadId":"58421","inReplyTo":"xmqqo7vkazuh.fsf@gitster.g","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-13T00:39:27Z","receivedAt":"2022-09-13T00:41:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 12, 2022 at 8:21 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > (2) In practice, I found that even after coloring those annotations in\n> > red, it was still easy for the eye to glide right over them in the\n> > output without really noticing them. Switching it to bold red helped a\n> > bit, but my eye still glided over them sometimes. One possible reason\n> > that the eye was able to glide over them may be because the \"?!FOO?!\"\n> > annotations are very short bits of text buried in the much larger and\n> > textually noisy test body.\n>\n> Maybe partly because I work with black-ink-on-white-paper terminal\n> setting, and maybe partly because my color perception is suboptimal,\n> I learned to use \"[diff.color] old = red reverse\", because non-bold\n> red letters do not stand out enough. Perhaps you may want to try\n> reverse output to see how well it makes them stand out for you.\n\nHmm, yes, that might be worth investigating. I also typically work\nwith black-ink-on-white-paper terminal, and although the problem is\nperhaps worse with that color scheme, I nevertheless found that my eye\nwould sometimes glide over the red annotations even when I tested with\nother color schemes (i.e. light-ink-on-dark-paper).\n"},{"id":"462963","messageId":"Yx/eG5xJonNh7Dsz@coredump.intra.peff.net","threadId":"58421","inReplyTo":"CAPig+cRTatQRS2MyOTfmz56UKqtz_x_Gk6j=rnYR-jTkM-CDdQ@mail.gmail.com","subject":"Re: [PATCH] chainlint: colorize problem annotations and test delimiters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-13T01:34:19Z","receivedAt":"2022-09-13T01:34:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2022 at 08:30:02PM -0400, Eric Sunshine wrote:\n\n> This is indeed a lot of new processes, but this color interrogation is\n> done lazily, only if a problem is detected, so it should be zero-cost\n> in the (hopefully) normal case of a lint-clean script.\n> \n> I had the exact same thought about the cost being paid by test-lib.sh\n> making all those `tput` invocations.\n\nAh, right, that's even better.\n\nI wondered if we could use the same trick in test-lib.sh, but it does\ncolor some output even on success. But on further thought, the reason\nthat I couldn't measure any impact of tput in my other message may have\njust been because I was running under \"prove\". So there's no tty and\nthus no coloring in the first place. Not to mention that I am using\n--verbose-log, which also suppresses color.\n\nSo I suspect there is really nothing to speed up at all. Most cases\nrunning all of the tests will end up turning off color anyway. And if\nthey are not, they are probably bottle-necked on the terminal speed. ;)\n\n-Peff\n"},{"id":"462964","messageId":"pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com","threadId":"58421","inReplyTo":"pull.1324.git.git.1663023888412.gitgitgadget@gmail.com","subject":"[PATCH v2] chainlint: colorize problem annotations and test delimiters","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-13T04:01:47Z","receivedAt":"2022-09-13T04:01:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nWhen `chainlint.pl` detects problems in a test definition, it emits the\ntest definition with \"?!FOO?!\" annotations highlighting the problems it\ndiscovered. For instance, given this problematic test:\n\n test_expect_success 'discombobulate frobnitz' '\n git frob babble &&\n (echo balderdash; echo gnabgib) >expect &&\n for i in three two one\n do\n git nitfol $i\n done >actual\n test_cmp expect actual\n '\n\nchainlint.pl will output:\n\n # chainlint: t1234-confusing.sh\n # chainlint: discombobulate frobnitz\n git frob babble &&\n (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&\n for i in three two one\n do\n git nitfol $i ?!LOOP?!\n done >actual ?!AMP?!\n test_cmp expect actual\n\nin which it may be difficult to spot the \"?!FOO?!\" annotations. The\nproblem is compounded when multiple tests, possibly in multiple\nscripts, fail \"linting\", in which case it may be difficult to spot the\n\"# chainlint:\" lines which delimit one problematic test from another.\n\nTo ameliorate this potential problem, colorize the \"?!FOO?!\" annotations\nin order to quickly draw the test author's attention to the problem\nspots, and colorize the \"# chainlint:\" lines to help the author identify\nthe name of each script and each problematic test.\n\nColorization is disabled automatically if output is not directed to a\nterminal or if NO_COLOR environment variable is set. The implementation\nis specific to Unix (it employs `tput` if available) but works equally\nwell in the Git for Windows development environment which emulates Unix\nsufficiently.\n\nSigned-off-by: Eric Sunshine \n---\n chainlint: colorize problem annotations and test delimiters\n \n This is a re-roll of [1] which colorizes the output of \"chainlint.pl\"\n when it detects problems in Git test definitions. During discussion, it\n was noted that the eye could sometimes glide right over[2] the bold-red\n \"?!FOO?!\" annotations, so Junio suggested using reverse video, which is\n what v2 does.\n \n Reverse video certainly makes the \"?!FOO?!\" annotations pop out and draw\n the reader's attention. I find that I don't have a strong preference\n between this version and v1 which merely used bold-red, but I suspect\n that v2 with its reverse video is probably the better approach.\n \n [1]\n https://lore.kernel.org/git/pull.1324.git.git.1663023888412.gitgitgadget@gmail.com/\n [2]\n https://lore.kernel.org/git/CAPig+cTq3j5M7cz3T14h9U6e+H5PAu8JJ_Svq87W3WviwS6_qA@mail.gmail.com/\n [3] https://lore.kernel.org/git/xmqqo7vkazuh.fsf@gitster.g/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1324%2Fsunshineco%2Fchainlintcolor-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1324/sunshineco/chainlintcolor-v2\nPull-Request: https://github.com/git/git/pull/1324\n\nRange-diff vs v1:\n\n 1: d670570e81f ! 1: acf9183ccc6 chainlint: colorize problem annotations and test delimiters\n @@ t/chainlint.pl: sub check_test {\n \t$checked =~ s/^\\n//;\n \t$checked =~ s/^ //mg;\n \t$checked =~ s/ $//mg;\n -+\t$checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n ++\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n -\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n +\tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n @@ t/chainlint.pl: if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n +# thread and ignore %ENV changes in subthreads.\n +$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};\n +\n -+my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');\n ++my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '', green => '', red => '');\n +my %COLORS = ();\n +sub get_colors {\n +\treturn \\%COLORS if %COLORS;\n +\tif (exists($ENV{NO_COLOR}) ||\n +\t system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n +\t system(\"tput bold >/dev/null 2>&1\") != 0 ||\n ++\t system(\"tput rev >/dev/null 2>&1\") != 0 ||\n +\t system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n +\t\t%COLORS = @NOCOLORS;\n +\t\treturn \\%COLORS;\n +\t}\n +\t%COLORS = (bold => `tput bold`,\n ++\t\t rev => `tput rev`,\n +\t\t reset => `tput sgr0`,\n +\t\t blue => `tput setaf 4`,\n +\t\t green => `tput setaf 2`,\n\n\n t/chainlint.pl | 46 +++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 386999ce65d..976db4b8a01 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -585,12 +585,14 @@ sub check_test {\n \tmy $parser = TestParser->new(\\$body);\n \tmy @tokens = $parser->parse();\n \treturn unless $emit_all || grep(/\\?![^?]+\\?!/, @tokens);\n+\tmy $c = main::fd_colors(1);\n \tmy $checked = join(' ', @tokens);\n \t$checked =~ s/^\\n//;\n \t$checked =~ s/^ //mg;\n \t$checked =~ s/ $//mg;\n+\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n-\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n+\tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n }\n \n sub parse_cmd {\n@@ -615,6 +617,41 @@ if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n \t$interval = sub { return Time::HiRes::tv_interval(shift); };\n }\n \n+# Restore TERM if test framework set it to \"dumb\" so 'tput' will work; do this\n+# outside of get_colors() since under 'ithreads' all threads use %ENV of main\n+# thread and ignore %ENV changes in subthreads.\n+$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};\n+\n+my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '', green => '', red => '');\n+my %COLORS = ();\n+sub get_colors {\n+\treturn \\%COLORS if %COLORS;\n+\tif (exists($ENV{NO_COLOR}) ||\n+\t system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n+\t system(\"tput bold >/dev/null 2>&1\") != 0 ||\n+\t system(\"tput rev >/dev/null 2>&1\") != 0 ||\n+\t system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n+\t\t%COLORS = @NOCOLORS;\n+\t\treturn \\%COLORS;\n+\t}\n+\t%COLORS = (bold => `tput bold`,\n+\t\t rev => `tput rev`,\n+\t\t reset => `tput sgr0`,\n+\t\t blue => `tput setaf 4`,\n+\t\t green => `tput setaf 2`,\n+\t\t red => `tput setaf 1`);\n+\tchomp(%COLORS);\n+\treturn \\%COLORS;\n+}\n+\n+my %FD_COLORS = ();\n+sub fd_colors {\n+\tmy $fd = shift;\n+\treturn $FD_COLORS{$fd} if exists($FD_COLORS{$fd});\n+\t$FD_COLORS{$fd} = -t $fd ? get_colors() : {@NOCOLORS};\n+\treturn $FD_COLORS{$fd};\n+}\n+\n sub ncores {\n \t# Windows\n \treturn $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});\n@@ -630,6 +667,8 @@ sub show_stats {\n \tmy $walltime = $interval->($start_time);\n \tmy ($usertime) = times();\n \tmy ($total_workers, $total_scripts, $total_tests, $total_errs) = (0, 0, 0, 0);\n+\tmy $c = fd_colors(2);\n+\tprint(STDERR $c->{green});\n \tfor (@$stats) {\n \t\tmy ($worker, $nscripts, $ntests, $nerrs) = @$_;\n \t\tprint(STDERR \"worker $worker: $nscripts scripts, $ntests tests, $nerrs errors\\n\");\n@@ -638,7 +677,7 @@ sub show_stats {\n \t\t$total_tests += $ntests;\n \t\t$total_errs += $nerrs;\n \t}\n-\tprintf(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);\n+\tprintf(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);\n }\n \n sub check_script {\n@@ -656,8 +695,9 @@ sub check_script {\n \t\tmy $parser = ScriptParser->new(\\$s);\n \t\t1 while $parser->parse_cmd();\n \t\tif (@{$parser->{output}}) {\n+\t\t\tmy $c = fd_colors(1);\n \t\t\tmy $s = join('', @{$parser->{output}});\n-\t\t\t$emit->(\"# chainlint: $path\\n\" . $s);\n+\t\t\t$emit->(\"$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\\n\" . $s);\n \t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n \t\t}\n \t\t$ntests += $parser->{ntests};\n\nbase-commit: 76d57e004b0391503ca7719c932df2a0bd617d0a\n-- \ngitgitgadget\n"},{"id":"463000","messageId":"YyDqycOlUYJO3332@coredump.intra.peff.net","threadId":"58421","inReplyTo":"pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] chainlint: colorize problem annotations and test delimiters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-13T20:40:41Z","receivedAt":"2022-09-13T20:40:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 13, 2022 at 04:01:47AM +0000, Eric Sunshine via GitGitGadget wrote:\n\n> Reverse video certainly makes the \"?!FOO?!\" annotations pop out and draw\n> the reader's attention. I find that I don't have a strong preference\n> between this version and v1 which merely used bold-red, but I suspect\n> that v2 with its reverse video is probably the better approach.\n\nI find this one slightly uglier, but they are equally\nattention-grabbing. And as I hope to rarely see them in the first place,\nI am fine either way. :)\n\nThanks again for adding this.\n\n-Peff\n"},{"id":"463002","messageId":"xmqqfsgv80jk.fsf@gitster.g","threadId":"58421","inReplyTo":"YyDqycOlUYJO3332@coredump.intra.peff.net","subject":"Re: [PATCH v2] chainlint: colorize problem annotations and test delimiters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-13T20:46:55Z","receivedAt":"2022-09-13T20:47:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 13, 2022 at 04:01:47AM +0000, Eric Sunshine via GitGitGadget wrote:\n>\n>> Reverse video certainly makes the \"?!FOO?!\" annotations pop out and draw\n>> the reader's attention. I find that I don't have a strong preference\n>> between this version and v1 which merely used bold-red, but I suspect\n>> that v2 with its reverse video is probably the better approach.\n>\n> I find this one slightly uglier, but they are equally\n> attention-grabbing. And as I hope to rarely see them in the first place,\n> I am fine either way. :)\n\nYup, I tend to think that reverse red is uglier and is more\nattention grabbing than bold red. Let's stop here for now and let\nothers paint it in other colors by introducing configuration knob or\nwhatnot but outside the topic.\n\n> Thanks again for adding this.\n\nThat too.\n"},{"id":"465609","messageId":"221024.86a65lee8i.gmgdl@evledraar.gmail.com","threadId":"58421","inReplyTo":"pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] chainlint: colorize problem annotations and test delimiters","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-24T09:57:00Z","receivedAt":"2022-10-24T09:57:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n>\n> When `chainlint.pl` detects problems in a test definition, it emits the\n> test definition with \"?!FOO?!\" annotations highlighting the problems it\n> discovered. For instance, given this problematic test:\n>\n> test_expect_success 'discombobulate frobnitz' '\n> git frob babble &&\n> (echo balderdash; echo gnabgib) >expect &&\n> for i in three two one\n> do\n> git nitfol $i\n> done >actual\n> test_cmp expect actual\n> '\n>\n> chainlint.pl will output:\n>\n> # chainlint: t1234-confusing.sh\n> # chainlint: discombobulate frobnitz\n> git frob babble &&\n> (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&\n> for i in three two one\n> do\n> git nitfol $i ?!LOOP?!\n> done >actual ?!AMP?!\n> test_cmp expect actual\n>\n> in which it may be difficult to spot the \"?!FOO?!\" annotations. The\n> problem is compounded when multiple tests, possibly in multiple\n> scripts, fail \"linting\", in which case it may be difficult to spot the\n> \"# chainlint:\" lines which delimit one problematic test from another.\n>\n> To ameliorate this potential problem, colorize the \"?!FOO?!\" annotations\n> in order to quickly draw the test author's attention to the problem\n> spots, and colorize the \"# chainlint:\" lines to help the author identify\n> the name of each script and each problematic test.\n>\n> Colorization is disabled automatically if output is not directed to a\n> terminal or if NO_COLOR environment variable is set. The implementation\n> is specific to Unix (it employs `tput` if available) but works equally\n> well in the Git for Windows development environment which emulates Unix\n> sufficiently.\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> chainlint: colorize problem annotations and test delimiters\n> \n> This is a re-roll of [1] which colorizes the output of \"chainlint.pl\"\n> when it detects problems in Git test definitions. During discussion, it\n> was noted that the eye could sometimes glide right over[2] the bold-red\n> \"?!FOO?!\" annotations, so Junio suggested using reverse video, which is\n> what v2 does.\n> \n> Reverse video certainly makes the \"?!FOO?!\" annotations pop out and draw\n> the reader's attention. I find that I don't have a strong preference\n> between this version and v1 which merely used bold-red, but I suspect\n> that v2 with its reverse video is probably the better approach.\n> \n> [1]\n> https://lore.kernel.org/git/pull.1324.git.git.1663023888412.gitgitgadget@gmail.com/\n> [2]\n> https://lore.kernel.org/git/CAPig+cTq3j5M7cz3T14h9U6e+H5PAu8JJ_Svq87W3WviwS6_qA@mail.gmail.com/\n> [3] https://lore.kernel.org/git/xmqqo7vkazuh.fsf@gitster.g/\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1324%2Fsunshineco%2Fchainlintcolor-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1324/sunshineco/chainlintcolor-v2\n> Pull-Request: https://github.com/git/git/pull/1324\n>\n> Range-diff vs v1:\n>\n> 1: d670570e81f ! 1: acf9183ccc6 chainlint: colorize problem annotations and test delimiters\n> @@ t/chainlint.pl: sub check_test {\n> \t$checked =~ s/^\\n//;\n> \t$checked =~ s/^ //mg;\n> \t$checked =~ s/ $//mg;\n> -+\t$checked =~ s/(\\?![^?]+\\?!)/$c->{bold}$c->{red}$1$c->{reset}/mg;\n> ++\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n> \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n> -\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n> +\tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n> @@ t/chainlint.pl: if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n> +# thread and ignore %ENV changes in subthreads.\n> +$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};\n> +\n> -+my @NOCOLORS = (bold => '', reset => '', blue => '', green => '', red => '');\n> ++my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '', green => '', red => '');\n> +my %COLORS = ();\n> +sub get_colors {\n> +\treturn \\%COLORS if %COLORS;\n> +\tif (exists($ENV{NO_COLOR}) ||\n> +\t system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput bold >/dev/null 2>&1\") != 0 ||\n> ++\t system(\"tput rev >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n> +\t\t%COLORS = @NOCOLORS;\n> +\t\treturn \\%COLORS;\n> +\t}\n> +\t%COLORS = (bold => `tput bold`,\n> ++\t\t rev => `tput rev`,\n> +\t\t reset => `tput sgr0`,\n> +\t\t blue => `tput setaf 4`,\n> +\t\t green => `tput setaf 2`,\n>\n>\n> t/chainlint.pl | 46 +++++++++++++++++++++++++++++++++++++++++++---\n> 1 file changed, 43 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/chainlint.pl b/t/chainlint.pl\n> index 386999ce65d..976db4b8a01 100755\n> --- a/t/chainlint.pl\n> +++ b/t/chainlint.pl\n> @@ -585,12 +585,14 @@ sub check_test {\n> \tmy $parser = TestParser->new(\\$body);\n> \tmy @tokens = $parser->parse();\n> \treturn unless $emit_all || grep(/\\?![^?]+\\?!/, @tokens);\n> +\tmy $c = main::fd_colors(1);\n> \tmy $checked = join(' ', @tokens);\n> \t$checked =~ s/^\\n//;\n> \t$checked =~ s/^ //mg;\n> \t$checked =~ s/ $//mg;\n> +\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n> \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n> -\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n> +\tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n> }\n> \n> sub parse_cmd {\n> @@ -615,6 +617,41 @@ if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n> \t$interval = sub { return Time::HiRes::tv_interval(shift); };\n> }\n> \n> +# Restore TERM if test framework set it to \"dumb\" so 'tput' will work; do this\n> +# outside of get_colors() since under 'ithreads' all threads use %ENV of main\n> +# thread and ignore %ENV changes in subthreads.\n> +$ENV{TERM} = $ENV{USER_TERM} if $ENV{USER_TERM};\n> +\n> +my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '', green => '', red => '');\n> +my %COLORS = ();\n> +sub get_colors {\n> +\treturn \\%COLORS if %COLORS;\n> +\tif (exists($ENV{NO_COLOR}) ||\n> +\t system(\"tput sgr0 >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput bold >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput rev >/dev/null 2>&1\") != 0 ||\n> +\t system(\"tput setaf 1 >/dev/null 2>&1\") != 0) {\n> +\t\t%COLORS = @NOCOLORS;\n> +\t\treturn \\%COLORS;\n> +\t}\n> +\t%COLORS = (bold => `tput bold`,\n> +\t\t rev => `tput rev`,\n> +\t\t reset => `tput sgr0`,\n> +\t\t blue => `tput setaf 4`,\n> +\t\t green => `tput setaf 2`,\n> +\t\t red => `tput setaf 1`);\n> +\tchomp(%COLORS);\n> +\treturn \\%COLORS;\n> +}\n> +\n> +my %FD_COLORS = ();\n> +sub fd_colors {\n> +\tmy $fd = shift;\n> +\treturn $FD_COLORS{$fd} if exists($FD_COLORS{$fd});\n> +\t$FD_COLORS{$fd} = -t $fd ? get_colors() : {@NOCOLORS};\n> +\treturn $FD_COLORS{$fd};\n> +}\n> +\n> sub ncores {\n> \t# Windows\n> \treturn $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});\n> @@ -630,6 +667,8 @@ sub show_stats {\n> \tmy $walltime = $interval->($start_time);\n> \tmy ($usertime) = times();\n> \tmy ($total_workers, $total_scripts, $total_tests, $total_errs) = (0, 0, 0, 0);\n> +\tmy $c = fd_colors(2);\n> +\tprint(STDERR $c->{green});\n> \tfor (@$stats) {\n> \t\tmy ($worker, $nscripts, $ntests, $nerrs) = @$_;\n> \t\tprint(STDERR \"worker $worker: $nscripts scripts, $ntests tests, $nerrs errors\\n\");\n> @@ -638,7 +677,7 @@ sub show_stats {\n> \t\t$total_tests += $ntests;\n> \t\t$total_errs += $nerrs;\n> \t}\n> -\tprintf(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);\n> +\tprintf(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);\n> }\n> \n> sub check_script {\n> @@ -656,8 +695,9 @@ sub check_script {\n> \t\tmy $parser = ScriptParser->new(\\$s);\n> \t\t1 while $parser->parse_cmd();\n> \t\tif (@{$parser->{output}}) {\n> +\t\t\tmy $c = fd_colors(1);\n> \t\t\tmy $s = join('', @{$parser->{output}});\n> -\t\t\t$emit->(\"# chainlint: $path\\n\" . $s);\n> +\t\t\t$emit->(\"$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\\n\" . $s);\n> \t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n> \t\t}\n> \t\t$ntests += $parser->{ntests};\n>\n> base-commit: 76d57e004b0391503ca7719c932df2a0bd617d0a\n\n"},{"id":"465610","messageId":"221024.865yg9ecsx.gmgdl@evledraar.gmail.com","threadId":"58421","inReplyTo":"pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com","subject":"chainlint.pl's new \"deparse\" output (was: [PATCH v2] [...])","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-24T09:57:36Z","receivedAt":"2022-10-24T10:28:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Please ignore the just-sent empty\nhttps://lore.kernel.org/git/221024.86a65lee8i.gmgdl@evledraar.gmail.com/;\nlocal PBCAK problem :)]\n\nOn Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n>\n> When `chainlint.pl` detects problems in a test definition, it emits the\n> test definition with \"?!FOO?!\" annotations highlighting the problems it\n> discovered. For instance, given this problematic test:\n>\n> test_expect_success 'discombobulate frobnitz' '\n> git frob babble &&\n> (echo balderdash; echo gnabgib) >expect &&\n> for i in three two one\n> do\n> git nitfol $i\n> done >actual\n> test_cmp expect actual\n> '\n>\n> chainlint.pl will output:\n>\n> # chainlint: t1234-confusing.sh\n> # chainlint: discombobulate frobnitz\n> git frob babble &&\n> (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&\n> for i in three two one\n> do\n> git nitfol $i ?!LOOP?!\n> done >actual ?!AMP?!\n> test_cmp expect actual\n\nI've noticed that chainlint.pl is better in some ways, but that the\n\"deparse\" output tends to be quite jarring. but I can't find version of\nit that emitted this \"will output\" here.\n\nBefore this patch, or fb41727b7ed (t: retire unused chainlint.sed,\n2022-09-01) we'd emit this instead:\n\t\n\tgit frob babble &&\n\t( echo balderdash ; ?!AMP?! echo gnabgib ) > expect &&\n\tfor i in three two one\n\tdo\n\tgit nitfol $i ?!LOOP?!\n\tdone > actual ?!AMP?!\n\ttest_cmp expect actual\n\nThe difference is in whitespace, e.g. \"( \", not \"(\", \"> \" not \">\". This\nis just because it's emitting \"raw\" tokenizer tokens.\n\nWas there maybe some local version where the whitespace munging you're\ndoing against $checked was different & this commit message was left\nover?\n\nAnyway, that sort of an aside, but I did go hunting for the version with slightly better whitespace output.\n\nBut to get to the actual point: I've found the new chainlint.pl output\nharder to read sometimes, because it goes through this parse & deparse\nstate, so you're preserving \"\\n\"''s.\n\nWhereas the old \"sed\" output also sucked because we couldn't note where\nthe issue was, but we spewed out the test source verbatim.\n\nBut it seem to me that we could get much better output if the\nShellParser/Lexer etc. just kept enough state to emit \"startcol\",\n\"endcol\" and \"linenum\" along with every token, or something like that\n(you might want offsets from the beginning of the parsed source\ninstead).\n\nThen when it has errors it could emit the actual source passed in, and\neven do gcc/clang-style underlining.\n\nI poked at getting that working for a few minutes, but quickly saw that\nsomeone more familiar with the code could do it much quicker, so\nconsider the above a feature request :)\n\nAnother thing: When a test *ends* in a \"&&\" (common when you copy/paste\ne.g. \"test_cmp expect actual &&\\n\" from another test) it doesn't spot\nit, but instead we get all the way to the eval/117, i.e. \"broken\n&&-chain or run-away HERE-DOC\".\n\nMore feature requests (because for some reason you've got infinite time,\nbut I don't :): This software is really close to being able to also\nchange the tests on the fly. If you could define callbacks where you\ncould change subsets of the parse stream, say a single command like:\n\n\tgrep some.*rx file\n\nTokenized as:\n\n\t[\"grep\", \"some.*rx\" \"file\"]\n\nIf you could define an interface to have a callback function e.g. as:\n\n\tsub munge_line_tokens {\n\t\tmy $t = shift;\n\n return unless $t->[0] eq \"grep\"; # no changes\n my @t = @$t;\n\n return [qw(if ! grep), @t[1..$#t],\n \tqw(then cat), $t[-1], qw(&& false fi)];\n\t}\n\nSo we could rewrite that into:\n\n if ! grep some.*rx foo\n\tthen\n\t\tcat foo &&\n\t\tfalse\n\tfi\n\nAnd other interesting auto-fixups and borderline coccinelle\ntransformations, e.g. changing our various:\n\n\ttest \"$(git ...) = \"\" &&\n\nInto:\n\n\tgit ... >out &&\n\ttest_must_be_empty out\n"},{"id":"465682","messageId":"CAPig+cT=cWYT6kicNWT+6RxfiKKMyVz72H3_9kwkF-f4Vuoe1w@mail.gmail.com","threadId":"58421","inReplyTo":"221024.865yg9ecsx.gmgdl@evledraar.gmail.com","subject":"Re: chainlint.pl's new \"deparse\" output (was: [PATCH v2] [...])","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-25T04:05:46Z","receivedAt":"2022-10-25T04:06:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote:\n> > When `chainlint.pl` detects problems in a test definition, it emits the\n> > test definition with \"?!FOO?!\" annotations highlighting the problems it\n> > discovered. For instance, given this problematic test:\n> >\n> > test_expect_success 'discombobulate frobnitz' '\n> > (echo balderdash; echo gnabgib) >expect &&\n> > '\n> >\n> > chainlint.pl will output:\n> >\n> > # chainlint: t1234-confusing.sh\n> > # chainlint: discombobulate frobnitz\n> > (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&\n>\n> I've noticed that chainlint.pl is better in some ways, but that the\n> \"deparse\" output tends to be quite jarring. but I can't find version of\n> it that emitted this \"will output\" here.\n\nThere is no such version.\n\n> Before this patch, or fb41727b7ed (t: retire unused chainlint.sed,\n> 2022-09-01) we'd emit this instead:\n>\n> ( echo balderdash ; ?!AMP?! echo gnabgib ) > expect &&\n>\n> The difference is in whitespace, e.g. \"( \", not \"(\", \"> \" not \">\". This\n> is just because it's emitting \"raw\" tokenizer tokens.\n>\n> Was there maybe some local version where the whitespace munging you're\n> doing against $checked was different & this commit message was left\n> over?\n\nNo, I botched the commit message. I typed the example test in by hand\nand then, also by hand, typed in the example output, forgetting to\ninsert the spaces which you correctly noted are missing from the\nexample output. I should have run the example test through\nchainlint.pl and copy/pasted its output into the commit message. (I\ndid, in fact, run the sample test through chanlint.pl _after_\nhand-typing the example output, and compared them by eye but missed\nmost of the whitespace differences.)\n\n> Anyway, that sort of an aside, but I did go hunting for the version with slightly better whitespace output.\n\nSorry, my fault for a faulty commit message.\n\n> But to get to the actual point: I've found the new chainlint.pl output\n> harder to read sometimes, because it goes through this parse & deparse\n> state, so you're preserving \"\\n\"''s.\n>\n> Whereas the old \"sed\" output also sucked because we couldn't note where\n> the issue was, but we spewed out the test source verbatim.\n\nSomewhat verbatim. chainlint.sed did swallow blank lines and comment\nlines, and it folded multi-line strings into one-line strings.\n\n> But it seem to me that we could get much better output if the\n> ShellParser/Lexer etc. just kept enough state to emit \"startcol\",\n> \"endcol\" and \"linenum\" along with every token, or something like that\n> (you might want offsets from the beginning of the parsed source\n> instead).\n>\n> Then when it has errors it could emit the actual source passed in, and\n> even do gcc/clang-style underlining.\n>\n> I poked at getting that working for a few minutes, but quickly saw that\n> someone more familiar with the code could do it much quicker, so\n> consider the above a feature request :)\n\nYes, there should be better integration between the lexer and parser\nfor emitting errors. Unfortunately, it didn't occur to me during\nimplementation, and I only thought about it when Peff mentioned the\ndifficult-to-read output in a different part of this discussion.\n\nAn alternative, somewhat hacky approach, might be to simply retain\nwhitespace as tokens in the token stream. That would require less\nretrofitting of the lexer, though perhaps more complexity/ugliness in\nthe parser. It wouldn't give you gcc/clang-level underlining, etc.,\nbut would more or less preserve whitespace in the test definition.\nDefinitely not a proper solution, but perhaps \"good enough\".\n\n> Another thing: When a test *ends* in a \"&&\" (common when you copy/paste\n> e.g. \"test_cmp expect actual &&\\n\" from another test) it doesn't spot\n> it, but instead we get all the way to the eval/117, i.e. \"broken\n> &&-chain or run-away HERE-DOC\".\n\nYes, I recall considering that case and others, but decided that\nthat's probably outside the scope of the linter. In particular, a\ntrailing \"&&\" is a plain old syntax error, and the shell itself is\nperfectly capable of diagnosing that problem along with all other\nsyntax errors, and you'll find out about syntax errors in your code\nwhen the shell tries running it. The linter, on the other hand, is\nmeant to catch semantic problems (per the project's best-practices) in\nwhat is assumed to be syntactically valid shell code. I suppose the\nlinter could be made to complain about this syntax error and others,\nbut it seems unnecessary to bloat it by duplicating behavior already\nprovided by the shell itself.\n\nIt is unfortunate, though, that the shell's \"syntax error\" output gets\nswallowed by the eval/117 checker in test-lib.sh and turned into a\nsomewhat less useful message. I'm not quite sure how we can fix the\neval/117 checker to not swallow genuine syntax errors like that,\nunless we perhaps specially recognize exit code 2 and, um, do\nsomething...\n\n> More feature requests (because for some reason you've got infinite time,\n> but I don't :): This software is really close to being able to also\n> change the tests on the fly. If you could define callbacks where you\n> could change subsets of the parse stream, say a single command like:\n>\n> grep some.*rx file\n>\n> So we could rewrite that into:\n>\n> if ! grep some.*rx foo\n> then\n> cat foo &&\n> false\n> fi\n>\n> And other interesting auto-fixups and borderline coccinelle\n> transformations, e.g. changing our various:\n>\n> test \"$(git ...) = \"\" &&\n>\n> Into:\n>\n> git ... >out &&\n> test_must_be_empty out\n\nThe lexer/parser implemented for chainlint.pl might indeed be useful\nfor such transformations. I could imagine a tool which someone runs on\nan old-style test script to help update it to modern conventions,\nafter which the person would, of course, carefully check all the\napplied transformations. That's not something we'd necessarily want to\ndo project-wide, but might be handy when already working on a test\nscript for some other reason.\n"},{"id":"465683","messageId":"CAPig+cQhsGpOa1XqfOj-zV1esc_uEkOPGg3hVUkSWrkVma+GNQ@mail.gmail.com","threadId":"58421","inReplyTo":"CAPig+cT=cWYT6kicNWT+6RxfiKKMyVz72H3_9kwkF-f4Vuoe1w@mail.gmail.com","subject":"Re: chainlint.pl's new \"deparse\" output (was: [PATCH v2] [...])","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-25T04:15:45Z","receivedAt":"2022-10-25T04:16:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 25, 2022 at 12:05 AM Eric Sunshine wrote:\n> On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason\n> wrote:\n> > Another thing: When a test *ends* in a \"&&\" (common when you copy/paste\n> > e.g. \"test_cmp expect actual &&\\n\" from another test) it doesn't spot\n> > it, but instead we get all the way to the eval/117, i.e. \"broken\n> > &&-chain or run-away HERE-DOC\".\n>\n> Yes, I recall considering that case and others, but decided that\n> that's probably outside the scope of the linter. [...]\n>\n> It is unfortunate, though, that the shell's \"syntax error\" output gets\n> swallowed by the eval/117 checker in test-lib.sh and turned into a\n> somewhat less useful message. I'm not quite sure how we can fix the\n> eval/117 checker to not swallow genuine syntax errors like that,\n> unless we perhaps specially recognize exit code 2 and, um, do\n> something...\n\nAnother \"fix\" would be to drop the eval/117 checker altogether. I\nretained it as a final safeguard in case something slipped past\nchainlint.pl, however, I'm not sure how much value the eval/117\nchecker really has since it misses so many real-world cases, such as\nany &&-chain break in the body of a compound context (if/fi,\ncase/esac, for/done, while/done, (...), {...}, $(...), etc.).\nMoreover, we see now that it's also obscuring useful error messages\n(such as \"syntax error\") from the shell itself. So, dropping it may be\nan option(?).\n"},{"id":"465696","messageId":"221025.86o7u0cimf.gmgdl@evledraar.gmail.com","threadId":"58421","inReplyTo":"CAPig+cT=cWYT6kicNWT+6RxfiKKMyVz72H3_9kwkF-f4Vuoe1w@mail.gmail.com","subject":"Re: chainlint.pl's new \"deparse\" output (was: [PATCH v2] [...])","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-25T10:07:43Z","receivedAt":"2022-10-25T10:21:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 25 2022, Eric Sunshine wrote:\n\n> On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote:\n>> > When `chainlint.pl` detects problems in a test definition, it emits the\n>> > test definition with \"?!FOO?!\" annotations highlighting the problems it\n>> > discovered. For instance, given this problematic test:\n>> >\n>> > test_expect_success 'discombobulate frobnitz' '\n>> > (echo balderdash; echo gnabgib) >expect &&\n>> > '\n>> >\n>> > chainlint.pl will output:\n>> >\n>> > # chainlint: t1234-confusing.sh\n>> > # chainlint: discombobulate frobnitz\n>> > (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&\n>>\n>> I've noticed that chainlint.pl is better in some ways, but that the\n>> \"deparse\" output tends to be quite jarring. but I can't find version of\n>> it that emitted this \"will output\" here.\n>\n> There is no such version.\n> [...]\n> No, I botched the commit message. I typed the example test in by hand\n> and then, also by hand, typed in the example output, forgetting to\n> insert the spaces which you correctly noted are missing from the\n> example output. I should have run the example test through\n> chainlint.pl and copy/pasted its output into the commit message. (I\n> did, in fact, run the sample test through chanlint.pl _after_\n> hand-typing the example output, and compared them by eye but missed\n> most of the whitespace differences.)\n>\n>> Anyway, that sort of an aside, but I did go hunting for the version with slightly better whitespace output.\n>\n> Sorry, my fault for a faulty commit message.\n\nNo worries!\n\n>> But to get to the actual point: I've found the new chainlint.pl output\n>> harder to read sometimes, because it goes through this parse & deparse\n>> state, so you're preserving \"\\n\"''s.\n>>\n>> Whereas the old \"sed\" output also sucked because we couldn't note where\n>> the issue was, but we spewed out the test source verbatim.\n>\n> Somewhat verbatim. chainlint.sed did swallow blank lines and comment\n> lines, and it folded multi-line strings into one-line strings.\n\nYeah, it had a lot of edge cases, the new one's much better overall. I\njust sometimes found it jarring to look at code that's not /quite/ my\nversion now, but anyway... :)\n\n>> But it seem to me that we could get much better output if the\n>> ShellParser/Lexer etc. just kept enough state to emit \"startcol\",\n>> \"endcol\" and \"linenum\" along with every token, or something like that\n>> (you might want offsets from the beginning of the parsed source\n>> instead).\n>>\n>> Then when it has errors it could emit the actual source passed in, and\n>> even do gcc/clang-style underlining.\n>>\n>> I poked at getting that working for a few minutes, but quickly saw that\n>> someone more familiar with the code could do it much quicker, so\n>> consider the above a feature request :)\n>\n> Yes, there should be better integration between the lexer and parser\n> for emitting errors. Unfortunately, it didn't occur to me during\n> implementation, and I only thought about it when Peff mentioned the\n> difficult-to-read output in a different part of this discussion.\n>\n> An alternative, somewhat hacky approach, might be to simply retain\n> whitespace as tokens in the token stream. That would require less\n> retrofitting of the lexer, though perhaps more complexity/ugliness in\n> the parser. It wouldn't give you gcc/clang-level underlining, etc.,\n> but would more or less preserve whitespace in the test definition.\n> Definitely not a proper solution, but perhaps \"good enough\".\n\nYeah, maybe.\n\n>> Another thing: When a test *ends* in a \"&&\" (common when you copy/paste\n>> e.g. \"test_cmp expect actual &&\\n\" from another test) it doesn't spot\n>> it, but instead we get all the way to the eval/117, i.e. \"broken\n>> &&-chain or run-away HERE-DOC\".\n>\n> Yes, I recall considering that case and others, but decided that\n> that's probably outside the scope of the linter. In particular, a\n> trailing \"&&\" is a plain old syntax error, and the shell itself is\n> perfectly capable of diagnosing that problem along with all other\n> syntax errors, and you'll find out about syntax errors in your code\n> when the shell tries running it. The linter, on the other hand, is\n> meant to catch semantic problems (per the project's best-practices) in\n> what is assumed to be syntactically valid shell code. I suppose the\n> linter could be made to complain about this syntax error and others,\n> but it seems unnecessary to bloat it by duplicating behavior already\n> provided by the shell itself.\n\nFWIW I thought it would be nice because it sometimes takes 10s or\nwhatever to get to the syntax error by running the test, but the linter\ncan find it right away.\n\n> It is unfortunate, though, that the shell's \"syntax error\" output gets\n> swallowed by the eval/117 checker in test-lib.sh and turned into a\n> somewhat less useful message. I'm not quite sure how we can fix the\n> eval/117 checker to not swallow genuine syntax errors like that,\n> unless we perhaps specially recognize exit code 2 and, um, do\n"}]}