Fwd: Add support for axiterm colors in parse_color

Fwd: Add support for axiterm colors in parse_color

[Eyal Soha]

https://en.wikipedia.org/wiki/ANSI_escape_code

ANSI color codes 90-97 and 100-107 are brighter versions of the 30-37
and 40-47 colors.  To view them, run `colortest-16`.  In that
colortest, they are named with a leading "+".  Maybe git config could
support those colors, too, with a leading plus in the name?  They are
nice for having a different shade of a color in a diff.

Here's a patch with tests.  I don't know how to submit it.  Thoughts?

Eyal

diff --git a/color.c b/color.c
index ebb222ec33..a39fe7d133 100644
--- a/color.c
+++ b/color.c
@@ -33,6 +33,7 @@ struct color {
                COLOR_UNSPECIFIED = 0,
                COLOR_NORMAL,
                COLOR_ANSI, /* basic 0-7 ANSI colors */
+               COLOR_AXITERM, /* brighter than 0-7 ANSI colors */
                COLOR_256,
                COLOR_RGB
        } type;
@@ -95,6 +96,12 @@ static int parse_color(struct color *out, const
char *name, int len)
                        out->value = i;
                        return 0;
                }
+               if (*name == '+' &&
+                   match_word(name+1, len-1, color_names[i])) {
+                       out->type = COLOR_AXITERM;
+                       out->value = i;
+                       return 0;
+               }
        }

        /* And finally try a literal 256-color-mode number */
@@ -166,23 +173,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,
+                          const char *type)
 {
        switch (c->type) {
        case COLOR_UNSPECIFIED:
        case COLOR_NORMAL:
                break;
        case COLOR_ANSI:
-               if (len < 2)
+       case COLOR_AXITERM:
+               if (len < strlen(type) + 1)
                        BUG("color parsing ran out of space");
-               *out++ = type;
-               *out++ = '0' + c->value;
+               out += xsnprintf(out, len, "%s%c", type, '0' + c->value);
                break;
        case COLOR_256:
-               out += xsnprintf(out, len, "%c8;5;%d", type, c->value);
+               out += xsnprintf(out, len, "%s8;5;%d", type, c->value);
                break;
        case COLOR_RGB:
-               out += xsnprintf(out, len, "%c8;2;%d;%d;%d", type,
+               out += xsnprintf(out, len, "%s8;2;%d;%d;%d", type,
                                 c->red, c->green, c->blue);
                break;
        }
@@ -279,14 +287,24 @@ 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');
+                       /* foreground colors are in the 3x range */
+                       char *range = "3";
+                       if (fg.type == COLOR_AXITERM) {
+                               /* axiterm fg colors are in the 9x range */
+                               range = "9";
+                       }
+                       dst = color_output(dst, end - dst, &fg, range);
                }
                if (!color_empty(&bg)) {
                        if (sep++)
                                OUT(';');
                        /* background colors are all in the 4x range */
-                       dst = color_output(dst, end - dst, &bg, '4');
+                       char *range = "4";
+                       if (bg.type == COLOR_AXITERM) {
+                               /* axiterm bg colors are in the 10x range */
+                               range = "10";
+                       }
+                       dst = color_output(dst, end - dst, &bg, range);
                }
                OUT('m');
        }
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..2b8430584f 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"
 '

Re: Fwd: Add support for axiterm colors in parse_color

[Jeff King]

On Tue, Jan 07, 2020 at 10:36:53AM -0500, Eyal Soha wrote:

> https://en.wikipedia.org/wiki/ANSI_escape_code
> 
> ANSI color codes 90-97 and 100-107 are brighter versions of the 30-37
> and 40-47 colors.  To view them, run `colortest-16`.  In that
> colortest, they are named with a leading "+".  Maybe git config could
> support those colors, too, with a leading plus in the name?  They are
> nice for having a different shade of a color in a diff.

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.

You can see how your terminal displays all of them with something like
this:

  reset=$(git config --type=color --default=reset foo.bar)
  for i in $(seq 0 255); do
    color=$(git config --type=color --default="$i" foo.bar)
    echo "$color$i$reset"
  done

Some terminals also show "bold red" as bright red, but many modern ones
seem to actually provide a bold font.

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.

> diff --git a/color.c b/color.c
> index ebb222ec33..a39fe7d133 100644
> --- a/color.c
> +++ b/color.c
> @@ -33,6 +33,7 @@ struct color {
>                 COLOR_UNSPECIFIED = 0,
>                 COLOR_NORMAL,
>                 COLOR_ANSI, /* basic 0-7 ANSI colors */
> +               COLOR_AXITERM, /* brighter than 0-7 ANSI colors */

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

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.

>   * 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,
> +                          const char *type)

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:

  /* handles ANSI; your bright colors would want to add 60 */
  out += xsnprintf(out, len, "%d", type + c->value);

And then likewise the 256-color and rgb paths would do:

  out += xsnprintf(out, len, "%d;5;%d", type + 8, c->value);

Bonus points for declaring:

  enum {
    COLOR_FOREGROUND = 30,
    COLOR_BACKGROUND = 40,
  } color_ground;

to make the caller more readable.

>         case COLOR_ANSI:
> -               if (len < 2)
> +       case COLOR_AXITERM:
> +               if (len < strlen(type) + 1)
>                         BUG("color parsing ran out of space");
> -               *out++ = type;
> -               *out++ = '0' + c->value;
> +               out += xsnprintf(out, len, "%s%c", type, '0' + c->value);

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

> @@ -279,14 +287,24 @@ 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');
> +                       /* foreground colors are in the 3x range */
> +                       char *range = "3";
> +                       if (fg.type == COLOR_AXITERM) {
> +                               /* axiterm fg colors are in the 9x range */
> +                               range = "9";
> +                       }
> +                       dst = color_output(dst, end - dst, &fg, range);

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

-Peff

Re: Fwd: Add support for axiterm colors in parse_color

[Eyal Soha]

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

Re: Fwd: Add support for axiterm colors in parse_color

[Jeff King]

On Thu, Jan 09, 2020 at 07:20:09PM -0500, Eyal Soha wrote:

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

That makes sense. I'm not too surprised to see a terminal that supports
one but not the other.

But I wonder if there are ones that go the other way around: supporting
256-color mode but not ANSI 90-97, etc. I doubt it, but I think it would
be nice to split that change out into a separate commit in case we do
run into problems.

> Done.  Here's a new patch!

Thanks. The content here is looking pretty good (though I have a few
comments below).

Can you also format it as described in Documentation/SubmittingPatches
and re-send? In particular, it needs a regular commit message and your
signoff.

> +enum {
> +       COLOR_BACKGROUND_OFFSET = 10,
> +       COLOR_FOREGROUND_ANSI = 30,
> +       COLOR_FOREGROUND_RGB = 38,
> +       COLOR_FOREGROUND_256 = 38,
> +       COLOR_FOREGROUND_BRIGHT_ANSI = 90,
> +};

The split in this enum mostly makes sense to me. It changes the meaning
of "value" for COLOR_ANSI, but this is all local to color.c, so your
changes to output_color() are all that's needed.

It might be easier to follow the patch if switching to this enum came in
a separate preparatory patch.

> @@ -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;
>                 }

It would be slightly simpler and more efficient handle the "+" outside
the loop, like:

  int offset = COLOR_FOREGROUND_ANSI;
  if (*name == '+') {
          offset = COLOR_FOREGROUND_BRIGHT_ANSI;
	  name++;
	  len--;
  }

and then in the loop just set "out->value = i + offset".

