| Message ID | de482cf9cf1c791418e4279523123580f330245b.1668152094.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5451877f87f8c0c820dd8605e03cb00ea794ca89 |
| Headers | show |
| Series | chainlint: emit line numbers alongside test definitions | expand |
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...
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/
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.
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
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; }