git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH v2] chainlint: colorize problem annotations and test delimiters
Date: Tue, 13 Sep 2022 04:01:47 +0000	[thread overview]
Message-ID: <pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1324.git.git.1663023888412.gitgitgadget@gmail.com>

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

  parent reply	other threads:[~2022-09-13  4:02 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-12 23:04 [PATCH] chainlint: colorize problem annotations and test delimiters Eric Sunshine via GitGitGadget
2022-09-12 23:55 ` Junio C Hamano
2022-09-13  0:14   ` Eric Sunshine
2022-09-13  0:21     ` Junio C Hamano
2022-09-13  0:39       ` Eric Sunshine
2022-09-13  0:16   ` Jeff King
2022-09-13  0:15 ` Jeff King
2022-09-13  0:30   ` Eric Sunshine
2022-09-13  1:34     ` Jeff King
2022-09-13  0:32   ` Jeff King
2022-09-13  4:01 ` Eric Sunshine via GitGitGadget [this message]
2022-09-13 20:40   ` [PATCH v2] " Jeff King
2022-09-13 20:46     ` Junio C Hamano
2022-10-24  9:57   ` Ævar Arnfjörð Bjarmason
2022-10-24  9:57   ` chainlint.pl's new "deparse" output (was: [PATCH v2] [...]) Ævar Arnfjörð Bjarmason
2022-10-25  4:05     ` Eric Sunshine
2022-10-25  4:15       ` Eric Sunshine
2022-10-25 10:07       ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).