You'd probably want to hoist this out to a separate function since
"name" needs to be unchanged in the later part of the function.

I dunno. It's almost certainly an unmeasurable micro-optimization, but
somehow the flow of it seems simpler to me.

I also wondered if we'd want English aliases like "brightred" that some
other systems use. It would be easy to add that to the check I showed
above without having to repeat as much.

> @@ -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;

And I think this 8-15 handling might want to be a separate commit on
top, just because it's possible it might regress some cases (though
again, I do doubt it).

>         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);

Looks like some unwanted tab/space conversion (here and elsewhere).

> +test_expect_success 'axiterm bright fg color' '
> +       color "+red" "[91m"

s/axi/aix/ (here and below).

> +test_expect_success '8-15 are aliases for aixterm color names' '
> +       color "12 13" "[94;105m"

Makes sense.

-Peff

Re: Fwd: Add support for axiterm colors in parse_color

[Eyal Soha]

Thanks.  I think that 3 patches are in order.  The first will refactor
and add the enums.  The second will add the aixterm colors.  The last
will alias RGB 8-15 to aixterm colors.

What's the correct way to email those patches?  I will try with
git-send-email.  BTW, is "git help send-email" supposed to work?

Eyal
On Fri, Jan 10, 2020 at 6:15 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Jan 09, 2020 at 07:20:09PM -0500, Eyal Soha wrote:
>
> > > 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.
>
> That makes sense. I'm not too surprised to see a terminal that supports
> one but not the other.
>
> But I wonder if there are ones that go the other way around: supporting
> 256-color mode but not ANSI 90-97, etc. I doubt it, but I think it would
> be nice to split that change out into a separate commit in case we do
> run into problems.
>
> > Done.  Here's a new patch!
>
> Thanks. The content here is looking pretty good (though I have a few
> comments below).
>
> Can you also format it as described in Documentation/SubmittingPatches
> and re-send? In particular, it needs a regular commit message and your
> signoff.
>
> > +enum {
> > +       COLOR_BACKGROUND_OFFSET = 10,
> > +       COLOR_FOREGROUND_ANSI = 30,
> > +       COLOR_FOREGROUND_RGB = 38,
> > +       COLOR_FOREGROUND_256 = 38,
> > +       COLOR_FOREGROUND_BRIGHT_ANSI = 90,
> > +};
>
> The split in this enum mostly makes sense to me. It changes the meaning
> of "value" for COLOR_ANSI, but this is all local to color.c, so your
> changes to output_color() are all that's needed.
>
> It might be easier to follow the patch if switching to this enum came in
> a separate preparatory patch.
>
> > @@ -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;
> >                 }
>
> It would be slightly simpler and more efficient handle the "+" outside
> the loop, like:
>
>   int offset = COLOR_FOREGROUND_ANSI;
>   if (*name == '+') {
>           offset = COLOR_FOREGROUND_BRIGHT_ANSI;
>           name++;
>           len--;
>   }
>
> and then in the loop just set "out->value = i + offset".
>
> You'd probably want to hoist this out to a separate function since
> "name" needs to be unchanged in the later part of the function.
>
> I dunno. It's almost certainly an unmeasurable micro-optimization, but
> somehow the flow of it seems simpler to me.
>
> I also wondered if we'd want English aliases like "brightred" that some
> other systems use. It would be easy to add that to the check I showed
> above without having to repeat as much.
>
> > @@ -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;
>
> And I think this 8-15 handling might want to be a separate commit on
> top, just because it's possible it might regress some cases (though
> again, I do doubt it).
>
> >         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);
>
> Looks like some unwanted tab/space conversion (here and elsewhere).
>
> > +test_expect_success 'axiterm bright fg color' '
> > +       color "+red" "[91m"
>
> s/axi/aix/ (here and below).
>
> > +test_expect_success '8-15 are aliases for aixterm color names' '
> > +       color "12 13" "[94;105m"
>
> Makes sense.
>
> -Peff

[PATCH 1/3] color.c: Refactor color_output to use enums

[Eyal Soha]

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/color.c b/color.c
index ebb222ec33..0549501f47 100644
--- a/color.c
+++ b/color.c
@@ -24,6 +24,13 @@ 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,
+};
+
 /* Ignore the RESET at the end when giving the size */
 const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
 
@@ -92,7 +99,7 @@ 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;
 		}
 	}
@@ -112,7 +119,7 @@ static int parse_color(struct color *out, const char *name, int len)
 		/* Rewrite low numbers as more-portable standard colors. */
 		} else if (val < 8) {
 			out->type = COLOR_ANSI;
-			out->value = val;
+			out->value = val + COLOR_FOREGROUND_ANSI;
 			return 0;
 		} else if (val < 256) {
 			out->type = COLOR_256;
@@ -166,23 +173,22 @@ 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,
+		out += xsnprintf(out, len, "%d;2;%d;%d;%d",
+				 COLOR_FOREGROUND_RGB + offset,
 				 c->red, c->green, c->blue);
 		break;
 	}
@@ -280,13 +286,13 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 			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');
 	}
-- 
2.24.1.591.g12029dc57d.dirty

[PATCH 2/3] color.c: Support bright aixterm colors

[Eyal Soha]

These colors are the bright variants of the 3-bit colors.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c          | 30 +++++++++++++++++++++++-------
 t/t4026-color.sh |  8 ++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/color.c b/color.c
index 0549501f47..4dbf12eff8 100644
--- a/color.c
+++ b/color.c
@@ -29,6 +29,7 @@ enum {
 	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 */
@@ -68,13 +69,32 @@ static int get_hex_color(const char *in, unsigned char *out)
 	return 0;
 }
 
