Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Eyal Soha <shawarmakarma@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: Fwd: Add support for axiterm colors in parse_color
Date: Thu, 9 Jan 2020 19:20:09 -0500
Message-ID: <CANsz78Lm3ggVZLrSxM5tc0MozFMdAmVBOix_3sjJT4+s3VHLPQ@mail.gmail.com> (raw)
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.

> 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"
 '

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CANsz78+ugmd62F4Qk+VT7Pi=ZPtMSkZjXOwLNRCFhoS9jrOkeQ@mail.gmail.com>
     [not found] ` <CANsz78K-BiswWPdhd_N25NuApcv7zSb2cw2Y9DSinkpNpuogYw@mail.gmail.com>
2020-01-07 15:36   ` Eyal Soha
2020-01-08  9:52     ` Jeff King
2020-01-10  0:20       ` Eyal Soha [this message]
2020-01-10 11:15         ` Jeff King
2020-01-10 15:02           ` Eyal Soha
2020-01-15 15:32             ` Eyal Soha
2020-01-10 15:05           ` [PATCH 1/3] color.c: Refactor color_output to use enums Eyal Soha
2020-01-10 15:05             ` [PATCH 2/3] color.c: Support bright aixterm colors Eyal Soha
2020-01-15 22:42               ` Jeff King
2020-01-10 15:05             ` [PATCH 3/3] color.c: Alias RGB colors 8-15 to " Eyal Soha
2020-01-15 22:45               ` Jeff King
2020-01-15 22:33             ` [PATCH 1/3] color.c: Refactor color_output to use enums Jeff King
2020-01-16 18:01             ` Junio C Hamano
2020-01-16 18:23               ` Jeff King
2020-01-16 19:25                 ` Eyal Soha
2020-01-18 14:53                   ` Eyal Soha
2020-01-18 14:53                     ` [PATCH 2/3] color.c: Support bright aixterm colors Eyal Soha
2020-01-18 18:47                       ` Junio C Hamano
2020-01-21 16:52                         ` Eyal Soha
2020-01-21 16:56                           ` [PATCH 1/3] color.c: refactor color_output arguments Eyal Soha
2020-01-21 16:56                             ` [PATCH 2/3] color.c: support bright aixterm colors Eyal Soha
2020-01-23 22:54                               ` Junio C Hamano
2020-01-21 16:56                             ` [PATCH 3/3] color.c: alias RGB colors 8-15 to " Eyal Soha
2020-01-23 22:50                             ` [PATCH 1/3] color.c: refactor color_output arguments Junio C Hamano
2020-02-11 19:36                             ` [PATCH v3 0/3] es/bright-colors (hopefully final) reroll Junio C Hamano
2020-02-11 19:36                               ` [PATCH v3 1/3] color.c: refactor color_output arguments Junio C Hamano
2020-02-11 19:46                                 ` Jeff King
2020-02-11 23:01                                   ` Eyal Soha
2020-02-11 23:06                                     ` Junio C Hamano
2020-02-11 19:36                               ` [PATCH v3 2/3] color.c: support bright aixterm colors Junio C Hamano
2020-02-11 19:36                               ` [PATCH v3 3/3] color.c: alias RGB colors 8-15 to " Junio C Hamano
2020-01-21 17:37                           ` [PATCH 2/3] color.c: Support bright " Junio C Hamano
2020-01-18 14:53                     ` [PATCH 3/3] color.c: Alias RGB colors 8-15 to " Eyal Soha
2020-01-18 18:34                       ` Junio C Hamano
2020-01-18 17:51                     ` [PATCH 1/3] color.c: Refactor color_output to use enums Junio C Hamano
2020-01-21 16:37                       ` Eyal Soha
2020-01-21 17:49                         ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANsz78Lm3ggVZLrSxM5tc0MozFMdAmVBOix_3sjJT4+s3VHLPQ@mail.gmail.com \
    --to=shawarmakarma@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git