diff mbox series

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

Message ID 20200211193625.231492-2-gitster@pobox.com (mailing list archive)
State New, archived
Headers show
Series es/bright-colors (hopefully final) reroll | expand

Commit Message

Junio C Hamano Feb. 11, 2020, 7:36 p.m. UTC 
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(-)

Comments

Jeff King Feb. 11, 2020, 7:46 p.m. UTC | #1 
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
Eyal Soha Feb. 11, 2020, 11:01 p.m. UTC | #2 
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
Junio C Hamano Feb. 11, 2020, 11:06 p.m. UTC | #3 
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.
diff mbox series

Patch

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