| Message ID | b85b28e5a6beea97c149f0b9de6ba8d0a4a7c1f9.1668013114.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | chainlint: emit line numbers alongside test definitions | expand |
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
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.
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.
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.
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.
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.
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; }