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

Message ID 20200118145318.5177-1-shawarmakarma@gmail.com
State New
Headers show
Series
  • [1/3] color.c: Refactor color_output to use enums
Related show

Commit Message

Eyal Soha Jan. 18, 2020, 2:53 p.m. UTC
Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 color.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Jan. 18, 2020, 5:51 p.m. UTC | #1
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.
Eyal Soha Jan. 21, 2020, 4:37 p.m. UTC | #2
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?
Junio C Hamano Jan. 21, 2020, 5:49 p.m. UTC | #3
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.

Patch
diff mbox series

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