-static int parse_color(struct color *out, const char *name, int len)
+static int parse_ansi_color(struct color *out, const char *name, int len)
 {
 	/* Positions in array must match ANSI color codes */
 	static const char * const color_names[] = {
 		"black", "red", "green", "yellow",
 		"blue", "magenta", "cyan", "white"
 	};
+
+	int color_offset = COLOR_FOREGROUND_ANSI;
+	if (strncasecmp(name, "bright", 6) == 0) {
+		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
+		name += 6;
+		len -= 6;
+	}
+	for (int i = 0; i < ARRAY_SIZE(color_names); i++) {
+		if (match_word(name, len, color_names[i])) {
+			out->type = COLOR_ANSI;
+			out->value = i + color_offset;
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
 	char *end;
 	int i;
 	long val;
@@ -96,12 +116,8 @@ static int parse_color(struct color *out, const char *name, int len)
 	}
 
 	/* Then pick from our human-readable color names... */
-	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		if (match_word(name, len, color_names[i])) {
-			out->type = COLOR_ANSI;
-			out->value = i + COLOR_FOREGROUND_ANSI;
-			return 0;
-		}
+	if (parse_ansi_color(out, name, len)) {
+		return 0;
 	}
 
 	/* And finally try a literal 256-color-mode number */
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..78c69de90a 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 'aixterm bright fg color' '
+	color "brightred" "[91m"
+'
+
+test_expect_success 'aixterm bright bg color' '
+	color "green brightblue" "[32;104m"
+'
+
 test_expect_success 'color name before attribute' '
 	color "red bold" "[1;31m"
 '
-- 
2.24.1.591.g12029dc57d.dirty

[PATCH 3/3] color.c: Alias RGB colors 8-15 to aixterm colors

[Eyal Soha]

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c          | 7 ++++++-
 t/t4026-color.sh | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index 4dbf12eff8..7f9f929fb6 100644
--- a/color.c
+++ b/color.c
@@ -132,11 +132,16 @@ 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 + 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;
 			out->value = val;
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 78c69de90a..c0b642c1ab 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -82,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"
 '
-- 
2.24.1.591.g12029dc57d.dirty

Re: Fwd: Add support for axiterm colors in parse_color

[Eyal Soha]

I sent out my patches.  Can someone take a look?

Thanks,

Eyal

On Fri, Jan 10, 2020 at 10:02 AM Eyal Soha <shawarmakarma@gmail.com> wrote:
>
> Thanks.  I think that 3 patches are in order.  The first will refactor
> and add the enums.  The second will add the aixterm colors.  The last
> will alias RGB 8-15 to aixterm colors.
>
> What's the correct way to email those patches?  I will try with
> git-send-email.  BTW, is "git help send-email" supposed to work?
>
> Eyal
> On Fri, Jan 10, 2020 at 6:15 AM Jeff King <peff@peff.net> wrote:
> >
> > On Thu, Jan 09, 2020 at 07:20:09PM -0500, Eyal Soha wrote:
> >
> > > > 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.
> >
> > That makes sense. I'm not too surprised to see a terminal that supports
> > one but not the other.
> >
> > But I wonder if there are ones that go the other way around: supporting
> > 256-color mode but not ANSI 90-97, etc. I doubt it, but I think it would
> > be nice to split that change out into a separate commit in case we do
> > run into problems.
> >
> > > Done.  Here's a new patch!
> >
> > Thanks. The content here is looking pretty good (though I have a few
> > comments below).
> >
> > Can you also format it as described in Documentation/SubmittingPatches
> > and re-send? In particular, it needs a regular commit message and your
> > signoff.
> >
> > > +enum {
> > > +       COLOR_BACKGROUND_OFFSET = 10,
> > > +       COLOR_FOREGROUND_ANSI = 30,
> > > +       COLOR_FOREGROUND_RGB = 38,
> > > +       COLOR_FOREGROUND_256 = 38,
> > > +       COLOR_FOREGROUND_BRIGHT_ANSI = 90,
> > > +};
> >
> > The split in this enum mostly makes sense to me. It changes the meaning
> > of "value" for COLOR_ANSI, but this is all local to color.c, so your
> > changes to output_color() are all that's needed.
> >
> > It might be easier to follow the patch if switching to this enum came in
> > a separate preparatory patch.
> >
> > > @@ -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;
> > >                 }
> >
> > It would be slightly simpler and more efficient handle the "+" outside
> > the loop, like:
> >
> >   int offset = COLOR_FOREGROUND_ANSI;
> >   if (*name == '+') {
> >           offset = COLOR_FOREGROUND_BRIGHT_ANSI;
> >           name++;
> >           len--;
> >   }
> >
> > and then in the loop just set "out->value = i + offset".
> >
> > You'd probably want to hoist this out to a separate function since
> > "name" needs to be unchanged in the later part of the function.
> >
> > I dunno. It's almost certainly an unmeasurable micro-optimization, but
> > somehow the flow of it seems simpler to me.
> >
> > I also wondered if we'd want English aliases like "brightred" that some
> > other systems use. It would be easy to add that to the check I showed
> > above without having to repeat as much.
> >
> > > @@ -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;
> >
> > And I think this 8-15 handling might want to be a separate commit on
> > top, just because it's possible it might regress some cases (though
> > again, I do doubt it).
> >
> > >         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);
> >
> > Looks like some unwanted tab/space conversion (here and elsewhere).
> >
> > > +test_expect_success 'axiterm bright fg color' '
> > > +       color "+red" "[91m"
> >
> > s/axi/aix/ (here and below).
> >
> > > +test_expect_success '8-15 are aliases for aixterm color names' '
> > > +       color "12 13" "[94;105m"
> >
> > Makes sense.
> >
> > -Peff

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Jeff King]

On Fri, Jan 10, 2020 at 10:05:45AM -0500, Eyal Soha wrote:

> Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
> ---
>  color.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)

The patch itself looks good, but it would be nice to have a commit
message explaining what's going on and why. Maybe something like:

  Using an enum reduces the number of magic numbers in the code. Note
  that we also use values that allow easier computations. For example,
  colors with type COLOR_ANSI now store the actual foreground code we
  expect (e.g., red=31) instead of an offset into our array, and we can
  switch between foreground/background for all types by adding an
  offset. That will help when we add new color types in a future patch.

> @@ -280,13 +286,13 @@ int color_parse_mem(const char *value, int value_len, char *dst)
>  			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);
>  		}

Your original dropped the comments here, since we're not saying "3" or
"4" literally anymore. I could go either way, but I think I slightly
preferred dropping them. It might be even nicer if we had
COLOR_FOREGROUND_OFFSET = 0, which you could use instead of the bare
"0".

-Peff

Re: [PATCH 2/3] color.c: Support bright aixterm colors

[Jeff King]

On Fri, Jan 10, 2020 at 10:05:46AM -0500, Eyal Soha wrote:

> These colors are the bright variants of the 3-bit colors.

It might be worth noting a few things we discussed. In particular:

  These can generally already be accessed as colors 8-15 of 256-color
  mode. But some terminals support these 16-color versions without
  supporting 256-color mode. And they're fewer bytes, which can make the
  output slightly more efficient.

>  color.c          | 30 +++++++++++++++++++++++-------
>  t/t4026-color.sh |  8 ++++++++
>  2 files changed, 31 insertions(+), 7 deletions(-)

I think we'd need a documentation change, too. These are discussed in
Documentation/config.txt (search for the "color::" heading).

> +	int color_offset = COLOR_FOREGROUND_ANSI;
> +	if (strncasecmp(name, "bright", 6) == 0) {
> +		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
> +		name += 6;
> +		len -= 6;
> +	}

This drops the "+" version. I think we _could_ do both, but just having
"bright" is probably fine.

Having to repeat "6" isn't ideal, but we sadly don't have a
case-insensitive version of skip_prefix(). We could do:

  static const char bright[] = "bright";
  ...

  if (istarts_with(name, bright)) {
	color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
	name += strlen(bright);
	len -= strlen(bright);
  }

but I'm not sure if it's worth it.

> +	for (int i = 0; i < ARRAY_SIZE(color_names); i++) {
> +		if (match_word(name, len, color_names[i])) {
> +			out->type = COLOR_ANSI;
> +			out->value = i + color_offset;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}

The 0/1 return here is unusual for our codebase. We'd usually return "0"
for success and "-1" for failure.

Otherwise, the patch looks good.

-Peff

Re: [PATCH 3/3] color.c: Alias RGB colors 8-15 to aixterm colors

[Jeff King]

On Fri, Jan 10, 2020 at 10:05:47AM -0500, Eyal Soha wrote:

> Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>

Some rationale might be helpful for people who find this commit later
(especially if it ends up being a regression for people with 256-color
support but not 16-color). Maybe:

  This results in shorter output, and is _probably_ more portable. There
  is at least one environment (GitHub Actions) which supports 16-color
  mode but not 256-color mode. It's possible there are environments
  which go the other way, but it seems unlikely.

>  color.c          | 7 ++++++-
>  t/t4026-color.sh | 4 ++++

The code itself looks good. We don't currently document the magic of
0-7, so we don't need to change the documentation there (though perhaps
we ought to).

-Peff

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

This space looks a bit underexplained.  I cannot tell, for example,
if the changes to out->value this patch makes (i.e. in COLOR_ANSI
mode, we used to use 0-7, but now it is offset to 30-37) is
intended.

