Re: [PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-11-11 15:07:20
On Fri, Nov 11 2022, Eric Sunshine via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Eric Sunshine <redacted> 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 <redacted> --- 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...