diff mbox series

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

Message ID 20200110150547.221314-1-shawarmakarma@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3] color.c: Refactor color_output to use enums | expand

Commit Message

Eyal Soha Jan. 10, 2020, 3:05 p.m. UTC 
Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

Jeff King Jan. 15, 2020, 10:33 p.m. UTC | #1 
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
Junio C Hamano Jan. 16, 2020, 6:01 p.m. UTC | #2 
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');
>  	}
Jeff King Jan. 16, 2020, 6:23 p.m. UTC | #3 
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
Eyal Soha Jan. 16, 2020, 7:25 p.m. UTC | #4 
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
diff mbox series

Patch

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