Re: [PATCH 2/2] chainlint: reduce annotation noise-factor

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2024, 15:55 UTC
Message-ID: <xmqqv7zjwcgq.fsf@gitster.g>
In-Reply-To: <20240829091625.41297-3-ericsunshine@charter.net>

Eric Sunshine <ericsunshine@charter.net> writes:

Show 27 quoted lines

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> When chainlint detects a problem in a test definition, it highlights the
> offending code with an "?!ERR ...?!" annotation. The rather curious "?!"
> delimiter was chosen to draw the reader's attention to the problem area.
>
> Later, chainlint learned to color its output when sent to a terminal.
> Problem annotations are colored with a red background which stands out
> well from surrounding text, thus easily draws the reader's attention. As
> such, the additional "?!" decoration became superfluous (when output is
> colored), however the decoration was retained since it serves as a good
> needle when using the terminal's search feature to "jump" to the next
> problem.
>
> Nevertheless, the "?!" decoration is noisy and ugly and makes it
> unnecessarily difficult for the reader to pluck the problem description
> from the annotation. For instance, it is easier to see at a glance what
> the problem is in:
>
>     ERR missing '&&'
>
> than in the noisier:
>
>     ?!ERR missing '&&'?!
>
> Therefore drop the "!?" decoration when output is colored (but retain it
> otherwise).

Wait.  That does not qualify "Therefore".

We talked about a "good needle" and then complained how ugly the string that was happened to be chosen as good needle is. That is not enough to explain why it is justified to "lose" the needle. The only thing you justified is to move away from the ugly pattern, as a typical "terminal's search feature" does not give us an easy way to "jump to the next text painted yellow".

> Note that the preceding change gave all problem annotations a uniform
> "ERR" prefix which serves as a reasonably suitable replacement needle
> when searching in a terminal, so loss of "?!" in the output should not
> be overly problematic.

Drop this separate paragraph, promote its contents up from "Note" status and as a proper part of the previous sentence in its rewrite, something like:

    Since the errors are all uniformly prefixed with "ERR", which
    can be used as the "good needle" instead, lose the "!?"
    decoration when output is colored.

to replace "Therefore" and everything that follow.

Show 6 quoted lines

> @@ -663,7 +666,7 @@ sub check_test {
>  	$checked =~ s/^\d+ \n//;
>  	$checked =~ s/(\s) \?!/$1?!/mg;
>  	$checked =~ s/\?! (\s)/?!$1/mg;
> -	$checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;
> +	$checked =~ s/\?!([^?]+)\?!/$erropen$1$errclose/mg;

Hmph. With $erropen and $errclose, I was hoping that we can shed the reliance on the "?!" mark even internally. This is especially true that in the early part of this sub, the problem description was very much structured piece of data, not something the consuming code need to pick out of an already formatted text like this, risking to get confused by the payload (i.e. the text that came from the problematic test script inside "substr($body, $start, $pos-$start)" may contain anything, including "?!", right?).

Show 27 quoted lines

>  	$checked =~ s/^\d+/$c->{dim}$&$c->{reset}/mg;
>  	$checked .= "\n" unless $checked =~ /\n$/;
>  	push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
> @@ -805,9 +808,9 @@ sub check_script {
>  			my $c = fd_colors(1);
>  			my $s = join('', @{$parser->{output}});
>  			$emit->("$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\n" . $s);
> -			$nerrs += () = $s =~ /\?![^?]+\?!/g;
>  		}
>  		$ntests += $parser->{ntests};
> +		$nerrs += $parser->{nerrs};
>  	}
>  	return [$id, $nscripts, $ntests, $nerrs];
>  }
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 54247604cb..b652cb98cd 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1606,7 +1606,7 @@ if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&
>     test "${GIT_TEST_EXT_CHAIN_LINT:-1}" != 0
>  then
>  	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" ||
> -		BUG "lint error (see '?!...!? annotations above)"
> +		BUG "lint error (see 'ERR' annotations above)"
>  fi
>  
>  # Last-minute variable setup

Overall the two patches looked great. Thanks.