chainlint: emit line numbers alongside test definitions

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/[email protected]/
[2]
https://lore.kernel.org/git/[email protected]/

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

From: Eric Sunshine <[email protected]>

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.

---
 t/chainlint.pl | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

index 7972c5bbe6f..fcf4d459249 100755
@@ -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

From: Eric Sunshine <[email protected]>

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.

---
 t/chainlint.pl | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

index fcf4d459249..01f261165b1 100755
@@ -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

From: Eric Sunshine <[email protected]>

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.

---
 t/Makefile     |  2 +-
 t/chainlint.pl | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

index 882782a519c..2c2b2522402 100644
@@ -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 && \

index 01f261165b1..48dde978480 100755
@@ -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

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

Thanks,
Taylor

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.

;-).

Thanks,
Taylor

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

brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

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)

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

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

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.

Maybe this, then?

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

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

Sure. I would also prefer to avoid that.

brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

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.

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.

Simplicity is nice. I think that seems like a good pattern.

brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

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/[email protected]/
[2]
https://lore.kernel.org/git/[email protected]/

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

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

From: Eric Sunshine <[email protected]>

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

---
 t/chainlint.pl | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

index 7972c5bbe6f..0ee5cc36437 100755
@@ -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

From: Eric Sunshine <[email protected]>

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.

---
 t/chainlint.pl | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

index 0ee5cc36437..67c2c5ebee8 100755
@@ -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

From: Eric Sunshine <[email protected]>

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.

---
 t/Makefile     |  2 +-
 t/chainlint.pl | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

index 882782a519c..2c2b2522402 100644
@@ -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 && \

index 67c2c5ebee8..4e47e808d01 100755
@@ -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

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

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.

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.

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.

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%[email protected]/
[2]: https://lore.kernel.org/git/Yx%[email protected]/
[3]: https://lore.kernel.org/git/Yx%[email protected]/
[4]: https://lore.kernel.org/git/CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com/

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.

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

Thanks,
Taylor