It is especially confusing that parse_color() stuffs 30-37 in
the .value field for COLOR_ANSI mode, and then color_output() takes
a struct color and then uses xsnprintf() on .value PLUS offset.  The
offset used to be "type" that was either "3" or "4", and now it is
either 0 or 10 (= COLOR_BACKGROUND_OFFSET), so it cancels out, but
when this step is viewed alone, it is not clear why the updated code
does not use 0-7 in .value and 30 or 40 in the offset, which is more
in line with how the original code worked.

In any case, "COLOR_BACKGROUND_OFFSET = 10" needs to be commented
much better---something like "In 'struct color', we chose to
represent the color value in the .value field with the ANSI
foreground color number between 30 and 37, and adding this offset
value makes the color to their background counterparts".

Not that I agree with the (untold) reasoning why we chose to use
30-37 instead of 0-7, though.  If this were up to me, I would have
rather defined COLOR_BACKGROUND_ANSI = 40, kept .value to 0-7 and
passed COLOR_{FORE,BACK}GROUPD_ANSI to callers of color_output().

Since I haven't seen 2/3 and 3/3, perhaps there is a good reason why
this step was done this way instead, though, I guess.

> +enum {
> +	COLOR_BACKGROUND_OFFSET = 10,
> +	COLOR_FOREGROUND_ANSI = 30,
> +	COLOR_FOREGROUND_RGB = 38,
> +	COLOR_FOREGROUND_256 = 38,
> +};
> +
>  /* Ignore the RESET at the end when giving the size */
>  const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
>  
> @@ -92,7 +99,7 @@ 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;
>  		}
>  	}
> @@ -112,7 +119,7 @@ static int parse_color(struct color *out, const char *name, int len)
>  		/* Rewrite low numbers as more-portable standard colors. */

This comment, being in the context, obviously is not a new problem
introduced by this patch, but my reading hiccupped on the verb
"rewrite"---what are we rewriting?---and had to read the logic twice
to realize that this comment is about choosing COLOR_ANSI over
COLOR_256 (i.e. we assume ANSI is more prevalent than 256, so any
color expressible in both is better shown using ANSI).  Perhaps

		/* Express low numbers in basic ANSI rather than 256 */

or something may have been easier to understand at least to me.

Thanks.

>  		} else if (val < 8) {
>  			out->type = COLOR_ANSI;
> -			out->value = val;
> +			out->value = val + COLOR_FOREGROUND_ANSI;
>  			return 0;
>  		} else if (val < 256) {
>  			out->type = COLOR_256;
> @@ -166,23 +173,22 @@ 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,
> +		out += xsnprintf(out, len, "%d;2;%d;%d;%d",
> +				 COLOR_FOREGROUND_RGB + offset,
>  				 c->red, c->green, c->blue);
>  		break;
>  	}
> @@ -280,13 +286,13 @@ int color_parse_mem(const char *value, int value_len, char *dst)
>  			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');
>  	}

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Jeff King]

On Thu, Jan 16, 2020 at 10:01:44AM -0800, Junio C Hamano wrote:

> Not that I agree with the (untold) reasoning why we chose to use
> 30-37 instead of 0-7, though.  If this were up to me, I would have
> rather defined COLOR_BACKGROUND_ANSI = 40, kept .value to 0-7 and
> passed COLOR_{FORE,BACK}GROUPD_ANSI to callers of color_output().
> 
> Since I haven't seen 2/3 and 3/3, perhaps there is a good reason why
> this step was done this way instead, though, I guess.

Yeah, it becomes more clear in patch 2, where the value can be either
"31" or "91", for the bright or non-bright variant, and adding "30" is
wrong. (But certainly I agree this needs to be explained here).

Another way to write it would be to store 0-7 in the value as before,
and then add a separate "bright" flag to "struct color". And then the
output becomes:

  COLOR_FOREGROUND_OFFSET + c->value + (c->bright ? COLOR_BRIGHT_OFFSET : 0)

or similar. One minor confusion there is that COLOR_256 and COLOR_RGB
would ignore the "bright" field.

-Peff

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Eyal Soha]

My original version of the change extended the enum to include both
COLOR_ANSI and COLOR_AIXTERM.  That preserves the 0-7 value and
instead adds more branching to figure out if you want to add 30 or 40
or 90 or 100.  All that extra branching didn't look great so we
instead used COLOR_ANSI for both.

I think that adding a bright flag to the color struct would be a poor
choice because it doesn't mean anything in the context of COLOR_256
and COLOR_RGB, as you've pointed out.

Having an argument to the color_output function called "type" that is
a char is really obtuse, especially considering that c->type exists,
too!  Perhaps the best way would really be to have a boolean argument
called "background" indicating if the color is meant to be foreground
or background and then let color_output do the math to add or not add
10.

Thoughts?

Eyal


Eyal

On Thu, Jan 16, 2020 at 1:23 PM Jeff King <peff@peff.net> wrote:
>
> On Thu, Jan 16, 2020 at 10:01:44AM -0800, Junio C Hamano wrote:
>
> > Not that I agree with the (untold) reasoning why we chose to use
> > 30-37 instead of 0-7, though.  If this were up to me, I would have
> > rather defined COLOR_BACKGROUND_ANSI = 40, kept .value to 0-7 and
> > passed COLOR_{FORE,BACK}GROUPD_ANSI to callers of color_output().
> >
> > Since I haven't seen 2/3 and 3/3, perhaps there is a good reason why
> > this step was done this way instead, though, I guess.
>
> Yeah, it becomes more clear in patch 2, where the value can be either
> "31" or "91", for the bright or non-bright variant, and adding "30" is
> wrong. (But certainly I agree this needs to be explained here).
>
> Another way to write it would be to store 0-7 in the value as before,
> and then add a separate "bright" flag to "struct color". And then the
> output becomes:
>
>   COLOR_FOREGROUND_OFFSET + c->value + (c->bright ? COLOR_BRIGHT_OFFSET : 0)
>
> or similar. One minor confusion there is that COLOR_256 and COLOR_RGB
> would ignore the "bright" field.
>
> -Peff

[PATCH 1/3] color.c: Refactor color_output to use enums

[Eyal Soha]

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/color.c b/color.c
index ebb222ec33..3b734ccffd 100644
--- a/color.c
+++ b/color.c
@@ -24,6 +24,13 @@ 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,
+};
+
 /* Ignore the RESET at the end when giving the size */
 const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
 
@@ -92,7 +99,7 @@ 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;
 		}
 	}
@@ -112,7 +119,7 @@ static int parse_color(struct color *out, const char *name, int len)
 		/* Rewrite low numbers as more-portable standard colors. */
 		} else if (val < 8) {
 			out->type = COLOR_ANSI;
-			out->value = val;
+			out->value = val + COLOR_FOREGROUND_ANSI;
 			return 0;
 		} else if (val < 256) {
 			out->type = COLOR_256;
@@ -166,23 +173,26 @@ 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 background)
 {
+	int offset = 0;
+	if (background) {
+		offset = COLOR_BACKGROUND_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,
+		out += xsnprintf(out, len, "%d;2;%d;%d;%d",
+				 COLOR_FOREGROUND_RGB + offset,
 				 c->red, c->green, c->blue);
 		break;
 	}
@@ -279,14 +289,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, 1);
 		}
 		OUT('m');
 	}
-- 
2.25.0.rc2.3.g8712c6e7f0

[PATCH 2/3] color.c: Support bright aixterm colors

[Eyal Soha]

