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