Re: diff-highlight highlight words?

Re: diff-highlight highlight words?

[Jeff King]

[+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

Re: diff-highlight highlight words?

[Scott Baker]

On 11/11/2014 11:56 PM, Jeff King wrote:
> [+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

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

[Jeff King]

On Wed, Nov 12, 2014 at 09:59:35AM -0800, Scott Baker wrote:

> 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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

[PATCH 4/7] parse_color: refactor color storage

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

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/

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Johannes Sixt]

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

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

[Jeff King]

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.

-Peff

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

[Johannes Sixt]

Am 09.12.2014 um 21:21 schrieb Jeff King:
> 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

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

[Eric Sunshine]

On Tue, Dec 9, 2014 at 3:14 PM, Johannes Sixt <j6t@kdbg.org> 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?

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

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

[Jeff King]

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