diff-highlight highlight words?

[+cc git@vger, since this may be of interest to others]

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:

index c4404d4..c99de99 100755
@@ -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):

index c4404d4..4e08f3c 100755
@@ -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:

index 4e08f3c..6f98db4 100755
@@ -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

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

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

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

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

index 9220725..f615a5c 100644
@@ -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

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

---
 builtin/config.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

index 8cc2604..fddafbb 100644
@@ -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

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.

---
 t/t4026-color.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

index 3726a0e..63e4238 100755
@@ -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

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

Argh, s/specifiers/specifies/

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

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.

---
 color.c | 138 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 32 deletions(-)

index 7941e93..6edbcae 100644
@@ -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

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.

Am 20.11.2014 um 16:17 schrieb Jeff King:

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:

index e2a0a99..809b359 100644
@@ -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

Am 09.12.2014 um 21:21 schrieb Jeff King:

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

-- Hannes

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.

---
 color.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index e2a0a99..809b359 100644
@@ -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

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

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"

---
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(-)

index f615a5c..a237b82 100644
@@ -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.

index 6edbcae..78cdbed 100644
@@ -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;
 }

index f5beab1..4ec34b4 100644
@@ -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,

index 63e4238..65386db 100755
@@ -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

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

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.

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.

---
 Documentation/config.txt |  3 ++-
 color.c                  |  8 +++++---
 color.h                  |  4 ++--
 t/t4026-color.sh         | 11 +++++++++++
 4 files changed, 20 insertions(+), 6 deletions(-)

index a237b82..7deae0b 100644
@@ -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

index 78cdbed..8dbd714 100644
@@ -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++)

index 4ec34b4..7fe77fb 100644
@@ -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,

index 65386db..267c43b 100755
@@ -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

Is "white noyellow" a new way to spell "blue" ;-)?

Thanks.

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.

---
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(-)

index 502e03b..836b97a 100644
@@ -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
 ----
 

2.2.0.rc2.402.g4519813