These colors are the bright variants of the 3-bit colors.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 Documentation/config.txt |  4 +++-
 color.c                  | 34 +++++++++++++++++++++++++++-------
 t/t4026-color.sh         |  8 ++++++++
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83e7bba872..08b13ba72b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -263,7 +263,9 @@ color::
 +
 The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
 `blue`, `magenta`, `cyan` and `white`.  The first color given is the
-foreground; the second is the background.
+foreground; the second is the background.  All the basic colors except
+`normal` have a bright variant that can be speficied by prefixing the
+color with `bright`, like `brightred`.
 +
 Colors may also be given as numbers between 0 and 255; these use ANSI
 256-color mode (but note that not all terminals may support this).  If
diff --git a/color.c b/color.c
index 3b734ccffd..66d32e1191 100644
--- a/color.c
+++ b/color.c
@@ -29,6 +29,7 @@ enum {
 	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 */
@@ -68,13 +69,36 @@ static int get_hex_color(const char *in, unsigned char *out)
 	return 0;
 }
 
-static int parse_color(struct color *out, const char *name, int len)
+/*
+ * If an ANSI color is recognized in "name", fill "out" and return 0.
+ * Otherwise, leave out unchanged and return -1.
+ */
+static int parse_ansi_color(struct color *out, const char *name, int len)
 {
 	/* Positions in array must match ANSI color codes */
 	static const char * const color_names[] = {
 		"black", "red", "green", "yellow",
 		"blue", "magenta", "cyan", "white"
 	};
+
+	int color_offset = COLOR_FOREGROUND_ANSI;
+	if (strncasecmp(name, "bright", 6) == 0) {
+		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
+		name += 6;
+		len -= 6;
+	}
+	for (int i = 0; i < ARRAY_SIZE(color_names); i++) {
+		if (match_word(name, len, color_names[i])) {
+			out->type = COLOR_ANSI;
+			out->value = i + color_offset;
+			return 0;
+		}
+	}
+	return -1;
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
 	char *end;
 	int i;
 	long val;
@@ -96,12 +120,8 @@ static int parse_color(struct color *out, const char *name, int len)
 	}
 
 	/* Then pick from our human-readable color names... */
-	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		if (match_word(name, len, color_names[i])) {
-			out->type = COLOR_ANSI;
-			out->value = i + COLOR_FOREGROUND_ANSI;
-			return 0;
-		}
+	if (parse_ansi_color(out, name, len) == 0) {
+		return 0;
 	}
 
 	/* And finally try a literal 256-color-mode number */
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..78c69de90a 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 'aixterm bright fg color' '
+	color "brightred" "[91m"
+'
+
+test_expect_success 'aixterm bright bg color' '
+	color "green brightblue" "[32;104m"
+'
+
 test_expect_success 'color name before attribute' '
 	color "red bold" "[1;31m"
 '
-- 
2.25.0.rc2.3.g8712c6e7f0

[PATCH 3/3] color.c: Alias RGB colors 8-15 to aixterm colors

[Eyal Soha]

This results in shorter output, and is _probably_ more portable. There
is at least one environment (GitHub Actions) which supports 16-color
mode but not 256-color mode. It's possible there are environments
which go the other way, but it seems unlikely.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c          | 7 ++++++-
 t/t4026-color.sh | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index 66d32e1191..ba067ed97a 100644
--- a/color.c
+++ b/color.c
@@ -136,11 +136,16 @@ 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 + 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;
 			out->value = val;
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 78c69de90a..c0b642c1ab 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -82,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"
 '
-- 
2.25.0.rc2.3.g8712c6e7f0

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> Subject: Re: [PATCH 1/3] color.c: Refactor color_output to use enums

Please downcase Refactor; that way this change would not
meaninglessly stand out in the "git shortlog --no-merges" output.

> Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
> ---
>  color.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)

You got help from multiple reviewers on your earlier round and at
least the discussion thread would have made it clear what kind of
things would help readers understand what design decision was made
(e.g. .value is not 0-7 but 30-37, the caller passes "am I talking
about the background color?" boolean, there are others) and why this
design was chosen (e.g. .value is not 0-7 because we want to add
brighter variant later, there would be others that correspond to the
"here are the design decisions I made before coming up with this
version").

The reviewing is *not* just about explaining your thought to and
convincing your reviewers---it is where reviewers help you to
explain what your change wanted to do and why it did so to future
readers of "git log".

The blank before your sign-off means all the times spent gets
discarded, which is not exactly encouraging to the reviewers.  

Re: [PATCH 3/3] color.c: Alias RGB colors 8-15 to aixterm colors

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> This results in shorter output, and is _probably_ more portable. There
> is at least one environment (GitHub Actions) which supports 16-color
> mode but not 256-color mode. It's possible there are environments
> which go the other way, but it seems unlikely.

Nicely explained.

>
> Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
> ---
>  color.c          | 7 ++++++-
>  t/t4026-color.sh | 4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/color.c b/color.c
> index 66d32e1191..ba067ed97a 100644
> --- a/color.c
> +++ b/color.c
> @@ -136,11 +136,16 @@ 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 + 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;
>  			out->value = val;
> diff --git a/t/t4026-color.sh b/t/t4026-color.sh
> index 78c69de90a..c0b642c1ab 100755
> --- a/t/t4026-color.sh
> +++ b/t/t4026-color.sh
> @@ -82,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"
>  '

Re: [PATCH 2/3] color.c: Support bright aixterm colors

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> These colors are the bright variants of the 3-bit colors.

OK, so this round the design is to reuse the ANSI mode instead of
introducing a new AIX mode that sits next to ANSI, 256 and RGB?

For this to work, not just the 90-97 range for bright-ansi orders
the colors the same way as 30-37 range (only brighter), but also
the differences between corresponding fore- and background colors
must also be 10 just like the regular ANSI colors.

So perhaps an additional sentence or two deserve to be there, e.g.

	... of the 3-bit colors.  Instead of 30-37 range for the
	foreground and 40-47 range for the background, they live in
	90-97 and 100-107 range, respectively.

or something like that, perhaps?

>  The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
>  `blue`, `magenta`, `cyan` and `white`.  The first color given is the
> -foreground; the second is the background.
> +foreground; the second is the background.  All the basic colors except
> +`normal` have a bright variant that can be speficied by prefixing the
> +color with `bright`, like `brightred`.

Nicely and readably written.

I have to wonder if spelling "bright<color>", i.e. two words smashed
together without anything in between words, is in widespread use (in
other words, are we following an established practice, or are we
inventing our own), or if we need to prepare for synonyms?  HTML/CSS
folks seem to use words-smashed-without-anything-in-betwen so they
should be fine with this design; I no longer recall what X did ;-)

Looking good.  Thanks.

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Eyal Soha]

On Sat, Jan 18, 2020 at 9:51 AM Junio C Hamano <gitster@pobox.com> wrote:
> Please downcase Refactor; that way this change would not
> meaninglessly stand out in the "git shortlog --no-merges" output.

Sure, no problem.

> The blank before your sign-off means all the times spent gets
> discarded, which is not exactly encouraging to the reviewers.

So I should make a better description for the patch?  Sure!  What
should I put?  It's kind of hard to get a good description that
describes the refactoring without digging into the reasoning behind
it, which is in the follow-up patch.  What kind of description should
I give?  How about like this:

    color.c: refactor color_output arguments

    color_output() now uses a more descriptive "background" argument
    instead of "type".

    Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>

Suits?

Re: [PATCH 2/3] color.c: Support bright aixterm colors

[Eyal Soha]

