On Fri, Nov 11, 2022 at 10:02 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

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

quoted

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/ (local)
[2]: https://lore.kernel.org/git/Yx%2FeG5xJonNh7Dsz@coredump.intra.peff.net/ (local)
[3]: https://lore.kernel.org/git/Yx%2FPnWnkYAuWToiz@coredump.intra.peff.net/ (local)
[4]: https://lore.kernel.org/git/CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com/ (local)