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

[Jeff King <peff@peff.net>]

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