Re: [PATCH v2 1/3] chainlint: sidestep impoverished macOS "terminfo"

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 11, 2022, 16:44 UTC
Message-ID: <CAPig+cRwDeGyniiVGqmdMePgmR6GiYQOvNP+GUeT__zpuWV1Fg@mail.gmail.com>
In-Reply-To: <221111.865yflo7p7.gmgdl@evledraar.gmail.com>

On Fri, Nov 11, 2022 at 10:02 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 10 quoted lines

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

Show 8 quoted lines

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

Show 7 quoted lines

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