[PATCH 0/3] chainlint: emit line numbers alongside test definitions

[PATCH 0/3] chainlint: emit line numbers alongside test definitions

[Eric Sunshine via GitGitGadget]

When chainlint detects problems in a test, it prints out the name of the
test script, the name of the problematic test, and a copy of the test
definition with "?!FOO?!" annotations inserted at the locations where
problems were detected. Taken together this information is sufficient for
the test author to identify the problematic code in the original test
definition. However, in a lengthy script or a lengthy test definition, the
author may still end up using the editor's search feature to home in on the
exact problem location.

This patch series further assists the test author by displaying line numbers
alongside the annotated test definition, thus allowing the author to jump
directly to each problematic line.

This feature was suggested by Ævar[1]. I suspect that Ævar's next nerd-snipe
attempt may be to have problems emitted in "path:line#:col#: message" format
to allow editors to jump directly to the problem without the user having to
type in the line number manually.

This is atop "es/chainlint-output"[2].

(Note to self: Fortify against Ævar's nerd-snipe blacklist evasion.)

FOOTNOTES

[1] https://lore.kernel.org/git/221108.86iljpqdvj.gmgdl@evledraar.gmail.com/
[2]
https://lore.kernel.org/git/pull.1375.git.git.1667934510.gitgitgadget@gmail.com/

Eric Sunshine (3):
  chainlint: sidestep impoverished macOS "terminfo"
  chainlint: latch line numbers at which each token starts and ends
  chainlint: prefix annotated test definition with line numbers

 t/Makefile     |  2 +-
 t/chainlint.pl | 69 ++++++++++++++++++++++++++++++++++----------------
 2 files changed, 48 insertions(+), 23 deletions(-)


base-commit: 73c768dae9ea4838736693965b25ba34e941ac88
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1413%2Fsunshineco%2Fchainlintline-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1413/sunshineco/chainlintline-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1413
-- 
gitgitgadget

[PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

Although the macOS Terminal.app is "xterm"-compatible, its corresponding
"terminfo" entry neglects to mention capabilities which Terminal.app
actually supports (such as "dim text"). This oversight on Apple's part
ends up penalizing users of "good citizen" console programs which
consult "terminfo" to tailor their output based upon reported terminal
capabilities (as opposed to programs which assume that the terminal
supports ANSI codes).

Sidestep this Apple problem by imbuing get_colors() with specific
knowledge of "xterm" capabilities rather than trusting "terminfo" to
report them correctly. Although hard-coding such knowledge is ugly,
"xterm" support is nearly ubiquitous these days, and Git itself sets
precedence by assuming support for ANSI color codes. For non-"xterm",
fall back to querying "terminfo" via `tput` as usual.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 7972c5bbe6f..fcf4d459249 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -653,21 +653,32 @@ 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) {
+	if (exists($ENV{NO_COLOR})) {
 		%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);
+	if ($ENV{TERM} =~ /\bxterm\b/) {
+		%COLORS = (bold  => "\e[1m",
+			   rev   => "\e[7m",
+			   reset => "\e[0m",
+			   blue  => "\e[34m",
+			   green => "\e[32m",
+			   red   => "\e[31m");
+		return \%COLORS;
+	}
+	if (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 = (bold  => `tput bold`,
+			   rev   => `tput rev`,
+			   reset => `tput sgr0`,
+			   blue  => `tput setaf 4`,
+			   green => `tput setaf 2`,
+			   red   => `tput setaf 1`);
+		return \%COLORS;
+	}
+	%COLORS = @NOCOLORS;
 	return \%COLORS;
 }
 
-- 
gitgitgadget

[PATCH 2/3] chainlint: latch line numbers at which each token starts and ends

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

When chainlint detects problems in a test, it prints out the name of the
test script, the name of the problematic test, and a copy of the test
definition with "?!FOO?!" annotations inserted at the locations where
problems were detected. Taken together this information is sufficient
for the test author to identify the problematic code in the original
test definition. However, in a lengthy script or a lengthy test
definition, the author may still end up using the editor's search
feature to home in on the exact problem location.

To further assist the test author, an upcoming change will display line
numbers along with the annotated test definition, thus allowing the
author to jump directly to each problematic line. As preparation,
upgrade Lexer to latch the line numbers at which each token starts and
ends, and return that information with the token itself.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index fcf4d459249..01f261165b1 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -67,6 +67,7 @@ sub new {
 	bless {
 		parser => $parser,
 		buff => $s,
+		lineno => 1,
 		heretags => []
 	} => $class;
 }
@@ -97,7 +98,9 @@ sub scan_op {
 sub scan_sqstring {
 	my $self = shift @_;
 	${$self->{buff}} =~ /\G([^']*'|.*\z)/sgc;
-	return "'" . $1;
+	my $s = $1;
+	$self->{lineno} += () = $s =~ /\n/sg;
+	return "'" . $s;
 }
 
 sub scan_dqstring {
@@ -115,7 +118,7 @@ sub scan_dqstring {
 		if ($c eq '\\') {
 			$s .= '\\', last unless $$b =~ /\G(.)/sgc;
 			$c = $1;
-			next if $c eq "\n"; # line splice
+			$self->{lineno}++, next if $c eq "\n"; # line splice
 			# backslash escapes only $, `, ", \ in dq-string
 			$s .= '\\' unless $c =~ /^[\$`"\\]$/;
 			$s .= $c;
@@ -123,6 +126,7 @@ sub scan_dqstring {
 		}
 		die("internal error scanning dq-string '$c'\n");
 	}
+	$self->{lineno} += () = $s =~ /\n/sg;
 	return $s;
 }
 
@@ -137,6 +141,7 @@ sub scan_balanced {
 		$depth--;
 		last if $depth == 0;
 	}
+	$self->{lineno} += () = $s =~ /\n/sg;
 	return $s;
 }
 
@@ -165,6 +170,8 @@ sub swallow_heredocs {
 	while (my $tag = shift @$tags) {
 		my $indent = $tag =~ s/^\t// ? '\\s*' : '';
 		$$b =~ /(?:\G|\n)$indent\Q$tag\E(?:\n|\z)/gc;
+		my $body = $&;
+		$self->{lineno} += () = $body =~ /\n/sg;
 	}
 }
 
@@ -172,11 +179,12 @@ sub scan_token {
 	my $self = shift @_;
 	my $b = $self->{buff};
 	my $token = '';
-	my $start;
+	my ($start, $startln);
 RESTART:
+	$startln = $self->{lineno};
 	$$b =~ /\G[ \t]+/gc; # skip whitespace (but not newline)
 	$start = pos($$b) || 0;
-	return ["\n", $start, pos($$b)] if $$b =~ /\G#[^\n]*(?:\n|\z)/gc; # comment
+	$self->{lineno}++, return ["\n", $start, pos($$b), $startln, $startln] if $$b =~ /\G#[^\n]*(?:\n|\z)/gc; # comment
 	while (1) {
 		# slurp up non-special characters
 		$token .= $1 if $$b =~ /\G([^\\;&|<>(){}'"\$\s]+)/gc;
@@ -188,20 +196,20 @@ RESTART:
 		$token .= $self->scan_sqstring(), next if $c eq "'";
 		$token .= $self->scan_dqstring(), next if $c eq '"';
 		$token .= $c . $self->scan_dollar(), next if $c eq '$';
-		$self->swallow_heredocs(), $token = $c, last if $c eq "\n";
+		$self->{lineno}++, $self->swallow_heredocs(), $token = $c, last if $c eq "\n";
 		$token = $self->scan_op($c), last if $c =~ /^[;&|<>]$/;
 		$token = $c, last if $c =~ /^[(){}]$/;
 		if ($c eq '\\') {
 			$token .= '\\', last unless $$b =~ /\G(.)/sgc;
 			$c = $1;
-			next if $c eq "\n" && length($token); # line splice
-			goto RESTART if $c eq "\n"; # line splice
+			$self->{lineno}++, next if $c eq "\n" && length($token); # line splice
+			$self->{lineno}++, goto RESTART if $c eq "\n"; # line splice
 			$token .= '\\' . $c;
 			next;
 		}
 		die("internal error scanning character '$c'\n");
 	}
-	return length($token) ? [$token, $start, pos($$b)] : undef;
+	return length($token) ? [$token, $start, pos($$b), $startln, $self->{lineno}] : undef;
 }
 
 # ShellParser parses POSIX shell scripts (with minor extensions for Bash). It
-- 
gitgitgadget

[PATCH 3/3] chainlint: prefix annotated test definition with line numbers

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

When chainlint detects problems in a test, it prints out the name of the
test script, the name of the problematic test, and a copy of the test
definition with "?!FOO?!" annotations inserted at the locations where
problems were detected. Taken together this information is sufficient
for the test author to identify the problematic code in the original
test definition. However, in a lengthy script or a lengthy test
definition, the author may still end up using the editor's search
feature to home in on the exact problem location.

To further assist the test author, display line numbers along with the
annotated test definition, thus allowing the author to jump directly to
each problematic line.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/Makefile     |  2 +-
 t/chainlint.pl | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 882782a519c..2c2b2522402 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -94,7 +94,7 @@ check-chainlint:
 		done \
 	} >'$(CHAINLINTTMP_SQ)'/expect && \
 	$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
-		grep -v '^[ 	]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \
+		sed -e 's/^[1-9][0-9]* //;/^[ 	]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
 	if test -f ../GIT-BUILD-OPTIONS; then \
 		. ../GIT-BUILD-OPTIONS; \
 	fi && \
diff --git a/t/chainlint.pl b/t/chainlint.pl
index 01f261165b1..48dde978480 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -613,6 +613,7 @@ sub check_test {
 	my $problems = $parser->{problems};
 	return unless $emit_all || @$problems;
 	my $c = main::fd_colors(1);
+	my $lineno = $_[1]->[3];
 	my $start = 0;
 	my $checked = '';
 	for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
@@ -622,10 +623,12 @@ sub check_test {
 		$start = $pos;
 	}
 	$checked .= substr($body, $start);
-	$checked =~ s/^\n//;
+	$checked =~ s/^/$lineno++ . ' '/mge;
+	$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/^\d+/$c->{dim}$&$c->{reset}/mg;
 	$checked .= "\n" unless $checked =~ /\n$/;
 	push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
 }
@@ -657,7 +660,7 @@ 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 => '', rev => '', reset => '', blue => '', green => '', red => '');
+my @NOCOLORS = (bold => '', rev => '', dim => '', reset => '', blue => '', green => '', red => '');
 my %COLORS = ();
 sub get_colors {
 	return \%COLORS if %COLORS;
@@ -668,6 +671,7 @@ sub get_colors {
 	if ($ENV{TERM} =~ /\bxterm\b/) {
 		%COLORS = (bold  => "\e[1m",
 			   rev   => "\e[7m",
+			   dim   => "\e[2m",
 			   reset => "\e[0m",
 			   blue  => "\e[34m",
 			   green => "\e[32m",
@@ -677,9 +681,11 @@ sub get_colors {
 	if (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 dim  >/dev/null 2>&1") == 0 &&
 	    system("tput setaf 1 >/dev/null 2>&1") == 0) {
 		%COLORS = (bold  => `tput bold`,
 			   rev   => `tput rev`,
+			   dim   => `tput dim`,
 			   reset => `tput sgr0`,
 			   blue  => `tput setaf 4`,
 			   green => `tput setaf 2`,
-- 
gitgitgadget

Re: [PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Taylor Blau]

On Wed, Nov 09, 2022 at 04:58:32PM +0000, Eric Sunshine via GitGitGadget wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Although the macOS Terminal.app is "xterm"-compatible, its corresponding
> "terminfo" entry neglects to mention capabilities which Terminal.app
> actually supports (such as "dim text"). This oversight on Apple's part
> ends up penalizing users of "good citizen" console programs which
> consult "terminfo" to tailor their output based upon reported terminal
> capabilities (as opposed to programs which assume that the terminal
> supports ANSI codes).

Hmmph. Too bad that Apple isn't doing the right thing here, but your
approach is reasonable and well-explained. Looking good.

Thanks,
Taylor

Re: [PATCH 0/3] chainlint: emit line numbers alongside test definitions

[Taylor Blau]

On Wed, Nov 09, 2022 at 04:58:31PM +0000, Eric Sunshine via GitGitGadget wrote:
> This patch series further assists the test author by displaying line numbers
> alongside the annotated test definition, thus allowing the author to jump
> directly to each problematic line.

This is really nifty. I applied it on top of your earlier series,
intentionally broke a test and got some very pleasing chainlint output
after trying to run it.

As previously, I am no expert in the chainlint code, but everything here
looks pretty reasonable to me. And certainly it works, so I'm inclined
to start merging this and the other topic down.

It would be nice to have some more familiar eyes take a look at it,
though.

> (Note to self: Fortify against Ævar's nerd-snipe blacklist evasion.)

;-).

Thanks,
Taylor

Re: [PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On 2022-11-09 at 16:58:32, Eric Sunshine via GitGitGadget wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Although the macOS Terminal.app is "xterm"-compatible, its corresponding
> "terminfo" entry neglects to mention capabilities which Terminal.app
> actually supports (such as "dim text"). This oversight on Apple's part
> ends up penalizing users of "good citizen" console programs which
> consult "terminfo" to tailor their output based upon reported terminal
> capabilities (as opposed to programs which assume that the terminal
> supports ANSI codes).
> 
> Sidestep this Apple problem by imbuing get_colors() with specific
> knowledge of "xterm" capabilities rather than trusting "terminfo" to
> report them correctly. Although hard-coding such knowledge is ugly,
> "xterm" support is nearly ubiquitous these days, and Git itself sets
> precedence by assuming support for ANSI color codes. For non-"xterm",
> fall back to querying "terminfo" via `tput` as usual.

Given the regex below, I think the question here is actually whether
XTerm itself supports these in all its variants (my Debian system lists
approximately 90 of them), many of which are quite old.  While I don't
expect most of them to see common use, given the interest some people
have in retrocomputing, I don't think we can exclude the possibility of
seeing people use esoteric xterm variants over an SSH (or, perhaps less
pleasantly, telnet) connection.

Terminal.app actually has its own set of terminal types, nsterm*, which
are properly used here instead, although I realize that most people
prefer the xterm* options for compatibility and ease of use.  However,
that kind of behaviour does result in breakage when the canonical
terminal for that type (in this case XTerm) implements new features that
aren't supported in other implementations.

Perhaps, instead of auditing all 90 terminal types, we should tighten
this to xterm, xterm-256color, and xterm-direct[0]?  That should cover
the vast majority of use cases in the real world today, including most
users of macOS and Terminal.app, while avoiding breaking some older
variants (e.g., xterm-old lacks setaf).

> +	if ($ENV{TERM} =~ /\bxterm\b/) {
> +		%COLORS = (bold  => "\e[1m",
> +			   rev   => "\e[7m",
> +			   reset => "\e[0m",
> +			   blue  => "\e[34m",
> +			   green => "\e[32m",
> +			   red   => "\e[31m");
> +		return \%COLORS;
> +	}

[0] *-direct is what's typically used by ncurses for true colour
variants.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Eric Sunshine]

On Wed, Nov 9, 2022 at 9:40 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On 2022-11-09 at 16:58:32, Eric Sunshine via GitGitGadget wrote:
> > Sidestep this Apple problem by imbuing get_colors() with specific
> > knowledge of "xterm" capabilities rather than trusting "terminfo" to
> > report them correctly. Although hard-coding such knowledge is ugly,
> > "xterm" support is nearly ubiquitous these days, and Git itself sets
> > precedence by assuming support for ANSI color codes. For non-"xterm",
> > fall back to querying "terminfo" via `tput` as usual.
>
> Given the regex below, I think the question here is actually whether
> XTerm itself supports these in all its variants (my Debian system lists
> approximately 90 of them), many of which are quite old.  While I don't
> expect most of them to see common use, given the interest some people
> have in retrocomputing, I don't think we can exclude the possibility of
> seeing people use esoteric xterm variants over an SSH (or, perhaps less
> pleasantly, telnet) connection.

I get your drift, but I have to wonder if the retrocomputing crowd is
really going to be crafting Git tests directly on their retrohardware.
(appropriate emoji here)

> Terminal.app actually has its own set of terminal types, nsterm*, which
> are properly used here instead, although I realize that most people
> prefer the xterm* options for compatibility and ease of use.

Hmm, on my machine "nsterm" also lacks the "dim" capability. I see
that Neovim docs recommend "nsterm" with Terminal.app, so perhaps that
ought to be handled specially here, as well. Do you think any
variations other than base "nsterm" are worth special-casing?

> Perhaps, instead of auditing all 90 terminal types, we should tighten
> this to xterm, xterm-256color, and xterm-direct[0]?  That should cover
> the vast majority of use cases in the real world today, including most
> users of macOS and Terminal.app, while avoiding breaking some older
> variants (e.g., xterm-old lacks setaf).

I don't mind tightening which terminal types are handled specially.
"xterm-direct" doesn't exist on my old macOS. Is it present on newer
macOS? If so, does it require special-casing (i.e. does it lack
"dim")? If we don't special-case "xterm-direct", it will fall back to
using `tput` interrogation, which should be fine as long as the
"xterm-direct" terminfo entry is accurate.

I notice that the iTerm2 FAQ also recommends "xterm-new" on macOS, and
that one lacks "dim", as well on my machine. So, it seems that it
should be special-cased too.

Taking all the above into account, perhaps this regex?

    /xterm|xterm-.*color|xterm-new|nsterm/

Of course, the other option is to follow Git's own lead by not
worrying about TERM and `tput` and just assume everyone understands
ANSI color codes. I'm too old-school to feel entirely comfortable with
that approach, but I would entertain it if others feel it is safe
enough.

Re: [PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

On 2022-11-10 at 03:37:16, Eric Sunshine wrote:
> Hmm, on my machine "nsterm" also lacks the "dim" capability. I see
> that Neovim docs recommend "nsterm" with Terminal.app, so perhaps that
> ought to be handled specially here, as well. Do you think any
> variations other than base "nsterm" are worth special-casing?

I'd say we should do nsterm, nsterm-256color, and nsterm-direct.

> I don't mind tightening which terminal types are handled specially.
> "xterm-direct" doesn't exist on my old macOS. Is it present on newer
> macOS? If so, does it require special-casing (i.e. does it lack
> "dim")? If we don't special-case "xterm-direct", it will fall back to
> using `tput` interrogation, which should be fine as long as the
> "xterm-direct" terminfo entry is accurate.

It's present in newer ncurses, so I expect it will make its way to macOS
eventually.  I don't know whether Apple's version of it will contain
the `dim` capability, but on Debian all three xterm variants do.

It sounds like Apple is specifically limiting their capabilities for
some reason when upstream ncurses doesn't.  I can't say why that is, but
perhaps it's for compatibility.  Debian had to do that for one release
with screen* when Screen added support for some new feature but tmux had
not.

> I notice that the iTerm2 FAQ also recommends "xterm-new" on macOS, and
> that one lacks "dim", as well on my machine. So, it seems that it
> should be special-cased too.
> 
> Taking all the above into account, perhaps this regex?
> 
>     /xterm|xterm-.*color|xterm-new|nsterm/

Maybe this, then?

/(xterm|nsterm)(-(256color|direct))?|xterm-new/

That matches the three special variants of each one here plus xterm-new.

> Of course, the other option is to follow Git's own lead by not
> worrying about TERM and `tput` and just assume everyone understands
> ANSI color codes. I'm too old-school to feel entirely comfortable with
> that approach, but I would entertain it if others feel it is safe
> enough.

Sure.  I would also prefer to avoid that.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Eric Sunshine]

On Thu, Nov 10, 2022 at 5:21 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On 2022-11-10 at 03:37:16, Eric Sunshine wrote:
> > I notice that the iTerm2 FAQ also recommends "xterm-new" on macOS, and
> > that one lacks "dim", as well on my machine. So, it seems that it
> > should be special-cased too.
> >
> > Taking all the above into account, perhaps this regex?
> >
> >     /xterm|xterm-.*color|xterm-new|nsterm/
>
> Maybe this, then?
>
> /(xterm|nsterm)(-(256color|direct))?|xterm-new/
>
> That matches the three special variants of each one here plus xterm-new.

I was thinking of targeting xterm-16color too, not just
xterm-256color, just to cover bases a bit better.

I also don't mind manually spelling out the regex:

    /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/

for simplicity's sake; sure it's verbose, but it's also dead-easy for
people to understand and extend in the future if necessary.

Re: [PATCH 1/3] chainlint: sidestep impoverished macOS "terminfo"

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On 2022-11-10 at 22:36:14, Eric Sunshine wrote:
> On Thu, Nov 10, 2022 at 5:21 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > On 2022-11-10 at 03:37:16, Eric Sunshine wrote:
> > > I notice that the iTerm2 FAQ also recommends "xterm-new" on macOS, and
> > > that one lacks "dim", as well on my machine. So, it seems that it
> > > should be special-cased too.
> > >
> > > Taking all the above into account, perhaps this regex?
> > >
> > >     /xterm|xterm-.*color|xterm-new|nsterm/
> >
> > Maybe this, then?
> >
> > /(xterm|nsterm)(-(256color|direct))?|xterm-new/
> >
> > That matches the three special variants of each one here plus xterm-new.
> 
> I was thinking of targeting xterm-16color too, not just
> xterm-256color, just to cover bases a bit better.

Sure, that seems like a good idea.  I know that was popular for a time,
although I feel like it's maybe less popular today with more colour
options.

> I also don't mind manually spelling out the regex:
> 
>     /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/
> 
> for simplicity's sake; sure it's verbose, but it's also dead-easy for
> people to understand and extend in the future if necessary.

Simplicity is nice.  I think that seems like a good pattern.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2 0/3] chainlint: emit line numbers alongside test definitions

[Eric Sunshine via GitGitGadget]

This is a re-roll of "es/chainlint-lineno"[1] which makes chainlint.pl
output line numbers alongside annotated test definitions in which problems
have been discovered.

Changes since v1:

Limit sidestepping of impoverished Apple "terminfo" entries to specific
terminal types known to be problematic and in common use on macOS, rather
than sledge-hammer matching all "xterm" since some old "xterm" variants may
legitimately not support ANSI capabilities (suggested by brian[2]).

Fix incorrect line number computation when swallowing here-doc bodies. v1
incorrectly incremented the line number by two regardless of the here-doc
body's actual size. I very much would like to add tests to catch this sort
of problem, however, the current chainlint self-test framework validates
chainlint behavior in relation only to test bodies, whereas this problem
manifested when a here-doc was present at the script level outside of any
test. It may be possible to expand the self-test framework in the future to
allow such testing, but that may be some time off, and needn't hold up this
series.

FOOTNOTES

[1]
https://lore.kernel.org/git/pull.1413.git.1668013114.gitgitgadget@gmail.com/
[2]
https://lore.kernel.org/git/Y2xkpJj4jLqfsggL@tapette.crustytoothpaste.net/

Eric Sunshine (3):
  chainlint: sidestep impoverished macOS "terminfo"
  chainlint: latch line numbers at which each token starts and ends
  chainlint: prefix annotated test definition with line numbers

 t/Makefile     |  2 +-
 t/chainlint.pl | 70 ++++++++++++++++++++++++++++++++++----------------
 2 files changed, 49 insertions(+), 23 deletions(-)


base-commit: 73c768dae9ea4838736693965b25ba34e941ac88
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1413%2Fsunshineco%2Fchainlintline-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1413/sunshineco/chainlintline-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1413

Range-diff vs v1:

 1:  b85b28e5a6b ! 1:  de482cf9cf1 chainlint: sidestep impoverished macOS "terminfo"
     @@ Commit message
          chainlint: sidestep impoverished macOS "terminfo"
      
          Although the macOS Terminal.app is "xterm"-compatible, its corresponding
     -    "terminfo" entry neglects to mention capabilities which Terminal.app
     +    "terminfo" entries -- such as "xterm", "xterm-256color", and
     +    "xterm-new"[1] -- neglect to mention capabilities which Terminal.app
          actually supports (such as "dim text"). This oversight on Apple's part
          ends up penalizing users of "good citizen" console programs which
          consult "terminfo" to tailor their output based upon reported terminal
          capabilities (as opposed to programs which assume that the terminal
     -    supports ANSI codes).
     +    supports ANSI codes). The same problem is present in other Apple
     +    "terminfo" entries, such as "nsterm"[2], with which macOS Terminal.app
     +    may be configured.
      
          Sidestep this Apple problem by imbuing get_colors() with specific
     -    knowledge of "xterm" capabilities rather than trusting "terminfo" to
     -    report them correctly. Although hard-coding such knowledge is ugly,
     -    "xterm" support is nearly ubiquitous these days, and Git itself sets
     -    precedence by assuming support for ANSI color codes. For non-"xterm",
     -    fall back to querying "terminfo" via `tput` as usual.
     +    knowledge of capabilities common to "xterm" and "nsterm", rather than
     +    trusting "terminfo" to report them correctly. Although hard-coding such
     +    knowledge is ugly, "xterm" support is nearly ubiquitous these days, and
     +    Git itself sets precedence by assuming support for ANSI color codes. For
     +    other terminal types, fall back to querying "terminfo" via `tput` as
     +    usual.
     +
     +    FOOTNOTES
     +
     +    [1] iTerm2 FAQ suggests "xterm-new": https://iterm2.com/faq.html
     +
     +    [2] Neovim documentation recommends terminal type "nsterm" with
     +        Terminal.app: https://neovim.io/doc/user/term.html#terminfo
      
          Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
      
     @@ t/chainlint.pl: my @NOCOLORS = (bold => '', rev => '', reset => '', blue => '',
      -		   green => `tput setaf 2`,
      -		   red   => `tput setaf 1`);
      -	chomp(%COLORS);
     -+	if ($ENV{TERM} =~ /\bxterm\b/) {
     ++	if ($ENV{TERM} =~ /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/) {
      +		%COLORS = (bold  => "\e[1m",
      +			   rev   => "\e[7m",
      +			   reset => "\e[0m",
 2:  c8a316426be ! 2:  84ddc6707fb chainlint: latch line numbers at which each token starts and ends
     @@ t/chainlint.pl: sub scan_balanced {
       }
       
      @@ t/chainlint.pl: sub swallow_heredocs {
     + 	my $b = $self->{buff};
     + 	my $tags = $self->{heretags};
       	while (my $tag = shift @$tags) {
     ++		my $start = pos($$b);
       		my $indent = $tag =~ s/^\t// ? '\\s*' : '';
       		$$b =~ /(?:\G|\n)$indent\Q$tag\E(?:\n|\z)/gc;
     -+		my $body = $&;
     ++		my $body = substr($$b, $start, pos($$b) - $start);
      +		$self->{lineno} += () = $body =~ /\n/sg;
       	}
       }
 3:  380b146abd1 ! 3:  3cb4ff4d330 chainlint: prefix annotated test definition with line numbers
     @@ t/chainlint.pl: if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {
       sub get_colors {
       	return \%COLORS if %COLORS;
      @@ t/chainlint.pl: sub get_colors {
     - 	if ($ENV{TERM} =~ /\bxterm\b/) {
     + 	if ($ENV{TERM} =~ /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/) {
       		%COLORS = (bold  => "\e[1m",
       			   rev   => "\e[7m",
      +			   dim   => "\e[2m",

-- 
gitgitgadget

[PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

Although the macOS Terminal.app is "xterm"-compatible, its corresponding
"terminfo" entries -- such as "xterm", "xterm-256color", and
"xterm-new"[1] -- neglect to mention capabilities which Terminal.app
actually supports (such as "dim text"). This oversight on Apple's part
ends up penalizing users of "good citizen" console programs which
consult "terminfo" to tailor their output based upon reported terminal
capabilities (as opposed to programs which assume that the terminal
supports ANSI codes). The same problem is present in other Apple
"terminfo" entries, such as "nsterm"[2], with which macOS Terminal.app
may be configured.

Sidestep this Apple problem by imbuing get_colors() with specific
knowledge of capabilities common to "xterm" and "nsterm", rather than
trusting "terminfo" to report them correctly. Although hard-coding such
knowledge is ugly, "xterm" support is nearly ubiquitous these days, and
Git itself sets precedence by assuming support for ANSI color codes. For
other terminal types, fall back to querying "terminfo" via `tput` as
usual.

FOOTNOTES

[1] iTerm2 FAQ suggests "xterm-new": https://iterm2.com/faq.html

[2] Neovim documentation recommends terminal type "nsterm" with
    Terminal.app: https://neovim.io/doc/user/term.html#terminfo

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 7972c5bbe6f..0ee5cc36437 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -653,21 +653,32 @@ 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) {
+	if (exists($ENV{NO_COLOR})) {
 		%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);
+	if ($ENV{TERM} =~ /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/) {
+		%COLORS = (bold  => "\e[1m",
+			   rev   => "\e[7m",
+			   reset => "\e[0m",
+			   blue  => "\e[34m",
+			   green => "\e[32m",
+			   red   => "\e[31m");
+		return \%COLORS;
+	}
+	if (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 = (bold  => `tput bold`,
+			   rev   => `tput rev`,
+			   reset => `tput sgr0`,
+			   blue  => `tput setaf 4`,
+			   green => `tput setaf 2`,
+			   red   => `tput setaf 1`);
+		return \%COLORS;
+	}
+	%COLORS = @NOCOLORS;
 	return \%COLORS;
 }
 
-- 
gitgitgadget

[PATCH v2 2/3] chainlint: latch line numbers at which each token starts and ends

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

When chainlint detects problems in a test, it prints out the name of the
test script, the name of the problematic test, and a copy of the test
definition with "?!FOO?!" annotations inserted at the locations where
problems were detected. Taken together this information is sufficient
for the test author to identify the problematic code in the original
test definition. However, in a lengthy script or a lengthy test
definition, the author may still end up using the editor's search
feature to home in on the exact problem location.

To further assist the test author, an upcoming change will display line
numbers along with the annotated test definition, thus allowing the
author to jump directly to each problematic line. As preparation,
upgrade Lexer to latch the line numbers at which each token starts and
ends, and return that information with the token itself.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 0ee5cc36437..67c2c5ebee8 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -67,6 +67,7 @@ sub new {
 	bless {
 		parser => $parser,
 		buff => $s,
+		lineno => 1,
 		heretags => []
 	} => $class;
 }
@@ -97,7 +98,9 @@ sub scan_op {
 sub scan_sqstring {
 	my $self = shift @_;
 	${$self->{buff}} =~ /\G([^']*'|.*\z)/sgc;
-	return "'" . $1;
+	my $s = $1;
+	$self->{lineno} += () = $s =~ /\n/sg;
+	return "'" . $s;
 }
 
 sub scan_dqstring {
@@ -115,7 +118,7 @@ sub scan_dqstring {
 		if ($c eq '\\') {
 			$s .= '\\', last unless $$b =~ /\G(.)/sgc;
 			$c = $1;
-			next if $c eq "\n"; # line splice
+			$self->{lineno}++, next if $c eq "\n"; # line splice
 			# backslash escapes only $, `, ", \ in dq-string
 			$s .= '\\' unless $c =~ /^[\$`"\\]$/;
 			$s .= $c;
@@ -123,6 +126,7 @@ sub scan_dqstring {
 		}
 		die("internal error scanning dq-string '$c'\n");
 	}
+	$self->{lineno} += () = $s =~ /\n/sg;
 	return $s;
 }
 
@@ -137,6 +141,7 @@ sub scan_balanced {
 		$depth--;
 		last if $depth == 0;
 	}
+	$self->{lineno} += () = $s =~ /\n/sg;
 	return $s;
 }
 
@@ -163,8 +168,11 @@ sub swallow_heredocs {
 	my $b = $self->{buff};
 	my $tags = $self->{heretags};
 	while (my $tag = shift @$tags) {
+		my $start = pos($$b);
 		my $indent = $tag =~ s/^\t// ? '\\s*' : '';
 		$$b =~ /(?:\G|\n)$indent\Q$tag\E(?:\n|\z)/gc;
+		my $body = substr($$b, $start, pos($$b) - $start);
+		$self->{lineno} += () = $body =~ /\n/sg;
 	}
 }
 
@@ -172,11 +180,12 @@ sub scan_token {
 	my $self = shift @_;
 	my $b = $self->{buff};
 	my $token = '';
-	my $start;
+	my ($start, $startln);
 RESTART:
+	$startln = $self->{lineno};
 	$$b =~ /\G[ \t]+/gc; # skip whitespace (but not newline)
 	$start = pos($$b) || 0;
-	return ["\n", $start, pos($$b)] if $$b =~ /\G#[^\n]*(?:\n|\z)/gc; # comment
+	$self->{lineno}++, return ["\n", $start, pos($$b), $startln, $startln] if $$b =~ /\G#[^\n]*(?:\n|\z)/gc; # comment
 	while (1) {
 		# slurp up non-special characters
 		$token .= $1 if $$b =~ /\G([^\\;&|<>(){}'"\$\s]+)/gc;
@@ -188,20 +197,20 @@ RESTART:
 		$token .= $self->scan_sqstring(), next if $c eq "'";
 		$token .= $self->scan_dqstring(), next if $c eq '"';
 		$token .= $c . $self->scan_dollar(), next if $c eq '$';
-		$self->swallow_heredocs(), $token = $c, last if $c eq "\n";
+		$self->{lineno}++, $self->swallow_heredocs(), $token = $c, last if $c eq "\n";
 		$token = $self->scan_op($c), last if $c =~ /^[;&|<>]$/;
 		$token = $c, last if $c =~ /^[(){}]$/;
 		if ($c eq '\\') {
 			$token .= '\\', last unless $$b =~ /\G(.)/sgc;
 			$c = $1;
-			next if $c eq "\n" && length($token); # line splice
-			goto RESTART if $c eq "\n"; # line splice
+			$self->{lineno}++, next if $c eq "\n" && length($token); # line splice
+			$self->{lineno}++, goto RESTART if $c eq "\n"; # line splice
 			$token .= '\\' . $c;
 			next;
 		}
 		die("internal error scanning character '$c'\n");
 	}
-	return length($token) ? [$token, $start, pos($$b)] : undef;
+	return length($token) ? [$token, $start, pos($$b), $startln, $self->{lineno}] : undef;
 }
 
 # ShellParser parses POSIX shell scripts (with minor extensions for Bash). It
-- 
gitgitgadget

[PATCH v2 3/3] chainlint: prefix annotated test definition with line numbers

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

When chainlint detects problems in a test, it prints out the name of the
test script, the name of the problematic test, and a copy of the test
definition with "?!FOO?!" annotations inserted at the locations where
problems were detected. Taken together this information is sufficient
for the test author to identify the problematic code in the original
test definition. However, in a lengthy script or a lengthy test
definition, the author may still end up using the editor's search
feature to home in on the exact problem location.

To further assist the test author, display line numbers along with the
annotated test definition, thus allowing the author to jump directly to
each problematic line.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/Makefile     |  2 +-
 t/chainlint.pl | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 882782a519c..2c2b2522402 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -94,7 +94,7 @@ check-chainlint:
 		done \
 	} >'$(CHAINLINTTMP_SQ)'/expect && \
 	$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
-		grep -v '^[ 	]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \
+		sed -e 's/^[1-9][0-9]* //;/^[ 	]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
 	if test -f ../GIT-BUILD-OPTIONS; then \
 		. ../GIT-BUILD-OPTIONS; \
 	fi && \
diff --git a/t/chainlint.pl b/t/chainlint.pl
index 67c2c5ebee8..4e47e808d01 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -614,6 +614,7 @@ sub check_test {
 	my $problems = $parser->{problems};
 	return unless $emit_all || @$problems;
 	my $c = main::fd_colors(1);
+	my $lineno = $_[1]->[3];
 	my $start = 0;
 	my $checked = '';
 	for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
@@ -623,10 +624,12 @@ sub check_test {
 		$start = $pos;
 	}
 	$checked .= substr($body, $start);
-	$checked =~ s/^\n//;
+	$checked =~ s/^/$lineno++ . ' '/mge;
+	$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/^\d+/$c->{dim}$&$c->{reset}/mg;
 	$checked .= "\n" unless $checked =~ /\n$/;
 	push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
 }
@@ -658,7 +661,7 @@ 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 => '', rev => '', reset => '', blue => '', green => '', red => '');
+my @NOCOLORS = (bold => '', rev => '', dim => '', reset => '', blue => '', green => '', red => '');
 my %COLORS = ();
 sub get_colors {
 	return \%COLORS if %COLORS;
@@ -669,6 +672,7 @@ sub get_colors {
 	if ($ENV{TERM} =~ /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/) {
 		%COLORS = (bold  => "\e[1m",
 			   rev   => "\e[7m",
+			   dim   => "\e[2m",
 			   reset => "\e[0m",
 			   blue  => "\e[34m",
 			   green => "\e[32m",
@@ -678,9 +682,11 @@ sub get_colors {
 	if (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 dim  >/dev/null 2>&1") == 0 &&
 	    system("tput setaf 1 >/dev/null 2>&1") == 0) {
 		%COLORS = (bold  => `tput bold`,
 			   rev   => `tput rev`,
+			   dim   => `tput dim`,
 			   reset => `tput sgr0`,
 			   blue  => `tput setaf 4`,
 			   green => `tput setaf 2`,
-- 
gitgitgadget

Re: [PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 11 2022, Eric Sunshine via GitGitGadget wrote:

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Although the macOS Terminal.app is "xterm"-compatible, its corresponding
> "terminfo" entries -- such as "xterm", "xterm-256color", and
> "xterm-new"[1] -- neglect to mention capabilities which Terminal.app
> actually supports (such as "dim text"). This oversight on Apple's part
> ends up penalizing users of "good citizen" console programs which
> consult "terminfo" to tailor their output based upon reported terminal
> capabilities (as opposed to programs which assume that the terminal
> supports ANSI codes). The same problem is present in other Apple
> "terminfo" entries, such as "nsterm"[2], with which macOS Terminal.app
> may be configured.
>
> Sidestep this Apple problem by imbuing get_colors() with specific
> knowledge of capabilities common to "xterm" and "nsterm", rather than
> trusting "terminfo" to report them correctly. Although hard-coding such
> knowledge is ugly, "xterm" support is nearly ubiquitous these days, and
> Git itself sets precedence by assuming support for ANSI color codes. For
> other terminal types, fall back to querying "terminfo" via `tput` as
> usual.
>
> FOOTNOTES
>
> [1] iTerm2 FAQ suggests "xterm-new": https://iterm2.com/faq.html
>
> [2] Neovim documentation recommends terminal type "nsterm" with
>     Terminal.app: https://neovim.io/doc/user/term.html#terminfo
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/chainlint.pl | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/t/chainlint.pl b/t/chainlint.pl
> index 7972c5bbe6f..0ee5cc36437 100755
> --- a/t/chainlint.pl
> +++ b/t/chainlint.pl
> @@ -653,21 +653,32 @@ 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) {
> +	if (exists($ENV{NO_COLOR})) {
>  		%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);
> +	if ($ENV{TERM} =~ /xterm|xterm-\d+color|xterm-new|xterm-direct|nsterm|nsterm-\d+color|nsterm-direct/) {
> +		%COLORS = (bold  => "\e[1m",
> +			   rev   => "\e[7m",
> +			   reset => "\e[0m",
> +			   blue  => "\e[34m",
> +			   green => "\e[32m",
> +			   red   => "\e[31m");
> +		return \%COLORS;
> +	}
> +	if (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 = (bold  => `tput bold`,
> +			   rev   => `tput rev`,
> +			   reset => `tput sgr0`,
> +			   blue  => `tput setaf 4`,
> +			   green => `tput setaf 2`,
> +			   red   => `tput setaf 1`);
> +		return \%COLORS;
> +	}
> +	%COLORS = @NOCOLORS;
>  	return \%COLORS;
>  }

Doesn't test-lib.sh have the same problem then?

This is somewhat of an aside, as we're hardcoding thees colors in
t/chainlint.pl now, but I wondered when that was added (but I don't
think I commented then) why it needed to be re-hardcoding the coloring
we've got in test-lib.sh.

I.e. if test-lib.sh is running it could we handle these cases, and just
export a variable with the color info for "bold" or whatever in
GIT_TEST_COLOR_BOLD, then pick that up?

I have a local semi-related patch which made much the same change to
test-lib.sh itself, to support --color without going through whether
tput thinks we support colors:
https://github.com/avar/git/commit/c4914db758b

I think this is fine for now if you don't want to poke more at it, but
maybe this should all be eventually combined?

I also wonder to what extent this needs to be re-inventing
Term::ANSIColor, which has shipped with Perl since 5.6, so we can use it
without worrying about version compat, but that's another topic...

Re: [PATCH 0/3] chainlint: emit line numbers alongside test definitions

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 09 2022, Eric Sunshine via GitGitGadget wrote:

> This is atop "es/chainlint-output"[2].
>
> (Note to self: Fortify against Ævar's nerd-snipe blacklist evasion.)

My only regret is not asking for a pony :)

This looks great, thanks. I read over the v2 (just commenting on the v1
CL for the above comment). I left a note about a potential follow-up
about the color detection, but that's aside from the main change here,
so I think it would be good to just get some version of your v2 as-is,
unless you're super keen to spend more time fiddling with this...

Re: [PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Eric Sunshine]

On Fri, Nov 11, 2022 at 10:02 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Fri, Nov 11 2022, Eric Sunshine via GitGitGadget wrote:
> > Sidestep this Apple problem by imbuing get_colors() with specific
> > knowledge of capabilities common to "xterm" and "nsterm", rather than
> > trusting "terminfo" to report them correctly. Although hard-coding such
> > knowledge is ugly, "xterm" support is nearly ubiquitous these days, and
> > Git itself sets precedence by assuming support for ANSI color codes. For
> > other terminal types, fall back to querying "terminfo" via `tput` as
> > usual.
>
> Doesn't test-lib.sh have the same problem then?

Generally speaking, yes, but in practice, no, not for this particular
case. I specifically wanted to use "dim" here in chainlint.pl, which
is (oddly) missing in Apple's terminfo. test-lib.sh just uses some
colors, bold, reverse, and "reset", all of which are present in
Apple's terminfo.

> This is somewhat of an aside, as we're hardcoding thees colors in
> t/chainlint.pl now, but I wondered when that was added (but I don't
> think I commented then) why it needed to be re-hardcoding the coloring
> we've got in test-lib.sh.
>
> I.e. if test-lib.sh is running it could we handle these cases, and just
> export a variable with the color info for "bold" or whatever in
> GIT_TEST_COLOR_BOLD, then pick that up?

When adding colorizing to chainlint.pl, one of my very first thoughts
was to somehow reuse the color information from test-lib.sh. That's
relatively easy in the case when the test script is being run
standalone because it has already "sourced" test-lib.sh before it runs
chainlint.pl, thus could pass the color information along to
chainlint.pl somehow. But the other case, when "make test" (or "make
test-chainlint", etc.) is used is harder because that's just the
Makefile running chainlint.pl directly, so test-lib.sh isn't involved
in the equation. It would probably be possible to make it work, but
the solutions seemed ugly and too invasive, especially for an initial
implementation of colorizing in chainlint.pl.

> I have a local semi-related patch which made much the same change to
> test-lib.sh itself, to support --color without going through whether
> tput thinks we support colors:
> https://github.com/avar/git/commit/c4914db758b
>
> I think this is fine for now if you don't want to poke more at it, but
> maybe this should all be eventually combined?

Peff also expressed such a sentiment[1][2][3]. I'm still somewhat
hesitant to make chainlint.pl dependent upon so much outside machinery
since it's still nicely standalone and _might_ be useful elsewhere.

> I also wonder to what extent this needs to be re-inventing
> Term::ANSIColor, which has shipped with Perl since 5.6, so we can use it
> without worrying about version compat, but that's another topic...

Gah, why didn't I know about this sooner?! (Note to self: hit self
over head.) Back when I was adding colorizing to chainlint.pl, I
searched around to determine if Perl had any built-in colorizing
support, but I completely overlooked this. I must have missed it
because I was focusing on "curses" since I thought I would need to
pull color codes directly from "curses", and Perl doesn't have a
standard (shipped) "curses" module. I even said as much to Peff[4].

Since it's been shipping with Perl for quite some time,
Term::ANSIColor would be a much nicer solution; worth looking into.

[1]: https://lore.kernel.org/git/Yx%2FLpUglpjY5ZNas@coredump.intra.peff.net/
[2]: https://lore.kernel.org/git/Yx%2FeG5xJonNh7Dsz@coredump.intra.peff.net/
[3]: https://lore.kernel.org/git/Yx%2FPnWnkYAuWToiz@coredump.intra.peff.net/
[4]: https://lore.kernel.org/git/CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com/

Re: [PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Eric Sunshine]

On Fri, Nov 11, 2022 at 11:44 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Nov 11, 2022 at 10:02 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > I also wonder to what extent this needs to be re-inventing
> > Term::ANSIColor, which has shipped with Perl since 5.6, so we can use it
> > without worrying about version compat, but that's another topic...
>
> Gah, why didn't I know about this sooner?! [...]
>
> Since it's been shipping with Perl for quite some time,
> Term::ANSIColor would be a much nicer solution; worth looking into.

In retrospect, I may have looked at Term::ANSIColor at the time but
decided to avoid it since it assumes the terminal understands ANSI
codes, and I was looking for a more general solution which respected
the terminal's capabilities as reported by "terminfo".

And, reading up on it now, I'm not finding much benefit to
Term::ANSIColor over what is already implemented in chainlint.pl.
Particularly disheartening is that (as far as I can tell)
Term::ANSIColor doesn't provide a way to interrogate whether or not it
is suitable to use ANSI codes with the terminal in question, but
instead makes a blanket assumption that the terminal supports ANSI
codes unconditionally.

So, I think the fixed-up colorizing as implemented by v2 of this patch
series is good enough for now. It can always be revisited later if
something warrants it.

Re: [PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"

[Taylor Blau]

On Fri, Nov 11, 2022 at 12:15:12PM -0500, Eric Sunshine wrote:
> So, I think the fixed-up colorizing as implemented by v2 of this patch
> series is good enough for now. It can always be revisited later if
> something warrants it.

Yeah, agree. Let's not let perfect be the enemy of the good ;-).

Thanks,
Taylor