Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eyal Soha <shawarmakarma@gmail.com>
Cc: peff@peff.net, git@vger.kernel.org
Subject: Re: [PATCH 1/3] color.c: Refactor color_output to use enums
Date: Thu, 16 Jan 2020 10:01:44 -0800
Message-ID: <xmqq5zhbi8l3.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20200110150547.221314-1-shawarmakarma@gmail.com> (Eyal Soha's message of "Fri, 10 Jan 2020 10:05:45 -0500")

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

  parent reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CANsz78+ugmd62F4Qk+VT7Pi=ZPtMSkZjXOwLNRCFhoS9jrOkeQ@mail.gmail.com>
     [not found] ` <CANsz78K-BiswWPdhd_N25NuApcv7zSb2cw2Y9DSinkpNpuogYw@mail.gmail.com>
2020-01-07 15:36   ` Fwd: Add support for axiterm colors in parse_color Eyal Soha
2020-01-08  9:52     ` Jeff King
2020-01-10  0:20       ` Eyal Soha
2020-01-10 11:15         ` Jeff King
2020-01-10 15:02           ` Eyal Soha
2020-01-15 15:32             ` Eyal Soha
2020-01-10 15:05           ` [PATCH 1/3] color.c: Refactor color_output to use enums Eyal Soha
2020-01-10 15:05             ` [PATCH 2/3] color.c: Support bright aixterm colors Eyal Soha
2020-01-15 22:42               ` Jeff King
2020-01-10 15:05             ` [PATCH 3/3] color.c: Alias RGB colors 8-15 to " Eyal Soha
2020-01-15 22:45               ` Jeff King
2020-01-15 22:33             ` [PATCH 1/3] color.c: Refactor color_output to use enums Jeff King
2020-01-16 18:01             ` Junio C Hamano [this message]
2020-01-16 18:23               ` Jeff King
2020-01-16 19:25                 ` Eyal Soha

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=xmqq5zhbi8l3.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=shawarmakarma@gmail.com \
    /path/to/YOUR_REPLY

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

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

Git Mailing List Archive on lore.kernel.org

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

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

Example config snippet for mirrors

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


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