On Thu, Aug 29, 2024 at 11:55 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <ericsunshine@xxxxxxxxxxx> writes:
> > 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.
Perhaps the following would make for a more palatable commit message?
When chainlint detects a problem in a test definition, it
highlights the offending code with a "?!...?!" annotation. The
rather curious "?!" decoration was chosen to draw the reader's
attention to the problem area and to act as a good "needle" when
using the terminal's search feature to "jump" to the next problem.
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. Together with the preceding change which gave
all problem annotations a uniform "ERR" prefix, the noisy "?!"
decoration has become superfluous as a search "needle" so omit it
when output is colored.
> > @@ -663,7 +666,7 @@ sub check_test {
> > $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.
Good point. Just above the shown context:
$checked .= substr($body, $start, $pos - $start) . " ?!ERR $err?! ";
unconditionally adds the "?!" decorations even when the output is
colored, and then the:
$checked =~ s/\?!([^?]+)\?!/$erropen$1$errclose/mg;
removes them. It would be nice to add the "?!" decoration only when
not coloring output, however, together with the explicit space added
before and after "?!...?!" by:
$checked .= substr($body, $start, $pos - $start) . " ?!ERR $err?! ";
the related:
$checked =~ s/(\s) \?!/$1?!/mg;
$checked =~ s/\?! (\s)/?!$1/mg;
ensure, for aesthetic reasons, that there is one, and only one, space
before and after the annotation.
It may be possible to do something like this instead (untested), but
I'm not sure it's worth the complexity:
$checked .= substr($body, $start, $pos - $start);
$checked .= ' ' unless $checked =~ /\s$/;
$checked .= "$erropenERR $err$errclose";
$checked .= ' ' unless $pos + 1 >= length($body) ||
substr($body, $pos + 1, 1) =~ /\s/;
> 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?).
As first implemented, there was no structured "problem description".
chainlint originally just output a stream of raw parse tokens (not the
original test text), and when a problem was discovered the "?!...?!"
annotations were embedded directly in the output stream. This was
still the case even when colored output was implemented[1]; in fact,
the annotations were colored after-the-fact by searching for "?!...?!"
in the output stream. It was only when chainlint was taught to output
the original test text verbatim[2] that problem descriptions became
structured data.
I was never overly concerned about "?!" appearing as part of the
actual payload, partly because it is an unusual character sequence to
be present in shell code anyhow (indeed, it only appears in t5510),
partly because it requires "?!" to be doubled up (i.e. "?!...?!"), and
partly because this processing kicks in only when a linting problem is
actually discovered,
[1]: 7c04aa7390 (chainlint: colorize problem annotations and test
delimiters, 2022-09-13)
[2]: 73c768dae9 (chainlint: annotate original test definition rather
than token stream, 2022-11-08)
