Italics formatting

Italics formatting

[Simon Courtois]

Hi,

I was looking for a way to use italics in my git log. I ended-up looking at the code dealing with colors and style and noticed that the italic code was skipped when defining the list (color.c:128 if I'm not mistaken).

I'd love to propose a contribution but I'm sadly not very well versed with C.

So I'm asking you if this could be something that's part of Git someday. I don't know the extent of dim/ul/blink support across the terms but since some of them support italics the style would have its rightful place amongst the others.

And I'd also like to take the occasion to thank everybody working on Git, I've been using it for 7 years now and it's a real pleasure! Thank you all.

Regards,

Simon Courtois

[PATCH 0/2] more ANSI attributes

[Jeff King]

On Thu, Jun 23, 2016 at 01:54:34PM +0200, Simon Courtois wrote:

> I was looking for a way to use italics in my git log. I ended-up
> looking at the code dealing with colors and style and noticed that the
> italic code was skipped when defining the list (color.c:128 if I'm not
> mistaken).
> 
> I'd love to propose a contribution but I'm sadly not very well versed
> with C.

My first suggestion was going to be that you can feed arbitrary numbers
yourself, without git having to have a name for it. But that is true
only of colors, not attributes. So it does need a patch.

Here is one, along with a minor cleanup. I think the attributes we don't
support now are:

  - 6; rapid blink (not supported by xterm)
  - 8; conceal (supported, but why would you want it?)
  - 9; crossed-out (supported, and at least plausible to want?)

We also don't support font-selection (10-19, or 20 for Fraktur) which
are not supported by xterm (of course xterm support is not the defining
criterion, if people have other terms that do support it. My point is
mostly "nobody is asking for it, and it is not even in xterm").

  [1/2]: color: fix max-size comment
  [2/2]: color: support "italic" attribute

-Peff

[PATCH 1/2] color: fix max-size comment

[Jeff King]

We use fixed-size buffers for colors, because we know our
parsing cannot grow beyond a particular bound. However, our
comment describing the max-size 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>
---
 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

[PATCH 2/2] color: support "italic" attribute

[Jeff King]

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 | 2 +-
 color.c                  | 8 ++++----
 color.h                  | 3 ++-
 t/t4026-color.sh         | 4 ++--
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 58673cf..4b97d8d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -154,7 +154,7 @@ color::
        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
+       `white`; the attributes are `bold`, `dim`, `italic`, `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
diff --git a/color.c b/color.c
index 8f85153..698682c 100644
--- a/color.c
+++ b/color.c
@@ -125,11 +125,11 @@ static int parse_color(struct color *out, const char *name, int len)
 
 static int parse_attr(const char *name, int len)
 {
-	static const int attr_values[] = { 1, 2, 4, 5, 7,
-					   22, 22, 24, 25, 27 };
+	static const int attr_values[] = { 1, 2, 3, 4, 5, 7,
+					   22, 22, 23, 24, 25, 27 };
 	static const char * const attr_names[] = {
-		"bold", "dim", "ul", "blink", "reverse",
-		"nobold", "nodim", "noul", "noblink", "noreverse"
+		"bold", "dim", "italic", "ul", "blink", "reverse",
+		"nobold", "nodim", "noitalic", "noul", "noblink", "noreverse"
 	};
 	int i;
 	for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
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 2b32c4f..05625c5 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -56,8 +56,8 @@ 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

Re: [PATCH 2/2] color: support "italic" attribute

[Simon Courtois]

Wow I wasn't expecting such a rapid response, you're awesome! :-D

Simon Courtois

On 23 June 2016 at 15:10:33, Jeff King (peff@peff.net) wrote:
> 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 
> ---
> Documentation/config.txt | 2 +-
> color.c | 8 ++++----
> color.h | 3 ++-
> t/t4026-color.sh | 4 ++--
> 4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 58673cf..4b97d8d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -154,7 +154,7 @@ color::
> 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
> + `white`; the attributes are `bold`, `dim`, `italic`, `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
> diff --git a/color.c b/color.c
> index 8f85153..698682c 100644
> --- a/color.c
> +++ b/color.c
> @@ -125,11 +125,11 @@ static int parse_color(struct color *out, const char *name, int 
> len)
> 
> static int parse_attr(const char *name, int len)
> {
> - static const int attr_values[] = { 1, 2, 4, 5, 7,
> - 22, 22, 24, 25, 27 };
> + static const int attr_values[] = { 1, 2, 3, 4, 5, 7,
> + 22, 22, 23, 24, 25, 27 };
> static const char * const attr_names[] = {
> - "bold", "dim", "ul", "blink", "reverse",
> - "nobold", "nodim", "noul", "noblink", "noreverse"
> + "bold", "dim", "italic", "ul", "blink", "reverse",
> + "nobold", "nodim", "noitalic", "noul", "noblink", "noreverse"
> };
> int i;
> for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
> 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 2b32c4f..05625c5 100755
> --- a/t/t4026-color.sh
> +++ b/t/t4026-color.sh
> @@ -56,8 +56,8 @@ 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
> 

Re: [PATCH 2/2] color: support "italic" attribute

[Junio C Hamano]

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.
>
> 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 | 2 +-
>  color.c                  | 8 ++++----
>  color.h                  | 3 ++-
>  t/t4026-color.sh         | 4 ++--
>  4 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 58673cf..4b97d8d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -154,7 +154,7 @@ color::
>         colors (at most two) and attributes (at most one), separated

This is describing the format accepted by color_parse_mem(), whose
main loop has /* [fg [bg]] [attr]... */ comment in front?

Is "at most one" still correct, or it was already stale before this
patch?

Re: [PATCH 2/2] color: support "italic" attribute

[Jeff King]

On Thu, Jun 23, 2016 at 09:46:57AM -0700, Junio C Hamano wrote:

> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 58673cf..4b97d8d 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -154,7 +154,7 @@ color::
> >         colors (at most two) and attributes (at most one), separated
> 
> This is describing the format accepted by color_parse_mem(), whose
> main loop has /* [fg [bg]] [attr]... */ comment in front?
> 
> Is "at most one" still correct, or it was already stale before this
> patch?

I think it was already stale.

I'm actually preparing a re-roll that makes some of the parsing a bit
less nasty, too. I'll fix that at the same time.

-Peff

[PATCH v2 0/7] more ANSI attributes

[Jeff King]

Here's a re-roll of my earlier patches to add "italic" support.

It fixes the documentation problem Junio mentioned (along with some
other issues there). It also cleans up parse_attr() to make it a bit
easier to read and maintain, and adds a few more features.

  [1/7]: color: fix max-size comment
  [2/7]: doc: refactor description of color format
  [3/7]: add skip_prefix_mem helper
  [4/7]: color: refactor parse_attr
  [5/7]: color: allow "no-" for negating attributes
  [6/7]: color: support "italic" attribute
  [7/7]: color: support strike-through attribute

-Peff

[PATCH v2 1/7] color: fix max-size comment

[Jeff King]

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

[PATCH v2 2/7] doc: refactor description of color format

[Jeff King]

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

[PATCH v2 3/7] add skip_prefix_mem helper

[Jeff King]

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

[PATCH v2 4/7] color: refactor parse_attr

[Jeff King]

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

[PATCH v2 5/7] color: allow "no-" for negating attributes

[Jeff King]

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

[PATCH v2 6/7] color: support "italic" attribute

[Jeff King]

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

[PATCH v2 7/7] color: support strike-through attribute

[Jeff King]

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

Re: [PATCH v2 6/7] color: support "italic" attribute

[Junio C Hamano]

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

Re: [PATCH v2 6/7] color: support "italic" attribute

[Jeff King]

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

Re: [PATCH v2 7/7] color: support strike-through attribute

[Junio C Hamano]

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

Re: [PATCH v2 7/7] color: support strike-through attribute

[Jeff King]

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

Re: [PATCH v2 7/7] color: support strike-through attribute

[Junio C Hamano]

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.

Re: [PATCH v2 7/7] color: support strike-through attribute

[Junio C Hamano]

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

Re: [PATCH v2 7/7] color: support strike-through attribute

[Jeff King]

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

Re: [PATCH v2 7/7] color: support strike-through attribute

[Junio C Hamano]

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.

Re: [PATCH v2 7/7] color: support strike-through attribute

[Junio C Hamano]

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.