Re: Fwd: Add support for axiterm colors in parse_color

From: Eyal Soha <shawarmakarma@gmail.com>
Date: Jan 10, 2020, 00:20 UTC
Message-ID: <CANsz78Lm3ggVZLrSxM5tc0MozFMdAmVBOix_3sjJT4+s3VHLPQ@mail.gmail.com>
In-Reply-To: <20200108095229.GC87523@coredump.intra.peff.net>

> Depending on your terminal, you can already access these colors via
> 256-color mode. Generally 0-7 are the standard colors there, then 8-15
> are the "bright" variants, and then an rgb cube.

Yes. According to the wikipedia page https://en.wikipedia.org/wiki/ANSI_escape_code#Colors :

     0-  7:  standard colors (as in ESC [ 30–37 m)
     8- 15:  high intensity colors (as in ESC [ 90–97 m)
    16-231:  6 × 6 × 6 cube (216 colors): 16 + 36 × r + 6 × g + b (0 ≤
r, g, b ≤ 5)
   232-255:  grayscale from black to white in 24 steps

So 8bit 0-7 and 8bit 8-15 match the ANSI colors 30-37 and 90-97. The background color versions match 40-47 and 100-107.

On my desktop, the RGB and ANSI colors match in color as described above.

> That said, I'm not entirely opposed to having more human-readable
> aliases. I'm not sure if it's worth using the 16-color version (e.g.,
> "ESC[91m" versus the 256-color version "ESC[38;5;9m"). It's possible it
> would be compatible with more terminals, and it is slightly fewer bytes.

My motivation for this patch was to fix the github workflow log output that doesn't support 8bit colors properly. Only the "ANSI" colors 0-15 worked. None of the 8-bit colors worked except for 30-37, 40-47, 90-97, and 100-107, which matched the ANSI colors. That is a very broken color scheme! But that's how it displayed.

> What's AXITERM? I couldn't find it mentioned anywhere around ANSI codes.

Oops, I misread it.  The wikipedia page mentions it, it's "aixterm".

> I kind of wonder if we could just call these ANSI as well, and have
> color_output() just decide whether to add an offset of 60 when we see a
> color number between 8 and 15. Or possibly c->value should just have the
> 60-offset value in the first place.

Done.

> This "type" field was already pretty ugly, and I think your patch makes
> it even worse. It would probably be less bad if we just passed in the
> integer 30 instead of '3', and then do:

Done.

Show 6 quoted lines

> Bonus points for declaring:
>
>   enum {
>     COLOR_FOREGROUND = 30,
>     COLOR_BACKGROUND = 40,
>   } color_ground;

Done in a slightly different way.

> This BUG() can go away. The point was to make sure we don't overflow
> "out", but xsnprintf() will check that for us (that's why there isn't
> one in the COLOR_256 and COLOR_RGB case arms).

Removed.

> And if we can handle the regular/bright stuff instead of color_output(),
> we don't need to have this extra fg.type check.

Done.  Here's a new patch!

diff --git a/color.c b/color.c
index ebb222ec33..efbe9a1858 100644
--- a/color.c
+++ b/color.c
@@ -24,6 +24,14 @@ const char *column_colors_ansi[] = {
        GIT_COLOR_RESET,
 };

+enum {
+       COLOR_BACKGROUND_OFFSET = 10,
+       COLOR_FOREGROUND_ANSI = 30,
+       COLOR_FOREGROUND_RGB = 38,
+       COLOR_FOREGROUND_256 = 38,
+       COLOR_FOREGROUND_BRIGHT_ANSI = 90,
+};
+
 /* Ignore the RESET at the end when giving the size */
 const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;

@@ -92,7 +100,13 @@ static int parse_color(struct color *out, const
char *name, int len)
        for (i = 0; i < ARRAY_SIZE(color_names); i++) {
                if (match_word(name, len, color_names[i])) {
                        out->type = COLOR_ANSI;
-                       out->value = i;
+                       out->value = i + COLOR_FOREGROUND_ANSI;
+                       return 0;
+               }
+               if (*name == '+' &&
+                   match_word(name+1, len-1, color_names[i])) {
+                       out->type = COLOR_ANSI;
+                       out->value = i + COLOR_FOREGROUND_BRIGHT_ANSI;
                        return 0;
                }
        }
@@ -109,10 +123,15 @@ static int parse_color(struct color *out, const
char *name, int len)
                else if (val < 0) {
                        out->type = COLOR_NORMAL;
                        return 0;
-               /* Rewrite low numbers as more-portable standard colors. */
+               /* Rewrite 0-7 as more-portable standard colors. */
                } else if (val < 8) {
                        out->type = COLOR_ANSI;
-                       out->value = val;
+                       out->value = val + COLOR_FOREGROUND_ANSI;
+                       return 0;
+               /* Rewrite 8-15 as more-portable aixterm colors. */
+               } else if (val < 16) {
+                       out->type = COLOR_ANSI;
+                       out->value = val - 8 + COLOR_FOREGROUND_BRIGHT_ANSI;
                        return 0;
                } else if (val < 256) {
                        out->type = COLOR_256;
@@ -166,24 +185,24 @@ int color_parse(const char *value, char *dst)
  * already have the ANSI escape code in it. "out" should have enough
  * space in it to fit any color.
  */
-static char *color_output(char *out, int len, const struct color *c, char type)
+static char *color_output(char *out, int len, const struct color *c,
+                         int offset)
 {
        switch (c->type) {
        case COLOR_UNSPECIFIED:
        case COLOR_NORMAL:
                break;
        case COLOR_ANSI:
-               if (len < 2)
-                       BUG("color parsing ran out of space");
-               *out++ = type;
-               *out++ = '0' + c->value;
+               out += xsnprintf(out, len, "%d", c->value + offset);
                break;
        case COLOR_256:
-               out += xsnprintf(out, len, "%c8;5;%d", type, c->value);
+         out += xsnprintf(out, len, "%d;5;%d", COLOR_FOREGROUND_256 + offset,
+                          c->value);
                break;
        case COLOR_RGB:
-               out += xsnprintf(out, len, "%c8;2;%d;%d;%d", type,
-                                c->red, c->green, c->blue);
+         out += xsnprintf(out, len, "%d;2;%d;%d;%d",
+                          COLOR_FOREGROUND_RGB + offset,
+                          c->red, c->green, c->blue);
                break;
        }
        return out;
@@ -279,14 +298,12 @@ int color_parse_mem(const char *value, int
value_len, char *dst)
                if (!color_empty(&fg)) {
                        if (sep++)
                                OUT(';');
-                       /* foreground colors are all in the 3x range */
-                       dst = color_output(dst, end - dst, &fg, '3');
+                       dst = color_output(dst, end - dst, &fg, 0);
                }
                if (!color_empty(&bg)) {
                        if (sep++)
                                OUT(';');
-                       /* background colors are all in the 4x range */
-                       dst = color_output(dst, end - dst, &bg, '4');
+                       dst = color_output(dst, end - dst, &bg,
COLOR_BACKGROUND_OFFSET);
                }
                OUT('m');
        }
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..e3f11a94f9 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -30,6 +30,14 @@ test_expect_success 'attribute before color name' '
        color "bold red" "[1;31m"
 '

+test_expect_success 'axiterm bright fg color' '
+       color "+red" "[91m"
+'
+
+test_expect_success 'axiterm bright bg color' '
+       color "green +blue" "[32;104m"
+'
+
 test_expect_success 'color name before attribute' '
        color "red bold" "[1;31m"
 '
@@ -74,6 +82,10 @@ test_expect_success '0-7 are aliases for basic ANSI
color names' '
        color "0 7" "[30;47m"
 '

+test_expect_success '8-15 are aliases for aixterm color names' '
+       color "12 13" "[94;105m"
+'
+
 test_expect_success '256 colors' '
        color "254 bold 255" "[1;38;5;254;48;5;255m"
 '