On Sat, Jan 18, 2020 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> OK, so this round the design is to reuse the ANSI mode instead of
> introducing a new AIX mode that sits next to ANSI, 256 and RGB?

Right.  Previously I had it with a new AIX enum parallel to ANSI, 256,
etc, but it just made the code longer for no good reason.

> For this to work, not just the 90-97 range for bright-ansi orders
> the colors the same way as 30-37 range (only brighter), but also
> the differences between corresponding fore- and background colors
> must also be 10 just like the regular ANSI colors.

Yes.  It's a happy coincidence that the background version is always
10 greater than the foreground version, for ANSI, for AIX, and even
for the 256-bit colors.   The code takes advantage of that.  If that
later proves to be not true, color_output needs to be modified.
However!, the modification would be just in color_output because the
input is now a boolean "background" instead of the previous char
"type".  I think that's a good improvement so that the caller of
color_output doesn't need to know that, ie, '3' is foreground and '4'
is background.

>
> So perhaps an additional sentence or two deserve to be there, e.g.
>
>         ... of the 3-bit colors.  Instead of 30-37 range for the
>         foreground and 40-47 range for the background, they live in
>         90-97 and 100-107 range, respectively.

Will do.

>
> or something like that, perhaps?
>
> >  The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
> >  `blue`, `magenta`, `cyan` and `white`.  The first color given is the
> > -foreground; the second is the background.
> > +foreground; the second is the background.  All the basic colors except
> > +`normal` have a bright variant that can be speficied by prefixing the
> > +color with `bright`, like `brightred`.
>
> Nicely and readably written.

Thanks.  I tried to keep the voice in line with the rest of the text.

>
> I have to wonder if spelling "bright<color>", i.e. two words smashed
> together without anything in between words, is in widespread use (in
> other words, are we following an established practice, or are we
> inventing our own), or if we need to prepare for synonyms?  HTML/CSS
> folks seem to use words-smashed-without-anything-in-betwen so they
> should be fine with this design; I no longer recall what X did ;-)

/usr/local/lib/X11/rgb.txt often uses smashed together:
https://github.com/vim/vim/blob/master/runtime/rgb.txt  Wikipedia
calls them "bright" consistently:
https://en.wikipedia.org/wiki/ANSI_escape_code#Colors .  So we've got
a vote for smashing them together and a vote for "bright".  Seems okay
by me!

Eyal

[PATCH 1/3] color.c: refactor color_output arguments

[Eyal Soha]

color_output() now uses a more descriptive "background" argument
instead of "type".

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/color.c b/color.c
index ebb222ec33..3b734ccffd 100644
--- a/color.c
+++ b/color.c
@@ -24,6 +24,13 @@ 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,
+};
+
 /* Ignore the RESET at the end when giving the size */
 const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
 
@@ -92,7 +99,7 @@ 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;
 		}
 	}
@@ -112,7 +119,7 @@ static int parse_color(struct color *out, const char *name, int len)
 		/* Rewrite low numbers as more-portable standard colors. */
 		} else if (val < 8) {
 			out->type = COLOR_ANSI;
-			out->value = val;
+			out->value = val + COLOR_FOREGROUND_ANSI;
 			return 0;
 		} else if (val < 256) {
 			out->type = COLOR_256;
@@ -166,23 +173,26 @@ 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 background)
 {
+	int offset = 0;
+	if (background) {
+		offset = COLOR_BACKGROUND_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,
+		out += xsnprintf(out, len, "%d;2;%d;%d;%d",
+				 COLOR_FOREGROUND_RGB + offset,
 				 c->red, c->green, c->blue);
 		break;
 	}
@@ -279,14 +289,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, 1);
 		}
 		OUT('m');
 	}
-- 
2.25.0.rc2.3.g8712c6e7f0

[PATCH 2/3] color.c: support bright aixterm colors

[Eyal Soha]

These colors are the bright variants of the 3-bit colors.  Instead of
30-37 range for the foreground and 40-47 range for the background,
they live in 90-97 and 100-107 range, respectively.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 Documentation/config.txt |  4 +++-
 color.c                  | 34 +++++++++++++++++++++++++++-------
 t/t4026-color.sh         |  8 ++++++++
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83e7bba872..08b13ba72b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -263,7 +263,9 @@ color::
 +
 The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
 `blue`, `magenta`, `cyan` and `white`.  The first color given is the
-foreground; the second is the background.
+foreground; the second is the background.  All the basic colors except
+`normal` have a bright variant that can be speficied by prefixing the
+color with `bright`, like `brightred`.
 +
 Colors may also be given as numbers between 0 and 255; these use ANSI
 256-color mode (but note that not all terminals may support this).  If
diff --git a/color.c b/color.c
index 3b734ccffd..66d32e1191 100644
--- a/color.c
+++ b/color.c
@@ -29,6 +29,7 @@ enum {
 	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 */
@@ -68,13 +69,36 @@ static int get_hex_color(const char *in, unsigned char *out)
 	return 0;
 }
 
-static int parse_color(struct color *out, const char *name, int len)
+/*
+ * If an ANSI color is recognized in "name", fill "out" and return 0.
+ * Otherwise, leave out unchanged and return -1.
+ */
+static int parse_ansi_color(struct color *out, const char *name, int len)
 {
 	/* Positions in array must match ANSI color codes */
 	static const char * const color_names[] = {
 		"black", "red", "green", "yellow",
 		"blue", "magenta", "cyan", "white"
 	};
+
+	int color_offset = COLOR_FOREGROUND_ANSI;
+	if (strncasecmp(name, "bright", 6) == 0) {
+		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
+		name += 6;
+		len -= 6;
+	}
+	for (int i = 0; i < ARRAY_SIZE(color_names); i++) {
+		if (match_word(name, len, color_names[i])) {
+			out->type = COLOR_ANSI;
+			out->value = i + color_offset;
+			return 0;
+		}
+	}
+	return -1;
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
 	char *end;
 	int i;
 	long val;
@@ -96,12 +120,8 @@ static int parse_color(struct color *out, const char *name, int len)
 	}
 
 	/* Then pick from our human-readable color names... */
-	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		if (match_word(name, len, color_names[i])) {
-			out->type = COLOR_ANSI;
-			out->value = i + COLOR_FOREGROUND_ANSI;
-			return 0;
-		}
+	if (parse_ansi_color(out, name, len) == 0) {
+		return 0;
 	}
 
 	/* And finally try a literal 256-color-mode number */
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..78c69de90a 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 'aixterm bright fg color' '
+	color "brightred" "[91m"
+'
+
+test_expect_success 'aixterm bright bg color' '
+	color "green brightblue" "[32;104m"
+'
+
 test_expect_success 'color name before attribute' '
 	color "red bold" "[1;31m"
 '
-- 
2.25.0.rc2.3.g8712c6e7f0

[PATCH 3/3] color.c: alias RGB colors 8-15 to aixterm colors

[Eyal Soha]

This results in shorter output, and is _probably_ more portable. There
is at least one environment (GitHub Actions) which supports 16-color
mode but not 256-color mode. It's possible there are environments
which go the other way, but it seems unlikely.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c          | 7 ++++++-
 t/t4026-color.sh | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index 66d32e1191..ba067ed97a 100644
--- a/color.c
+++ b/color.c
@@ -136,11 +136,16 @@ 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 + 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;
 			out->value = val;
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 78c69de90a..c0b642c1ab 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -82,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"
 '
-- 
2.25.0.rc2.3.g8712c6e7f0

