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

From: Jeff King <peff@peff.net>
Date: Sep 13, 2022, 00:15 UTC
Message-ID: <Yx/LpUglpjY5ZNas@coredump.intra.peff.net>
In-Reply-To: <pull.1324.git.git.1663023888412.gitgitgadget@gmail.com>

On Mon, Sep 12, 2022 at 11:04:48PM +0000, Eric Sunshine via GitGitGadget wrote:

Show 14 quoted lines

> @@ -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.

Show 19 quoted lines

> +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