From 0111681ecfe4d04a354536e62af1cde7c49e1c40 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:31:44 -0400 Subject: [PATCH 1/7] color: fix max-size comment We use fixed-size buffers for colors, because we know our parsing cannot grow beyond a particular bound. However, our comment description has two issues: 1. It has the description in two forms: a short one, and one with more explanation. Over time the latter has been updated, but the former has not. Let's just drop the short one (after making sure everything it says is in the long one). 2. As of ff40d18 (parse_color: recognize "no$foo" to clear the $foo attribute, 2014-11-20), the per-attribute size bumped to "3" (because "nobold" is actually "21;"). But that's not quite enough, as somebody may use both "bold" and "nobold", requiring 5 characters. This wasn't a problem for the final count, because we over-estimated in other ways, but let's clarify how we got to the final number. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- color.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/color.h b/color.h index 7fe77fb55e..2d0507ff7e 100644 --- a/color.h +++ b/color.h @@ -3,18 +3,20 @@ struct strbuf; -/* 2 + (2 * num_attrs) + 8 + 1 + 8 + 'm' + NUL */ -/* "\033[1;2;4;5;7;38;5;2xx;48;5;2xxm\0" */ /* * The maximum length of ANSI color sequence we would generate: * - leading ESC '[' 2 - * - attr + ';' 3 * 10 (e.g. "1;") + * - attr + ';' 2 * num_attr (e.g. "1;") + * - no-attr + ';' 3 * num_attr (e.g. "22;") * - fg color + ';' 17 (e.g. "38;2;255;255;255;") * - bg color + ';' 17 (e.g. "48;2;255;255;255;") * - terminating 'm' NUL 2 * - * The above overcounts attr (we only use 5 not 8) and one semicolon - * but it is close enough. + * The above overcounts by one semicolon but it is close enough. + * + * The space for attributes is also slightly overallocated, as + * the negation for some attributes is the same (e.g., nobold and nodim). + * We also allocate space for 6 attributes (even though we have only 5). */ #define COLOR_MAXLEN 70 From adb3356664fbf15646fd90eb1d5ddd9e66ce913f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:32:30 -0400 Subject: [PATCH 2/7] doc: refactor description of color format This is a general cleanup of the description of colors in git-config, mostly to address inaccuracies and confusion that had grown over time: - you can have many attributes, not just one - the discussion flip-flopped between colors and attributes; now we discuss everything about colors, then everything about attributes - many concepts were lumped into the first paragraph, making it hard to read, and especially to find the actual lists of colors and attributes. I stopped short of breaking those out into their own lists, as it seemed like an excessive use of vertical screen real estate. - we introduced negated attributes, but then the next paragraph basically explains how each item starts off with no attributes. So why would one need negated attributes? We now explain. - minor typo, language, and typography fixes Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 43 ++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 2e919f0df8..836f731873 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -147,27 +147,32 @@ integer:: 1024", "by 1024x1024", etc. color:: - The value for a variables that takes a color is a list of - colors (at most two) and attributes (at most one), separated - by spaces. The colors accepted are `normal`, `black`, - `red`, `green`, `yellow`, `blue`, `magenta`, `cyan` and - `white`; the attributes are `bold`, `dim`, `ul`, `blink` and - `reverse`. The first color given is the foreground; the - second is the background. The position of the attribute, if - any, doesn't matter. Attributes may be turned off specifically - by prefixing them with `no` (e.g., `noreverse`, `noul`, etc). + The value for a variable that takes a color is a list of + colors (at most two, one for foreground and one for background) + and attributes (as many as you want), separated by spaces. + -Colors (foreground and background) may also be given as numbers between -0 and 255; these use ANSI 256-color mode (but note that not all -terminals may support this). If your terminal supports it, you may also -specify 24-bit RGB values as hex, like `#ff0ab3`. +The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`, +`blue`, `magenta`, `cyan` and `white`. The first color given is the +foreground; the second is the background. + -The attributes are meant to be reset at the beginning of each item -in the colored output, so setting color.decorate.branch to `black` -will paint that branch name in a plain `black`, even if the previous -thing on the same output line (e.g. opening parenthesis before the -list of branch names in `log --decorate` output) is set to be -painted with `bold` or some other attribute. +Colors may also be given as numbers between 0 and 255; these use ANSI +256-color mode (but note that not all terminals may support this). If +your terminal supports it, you may also specify 24-bit RGB values as +hex, like `#ff0ab3`. ++ +The accepted attributes are `bold`, `dim`, `ul`, `blink`, and `reverse`. +The position of any attributes with respect to the colors (before, after, +or in between), doesn't matter. Specific attributes may be turned off +by prefixing them with `no` (e.g., `noreverse`, `noul`, etc). ++ +For git's pre-defined color slots, the attributes are meant to be reset +at the beginning of each item in the colored output. So setting +`color.decorate.branch` to `black` will paint that branch name in a +plain `black`, even if the previous thing on the same output line (e.g. +opening parenthesis before the list of branch names in `log --decorate` +output) is set to be painted with `bold` or some other attribute. +However, custom log formats may do more complicated and layered +coloring, and the negated forms may be useful there. Variables From ae989a61dad98debe9899823ca987305f8e8020d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:33:57 -0400 Subject: [PATCH 3/7] add skip_prefix_mem helper The skip_prefix function has been very useful for simplifying pointer arithmetic and avoiding repeated magic numbers, but we have no equivalent for length-limited buffers. So we're stuck with: if (3 <= len && skip_prefix(buf, "foo", &buf)) len -= 3; That's not that complicated, but it needs to use magic numbers for the length of the prefix (or else write out strlen("foo"), repeating the string). By using a helper, we can get the string length behind the scenes (and often at compile time for string literals). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-compat-util.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 061e33c774..8e808c01d2 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -463,6 +463,23 @@ static inline int skip_prefix(const char *str, const char *prefix, return 0; } +/* + * Like skip_prefix, but promises never to read past "len" bytes of the input + * buffer, and returns the remaining number of bytes in "out" via "outlen". + */ +static inline int skip_prefix_mem(const char *buf, size_t len, + const char *prefix, + const char **out, size_t *outlen) +{ + size_t prefix_len = strlen(prefix); + if (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) { + *out = buf + prefix_len; + *outlen = len - prefix_len; + return 1; + } + return 0; +} + /* * If buf ends with suffix, return 1 and subtract the length of the suffix * from *len. Otherwise, return 0 and leave *len untouched. From df8e472cc1bbd14a60d22b0b124f07046c6e1fa2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:38:13 -0400 Subject: [PATCH 4/7] color: refactor parse_attr The list of attributes we recognize is a bit unwieldy, as we actually have two arrays that must be kept in sync. Instead, let's have a single array-of-struct to represent our mapping. That means we can never have an accident that causes us to read off the end of an array, and it makes diffs for adding new attributes much easier to read. This also makes it easy to handle the "no" cases without having to repeat each attribute (this shortens the list, making it easier to read, but also also cuts the size of our linear search in half). Technically this makes it impossible for us to add an attribute that starts with "no" (we could confuse "nobody" for the negation of "body"), but since this is a constrained set of attributes, that's OK. Since we can also store the length of each name in the struct, that makes it easy for us to avoid reading past the "len" parameter given to us (though in practice it was not a bug, since all of our current callers are interested in a subset of a NUL-terminated buffer, not a true undelimited range of memory). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- color.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/color.c b/color.c index 9027352ad7..63e7b0c9a1 100644 --- a/color.c +++ b/color.c @@ -123,19 +123,30 @@ static int parse_color(struct color *out, const char *name, int len) return -1; } -static int parse_attr(const char *name, int len) +static int parse_attr(const char *name, size_t len) { - static const int attr_values[] = { 1, 2, 4, 5, 7, - 22, 22, 24, 25, 27 }; - static const char * const attr_names[] = { - "bold", "dim", "ul", "blink", "reverse", - "nobold", "nodim", "noul", "noblink", "noreverse" + static const struct { + const char *name; + size_t len; + int val, neg; + } attrs[] = { +#define ATTR(x, val, neg) { (x), sizeof(x)-1, (val), (neg) } + ATTR("bold", 1, 22), + ATTR("dim", 2, 22), + ATTR("ul", 4, 24), + ATTR("blink", 5, 25), + ATTR("reverse", 7, 27) +#undef ATTR }; + int negate = 0; int i; - for (i = 0; i < ARRAY_SIZE(attr_names); i++) { - const char *str = attr_names[i]; - if (!strncasecmp(name, str, len) && !str[len]) - return attr_values[i]; + + if (skip_prefix_mem(name, len, "no", &name, &len)) + negate = 1; + + for (i = 0; i < ARRAY_SIZE(attrs); i++) { + if (attrs[i].len == len && !memcmp(attrs[i].name, name, len)) + return negate ? attrs[i].neg : attrs[i].val; } return -1; } From 5621068f3d3c537b79b76201928c0b06025479ee Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:38:44 -0400 Subject: [PATCH 5/7] color: allow "no-" for negating attributes Using "no-bold" rather than "nobold" is easier to read and more natural to type (to me, anyway, even though I was the person who introduced "nobold" in the first place). It's easy to allow both. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 2 +- color.c | 4 +++- t/t4026-color.sh | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 836f731873..93ecd728ab 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -163,7 +163,7 @@ hex, like `#ff0ab3`. The accepted attributes are `bold`, `dim`, `ul`, `blink`, and `reverse`. The position of any attributes with respect to the colors (before, after, or in between), doesn't matter. Specific attributes may be turned off -by prefixing them with `no` (e.g., `noreverse`, `noul`, etc). +by prefixing them with `no` or `no-` (e.g., `noreverse`, `no-ul`, etc). + For git's pre-defined color slots, the attributes are meant to be reset at the beginning of each item in the colored output. So setting diff --git a/color.c b/color.c index 63e7b0c9a1..a22d835842 100644 --- a/color.c +++ b/color.c @@ -141,8 +141,10 @@ static int parse_attr(const char *name, size_t len) int negate = 0; int i; - if (skip_prefix_mem(name, len, "no", &name, &len)) + if (skip_prefix_mem(name, len, "no", &name, &len)) { + skip_prefix_mem(name, len, "-", &name, &len); negate = 1; + } for (i = 0; i < ARRAY_SIZE(attrs); i++) { if (attrs[i].len == len && !memcmp(attrs[i].name, name, len)) diff --git a/t/t4026-color.sh b/t/t4026-color.sh index 2b32c4fbe6..2065752ff9 100755 --- a/t/t4026-color.sh +++ b/t/t4026-color.sh @@ -50,6 +50,10 @@ test_expect_success 'attr negation' ' color "nobold nodim noul noblink noreverse" "[22;24;25;27m" ' +test_expect_success '"no-" variant of negation' ' + color "no-bold no-blink" "[22;25m" +' + test_expect_success 'long color specification' ' color "254 255 bold dim ul blink reverse" "[1;2;4;5;7;38;5;254;48;5;255m" ' From 54590a0eda10ecfdc39398d662ab3f663491067e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:39:07 -0400 Subject: [PATCH 6/7] color: support "italic" attribute We already support bold, underline, and similar attributes. Let's add italic to the mix. According to the Wikipedia page on ANSI colors, this attribute is "not widely supported", but it does seem to work on my xterm. We don't have to bump the maximum color size because we were already over-allocating it (but we do adjust the comment appropriately). Requested-by: Simon Courtois Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 9 +++++---- color.c | 1 + color.h | 3 ++- t/t4026-color.sh | 5 +++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 93ecd728ab..cea3835c8f 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -160,10 +160,11 @@ Colors may also be given as numbers between 0 and 255; these use ANSI your terminal supports it, you may also specify 24-bit RGB values as hex, like `#ff0ab3`. + -The accepted attributes are `bold`, `dim`, `ul`, `blink`, and `reverse`. -The position of any attributes with respect to the colors (before, after, -or in between), doesn't matter. Specific attributes may be turned off -by prefixing them with `no` or `no-` (e.g., `noreverse`, `no-ul`, etc). +The accepted attributes are `bold`, `dim`, `ul`, `blink`, `reverse`, and +`italic`. The position of any attributes with respect to the colors +(before, after, or in between), doesn't matter. Specific attributes may +be turned off by prefixing them with `no` or `no-` (e.g., `noreverse`, +`no-ul`, etc). + For git's pre-defined color slots, the attributes are meant to be reset at the beginning of each item in the colored output. So setting diff --git a/color.c b/color.c index a22d835842..b6933a16fe 100644 --- a/color.c +++ b/color.c @@ -133,6 +133,7 @@ static int parse_attr(const char *name, size_t len) #define ATTR(x, val, neg) { (x), sizeof(x)-1, (val), (neg) } ATTR("bold", 1, 22), ATTR("dim", 2, 22), + ATTR("italic", 3, 23), ATTR("ul", 4, 24), ATTR("blink", 5, 25), ATTR("reverse", 7, 27) diff --git a/color.h b/color.h index 2d0507ff7e..e6f82cc3eb 100644 --- a/color.h +++ b/color.h @@ -16,7 +16,8 @@ struct strbuf; * * The space for attributes is also slightly overallocated, as * the negation for some attributes is the same (e.g., nobold and nodim). - * We also allocate space for 6 attributes (even though we have only 5). + * + * We allocate space for 6 attributes. */ #define COLOR_MAXLEN 70 diff --git a/t/t4026-color.sh b/t/t4026-color.sh index 2065752ff9..13690f7c31 100755 --- a/t/t4026-color.sh +++ b/t/t4026-color.sh @@ -60,8 +60,9 @@ test_expect_success 'long color specification' ' test_expect_success 'absurdly long color specification' ' color \ - "#ffffff #ffffff bold nobold dim nodim ul noul blink noblink reverse noreverse" \ - "[1;2;4;5;7;22;24;25;27;38;2;255;255;255;48;2;255;255;255m" + "#ffffff #ffffff bold nobold dim nodim italic noitalic + ul noul blink noblink reverse noreverse" \ + "[1;2;3;4;5;7;22;23;24;25;27;38;2;255;255;255;48;2;255;255;255m" ' test_expect_success '0-7 are aliases for basic ANSI color names' ' From 9dc3515cf005639317fb95492b3157aadf056923 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 23 Jun 2016 13:40:16 -0400 Subject: [PATCH 7/7] color: support strike-through attribute This is the only remaining attribute that is commonly supported (at least by xterm) that we don't support. Let's add it for completeness. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 5 +++-- color.c | 3 ++- color.h | 4 ++-- t/t4026-color.sh | 4 ++-- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index cea3835c8f..6882e7065a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -160,8 +160,9 @@ Colors may also be given as numbers between 0 and 255; these use ANSI your terminal supports it, you may also specify 24-bit RGB values as hex, like `#ff0ab3`. + -The accepted attributes are `bold`, `dim`, `ul`, `blink`, `reverse`, and -`italic`. The position of any attributes with respect to the colors +The accepted attributes are `bold`, `dim`, `ul`, `blink`, `reverse`, +`italic`, and `strike` (for crossed-out or "strikethrough" letters). +The position of any attributes with respect to the colors (before, after, or in between), doesn't matter. Specific attributes may be turned off by prefixing them with `no` or `no-` (e.g., `noreverse`, `no-ul`, etc). diff --git a/color.c b/color.c index b6933a16fe..a5b6581215 100644 --- a/color.c +++ b/color.c @@ -136,7 +136,8 @@ static int parse_attr(const char *name, size_t len) ATTR("italic", 3, 23), ATTR("ul", 4, 24), ATTR("blink", 5, 25), - ATTR("reverse", 7, 27) + ATTR("reverse", 7, 27), + ATTR("strike", 9, 29) #undef ATTR }; int negate = 0; diff --git a/color.h b/color.h index e6f82cc3eb..2af1019a60 100644 --- a/color.h +++ b/color.h @@ -17,9 +17,9 @@ struct strbuf; * The space for attributes is also slightly overallocated, as * the negation for some attributes is the same (e.g., nobold and nodim). * - * We allocate space for 6 attributes. + * We allocate space for 7 attributes. */ -#define COLOR_MAXLEN 70 +#define COLOR_MAXLEN 75 /* * IMPORTANT: Due to the way these color codes are emulated on Windows, diff --git a/t/t4026-color.sh b/t/t4026-color.sh index 13690f7c31..ec78c5e3ac 100755 --- a/t/t4026-color.sh +++ b/t/t4026-color.sh @@ -61,8 +61,8 @@ test_expect_success 'long color specification' ' test_expect_success 'absurdly long color specification' ' color \ "#ffffff #ffffff bold nobold dim nodim italic noitalic - ul noul blink noblink reverse noreverse" \ - "[1;2;3;4;5;7;22;23;24;25;27;38;2;255;255;255;48;2;255;255;255m" + ul noul blink noblink reverse noreverse strike nostrike" \ + "[1;2;3;4;5;7;9;22;23;24;25;27;29;38;2;255;255;255;48;2;255;255;255m" ' test_expect_success '0-7 are aliases for basic ANSI color names' '