threads / discuss / 37937

Re: diff-highlight highlight words?

Subject: Re: diff-highlight highlight words?

## tl;dr

22 messages between Nov 12, 2014 and Dec 9, 2014.

replies: 21people: 5as markdown or json

Jeff King· Nov 12, 2014, 07:56 UTC · lore
[+cc git@vger, since this may be of interest to others]
On Tue, Nov 11, 2014 at 02:40:59PM -0800, Scott Baker wrote:
> I'd like to recreate the github style diffs on the command line. It
> appears that your diff-highlight is very close. The current version only
> allows you to "invert the colors" which isn't ideal.

Yes, I never built any configurability into the script. However, you can tweak the definitions at the top to get different effects. Traditionally, ANSI colors on the terminal only came in two flavors: "normal" and "bright" (which is attached to the "bold" attribute"). Instead of reversing video, you can switch on brightness like this:

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index c4404d4..c99de99 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -5,8 +5,8 @@ use strict;
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
-my $HIGHLIGHT   = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
+my $HIGHLIGHT   = "\x1b[1m";
+my $UNHIGHLIGHT = "\x1b[22m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 

However on most modern terminals the color difference between bright and
normal is very subtle, and this doesn't look good.

XTerm (and other modern terminals) has 256-color support, so you could
do better there (assuming your terminal supports it). The current code
builds on the existing colors produced by git (because the operations
are only "invert colors" and "uninvert colors"). Doing anything fancier
requires handling add/del differently.  That patch might look something
like this (which uses dark red/green for most of the line, and a much
brighter variant for the highlighted text):

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index c4404d4..4e08f3c 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -5,11 +5,16 @@ use strict;
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
-my $HIGHLIGHT   = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
+# Elements:
+# 0 - highlighted text
+# 1 - unhighlighted text
+# 2 - reset to normal
+my @ADD_HIGHLIGHT = ("\x1b[38;2;100;255;100m", "\x1b[38;2;0;255;0m", "\x1b[30m");
+my @DEL_HIGHLIGHT = ("\x1b[38;2;255;100;100m", "\x1b[38;2;255;0;0m", "\x1b[30m");
+
 my @removed;
 my @added;
 my $in_hunk;
@@ -128,8 +133,8 @@ sub highlight_pair {
 	}
 
 	if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
-		return highlight_line(\@a, $pa, $sa),
-		       highlight_line(\@b, $pb, $sb);
+		return highlight_line(\@a, $pa, $sa, @DEL_HIGHLIGHT),
+		       highlight_line(\@b, $pb, $sb, @ADD_HIGHLIGHT);
 	}
 	else {
 		return join('', @a),
@@ -144,15 +149,18 @@ sub split_line {
 }
 
 sub highlight_line {
-	my ($line, $prefix, $suffix) = @_;
+	my ($line, $prefix, $suffix, $highlight, $unhighlight, $reset) = @_;
 
-	return join('',
+	my $r = join('',
+		$unhighlight,
 		@{$line}[0..($prefix-1)],
-		$HIGHLIGHT,
+		$highlight,
 		@{$line}[$prefix..$suffix],
-		$UNHIGHLIGHT,
-		@{$line}[($suffix+1)..$#$line]
+		$unhighlight,
+		@{$line}[($suffix+1)..$#$line],
 	);
+	$r =~ s/\n$/$reset$&/;
+	return $r;
 }
 
 # Pairs are interesting to highlight only if we are going to end up


The result does not look terrible to me, though I think I find the
reverse-video more obvious when scanning the diff. To look more like
GitHub's view, you could instead set the background by doing this on
top:

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 4e08f3c..6f98db4 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -12,8 +12,8 @@ my $BORING = qr/$COLOR|\s/;
 # 0 - highlighted text
 # 1 - unhighlighted text
 # 2 - reset to normal
-my @ADD_HIGHLIGHT = ("\x1b[38;2;100;255;100m", "\x1b[38;2;0;255;0m", "\x1b[30m");
-my @DEL_HIGHLIGHT = ("\x1b[38;2;255;100;100m", "\x1b[38;2;255;0;0m", "\x1b[30m");
+my @ADD_HIGHLIGHT = ("\x1b[30;48;2;100;255;100m", "\x1b[30;48;2;0;255;0m", "\x1b[0m");
+my @DEL_HIGHLIGHT = ("\x1b[30;48;2;255;100;100m", "\x1b[30;48;2;255;0;0m", "\x1b[0m");
 
 my @removed;
 my @added;
@@ -151,14 +151,18 @@ sub split_line {
 sub highlight_line {
 	my ($line, $prefix, $suffix, $highlight, $unhighlight, $reset) = @_;
 
+	# strip out existing colors from git, which will clash
+	# both due to contrast and because of random ANSI resets
+	# inside the content
+	my $p = join('', @{$line}[0..($prefix-1)]);
+	my $t = join('', @{$line}[$prefix..$suffix]);
+	my $s = join('', @{$line}[($suffix+1)..$#$line]);
+	s/$COLOR//g for ($p, $t, $s);
+
 	my $r = join('',
-		$unhighlight,
-		@{$line}[0..($prefix-1)],
-		$highlight,
-		@{$line}[$prefix..$suffix],
-		$unhighlight,
-		@{$line}[($suffix+1)..$#$line],
-	);
+		     $unhighlight, $p,
+		     $highlight, $t,
+		     $unhighlight, $s);
 	$r =~ s/\n$/$reset$&/;
 	return $r;
 }

I'm not wild about that either. I dunno. I still like the reverse-video
the best, but it may be that with a few tweaks somebody could make it
look less ugly.

-Peff
Scott Baker· Nov 12, 2014, 17:59 UTC · re: Jeff King · lore
On 11/11/2014 11:56 PM, Jeff King wrote:
Show 13 quoted lines
> [+cc git@vger, since this may be of interest to others]
>
> On Tue, Nov 11, 2014 at 02:40:59PM -0800, Scott Baker wrote:
>
> > I'd like to recreate the github style diffs on the command line. It
> > appears that your diff-highlight is very close. The current version only
> > allows you to "invert the colors" which isn't ideal.
>
> Yes, I never built any configurability into the script. However, you can
> tweak the definitions at the top to get different effects.
> Traditionally, ANSI colors on the terminal only came in two flavors:
> "normal" and "bright" (which is attached to the "bold" attribute").
> Instead of reversing video, you can switch on brightness like this:

It's 2014, most terminals are at least 256 colors. I'm fine if the defaults are 16 colors (that's safest), but it would be really cool if we could have an option for:

line add color line remove color word add color word remove color

I would then configure appropriate colors from the 256 color palette. I think the Github style diffs which include the lines/words that are changed are very readable and make dealing with diffs easier.

-- 
Scott Baker - Canby Telcom 
Senior System Administrator - RHCE
Jeff King· Nov 20, 2014, 15:14 UTC · re: Scott Baker · lore

[PATCH 0/7] color fixes and configurable diff-highlight

On Wed, Nov 12, 2014 at 09:59:35AM -0800, Scott Baker wrote:
Show 12 quoted lines
> It's 2014, most terminals are at least 256 colors. I'm fine if the
> defaults are 16 colors (that's safest), but it would be really cool if
> we could have an option for:
> 
> line add color
> line remove color
> word add color
> word remove color
> 
> I would then configure appropriate colors from the 256 color palette. I
> think the Github style diffs which include the lines/words that are
> changed are very readable and make dealing with diffs easier.

I thought I'd just procrastinate for an hour by doing this, but somehow it turned into a 7-patch series.

The first few are actual fixes I noticed along the way.

Patches 4 and 5 support RGB-mode, which works on XTerm, at least (we This is probably excessive over 256-color mode (which we already supported), but I find the resulting color specifications significantly easier to understand (quick, what's ANSI color 137?).

Patch 6 implements negative attributes (like "nobold"). This is probably not all that useful for normal git color specs, but is required for diff-highlight, which wants to leave some attributes untouched.

Patch 7 is the part you actually asked for. :)
  [1/7]: docs: describe ANSI 256-color mode
  [2/7]: config: fix parsing of "git config --get-color some.key -1"
  [3/7]: t4026: test "normal" color
  [4/7]: parse_color: refactor color storage
  [5/7]: parse_color: support 24-bit RGB values
  [6/7]: parse_color: recognize "no$foo" to clear the $foo attribute
  [7/7]: diff-highlight: allow configurable colors
-Peff
Jeff King· Nov 20, 2014, 15:15 UTC · re: Jeff King · lore

[PATCH 1/7] docs: describe ANSI 256-color mode

Our color specifications have supported the 256-color ANSI extension for years, but we never documented it.

Signed-off-by: Jeff King <peff@peff.net>
---
I have no clue which terminals do and don't support this. I would hope
the answer is "everything" by now, but I have seen some pretty awful
terminal emulators in my time.
 Documentation/config.txt | 4 ++++
 1 file changed, 4 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9220725..f615a5c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -839,6 +839,10 @@ accepted are `normal`, `black`, `red`, `green`, `yellow`, `blue`,
 `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.
++
+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).
 
 color.diff::
 	Whether to use ANSI escape sequences to add color to patches.
-- 
2.2.0.rc2.402.g4519813
Jeff King· Nov 20, 2014, 15:15 UTC · re: Jeff King · lore

[PATCH 2/7] config: fix parsing of "git config --get-color some.key -1"

Most of git-config's command line options use OPT_BIT to choose an action, and then parse the non-option arguments in a context-dependent way. However, --get-color and --get-colorbool are unlike the rest of the options, in that they are OPT_STRING, taking the option name as a parameter.

This generally works, because we then use the presence of those strings to set an action bit anyway. But it does mean that the option-parser will continue looking for options even after the key (because it is not a non-option; it is an argument to an option). And running:

  git config --get-color some.key -1

(to use "-1" as the default color spec) will barf, claiming that "-1" is not an option. Instead, we should treat --get-color and --get-colorbool as action bits, just like --add, --get, and all the other actions, and then check that the non-option arguments we got are sane. This fixes the weirdness above, and makes those two options like all the others.

This "fixes" a test in t4026, which checked that feeding "-2" as a color should fail (it does fail, but prior to this patch, because parseopt barfed, not because we actually ever tried to parse the color).

This also catches other errors, like:
  git config --get-color some.key black blue

which previously silently ignored "blue" (and now will complain that you gave too many arguments).

There are some possible regressions, though. We now disallow these, which currently do what you would expect:

  # specifying other options after the action
  git config --get-color some.key --file whatever
  # using long-arg syntax
  git config --get-color=some.key

However, we have never advertised these in the documentation, and in fact they did not work in some older versions of git. The behavior was apparently switched as an accidental side effect of d64ec16 (git config: reorganize to use parseopt, 2009-02-21).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8cc2604..fddafbb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -69,8 +69,8 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
 	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
-	OPT_STRING(0, "get-color", &get_color_slot, N_("slot"), N_("find the color configured: [default]")),
-	OPT_STRING(0, "get-colorbool", &get_colorbool_slot, N_("slot"), N_("find the color setting: [stdout-is-tty]")),
+	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
+	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
 	OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
 	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
@@ -303,8 +303,9 @@ static int git_get_color_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
-static void get_color(const char *def_color)
+static void get_color(const char *var, const char *def_color)
 {
+	get_color_slot = var;
 	get_color_found = 0;
 	parsed_color[0] = '\0';
 	git_config_with_options(git_get_color_config, NULL,
@@ -333,8 +334,9 @@ static int git_get_colorbool_config(const char *var, const char *value,
 	return 0;
 }
 
-static int get_colorbool(int print)
+static int get_colorbool(const char *var, int print)
 {
+	get_colorbool_slot = var;
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
 	get_color_ui_found = -1;
@@ -532,12 +534,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
-	if (get_color_slot)
-	    actions |= ACTION_GET_COLOR;
-	if (get_colorbool_slot)
-	    actions |= ACTION_GET_COLORBOOL;
-
-	if ((get_color_slot || get_colorbool_slot) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
@@ -683,12 +680,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			die("No such section!");
 	}
 	else if (actions == ACTION_GET_COLOR) {
-		get_color(argv[0]);
+		check_argc(argc, 1, 2);
+		get_color(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_GET_COLORBOOL) {
-		if (argc == 1)
-			color_stdout_is_tty = git_config_bool("command line", argv[0]);
-		return get_colorbool(argc != 0);
+		check_argc(argc, 1, 2);
+		if (argc == 2)
+			color_stdout_is_tty = git_config_bool("command line", argv[1]);
+		return get_colorbool(argv[0], argc == 2);
 	}
 
 	return 0;
-- 
2.2.0.rc2.402.g4519813
Jeff King· Nov 20, 2014, 15:16 UTC · re: Jeff King · lore

[PATCH 3/7] t4026: test "normal" color

If the user specifiers "normal" for a foreground color, this should be a noop (while this may sound useless, it is the only way to specify an unchanged foreground color followed by a specific background color).

We also check that color "-1" does the same thing. This is not documented, but has worked forever, so let's make sure we keep supporting it.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4026-color.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 3726a0e..63e4238 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -53,6 +53,14 @@ test_expect_success '256 colors' '
 	color "254 bold 255" "[1;38;5;254;48;5;255m"
 '
 
+test_expect_success '"normal" yields no color at all"' '
+	color "normal black" "[40m"
+'
+
+test_expect_success '-1 is a synonym for "normal"' '
+	color "-1 black" "[40m"
+'
+
 test_expect_success 'color too small' '
 	invalid_color "-2"
 '
-- 
2.2.0.rc2.402.g4519813
Junio C Hamano· Nov 20, 2014, 18:53 UTC · re: Jeff King · lore

Re: [PATCH 3/7] t4026: test "normal" color

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> If the user specifiers "normal" for a foreground color, this
> should be a noop (while this may sound useless, it is the
> only way to specify an unchanged foreground color followed
> by a specific background color).
>
> We also check that color "-1" does the same thing. This is
> not documented, but has worked forever, so let's make sure
> we keep supporting it.

YLNTED, really? I do not object to the conclusion, but I am mildly surprised ;-)

Show 25 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t4026-color.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t4026-color.sh b/t/t4026-color.sh
> index 3726a0e..63e4238 100755
> --- a/t/t4026-color.sh
> +++ b/t/t4026-color.sh
> @@ -53,6 +53,14 @@ test_expect_success '256 colors' '
>  	color "254 bold 255" "[1;38;5;254;48;5;255m"
>  '
>  
> +test_expect_success '"normal" yields no color at all"' '
> +	color "normal black" "[40m"
> +'
> +
> +test_expect_success '-1 is a synonym for "normal"' '
> +	color "-1 black" "[40m"
> +'
> +
>  test_expect_success 'color too small' '
>  	invalid_color "-2"
>  '
Jeff King· Nov 20, 2014, 19:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/7] t4026: test "normal" color

On Thu, Nov 20, 2014 at 10:53:56AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > If the user specifiers "normal" for a foreground color, this
Argh, s/specifiers/specifies/
Show 6 quoted lines
> > We also check that color "-1" does the same thing. This is
> > not documented, but has worked forever, so let's make sure
> > we keep supporting it.
> 
> YLNTED, really?  I do not object to the conclusion, but I am
> mildly surprised ;-)

I was surprised by it, too, when writing the refactoring patch that comes next in the series. :)

I was also surprised that we further check that "-2" is _not_ valid in the tests. I do not mind declaring everything negative to be the same (either invalid, or "normal"), but I decided that there was really no benefit to breaking compatibility in this case. And I suppose if you are using 256-color mode, then "-1 255" is perhaps a natural way to write it.

-Peff
Jeff King· Nov 20, 2014, 15:17 UTC · re: Jeff King · lore

[PATCH 4/7] parse_color: refactor color storage

When we parse a color name like "red" into its ANSI color value, we pack the storage into a single int that may take on many values:

  1. If it's "-2", no value has been specified.
  2. If it's "-1", the value is "normal" (i.e., no color).
  3. If it's 0 through 7, the value is a standard ANSI
     color.
  4. If it's larger (up to 255), it is a 256-color extended
     value.

Given these magic numbers, it is often hard to see what is going on in the code. Let's refactor this into a struct with a flag that tells which scheme we are using, along with a numeric value. This is more verbose, but should hopefully be simpler to follow. It will also allow us to easily add support for more schemes, like 24-bit RGB values.

The result is also slightly less efficient to store, but that's OK; we only store this intermediate state during the parse, after which we write out the actual ANSI bytes.

Signed-off-by: Jeff King <peff@peff.net>
---
 color.c | 138 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 32 deletions(-)
diff --git a/color.c b/color.c
index 7941e93..6edbcae 100644
--- a/color.c
+++ b/color.c
@@ -26,23 +26,77 @@ const char *column_colors_ansi[] = {
 /* Ignore the RESET at the end when giving the size */
 const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
 
-static int parse_color(const char *name, int len)
+/* An individual foreground or background color. */
+struct color {
+	enum {
+		COLOR_UNSPECIFIED = 0,
+		COLOR_NORMAL,
+		COLOR_ANSI, /* basic 0-7 ANSI colors */
+		COLOR_256
+	} state;
+	/* The numeric value for ANSI and 256-color modes */
+	unsigned char value;
+};
+
+/*
+ * "word" is a buffer of length "len"; does it match the NUL-terminated
+ * "match" exactly?
+ */
+static int match_word(const char *word, int len, const char *match)
 {
+	return !strncasecmp(word, match, len) && !match[len];
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
+	/* Positions in array must match ANSI color codes */
 	static const char * const color_names[] = {
-		"normal", "black", "red", "green", "yellow",
+		"black", "red", "green", "yellow",
 		"blue", "magenta", "cyan", "white"
 	};
 	char *end;
 	int i;
+	long val;
+
+	/* First try the special word "normal"... */
+	if (match_word(name, len, "normal")) {
+		out->state = COLOR_NORMAL;
+		return 0;
+	}
+
+	/* Then pick from our human-readable color names... */
 	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		const char *str = color_names[i];
-		if (!strncasecmp(name, str, len) && !str[len])
-			return i - 1;
+		if (match_word(name, len, color_names[i])) {
+			out->state = COLOR_ANSI;
+			out->value = i;
+			return 0;
+		}
 	}
-	i = strtol(name, &end, 10);
-	if (end - name == len && i >= -1 && i <= 255)
-		return i;
-	return -2;
+
+	/* And finally try a literal 256-color-mode number */
+	val = strtol(name, &end, 10);
+	if (end - name == len) {
+		/*
+		 * Allow "-1" as an alias for "normal", but other negative
+		 * numbers are bogus.
+		 */
+		if (val < -1)
+			; /* fall through to error */
+		else if (val < 0) {
+			out->state = COLOR_NORMAL;
+			return 0;
+		/* Rewrite low numbers as more-portable standard colors. */
+		} else if (val < 8) {
+			out->state = COLOR_ANSI;
+			out->value = val;
+		} else if (val < 256) {
+			out->state = COLOR_256;
+			out->value = val;
+			return 0;
+		}
+	}
+
+	return -1;
 }
 
 static int parse_attr(const char *name, int len)
@@ -65,13 +119,43 @@ int color_parse(const char *value, char *dst)
 	return color_parse_mem(value, strlen(value), dst);
 }
 
+#define COLOR_FOREGROUND '3'
+#define COLOR_BACKGROUND '4'
+
+/*
+ * Write the ANSI color codes for "c" to "out"; the string should
+ * already have the ANSI escape code in it. "out" should have enough
+ * space in it to fit any color.
+ */
+static char *color_output(char *out, const struct color *c, char type)
+{
+	switch (c->state) {
+	case COLOR_UNSPECIFIED:
+	case COLOR_NORMAL:
+		break;
+	case COLOR_ANSI:
+		*out++ = type;
+		*out++ = '0' + c->value;
+		break;
+	case COLOR_256:
+		out += sprintf(out, "%c8;5;%d", type, c->value);
+		break;
+	}
+	return out;
+}
+
+static int color_empty(const struct color *c)
+{
+	return c->state <= COLOR_NORMAL;
+}
+
 int color_parse_mem(const char *value, int value_len, char *dst)
 {
 	const char *ptr = value;
 	int len = value_len;
 	unsigned int attr = 0;
-	int fg = -2;
-	int bg = -2;
+	struct color fg = { COLOR_UNSPECIFIED };
+	struct color bg = { COLOR_UNSPECIFIED };
 
 	if (!strncasecmp(value, "reset", len)) {
 		strcpy(dst, GIT_COLOR_RESET);
@@ -81,6 +165,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 	/* [fg [bg]] [attr]... */
 	while (len > 0) {
 		const char *word = ptr;
+		struct color c;
 		int val, wordlen = 0;
 
 		while (len > 0 && !isspace(word[wordlen])) {
@@ -94,14 +179,13 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 			len--;
 		}
 
-		val = parse_color(word, wordlen);
-		if (val >= -1) {
-			if (fg == -2) {
-				fg = val;
+		if (!parse_color(&c, word, wordlen)) {
+			if (fg.state == COLOR_UNSPECIFIED) {
+				fg = c;
 				continue;
 			}
-			if (bg == -2) {
-				bg = val;
+			if (bg.state == COLOR_UNSPECIFIED) {
+				bg = c;
 				continue;
 			}
 			goto bad;
@@ -113,7 +197,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 			goto bad;
 	}
 
-	if (attr || fg >= 0 || bg >= 0) {
+	if (attr || !color_empty(&fg) || !color_empty(&bg)) {
 		int sep = 0;
 		int i;
 
@@ -129,25 +213,15 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 				*dst++ = ';';
 			*dst++ = '0' + i;
 		}
-		if (fg >= 0) {
+		if (!color_empty(&fg)) {
 			if (sep++)
 				*dst++ = ';';
-			if (fg < 8) {
-				*dst++ = '3';
-				*dst++ = '0' + fg;
-			} else {
-				dst += sprintf(dst, "38;5;%d", fg);
-			}
+			dst = color_output(dst, &fg, COLOR_FOREGROUND);
 		}
-		if (bg >= 0) {
+		if (!color_empty(&bg)) {
 			if (sep++)
 				*dst++ = ';';
-			if (bg < 8) {
-				*dst++ = '4';
-				*dst++ = '0' + bg;
-			} else {
-				dst += sprintf(dst, "48;5;%d", bg);
-			}
+			dst = color_output(dst, &bg, COLOR_BACKGROUND);
 		}
 		*dst++ = 'm';
 	}
-- 
2.2.0.rc2.402.g4519813
Junio C Hamano· Nov 20, 2014, 19:37 UTC · re: Jeff King · lore

Re: [PATCH 4/7] parse_color: refactor color storage

Jeff King <peff@peff.net> writes:
> The result is also slightly less efficient to store, but
> that's OK; we only store this intermediate state during the
> parse, after which we write out the actual ANSI bytes.

You need up to 24 bits for the value and then 2 bits for what type of color specification you have (what you called "state"), for which 32-bit should be sufficient. That would mean it is more efficient than a single int on 64-bit archs, and you can arrange the struct to be packed if we ever need to keep dozens of these things. Until then, the updated simple struct is just fine, and either way it is much easier to read ;-)

Thanks.
Johannes Sixt· Dec 9, 2014, 20:14 UTC · re: Jeff King · lore

Re: [PATCH 4/7] parse_color: refactor color storage

Am 20.11.2014 um 16:17 schrieb Jeff King:
> +#define COLOR_FOREGROUND '3'
> +#define COLOR_BACKGROUND '4'

This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows, because we inherit a definition from a Windows header. How would you like it fixed? A different name or #undef in front of it?

-- Hannes
Jeff King· Dec 9, 2014, 20:21 UTC · re: Johannes Sixt · lore

Re: [PATCH 4/7] parse_color: refactor color storage

On Tue, Dec 09, 2014 at 09:14:29PM +0100, Johannes Sixt wrote:
Show 7 quoted lines
> Am 20.11.2014 um 16:17 schrieb Jeff King:
> > +#define COLOR_FOREGROUND '3'
> > +#define COLOR_BACKGROUND '4'
> 
> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
> because we inherit a definition from a Windows header. How would you
> like it fixed? A different name or #undef in front of it?

I think a different name would be fine. The constants are actually only used once each. Their main function is to avoid a confusing parameter '3' to the color_output function. But we could use the literal constants with a parameter, like:

diff --git a/color.c b/color.c
index e2a0a99..809b359 100644
--- a/color.c
+++ b/color.c
@@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
 	return color_parse_mem(value, strlen(value), dst);
 }
 
-#define COLOR_FOREGROUND '3'
-#define COLOR_BACKGROUND '4'
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 		if (!color_empty(&fg)) {
 			if (sep++)
 				*dst++ = ';';
-			dst = color_output(dst, &fg, COLOR_FOREGROUND);
+			/* foreground colors are all in the 3x range */
+			dst = color_output(dst, &fg, '3');
 		}
 		if (!color_empty(&bg)) {
 			if (sep++)
 				*dst++ = ';';
-			dst = color_output(dst, &bg, COLOR_BACKGROUND);
+			/* background colors are all in the 4x range */
+			dst = color_output(dst, &bg, '4');
 		}
 		*dst++ = 'm';
 	}

We could also pass in integer "30" and "40", and then format it with %d
inside color_output. I don't know if that would be more obvious or not.

-Peff
Johannes Sixt· Dec 9, 2014, 20:52 UTC · re: Jeff King · lore

Re: [PATCH 4/7] parse_color: refactor color storage

Am 09.12.2014 um 21:21 schrieb Jeff King:
Show 49 quoted lines
> On Tue, Dec 09, 2014 at 09:14:29PM +0100, Johannes Sixt wrote:
> 
>> Am 20.11.2014 um 16:17 schrieb Jeff King:
>>> +#define COLOR_FOREGROUND '3'
>>> +#define COLOR_BACKGROUND '4'
>>
>> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
>> because we inherit a definition from a Windows header. How would you
>> like it fixed? A different name or #undef in front of it?
> 
> I think a different name would be fine. The constants are actually only
> used once each. Their main function is to avoid a confusing parameter
> '3' to the color_output function. But we could use the literal
> constants with a parameter, like:
> 
> diff --git a/color.c b/color.c
> index e2a0a99..809b359 100644
> --- a/color.c
> +++ b/color.c
> @@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
>  	return color_parse_mem(value, strlen(value), dst);
>  }
>  
> -#define COLOR_FOREGROUND '3'
> -#define COLOR_BACKGROUND '4'
> -
>  /*
>   * Write the ANSI color codes for "c" to "out"; the string should
>   * already have the ANSI escape code in it. "out" should have enough
> @@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
>  		if (!color_empty(&fg)) {
>  			if (sep++)
>  				*dst++ = ';';
> -			dst = color_output(dst, &fg, COLOR_FOREGROUND);
> +			/* foreground colors are all in the 3x range */
> +			dst = color_output(dst, &fg, '3');
>  		}
>  		if (!color_empty(&bg)) {
>  			if (sep++)
>  				*dst++ = ';';
> -			dst = color_output(dst, &bg, COLOR_BACKGROUND);
> +			/* background colors are all in the 4x range */
> +			dst = color_output(dst, &bg, '4');
>  		}
>  		*dst++ = 'm';
>  	}
> 
> We could also pass in integer "30" and "40", and then format it with %d
> inside color_output. I don't know if that would be more obvious or not.

This patch would actually be my personal preference. The comment near the literal makes it all clear.

-- Hannes
Jeff King· Dec 9, 2014, 21:01 UTC · re: Johannes Sixt · lore

Re: [PATCH 4/7] parse_color: refactor color storage

On Tue, Dec 09, 2014 at 09:52:10PM +0100, Johannes Sixt wrote:
> This patch would actually be my personal preference. The comment near
> the literal makes it all clear.
Thanks. Here it is ready to be applied.
Junio, this goes on top of the jk/colors topic.
-- >8 --
Subject: [PATCH] parse_color: drop COLOR_BACKGROUND macro

Commit 695d95d (parse_color: refactor color storage, 2014-11-20) introduced two macros, COLOR_FOREGROUND and COLOR_BACKGROUND. The latter conflicts with a system macro defined on Windows, breaking compilation there.

The simplest solution is to just get rid of these macros entirely. They are constants that are only used in one place (since the whole point of 695d95d was to avoid repeating ourselves). Their main function is to make the magic character constants more readable, but we can do the same thing with a comment.

Reported-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 color.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/color.c b/color.c
index e2a0a99..809b359 100644
--- a/color.c
+++ b/color.c
@@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
 	return color_parse_mem(value, strlen(value), dst);
 }
 
-#define COLOR_FOREGROUND '3'
-#define COLOR_BACKGROUND '4'
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 		if (!color_empty(&fg)) {
 			if (sep++)
 				*dst++ = ';';
-			dst = color_output(dst, &fg, COLOR_FOREGROUND);
+			/* foreground colors are all in the 3x range */
+			dst = color_output(dst, &fg, '3');
 		}
 		if (!color_empty(&bg)) {
 			if (sep++)
 				*dst++ = ';';
-			dst = color_output(dst, &bg, COLOR_BACKGROUND);
+			/* background colors are all in the 4x range */
+			dst = color_output(dst, &bg, '4');
 		}
 		*dst++ = 'm';
 	}
-- 
2.2.0.454.g7eca6b7
Eric Sunshine· Dec 9, 2014, 20:56 UTC · re: Johannes Sixt · lore

Re: [PATCH 4/7] parse_color: refactor color storage

On Tue, Dec 9, 2014 at 3:14 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 7 quoted lines
> Am 20.11.2014 um 16:17 schrieb Jeff King:
>> +#define COLOR_FOREGROUND '3'
>> +#define COLOR_BACKGROUND '4'
>
> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
> because we inherit a definition from a Windows header. How would you
> like it fixed? A different name or #undef in front of it?

One way this sort of problem was handled in the past was the sanitize the preprocessor namespace [1].

[1]: https://github.com/git/git/blob/master/compat/mingw.h#L80
Jeff King· Nov 20, 2014, 15:25 UTC · re: Jeff King · lore

[PATCH 5/7] parse_color: support 24-bit RGB values

Some terminals (like XTerm) allow full 24-bit RGB color specifications using an extension to the regular ANSI color scheme. Let's allow users to specify hex RGB colors, enabling the all-important feature of hot pink ref decorations:

  git log --format="%h%C(#ff69b4)%d%C(reset) %s"
Signed-off-by: Jeff King <peff@peff.net>
---
Also no clue on which terminals support it. I did all of my testing on
a recent version of XTerm. It looks like it doesn't provide true 24-bit
support, though. It is happy to accept the 24-bit colors, but if you do:
  for b in $(seq 255); do
    h=$(printf %02x $b)
    git --no-pager log -1 --format="%C(#0000$h)$b%C(reset)"
  done

the gradient seems to "jump" in discrete steps. That's fine, though. It's a quality-of-implementation issue for the terminal, and I still think that the RGB spec is way more readable than the 256-color mode ones.

 Documentation/config.txt |  3 ++-
 color.c                  | 29 ++++++++++++++++++++++++++++-
 color.h                  |  6 +++---
 t/t4026-color.sh         |  4 ++++
 4 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f615a5c..a237b82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -842,7 +842,8 @@ doesn't matter.
 +
 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).
+terminals may support this).  If your terminal supports it, you may also
+specify 24-bit RGB values as hex, like `#ff0ab3`.
 
 color.diff::
 	Whether to use ANSI escape sequences to add color to patches.
diff --git a/color.c b/color.c
index 6edbcae..78cdbed 100644
--- a/color.c
+++ b/color.c
@@ -32,10 +32,13 @@ struct color {
 		COLOR_UNSPECIFIED = 0,
 		COLOR_NORMAL,
 		COLOR_ANSI, /* basic 0-7 ANSI colors */
-		COLOR_256
+		COLOR_256,
+		COLOR_RGB
 	} state;
 	/* The numeric value for ANSI and 256-color modes */
 	unsigned char value;
+	/* 24-bit RGB color values */
+	unsigned char red, green, blue;
 };
 
 /*
@@ -47,6 +50,16 @@ static int match_word(const char *word, int len, const char *match)
 	return !strncasecmp(word, match, len) && !match[len];
 }
 
+static int get_hex_color(const char *in, unsigned char *out)
+{
+	unsigned int val;
+	val = (hexval(in[0]) << 4) | hexval(in[1]);
+	if (val & ~0xff)
+		return -1;
+	*out = val;
+	return 0;
+}
+
 static int parse_color(struct color *out, const char *name, int len)
 {
 	/* Positions in array must match ANSI color codes */
@@ -64,6 +77,16 @@ static int parse_color(struct color *out, const char *name, int len)
 		return 0;
 	}
 
+	/* Try a 24-bit RGB value */
+	if (len == 7 && name[0] == '#') {
+		if (!get_hex_color(name + 1, &out->red) &&
+		    !get_hex_color(name + 3, &out->green) &&
+		    !get_hex_color(name + 5, &out->blue)) {
+			out->state = COLOR_RGB;
+			return 0;
+		}
+	}
+
 	/* Then pick from our human-readable color names... */
 	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
 		if (match_word(name, len, color_names[i])) {
@@ -140,6 +163,10 @@ static char *color_output(char *out, const struct color *c, char type)
 	case COLOR_256:
 		out += sprintf(out, "%c8;5;%d", type, c->value);
 		break;
+	case COLOR_RGB:
+		out += sprintf(out, "%c8;2;%d;%d;%d", type,
+			       c->red, c->green, c->blue);
+		break;
 	}
 	return out;
 }
diff --git a/color.h b/color.h
index f5beab1..4ec34b4 100644
--- a/color.h
+++ b/color.h
@@ -9,14 +9,14 @@ struct strbuf;
  * The maximum length of ANSI color sequence we would generate:
  * - leading ESC '['            2
  * - attr + ';'                 2 * 8 (e.g. "1;")
- * - fg color + ';'             9 (e.g. "38;5;2xx;")
- * - fg color + ';'             9 (e.g. "48;5;2xx;")
+ * - 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.
  */
-#define COLOR_MAXLEN 40
+#define COLOR_MAXLEN 56
 
 /*
  * 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 63e4238..65386db 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -53,6 +53,10 @@ test_expect_success '256 colors' '
 	color "254 bold 255" "[1;38;5;254;48;5;255m"
 '
 
+test_expect_success '24-bit colors' '
+	color "#ff00ff black" "[38;2;255;0;255;40m"
+'
+
 test_expect_success '"normal" yields no color at all"' '
 	color "normal black" "[40m"
 '
-- 
2.2.0.rc2.402.g4519813
Junio C Hamano· Nov 20, 2014, 19:44 UTC · re: Jeff King · lore

Re: [PATCH 5/7] parse_color: support 24-bit RGB values

Jeff King <peff@peff.net> writes:
Show 60 quoted lines
> Some terminals (like XTerm) allow full 24-bit RGB color
> specifications using an extension to the regular ANSI color
> scheme. Let's allow users to specify hex RGB colors,
> enabling the all-important feature of hot pink ref
> decorations:
>
>   git log --format="%h%C(#ff69b4)%d%C(reset) %s"
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Also no clue on which terminals support it. I did all of my testing on
> a recent version of XTerm. It looks like it doesn't provide true 24-bit
> support, though. It is happy to accept the 24-bit colors, but if you do:
>
>   for b in $(seq 255); do
>     h=$(printf %02x $b)
>     git --no-pager log -1 --format="%C(#0000$h)$b%C(reset)"
>   done
>
> the gradient seems to "jump" in discrete steps. That's fine, though.
> It's a quality-of-implementation issue for the terminal, and I still
> think that the RGB spec is way more readable than the 256-color mode
> ones.
>
>  Documentation/config.txt |  3 ++-
>  color.c                  | 29 ++++++++++++++++++++++++++++-
>  color.h                  |  6 +++---
>  t/t4026-color.sh         |  4 ++++
>  4 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f615a5c..a237b82 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -842,7 +842,8 @@ doesn't matter.
>  +
>  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).
> +terminals may support this).  If your terminal supports it, you may also
> +specify 24-bit RGB values as hex, like `#ff0ab3`.
>  
>  color.diff::
>  	Whether to use ANSI escape sequences to add color to patches.
> diff --git a/color.c b/color.c
> index 6edbcae..78cdbed 100644
> --- a/color.c
> +++ b/color.c
> @@ -32,10 +32,13 @@ struct color {
>  		COLOR_UNSPECIFIED = 0,
>  		COLOR_NORMAL,
>  		COLOR_ANSI, /* basic 0-7 ANSI colors */
> -		COLOR_256
> +		COLOR_256,
> +		COLOR_RGB
>  	} state;
>  	/* The numeric value for ANSI and 256-color modes */
>  	unsigned char value;
> +	/* 24-bit RGB color values */
> +	unsigned char red, green, blue;

Do value and rgb have to be both valid at the same time, or is this "we are not wasting a byte by not using a union because it will be in the padding of the outer struct anyway"?

Not a satirical and/or rhetorical question.
Jeff King· Nov 20, 2014, 20:10 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/7] parse_color: support 24-bit RGB values

On Thu, Nov 20, 2014 at 11:44:26AM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> > @@ -32,10 +32,13 @@ struct color {
> >  		COLOR_UNSPECIFIED = 0,
> >  		COLOR_NORMAL,
> >  		COLOR_ANSI, /* basic 0-7 ANSI colors */
> > -		COLOR_256
> > +		COLOR_256,
> > +		COLOR_RGB
> >  	} state;
> >  	/* The numeric value for ANSI and 256-color modes */
> >  	unsigned char value;
> > +	/* 24-bit RGB color values */
> > +	unsigned char red, green, blue;
> 
> Do value and rgb have to be both valid at the same time, or is this
> "we are not wasting a byte by not using a union because it will be
> in the padding of the outer struct anyway"?

The latter. I started with a union, and then realized that COLOR_ANSI and COLOR_256 shared the value, so the union was not saving space and just getting in the way (mostly because I had to think of useful names for each of the members).

I'd be happy to do it as a union if you think that makes it clearer.

Also, the name "state" should perhaps be "type". It originally started as "unspecified or an actual value", which is a state, but as I worked, it grew into something more.

-Peff
Junio C Hamano· Nov 20, 2014, 20:25 UTC · re: Jeff King · lore

Re: [PATCH 5/7] parse_color: support 24-bit RGB values

Jeff King <peff@peff.net> writes:
Show 29 quoted lines
> On Thu, Nov 20, 2014 at 11:44:26AM -0800, Junio C Hamano wrote:
>
>> > @@ -32,10 +32,13 @@ struct color {
>> >  		COLOR_UNSPECIFIED = 0,
>> >  		COLOR_NORMAL,
>> >  		COLOR_ANSI, /* basic 0-7 ANSI colors */
>> > -		COLOR_256
>> > +		COLOR_256,
>> > +		COLOR_RGB
>> >  	} state;
>> >  	/* The numeric value for ANSI and 256-color modes */
>> >  	unsigned char value;
>> > +	/* 24-bit RGB color values */
>> > +	unsigned char red, green, blue;
>> 
>> Do value and rgb have to be both valid at the same time, or is this
>> "we are not wasting a byte by not using a union because it will be
>> in the padding of the outer struct anyway"?
>
> The latter. I started with a union, and then realized that COLOR_ANSI
> and COLOR_256 shared the value, so the union was not saving space and
> just getting in the way (mostly because I had to think of useful names
> for each of the members).
>
> I'd be happy to do it as a union if you think that makes it clearer.
>
> Also, the name "state" should perhaps be "type". It originally
> started as "unspecified or an actual value", which is a state, but
> as I worked, it grew into something more.
I think use of union might be more "kosher", e.g.
	struct color_spec {
        	enum { ... } type;
                union {
                	struct { unsigned char r, g, b; } rgb;
                        unsigned char ansi;
		} u;
	} c;

but it is not like you have an array of these things for each slot, and with the intervening ".u.<type>" you have to write every time you refer to these fields, the result is probably much uglier and harder to read. So let's only do s/state/type/ and leave these "ought to be union but that will be uglier" ones as they are.

Jeff King· Nov 20, 2014, 15:25 UTC · re: Jeff King · lore

[PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute

You can turn on ANSI text attributes like "reverse" by putting "reverse" in your color spec. However, you cannot ask to turn reverse off.

For common cases, this does not matter. You would turn on "reverse" at the start of a colored section, and then clear all attributes with a "reset". However, you may wish to turn on some attributes, then selectively disable others. For example:

  git log --format="%C(bold ul yellow)%h%C(noul) %s"

underlines just the hash, but without the need to re-specify the rest of the attributes. This can also help third-party programs, like contrib/diff-highlight, that want to turn some attribute on/off without disrupting existing coloring.

Note that some attribute specifications are probably nonsensical (e.g., "bold nobold"). We do not bother to flag such constructs, and instead let the terminal sort it out.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt |  3 ++-
 color.c                  |  8 +++++---
 color.h                  |  4 ++--
 t/t4026-color.sh         | 11 +++++++++++
 4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a237b82..7deae0b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -838,7 +838,8 @@ 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.
+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
diff --git a/color.c b/color.c
index 78cdbed..8dbd714 100644
--- a/color.c
+++ b/color.c
@@ -124,9 +124,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 };
+	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"
+		"bold", "dim", "ul", "blink", "reverse",
+		"nobold", "nodim", "noul", "noblink", "noreverse"
 	};
 	int i;
 	for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
@@ -238,7 +240,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 			attr &= ~bit;
 			if (sep++)
 				*dst++ = ';';
-			*dst++ = '0' + i;
+			dst += sprintf(dst, "%d", i);
 		}
 		if (!color_empty(&fg)) {
 			if (sep++)
diff --git a/color.h b/color.h
index 4ec34b4..7fe77fb 100644
--- a/color.h
+++ b/color.h
@@ -8,7 +8,7 @@ struct strbuf;
 /*
  * The maximum length of ANSI color sequence we would generate:
  * - leading ESC '['            2
- * - attr + ';'                 2 * 8 (e.g. "1;")
+ * - attr + ';'                 3 * 10 (e.g. "1;")
  * - fg color + ';'             17 (e.g. "38;2;255;255;255;")
  * - bg color + ';'             17 (e.g. "48;2;255;255;255;")
  * - terminating 'm' NUL        2
@@ -16,7 +16,7 @@ struct strbuf;
  * The above overcounts attr (we only use 5 not 8) and one semicolon
  * but it is close enough.
  */
-#define COLOR_MAXLEN 56
+#define COLOR_MAXLEN 70
 
 /*
  * 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 65386db..267c43b 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -45,10 +45,21 @@ test_expect_success 'fg bg attr...' '
 	color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
 '
 
+# note that nobold and nodim are the same code (22)
+test_expect_success 'attr negation' '
+	color "nobold nodim noul noblink noreverse" "[22;24;25;27m"
+'
+
 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"
 '
 
+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"
+'
+
 test_expect_success '256 colors' '
 	color "254 bold 255" "[1;38;5;254;48;5;255m"
 '
-- 
2.2.0.rc2.402.g4519813
Junio C Hamano· Nov 20, 2014, 19:46 UTC · re: Jeff King · lore

Re: [PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> You can turn on ANSI text attributes like "reverse" by
> putting "reverse" in your color spec. However, you cannot
> ask to turn reverse off.
>
> For common cases, this does not matter. You would turn on
> "reverse" at the start of a colored section, and then clear
> all attributes with a "reset". However, you may wish to turn
> on some attributes, then selectively disable others. For
> example:
>
>   git log --format="%C(bold ul yellow)%h%C(noul) %s"
>
> underlines just the hash, but without the need to re-specify
> the rest of the attributes. This can also help third-party
> programs, like contrib/diff-highlight, that want to turn
> some attribute on/off without disrupting existing coloring.
>
> Note that some attribute specifications are probably
> nonsensical (e.g., "bold nobold"). We do not bother to flag
> such constructs, and instead let the terminal sort it out.
>
> Signed-off-by: Jeff King <peff@peff.net>
Is "white noyellow" a new way to spell "blue" ;-)?
Thanks.
Jeff King· Nov 20, 2014, 15:29 UTC · re: Jeff King · lore

[PATCH 7/7] diff-highlight: allow configurable colors

Until now, the highlighting colors were hard-coded in the script (as "reverse" and "noreverse"), and you had to edit the script to change them. This patch teaches diff-highlight to read from color.diff-highlight.* to set them.

In addition, it expands the possiblities considerably by adding two features:

  1. Old/new lines can be colored independently (so you can
     use a color scheme that complements existing line
     coloring).
  2. Normal, unhighlighted parts of the lines can be colored,
     too. Technically this can be done by separately
     configuring color.diff.old/new and matching it to your
     diff-highlight colors. But you may want a different
     look for your highlighted diffs versus your regular
     diffs.
Signed-off-by: Jeff King <peff@peff.net>
---
I originally designed the (2) feature above to do the GitHub-style
background outlining that you mentioned. But it occurs to me that you
probably _do_ want to set your color.diff.old and color.diff.new
variables, as diff-highlight will only ever touch highlighted lines (so
a hunk with a single added line will not be touched at all). So I dunno.
Maybe part (2) here should be dropped, as it adds a reasonable amount of
complexity (just read the docs below to see what I mean).

I do not plan on using the feature myself. After experimenting with several color options, I do not really like any of them, and am perfectly happy to stick with "reverse/noreverse". :)

 contrib/diff-highlight/README         | 41 +++++++++++++++++++++++
 contrib/diff-highlight/diff-highlight | 62 +++++++++++++++++++++++++++--------
 2 files changed, 90 insertions(+), 13 deletions(-)
diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 502e03b..836b97a 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -58,6 +58,47 @@ following in your git configuration:
 	diff = diff-highlight | less
 ---------------------------------------------
 
+
+Color Config
+------------
+
+You can configure the highlight colors and attributes using git's
+config. The colors for "old" and "new" lines can be specified
+independently. There are two "modes" of configuration:
+
+  1. You can specify a "highlight" color and a matching "reset" color.
+     This will retain any existing colors in the diff, and apply the
+     "highlight" and "reset" colors before and after the highlighted
+     portion.
+
+  2. You can specify a "normal" color and a "highlight" color. In this
+     case, existing colors are dropped from that line. The non-highlighted
+     bits of the line get the "normal" color, and the highlights get the
+     "highlight" color.
+
+If no "new" colors are specified, they default to the "old" colors. If
+no "old" colors are specified, the default is to reverse the foreground
+and background for highlighted portions.
+
+Examples:
+
+---------------------------------------------
+# Underline highlighted portions
+[color "diff-highlight"]
+oldHighlight = ul
+oldReset = noul
+---------------------------------------------
+
+---------------------------------------------
+# Varying background intensities
+[color "diff-highlight"]
+oldNormal = "black #f8cbcb"
+oldHighlight = "black #ffaaaa"
+newNormal = "black #cbeecb"
+newHighlight = "black #aaffaa"
+---------------------------------------------
+
+
 Bugs
 ----
 
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 69a652e..08c88bb 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -5,8 +5,18 @@ use strict;
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
-my $HIGHLIGHT   = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
+my @OLD_HIGHLIGHT = (
+	color_config('color.diff-highlight.oldnormal'),
+	color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
+	color_config('color.diff-highlight.oldreset', "\x1b[27m")
+);
+my @NEW_HIGHLIGHT = (
+	color_config('color.diff-highlight.newnormal', $OLD_HIGHLIGHT[0]),
+	color_config('color.diff-highlight.newhighlight', $OLD_HIGHLIGHT[1]),
+	color_config('color.diff-highlight.newreset', $OLD_HIGHLIGHT[2])
+);
+
+my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
@@ -57,6 +67,17 @@ show_hunk(\@removed, \@added);
 
 exit 0;
 
+# Ideally we would feed the default as a human-readable color to
+# git-config as the fallback value. But diff-highlight does
+# not otherwise depend on git at all, and there are reports
+# of it being used in other settings. Let's handle our own
+# fallback, which means we will work even if git can't be run.
+sub color_config {
+	my ($key, $default) = @_;
+	my $s = `git config --get-color $key 2>/dev/null`;
+	return length($s) ? $s : $default;
+}
+
 sub show_hunk {
 	my ($a, $b) = @_;
 
@@ -132,8 +153,8 @@ sub highlight_pair {
 	}
 
 	if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
-		return highlight_line(\@a, $pa, $sa),
-		       highlight_line(\@b, $pb, $sb);
+		return highlight_line(\@a, $pa, $sa, \@OLD_HIGHLIGHT),
+		       highlight_line(\@b, $pb, $sb, \@NEW_HIGHLIGHT);
 	}
 	else {
 		return join('', @a),
@@ -148,15 +169,30 @@ sub split_line {
 }
 
 sub highlight_line {
-	my ($line, $prefix, $suffix) = @_;
-
-	return join('',
-		@{$line}[0..($prefix-1)],
-		$HIGHLIGHT,
-		@{$line}[$prefix..$suffix],
-		$UNHIGHLIGHT,
-		@{$line}[($suffix+1)..$#$line]
-	);
+	my ($line, $prefix, $suffix, $theme) = @_;
+
+	my $start = join('', @{$line}[0..($prefix-1)]);
+	my $mid = join('', @{$line}[$prefix..$suffix]);
+	my $end = join('', @{$line}[($suffix+1)..$#$line]);
+
+	# If we have a "normal" color specified, then take over the whole line.
+	# Otherwise, we try to just manipulate the highlighted bits.
+	if (defined $theme->[0]) {
+		s/$COLOR//g for ($start, $mid, $end);
+		chomp $end;
+		return join('',
+			$theme->[0], $start, $RESET,
+			$theme->[1], $mid, $RESET,
+			$theme->[0], $end, $RESET,
+			"\n"
+		);
+	} else {
+		return join('',
+			$start,
+			$theme->[1], $mid, $theme->[2],
+			$end
+		);
+	}
 }
 
 # Pairs are interesting to highlight only if we are going to end up
-- 
2.2.0.rc2.402.g4519813

← back to recent threads