Re: [PATCH v2] chainlint: colorize problem annotations and test delimiters

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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