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

From: Jeff King <peff@peff.net>
Date: Jan 15, 2020, 22:42 UTC
Message-ID: <20200115224244.GB4117117@coredump.intra.peff.net>
In-Reply-To: <20200110150547.221314-2-shawarmakarma@gmail.com>

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

Show 6 quoted lines

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

Show 9 quoted lines

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