Re: [PATCH 2/3] color.c: Support bright aixterm colors

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

>> I have to wonder if spelling "bright<color>", i.e. two words smashed
>> together without anything in between words, is in widespread use (in
>> other words, are we following an established practice, or are we
>> inventing our own), or if we need to prepare for synonyms?  HTML/CSS
>> folks seem to use words-smashed-without-anything-in-betwen so they
>> should be fine with this design; I no longer recall what X did ;-)
>
> /usr/local/lib/X11/rgb.txt often uses smashed together:
> https://github.com/vim/vim/blob/master/runtime/rgb.txt  Wikipedia
> calls them "bright" consistently:
> https://en.wikipedia.org/wiki/ANSI_escape_code#Colors .  So we've got
> a vote for smashing them together and a vote for "bright".  Seems okay
> by me!

OK.  By synonym, I did not mean any word other than "bright"; in the
context of my response, "brightred", "bright red" and "bright-red"
would have been synonyms, but I think it is sufficient to support
only the first one.

Of course, somebody may come up with a bright idea to treat the
"bright" adjective just like "underline" and "bold" and that might
improve the end user experience slightly better, but I dunno ;-)

Re: [PATCH 1/3] color.c: Refactor color_output to use enums

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> On Sat, Jan 18, 2020 at 9:51 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Please downcase Refactor; that way this change would not
>> meaninglessly stand out in the "git shortlog --no-merges" output.
>
> Sure, no problem.
>
>> The blank before your sign-off means all the times spent gets
>> discarded, which is not exactly encouraging to the reviewers.
>
> So I should make a better description for the patch?  Sure!  What
> should I put?  It's kind of hard to get a good description that
> describes the refactoring without digging into the reasoning behind
> it, which is in the follow-up patch.  What kind of description should
> I give?  How about like this:
>
>     color.c: refactor color_output arguments
>
>     color_output() now uses a more descriptive "background" argument
>     instead of "type".
>
>     Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
>
> Suits?

Quite a lot is missing from these two lines what I mentioned as
examples in the part you omitted from your quote, I think.

 - what design decision was made?  e.g. how .value is expressed
   differently from the code before this patch, e.g. how "fore/back"
   information is passed from the caller differently between the
   code before and after this patch, etc.

 - why these design choices are good ones?  e.g. making .value 30-37
   range instead of 0-7 range and pass 0/10 as offset from the base
   foreground value when the caller wants to give background color
   allows us to do X better than the original arrangement?

Perhaps there are some other things we discussed in the review
thread that may be worth resurrecting, but at least I recall I had
trouble understanding why you chose to do things the way the patch
did for the above two points.

After all, anything that reviewers needed help in their first
reading with your explanation to understand is a good candidate
[*1*] that needs clarification to help future readers of the "git
show" output of the commit resulting from your final version of the
patch.

Thanks.


[Footnote]

*1* There of course are cases where a simple explanation results in
    a reviewer who was initially confused to say "Ah, I misread a
    word, but your original is good after I re-read it carefully",
    so not everything a reviewer gets confused necessarily deserves
    mention in the final version of the log message.  But these are
    good starting points to anticipate confusion by future readers.

Re: [PATCH 1/3] color.c: refactor color_output arguments

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> @@ -166,23 +173,26 @@ 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 background)
>  {
> +	int offset = 0;

Have a blank line here, between the end of decl/defn and the first stmt.

> +	if (background) {
> +		offset = COLOR_BACKGROUND_OFFSET;
> +	}

No {} around a single statement block.

>  	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 the line after the format string instead, i.e.

> +		out += xsnprintf(out, len, "%d;5;%d",
> +				 COLOR_FOREGROUND_256 + offset, c->value);

that would make it easier to see which parameter corresponds to
which placeholder and also saves line width at the same time.

>  		break;
>  	case COLOR_RGB:
> -		out += xsnprintf(out, len, "%c8;2;%d;%d;%d", type,
> +		out += xsnprintf(out, len, "%d;2;%d;%d;%d",
> +				 COLOR_FOREGROUND_RGB + offset,
>  				 c->red, c->green, c->blue);

... like you did here.

Re: [PATCH 2/3] color.c: support bright aixterm colors

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> +static int parse_ansi_color(struct color *out, const char *name, int len)
>  {
>  	/* Positions in array must match ANSI color codes */
>  	static const char * const color_names[] = {
>  		"black", "red", "green", "yellow",
>  		"blue", "magenta", "cyan", "white"
>  	};
> +
> +	int color_offset = COLOR_FOREGROUND_ANSI;

No need for the blank line before this line, but a blank line after
the last decl/defn is a good idea.  You'd need to define "int i"
here, too, as...

> +	if (strncasecmp(name, "bright", 6) == 0) {
> +		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
> +		name += 6;
> +		len -= 6;
> +	}
> +	for (int i = 0; i < ARRAY_SIZE(color_names); i++) {

... we do not (unfortunately) allow declaring a variable in the
set-up part of a for loop yet (see Documentation/CodingGuidelines).

> +static int parse_color(struct color *out, const char *name, int len)
> +{
>  	char *end;
>  	int i;
>  	long val;

With the removal of the loop, "int i" no longer is used in this
function.  Remove its defn here.

> @@ -96,12 +120,8 @@ static int parse_color(struct color *out, const char *name, int len)
>  	}
>  
>  	/* Then pick from our human-readable color names... */
> -	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
> -		if (match_word(name, len, color_names[i])) {
> -			out->type = COLOR_ANSI;
> -			out->value = i + COLOR_FOREGROUND_ANSI;
> -			return 0;
> -		}
> +	if (parse_ansi_color(out, name, len) == 0) {
> +		return 0;
>  	}

Thanks.

[PATCH v3 0/3] es/bright-colors (hopefully final) reroll

[Junio C Hamano]

As I've got tired of waiting for an update for the finishing touches
to this topic that has otherwise been polished adequately, I've
applied all the changes suggested during the review and also updated
the explanation in the first step.

Eyal Soha (3):
  color.c: refactor color_output arguments
  color.c: support bright aixterm colors
  color.c: alias RGB colors 8-15 to aixterm colors

 Documentation/config.txt |  4 ++-
 color.c                  | 75 +++++++++++++++++++++++++++++-----------
 t/t4026-color.sh         | 12 +++++++
 3 files changed, 69 insertions(+), 22 deletions(-)

-- 
2.25.0-453-g769eb9f0f1

[PATCH v3 1/3] color.c: refactor color_output arguments

[Junio C Hamano]

From: Eyal Soha <shawarmakarma@gmail.com>

color_output() takes a "type" parameter, which is either '3' or '4',
and that byte is shown in front of '0'-'7' to form "30"-"37" or
"40"-"47" in ANSI output mode for fore-ground and back-ground
colors.

Clarify the purpose of the parameter by renaming it to the
"background" that is a boolean.

Also, change the .value field in the color struct from storing 0-7
for basic 8 colors to storing 30-37 for ANSI colors.  This aligns
the code to show ANSI colors to the code for the 256 color scheme,
which already uses the actual value to be sent to the terminal.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 color.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/color.c b/color.c
index ebb222ec33..4ee690bd4e 100644
--- a/color.c
+++ b/color.c
@@ -24,6 +24,13 @@ 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,
+};
+
 /* Ignore the RESET at the end when giving the size */
 const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
 
@@ -92,7 +99,7 @@ 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;
 		}
 	}
@@ -112,7 +119,7 @@ static int parse_color(struct color *out, const char *name, int len)
 		/* Rewrite low numbers as more-portable standard colors. */
 		} else if (val < 8) {
 			out->type = COLOR_ANSI;
-			out->value = val;
+			out->value = val + COLOR_FOREGROUND_ANSI;
 			return 0;
 		} else if (val < 256) {
 			out->type = COLOR_256;
@@ -166,23 +173,26 @@ 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 background)
 {
+	int offset = 0;
+
+	if (background)
+		offset = COLOR_BACKGROUND_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,
+		out += xsnprintf(out, len, "%d;2;%d;%d;%d",
+				 COLOR_FOREGROUND_RGB + offset,
 				 c->red, c->green, c->blue);
 		break;
 	}
