* [PATCH v2 1/7] color: fix max-size comment
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
@ 2016-06-23 17:31 ` Jeff King
2016-06-23 17:32 ` [PATCH v2 2/7] doc: refactor description of color format Jeff King
` (5 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:31 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <peff@peff.net>
---
Unchanged from v1.
color.h | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/color.h b/color.h
index e155d13..e24fa0b 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
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 2/7] doc: refactor description of color format
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
2016-06-23 17:31 ` [PATCH v2 1/7] color: fix max-size comment Jeff King
@ 2016-06-23 17:32 ` Jeff King
2016-06-23 17:33 ` [PATCH v2 3/7] add skip_prefix_mem helper Jeff King
` (4 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:32 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <peff@peff.net>
---
Documentation/config.txt | 47 ++++++++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 58673cf..f8460d4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -150,27 +150,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).
-+
-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 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.
+ 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.
++
+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.
++
+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.
pathname::
A variable that takes a pathname value can be given a
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 3/7] add skip_prefix_mem helper
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
2016-06-23 17:31 ` [PATCH v2 1/7] color: fix max-size comment Jeff King
2016-06-23 17:32 ` [PATCH v2 2/7] doc: refactor description of color format Jeff King
@ 2016-06-23 17:33 ` Jeff King
2016-06-23 17:38 ` [PATCH v2 4/7] color: refactor parse_attr Jeff King
` (3 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:33 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <peff@peff.net>
---
Obviously a helper to be used later in the series. I didn't hunt around
for other places that could make use of it, but I suspect there are
some.
git-compat-util.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 49d4029..c99cddc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -473,6 +473,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.
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 4/7] color: refactor parse_attr
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
` (2 preceding siblings ...)
2016-06-23 17:33 ` [PATCH v2 3/7] add skip_prefix_mem helper Jeff King
@ 2016-06-23 17:38 ` Jeff King
2016-06-23 17:38 ` [PATCH v2 5/7] color: allow "no-" for negating attributes Jeff King
` (2 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:38 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <peff@peff.net>
---
color.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/color.c b/color.c
index 8f85153..d787dec 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;
}
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 5/7] color: allow "no-" for negating attributes
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
` (3 preceding siblings ...)
2016-06-23 17:38 ` [PATCH v2 4/7] color: refactor parse_attr Jeff King
@ 2016-06-23 17:38 ` Jeff King
2016-06-23 17:39 ` [PATCH v2 6/7] color: support "italic" attribute Jeff King
2016-06-23 17:40 ` [PATCH v2 7/7] color: support strike-through attribute Jeff King
6 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:38 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <peff@peff.net>
---
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 f8460d4..4b13c90 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -166,7 +166,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 d787dec..a02a935 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 2b32c4f..2065752 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"
'
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 6/7] color: support "italic" attribute
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
` (4 preceding siblings ...)
2016-06-23 17:38 ` [PATCH v2 5/7] color: allow "no-" for negating attributes Jeff King
@ 2016-06-23 17:39 ` Jeff King
2016-06-23 18:34 ` Junio C Hamano
2016-06-23 17:40 ` [PATCH v2 7/7] color: support strike-through attribute Jeff King
6 siblings, 1 reply; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:39 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <scourtois@cubyx.fr>
Signed-off-by: Jeff King <peff@peff.net>
---
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 4b13c90..c7818ff 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -163,10 +163,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 a02a935..dcfaea8 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), strlen(x), (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 e24fa0b..3af01a6 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 2065752..13690f7 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' '
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 6/7] color: support "italic" attribute
2016-06-23 17:39 ` [PATCH v2 6/7] color: support "italic" attribute Jeff King
@ 2016-06-23 18:34 ` Junio C Hamano
2016-06-23 18:36 ` Jeff King
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2016-06-23 18:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Simon Courtois
Jeff King <peff@peff.net> writes:
> 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.
> ...
> @@ -133,6 +133,7 @@ static int parse_attr(const char *name, size_t len)
> #define ATTR(x, val, neg) { (x), strlen(x), (val), (neg) }
I see this one was from an earlier reroll that did not use sizeof(x)-1;
easy to fixup, though ;-)
> 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 e24fa0b..3af01a6 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 2065752..13690f7 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' '
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 6/7] color: support "italic" attribute
2016-06-23 18:34 ` Junio C Hamano
@ 2016-06-23 18:36 ` Jeff King
0 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 18:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Simon Courtois
On Thu, Jun 23, 2016 at 11:34:00AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 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.
> > ...
> > @@ -133,6 +133,7 @@ static int parse_attr(const char *name, size_t len)
> > #define ATTR(x, val, neg) { (x), strlen(x), (val), (neg) }
>
> I see this one was from an earlier reroll that did not use sizeof(x)-1;
> easy to fixup, though ;-)
Heh, whoops, yeah.
Gcc was actually happy with the `strlen` there, but I suspect that other
compilers might not be.
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 17:30 ` [PATCH v2 0/7] more ANSI attributes Jeff King
` (5 preceding siblings ...)
2016-06-23 17:39 ` [PATCH v2 6/7] color: support "italic" attribute Jeff King
@ 2016-06-23 17:40 ` Jeff King
2016-06-23 18:36 ` Junio C Hamano
6 siblings, 1 reply; 24+ messages in thread
From: Jeff King @ 2016-06-23 17:40 UTC (permalink / raw)
To: git; +Cc: Simon Courtois, Junio C Hamano
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 <peff@peff.net>
---
This was mostly for fun. I can't think of a way in which it would be
useful, and I'm not sure how compelling completionism is as an argument
for inclusion. I'm OK if we drop this one.
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 c7818ff..b10e49d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -163,8 +163,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 dcfaea8..eb028bd 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 3af01a6..6cae166 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 13690f7..ec78c5e 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' '
--
2.9.0.209.g845fbc1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 17:40 ` [PATCH v2 7/7] color: support strike-through attribute Jeff King
@ 2016-06-23 18:36 ` Junio C Hamano
2016-06-23 18:39 ` Jeff King
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2016-06-23 18:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Simon Courtois
Jeff King <peff@peff.net> writes:
> 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 <peff@peff.net>
> ---
> This was mostly for fun. I can't think of a way in which it would be
> useful, and I'm not sure how compelling completionism is as an argument
> for inclusion. I'm OK if we drop this one.
It indeed is fun and it even makes sense in this context:
$ ./git -c diff.color.old='red strike' show
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 18:36 ` Junio C Hamano
@ 2016-06-23 18:39 ` Jeff King
2016-06-23 18:52 ` Junio C Hamano
2016-06-23 19:56 ` Junio C Hamano
0 siblings, 2 replies; 24+ messages in thread
From: Jeff King @ 2016-06-23 18:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Simon Courtois
On Thu, Jun 23, 2016 at 11:36:23AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 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 <peff@peff.net>
> > ---
> > This was mostly for fun. I can't think of a way in which it would be
> > useful, and I'm not sure how compelling completionism is as an argument
> > for inclusion. I'm OK if we drop this one.
>
> It indeed is fun and it even makes sense in this context:
>
> $ ./git -c diff.color.old='red strike' show
Ooh, I hadn't thought of that. It's a bit noisy for my tastes in a
line-oriented diff, but with --color-words, it actually helps quite a
bit (try it on the documentation patch from this series, for example).
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 18:39 ` Jeff King
@ 2016-06-23 18:52 ` Junio C Hamano
2016-06-23 19:03 ` Junio C Hamano
2016-06-23 19:04 ` Jeff King
2016-06-23 19:56 ` Junio C Hamano
1 sibling, 2 replies; 24+ messages in thread
From: Junio C Hamano @ 2016-06-23 18:52 UTC (permalink / raw)
To: Jeff King; +Cc: git, Simon Courtois
Jeff King <peff@peff.net> writes:
> On Thu, Jun 23, 2016 at 11:36:23AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > 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 <peff@peff.net>
>> > ---
>> > This was mostly for fun. I can't think of a way in which it would be
>> > useful, and I'm not sure how compelling completionism is as an argument
>> > for inclusion. I'm OK if we drop this one.
>>
>> It indeed is fun and it even makes sense in this context:
>>
>> $ ./git -c diff.color.old='red strike' show
>
> Ooh, I hadn't thought of that. It's a bit noisy for my tastes in a
> line-oriented diff, but with --color-words, it actually helps quite a
> bit (try it on the documentation patch from this series, for example).
What I usually use is diff.color.old='red reverse' because I cannot
easily tell between black and dark red in small font on my white
background. s/reverse/strike/ makes it much less noisy.
What is sad for me is that I usually work in GNU screen, displaying
on either xterm or gnome-terminal. Without screen, strike shows but
inside it I cannot seem to be able to get strike-thru in effect.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 18:52 ` Junio C Hamano
@ 2016-06-23 19:03 ` Junio C Hamano
2016-06-23 19:04 ` Jeff King
1 sibling, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2016-06-23 19:03 UTC (permalink / raw)
To: git; +Cc: Jeff King, Simon Courtois
Junio C Hamano <gitster@pobox.com> writes:
> What is sad for me is that I usually work in GNU screen, displaying
> on either xterm or gnome-terminal. Without screen, strike shows but
> inside it I cannot seem to be able to get strike-thru in effect.
... which unfortunately is expected.
https://www.gnu.org/software/screen/manual/screen.html#Control-Sequences
(look for a string "Negative Image" that appears only once)
indicates that 9/29 are missing.
Not fun X-<.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 18:52 ` Junio C Hamano
2016-06-23 19:03 ` Junio C Hamano
@ 2016-06-23 19:04 ` Jeff King
2016-06-23 19:11 ` Junio C Hamano
1 sibling, 1 reply; 24+ messages in thread
From: Jeff King @ 2016-06-23 19:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Simon Courtois
On Thu, Jun 23, 2016 at 11:52:51AM -0700, Junio C Hamano wrote:
> >> It indeed is fun and it even makes sense in this context:
> >>
> >> $ ./git -c diff.color.old='red strike' show
> >
> > Ooh, I hadn't thought of that. It's a bit noisy for my tastes in a
> > line-oriented diff, but with --color-words, it actually helps quite a
> > bit (try it on the documentation patch from this series, for example).
>
> What I usually use is diff.color.old='red reverse' because I cannot
> easily tell between black and dark red in small font on my white
> background. s/reverse/strike/ makes it much less noisy.
You may find "bold red" a little easier, as it often uses a brighter
variant. We also support 8-bit and 24-bit color these days. So you can
probably do something like diff.color.old='#ff0000'. That's all old,
though, so I imagine you might have played with it long ago.
I have a black background myself, and save "reverse" for diff-highlight.
> What is sad for me is that I usually work in GNU screen, displaying on
> either xterm or gnome-terminal. Without screen, strike shows but
> inside it I cannot seem to be able to get strike-thru in effect.
Hmm. I see the same thing screen and with tmux, as well (though I don't
usually use either myself). I suspect they have to filter ANSI codes
because they're using the codes themselves (so anything that moves the
cursor is going to be a definite problem), and strike-through probably
just isn't common enough to have been added to the whitelist.
It does work with dtach, but that is probably because that program only
does the remote-socket parts of screen/tmux, and so wouldn't need any
filtering at all.
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 19:04 ` Jeff King
@ 2016-06-23 19:11 ` Junio C Hamano
0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2016-06-23 19:11 UTC (permalink / raw)
To: Jeff King; +Cc: git, Simon Courtois
Jeff King <peff@peff.net> writes:
> Hmm. I see the same thing screen and with tmux, as well (though I don't
> usually use either myself). I suspect they have to filter ANSI codes
> because they're using the codes themselves (so anything that moves the
> cursor is going to be a definite problem), and strike-through probably
> just isn't common enough to have been added to the whitelist.
screen needs to do a full emulation, not working as a passthru. It
pretends to be a VT100/ANSI and attempt to show on non-VT100/ANSI
terminal via termcap/terminfo ;-)
So it is understandable that the omitted support for less common
ones.
But we are straying deep into an uninteresting (to the list)
tangent, so let's stop it here.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 7/7] color: support strike-through attribute
2016-06-23 18:39 ` Jeff King
2016-06-23 18:52 ` Junio C Hamano
@ 2016-06-23 19:56 ` Junio C Hamano
1 sibling, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2016-06-23 19:56 UTC (permalink / raw)
To: Jeff King; +Cc: git, Simon Courtois
Jeff King <peff@peff.net> writes:
> ... but with --color-words, it actually helps quite a
> bit (try it on the documentation patch from this series, for example).
This gets me back to another tangent, but this time a one that is
quite a lot more relevant to Git.
There is this change in "git show --word-diff" for the "italic"
patch:
The accepted attributes are `bold`, `dim`, `ul`, `blink`,
{+`reverse`,+} and [-`reverse`.-]{+`italic`.+}
If we imagine that the pre- and post- image expressed in "one token
per line" format, the text before and after the patch would have
read like this:
preimage postimage
----------- ------------
... ...
blink blink
, ,
and reverse
reverse ,
. and
reverse
.
And the current output is showing an equivalent of this diff:
...
blink
,
+reverse
+,
and
-reverse
-.
+italic
+.
But if we were doing line-level diff for the above two, I would
think this is much easier to read:
...
blink
,
-and
reverse
+,
+and
+italic
+.
That would give us a word-diff more like this, I would imagine,
The accepted attributes are `bold`, `dim`, `ul`, `blink`,
[-and-] reverse[-,-] {+and `italic`}.
and that would be much easier to read than the current --word-diff
output.
^ permalink raw reply [flat|nested] 24+ messages in thread