On Mon, Sep 12, 2022 at 8:15 PM Jeff King [off-list ref] wrote:

On Mon, Sep 12, 2022 at 11:04:48PM +0000, Eric Sunshine via GitGitGadget wrote:

quoted

+my @NOCOLORS = (bold => '', 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 setaf 1 >/dev/null 2>&1") != 0) {
+             %COLORS = @NOCOLORS;
+             return \%COLORS;
+     }
+     %COLORS = (bold  => `tput bold`,
+                reset => `tput sgr0`,
+                blue  => `tput setaf 4`,
+                green => `tput setaf 2`,
+                red   => `tput setaf 1`);
+     chomp(%COLORS);
+     return \%COLORS;
+}

This is a lot of new processes. Should be OK in the run-once-for-all-tests
mode. It does make me wonder how much time regular test-lib.sh spends
doing these tput checks for every script (at least it's not every
snippet!).

This is indeed a lot of new processes, but this color interrogation is
done lazily, only if a problem is detected, so it should be zero-cost
in the (hopefully) normal case of a lint-clean script.

I had the exact same thought about the cost being paid by test-lib.sh
making all those `tput` invocations.

It feels like we could build a color.sh snippet once and then include it
in each script. But maybe that is dumb, since you could in theory build
in one terminal and then run in another. Unlikely, but it shows that
file dependencies are a mismatch. I guess a better match would be
stuffing it into the environment before starting all of the tests.

That might be worth considering at some point.

I ran this on my pre-fixup state where I had a half-dozen linter checks.
It's _so_ much more readable. Thanks for working on it.

Good to hear.