@@ -279,14 +289,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, 1);
 		}
 		OUT('m');
 	}
-- 
2.25.0-453-g769eb9f0f1

[PATCH v3 2/3] color.c: support bright aixterm colors

[Junio C Hamano]

From: Eyal Soha <shawarmakarma@gmail.com>

These colors are the bright variants of the 3-bit colors.  Instead of
30-37 range for the foreground and 40-47 range for the background,
they live in 90-97 and 100-107 range, respectively.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |  4 +++-
 color.c                  | 36 ++++++++++++++++++++++++++++--------
 t/t4026-color.sh         |  8 ++++++++
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83e7bba872..08b13ba72b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -263,7 +263,9 @@ color::
 +
 The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
 `blue`, `magenta`, `cyan` and `white`.  The first color given is the
-foreground; the second is the background.
+foreground; the second is the background.  All the basic colors except
+`normal` have a bright variant that can be speficied by prefixing the
+color with `bright`, like `brightred`.
 +
 Colors may also be given as numbers between 0 and 255; these use ANSI
 256-color mode (but note that not all terminals may support this).  If
diff --git a/color.c b/color.c
index 4ee690bd4e..0c0ec4672f 100644
--- a/color.c
+++ b/color.c
@@ -29,6 +29,7 @@ enum {
 	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 */
@@ -68,15 +69,38 @@ static int get_hex_color(const char *in, unsigned char *out)
 	return 0;
 }
 
-static int parse_color(struct color *out, const char *name, int len)
+/*
+ * If an ANSI color is recognized in "name", fill "out" and return 0.
+ * Otherwise, leave out unchanged and return -1.
+ */
+static int parse_ansi_color(struct color *out, const char *name, int len)
 {
 	/* Positions in array must match ANSI color codes */
 	static const char * const color_names[] = {
 		"black", "red", "green", "yellow",
 		"blue", "magenta", "cyan", "white"
 	};
-	char *end;
 	int i;
+	int color_offset = COLOR_FOREGROUND_ANSI;
+
+	if (strncasecmp(name, "bright", 6) == 0) {
+		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
+		name += 6;
+		len -= 6;
+	}
+	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
+		if (match_word(name, len, color_names[i])) {
+			out->type = COLOR_ANSI;
+			out->value = i + color_offset;
+			return 0;
+		}
+	}
+	return -1;
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
+	char *end;
 	long val;
 
 	/* First try the special word "normal"... */
@@ -96,12 +120,8 @@ static int parse_color(struct color *out, const char *name, int len)
 	}
 
 	/* Then pick from our human-readable color names... */
-	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		if (match_word(name, len, color_names[i])) {
-			out->type = COLOR_ANSI;
-			out->value = i + COLOR_FOREGROUND_ANSI;
-			return 0;
-		}
+	if (parse_ansi_color(out, name, len) == 0) {
+		return 0;
 	}
 
 	/* And finally try a literal 256-color-mode number */
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..78c69de90a 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 'aixterm bright fg color' '
+	color "brightred" "[91m"
+'
+
+test_expect_success 'aixterm bright bg color' '
+	color "green brightblue" "[32;104m"
+'
+
 test_expect_success 'color name before attribute' '
 	color "red bold" "[1;31m"
 '
-- 
2.25.0-453-g769eb9f0f1

[PATCH v3 3/3] color.c: alias RGB colors 8-15 to aixterm colors

[Junio C Hamano]

From: Eyal Soha <shawarmakarma@gmail.com>

This results in shorter output, and is _probably_ more portable. There
is at least one environment (GitHub Actions) which supports 16-color
mode but not 256-color mode. It's possible there are environments
which go the other way, but it seems unlikely.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 color.c          | 7 ++++++-
 t/t4026-color.sh | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index 0c0ec4672f..64f52a4f93 100644
--- a/color.c
+++ b/color.c
@@ -136,11 +136,16 @@ 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 + 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;
 			out->value = val;
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 78c69de90a..c0b642c1ab 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -82,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"
 '
-- 
2.25.0-453-g769eb9f0f1

Re: [PATCH v3 1/3] color.c: refactor color_output arguments

[Jeff King]

On Tue, Feb 11, 2020 at 11:36:23AM -0800, Junio C Hamano wrote:

> +enum {
> +	COLOR_BACKGROUND_OFFSET = 10,
> +	COLOR_FOREGROUND_ANSI = 30,
> +	COLOR_FOREGROUND_RGB = 38,
> +	COLOR_FOREGROUND_256 = 38,
> +};

I had to double-check to make sure the duplication in the last two
wasn't a bug. It's correct, because "38" here is really "set the
foreground color", and they're followed by more magic for "256" or
"RGB".

So really this could be a single COLOR_FOREGROUND_EXTENDED or similar
that gets used in both places. But I don't know that it really matters
that much.

Other than that nitpick, the patches all looked OK to me. Thanks for
tying up this loose end.

-Peff

Re: [PATCH v3 1/3] color.c: refactor color_output arguments

[Eyal Soha]

Had I gotten any of those constants wrong, unit tests would fail.
It's probably on purpose that those values were chosen to be the same
but for us it's just a happy coincidence.

I also have just one value for COLOR_BACKGROUND_OFFSET, because for
both the ANSI colors and the AIXTERM, the difference is 10.  Just
another happy coincidence.  I could have split those into two
constants like with RGB vs 256.

None of those values are likely to ever change.  I think that the most
important feature of the constants is that they are descriptive.

Thanks for your help.

Eyal

On Tue, Feb 11, 2020 at 2:46 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Feb 11, 2020 at 11:36:23AM -0800, Junio C Hamano wrote:
>
> > +enum {
> > +     COLOR_BACKGROUND_OFFSET = 10,
> > +     COLOR_FOREGROUND_ANSI = 30,
> > +     COLOR_FOREGROUND_RGB = 38,
> > +     COLOR_FOREGROUND_256 = 38,
> > +};
>
> I had to double-check to make sure the duplication in the last two
> wasn't a bug. It's correct, because "38" here is really "set the
> foreground color", and they're followed by more magic for "256" or
> "RGB".
>
> So really this could be a single COLOR_FOREGROUND_EXTENDED or similar
> that gets used in both places. But I don't know that it really matters
> that much.
>
> Other than that nitpick, the patches all looked OK to me. Thanks for
> tying up this loose end.
>
> -Peff

Re: [PATCH v3 1/3] color.c: refactor color_output arguments

[Junio C Hamano]

Eyal Soha <shawarmakarma@gmail.com> writes:

> Had I gotten any of those constants wrong, unit tests would fail.

To be fair, the tests only look for hardcoded numeric constants like
91 and 104, so it won't be able to catch a mistake like "oh, we
thought bright red was 91 but actually it was 111" ;-)

> None of those values are likely to ever change.  I think that the most
> important feature of the constants is that they are descriptive.

Yup.  Thanks for bringing up and working on the topic.