[PATCH 0/8] some diff-highlight tweaks

[PATCH 0/8] some diff-highlight tweaks

[Jeff King]

Here are a few small changes to diff-highlight. The main motivation is
working better with diff-so-fancy, which uses DiffHighlight.pm under the
hood. But it was a good opportunity to polish up the tests and README,
and the final patch implements a small optimization I'd been meaning to
do for a while.

I based these on the bugfix patch I sent a few days ago in:

  https://lore.kernel.org/git/20260317230223.GA716496@coredump.intra.peff.net/

They don't _need_ to come after that, but there are otherwise textual
conflicts as they both add new tests in the same spot.

  [1/8]: diff-highlight: mention build instructions
  [2/8]: diff-highlight: drop perl version dependency back to 5.8
  [3/8]: diff-highlight: check diff-highlight exit status in tests
  [4/8]: t: add matching negative attributes to test_decode_color
  [5/8]: diff-highlight: use test_decode_color in tests
  [6/8]: diff-highlight: test color config
  [7/8]: diff-highlight: allow module callers to pass in color config
  [8/8]: diff-highlight: fetch all config with one process

 contrib/diff-highlight/DiffHighlight.pm       | 57 ++++++++++++----
 contrib/diff-highlight/README                 | 19 +++++-
 .../diff-highlight/t/t9400-diff-highlight.sh  | 67 +++++++++++++------
 t/test-lib-functions.sh                       |  3 +
 4 files changed, 111 insertions(+), 35 deletions(-)

-Peff

[PATCH 1/8] diff-highlight: mention build instructions

[Jeff King]

Once upon a time, this was just a script in a directory that could be
run directly. That changed in 0c977dbc81 (diff-highlight: split code
into module, 2017-06-15). Let's update the README to make it more clear
that you need to run make.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/README | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 1db4440e68..9c89146fb0 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -39,10 +39,21 @@ visually distracting.  Non-diff lines and existing diff coloration is
 preserved; the intent is that the output should look exactly the same as
 the input, except for the occasional highlight.
 
+Build/Install
+-------------
+
+You can build the `diff-highlight` script by running `make` from within
+the diff-highlight directory. There is no `make install` target; you can
+copy the built script to your $PATH.
+
+You can run diff-highlight's internal tests by running `make test`. Note
+that you must also build Git itself first (by running `make` from the
+top-level of the project).
+
 Use
 ---
 
-You can try out the diff-highlight program with:
+You can try out the built diff-highlight program with:
 
 ---------------------------------------------
 git log -p --color | /path/to/diff-highlight
-- 
2.53.0.945.ge67b727e8d

[PATCH 2/8] diff-highlight: drop perl version dependency back to 5.8

[Jeff King]

From: Scott Baker <scott@perturb.org>

The diff-highlight code does not rely on any perl features beyond what
perl 5.8 provides. We bumped it to v5.26 along with the rest of the
project's perl scripts in 702d8c1f3b (Require Perl 5.26.0, 2024-10-23).

There's some value in just having a uniform baseline for the project,
but I think diff-highlight is special here:

  - it's in a contrib/ directory that is not frequently touched, so
    there is little risk of Git developers getting annoyed that modern
    perl features are not available

  - it provides a module used by other projects. In particular,
    diff-so-fancy relies on DiffHighlight.pm but does not otherwise
    require a perl version more modern than 5.8.

Let's drop back to the more conservative requirement.

Signed-off-by: Scott Baker <scott@perturb.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/DiffHighlight.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index f0607a4b68..a5e5de3b18 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -1,6 +1,6 @@
 package DiffHighlight;
 
-require v5.26;
+require v5.008;
 use warnings FATAL => 'all';
 use strict;
 
-- 
2.53.0.945.ge67b727e8d

[PATCH 3/8] diff-highlight: check diff-highlight exit status in tests

[Jeff King]

When testing diff-highlight, we pipe the output through a sanitizing
function. This loses the exit status of diff-highlight itself, which
could mean we are missing cases where it crashes or exits unexpectedly.
Use an extra tempfile to avoid the pipe.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 2a9b68cf3b..42d331c6cd 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -41,8 +41,10 @@ dh_test () {
 		git show >commit.raw
 	} >/dev/null &&
 
-	"$DIFF_HIGHLIGHT" <diff.raw | test_strip_patch_header >diff.act &&
-	"$DIFF_HIGHLIGHT" <commit.raw | test_strip_patch_header >commit.act &&
+	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
+	test_strip_patch_header <diff.hi >diff.act
+	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
+	test_strip_patch_header <commit.hi >commit.act &&
 	test_cmp patch.exp diff.act &&
 	test_cmp patch.exp commit.act
 }
-- 
2.53.0.945.ge67b727e8d

[PATCH 4/8] t: add matching negative attributes to test_decode_color

[Jeff King]

Most of the ANSI color attributes have an "off" variant. We don't use
these yet in our test suite, so we never bothered to decode them. Add
the ones that match the attributes we encode so we can make use of them.

There are even more attributes not covered on the positive side, so this
is meant to be useful but not all-inclusive.

Note that "nobold" and "nodim" are the same code, so I've decoded this
as "normal intensity".

Signed-off-by: Jeff King <peff@peff.net>
---
This is the only patch that touches anything outside of
contrib/diff-highlight, but hopefully it is uncontroversial. ;)

 t/test-lib-functions.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 14e238d24d..f3af10fb7e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -48,6 +48,9 @@ test_decode_color () {
 			if (n == 2) return "FAINT";
 			if (n == 3) return "ITALIC";
 			if (n == 7) return "REVERSE";
+			if (n == 22) return "NORMAL_INTENSITY";
+			if (n == 23) return "NOITALIC";
+			if (n == 27) return "NOREVERSE";
 			if (n == 30) return "BLACK";
 			if (n == 31) return "RED";
 			if (n == 32) return "GREEN";
-- 
2.53.0.945.ge67b727e8d

[PATCH 5/8] diff-highlight: use test_decode_color in tests

[Jeff King]

The diff-highlight tests use raw color bytes when comparing expected and
actual output. Let's use test_decode_color, which is our usual technique
in other tests. It makes reading test output diffs a bit easier, since
you're not relying on your terminal to interpret the result (or worse,
interpreting characters yourself via "cat -A").

This will also make it easier to add tests with new colors/attributes,
without having to pre-define the byte sequences ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 .../diff-highlight/t/t9400-diff-highlight.sh  | 37 +++++++++----------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 42d331c6cd..ba80cda7c8 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -7,9 +7,6 @@ TEST_OUTPUT_DIRECTORY=$(pwd)
 TEST_DIRECTORY="$CURR_DIR"/../../../t
 DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
 
-CW="$(printf "\033[7m")"	# white
-CR="$(printf "\033[27m")"	# reset
-
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . "$TEST_DIRECTORY"/test-lib.sh
@@ -42,9 +39,9 @@ dh_test () {
 	} >/dev/null &&
 
 	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
-	test_strip_patch_header <diff.hi >diff.act
+	test_strip_patch_header <diff.hi | test_decode_color >diff.act
 	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
-	test_strip_patch_header <commit.hi >commit.act &&
+	test_strip_patch_header <commit.hi | test_decode_color >commit.act &&
 	test_cmp patch.exp diff.act &&
 	test_cmp patch.exp commit.act
 }
@@ -126,8 +123,8 @@ test_expect_success 'diff-highlight highlights the beginning of a line' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-${CW}b${CR}bb
-		+${CW}0${CR}bb
+		-<REVERSE>b<NOREVERSE>bb
+		+<REVERSE>0<NOREVERSE>bb
 		 ccc
 	EOF
 '
@@ -148,8 +145,8 @@ test_expect_success 'diff-highlight highlights the end of a line' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-bb${CW}b${CR}
-		+bb${CW}0${CR}
+		-bb<REVERSE>b<NOREVERSE>
+		+bb<REVERSE>0<NOREVERSE>
 		 ccc
 	EOF
 '
@@ -170,8 +167,8 @@ test_expect_success 'diff-highlight highlights the middle of a line' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-b${CW}b${CR}b
-		+b${CW}0${CR}b
+		-b<REVERSE>b<NOREVERSE>b
+		+b<REVERSE>0<NOREVERSE>b
 		 ccc
 	EOF
 '
@@ -213,8 +210,8 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-b${CW}b${CR}b
-		+b${CW}0${CR}b
+		-b<REVERSE>b<NOREVERSE>b
+		+b<REVERSE>0<NOREVERSE>b
 		+ccc
 	EOF
 '
@@ -232,8 +229,8 @@ test_expect_success 'diff-highlight treats multibyte utf-8 as a unit' '
 	echo "unic${o_stroke}de" >b &&
 	dh_test a b <<-EOF
 		@@ -1 +1 @@
-		-unic${CW}${o_accent}${CR}de
-		+unic${CW}${o_stroke}${CR}de
+		-unic<REVERSE>${o_accent}<NOREVERSE>de
+		+unic<REVERSE>${o_stroke}<NOREVERSE>de
 	EOF
 '
 
@@ -250,8 +247,8 @@ test_expect_failure 'diff-highlight treats combining code points as a unit' '
 	echo "unico${combine_circum}de" >b &&
 	dh_test a b <<-EOF
 		@@ -1 +1 @@
-		-unic${CW}o${combine_accent}${CR}de
-		+unic${CW}o${combine_circum}${CR}de
+		-unic<REVERSE>o${combine_accent}<NOREVERSE>de
+		+unic<REVERSE>o${combine_circum}<NOREVERSE>de
 	EOF
 '
 
@@ -333,12 +330,12 @@ test_expect_success 'diff-highlight handles --graph with leading dash' '
 	+++ b/file
 	@@ -1,3 +1,3 @@
 	 before
-	-the ${CW}old${CR} line
-	+the ${CW}new${CR} line
+	-the <REVERSE>old<NOREVERSE> line
+	+the <REVERSE>new<NOREVERSE> line
 	 -leading dash
 	EOF
 	git log --graph -p -1 | "$DIFF_HIGHLIGHT" >actual.raw &&
-	trim_graph <actual.raw | sed -n "/^---/,\$p" >actual &&
+	trim_graph <actual.raw | sed -n "/^---/,\$p" | test_decode_color >actual &&
 	test_cmp expect actual
 '
 
-- 
2.53.0.945.ge67b727e8d

[PATCH 6/8] diff-highlight: test color config

[Jeff King]

We added configurable colors long ago in bca45fbc1f (diff-highlight:
allow configurable colors, 2014-11-20), but never actually tested it.
Since we'll be touching the color code in a moment, this is a good time
to beef up the tests.

Note that we cover both the highlight/reset style used by the default
colors, as well as the normal/highlight style added by that commit
(which was previously totally untested).

Signed-off-by: Jeff King <peff@peff.net>
---
 .../diff-highlight/t/t9400-diff-highlight.sh  | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index ba80cda7c8..828d59e9c6 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -350,4 +350,32 @@ test_expect_success 'highlight diff that removes final newline' '
 	EOF
 '
 
+test_expect_success 'configure set/reset colors' '
+	test_config color.diff-highlight.oldhighlight bold &&
+	test_config color.diff-highlight.oldreset nobold &&
+	test_config color.diff-highlight.newhighlight italic &&
+	test_config color.diff-highlight.newreset noitalic &&
+	echo "prefix a suffix" >a &&
+	echo "prefix b suffix" >b &&
+	dh_test a b <<-\EOF
+	@@ -1 +1 @@
+	-prefix <BOLD>a<NORMAL_INTENSITY> suffix
+	+prefix <ITALIC>b<NOITALIC> suffix
+	EOF
+'
+
+test_expect_success 'configure normal/highlight colors' '
+	test_config color.diff-highlight.oldnormal red &&
+	test_config color.diff-highlight.oldhighlight magenta &&
+	test_config color.diff-highlight.newnormal green &&
+	test_config color.diff-highlight.newhighlight yellow &&
+	echo "prefix a suffix" >a &&
+	echo "prefix b suffix" >b &&
+	dh_test a b <<-\EOF
+	@@ -1 +1 @@
+	<RED>-prefix <RESET><MAGENTA>a<RESET><RED> suffix<RESET>
+	<GREEN>+prefix <RESET><YELLOW>b<RESET><GREEN> suffix<RESET>
+	EOF
+'
+
 test_done
-- 
2.53.0.945.ge67b727e8d

[PATCH 7/8] diff-highlight: allow module callers to pass in color config

[Jeff King]

From: Scott Baker <scott@perturb.org>

Users of the module may want to pass in their own color config for a few
obvious reasons:

  - they are pulling the config from different variables than
    diff-highlight itself uses

  - they are loading the config in a more efficient way (say, by parsing
    git-config --list) and don't want to incur the six (!) git-config
    calls that DiffHighlight.pm runs to check all config

Let's allow users of the module to pass in the color config, and
lazy-load it when needed if they haven't.

Signed-off-by: Scott Baker <scott@perturb.org>
Signed-off-by: Jeff King <peff@peff.net>
---
The next commit improves the six-process situation, but I think
diff-so-fancy will still want this, as it uses a single invocation
which checks other non-color config (and already bit the bullet on
implementing its own color parsing).

 contrib/diff-highlight/DiffHighlight.pm | 41 +++++++++++++++++--------
 contrib/diff-highlight/README           |  6 ++++
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index a5e5de3b18..96369eadf9 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -9,18 +9,11 @@ package DiffHighlight;
 
 my $NULL = File::Spec->devnull();
 
-# Highlight by reversing foreground and background. You could do
-# other things like bold or underline if you prefer.
-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])
-);
+# The color theme is initially set to nothing here to allow outside callers
+# to set the colors for their application. If nothing is sent in we use
+# colors from git config in load_color_config().
+our @OLD_HIGHLIGHT = ();
+our @NEW_HIGHLIGHT = ();
 
 my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
@@ -170,6 +163,29 @@ sub show_hunk {
 	$line_cb->(@queue);
 }
 
+sub load_color_config {
+	# If the colors were NOT set from outside this module we load them on-demand
+	# from the git config. Note that only one of elements 0 and 2 in each
+	# array is used (depending on whether you are doing set/unset on an
+	# attribute, or specifying normal vs highlighted coloring). So we use
+	# element 1 as our check for whether colors were passed in; it should
+	# always be set if you want highlighting to do anything.
+	if (!defined $OLD_HIGHLIGHT[1]) {
+		@OLD_HIGHLIGHT = (
+			color_config('color.diff-highlight.oldnormal'),
+			color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
+			color_config('color.diff-highlight.oldreset', "\x1b[27m")
+		);
+	}
+	if (!defined $NEW_HIGHLIGHT[1]) {
+		@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])
+		);
+	};
+}
+
 sub highlight_pair {
 	my @a = split_line(shift);
 	my @b = split_line(shift);
@@ -218,6 +234,7 @@ sub highlight_pair {
 	}
 
 	if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
+		load_color_config();
 		return highlight_line(\@a, $pa, $sa, \@OLD_HIGHLIGHT),
 		       highlight_line(\@b, $pb, $sb, \@NEW_HIGHLIGHT);
 	}
diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 9c89146fb0..ed8d876a18 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -138,6 +138,12 @@ Your script may set up one or more of the following variables:
     processing a logical chunk of input). The default function flushes
     stdout.
 
+  - @DiffHighlight::OLD_HIGHLIGHT and @DiffHighlight::NEW_HIGHLIGHT - these
+    arrays specify the normal, highlighted, and reset colors (in that order)
+    for old/new lines. If unset, values will be retrieved by calling `git
+    config` (see "Color Config" above). Note that these should be the literal
+    color bytes (starting with an ANSI escape code), not color names.
+
 The script may then feed lines, one at a time, to DiffHighlight::handle_line().
 When lines are done processing, they will be fed to $line_cb. Note that
 DiffHighlight may queue up many input lines (to analyze a whole hunk)
-- 
2.53.0.945.ge67b727e8d

[PATCH 8/8] diff-highlight: fetch all config with one process

[Jeff King]

When diff-highlight was written, there was no way to fetch multiple
config keys _and_ have them interpreted as colors. So we were stuck
with either invoking git-config once for each config key, or fetching
them all and converting human-readable color names into ANSI codes
ourselves.

I chose the former, but it means that diff-highlight kicks off 6
git-config processes (even if you haven't configured anything, it has to
check each one).

But since Git 2.18.0, we can do:

   git config --type=color --get-regexp=^color\.diff-highlight\.

to get all of them in one shot.

Note that any callers which pass in colors directly to the module via
@OLD_HIGHLIGHT and @NEW_HIGHLIGHT (like diff-so-fancy plans to do) are
unaffected; those colors suppress any config lookup we'd do ourselves.

You can see the effect like:

  # diff-highlight suppresses git-config's stderr, so dump
  # trace through descriptor 3
  git show d1f33c753d | GIT_TRACE=3 diff-highlight 3>&2 >/dev/null

which drops from 6 lines down to 1.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/DiffHighlight.pm | 26 ++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index 96369eadf9..a22ba7a851 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -131,8 +131,20 @@ sub highlight_stdin {
 # 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 {
+	our $cached_config;
 	my ($key, $default) = @_;
-	my $s = `git config --get-color $key 2>$NULL`;
+
+	if (!defined $cached_config) {
+		$cached_config = {};
+		my $data = `git config --type=color --get-regexp '^color\.diff-highlight\.' 2>$NULL`;
+		for my $line (split /\n/, $data) {
+			my ($key, $color) = split ' ', $line, 2;
+			$key =~ s/^color\.diff-highlight\.// or next;
+			$cached_config->{$key} = $color;
+		}
+	}
+
+	my $s = $cached_config->{$key};
 	return length($s) ? $s : $default;
 }
 
@@ -172,16 +184,16 @@ sub load_color_config {
 	# always be set if you want highlighting to do anything.
 	if (!defined $OLD_HIGHLIGHT[1]) {
 		@OLD_HIGHLIGHT = (
-			color_config('color.diff-highlight.oldnormal'),
-			color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
-			color_config('color.diff-highlight.oldreset', "\x1b[27m")
+			color_config('oldnormal'),
+			color_config('oldhighlight', "\x1b[7m"),
+			color_config('oldreset', "\x1b[27m")
 		);
 	}
 	if (!defined $NEW_HIGHLIGHT[1]) {
 		@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])
+			color_config('newnormal', $OLD_HIGHLIGHT[0]),
+			color_config('newhighlight', $OLD_HIGHLIGHT[1]),
+			color_config('newreset', $OLD_HIGHLIGHT[2])
 		);
 	};
 }
-- 
2.53.0.945.ge67b727e8d

Re: [PATCH 8/8] diff-highlight: fetch all config with one process

[Tian Yuchen]

Hi Jeff,

On 3/20/26 08:48, Jeff King wrote:
> When diff-highlight was written, there was no way to fetch multiple
> config keys _and_ have them interpreted as colors. So we were stuck
> with either invoking git-config once for each config key, or fetching
> them all and converting human-readable color names into ANSI codes
> ourselves.
> 
> I chose the former, but it means that diff-highlight kicks off 6
> git-config processes (even if you haven't configured anything, it has to
> check each one).
> 
> But since Git 2.18.0, we can do:
> 
>     git config --type=color --get-regexp=^color\.diff-highlight\.
> 
> to get all of them in one shot.
> 
> Note that any callers which pass in colors directly to the module via
> @OLD_HIGHLIGHT and @NEW_HIGHLIGHT (like diff-so-fancy plans to do) are
> unaffected; those colors suppress any config lookup we'd do ourselves.
> 
> You can see the effect like:
> 
>    # diff-highlight suppresses git-config's stderr, so dump
>    # trace through descriptor 3
>    git show d1f33c753d | GIT_TRACE=3 diff-highlight 3>&2 >/dev/null
> 
> which drops from 6 lines down to 1.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   contrib/diff-highlight/DiffHighlight.pm | 26 ++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
> index 96369eadf9..a22ba7a851 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -131,8 +131,20 @@ sub highlight_stdin {
>   # 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 {
> +	our $cached_config;
>   	my ($key, $default) = @_;

$key...

> -	my $s = `git config --get-color $key 2>$NULL`;
> +
> +	if (!defined $cached_config) {
> +		$cached_config = {};
> +		my $data = `git config --type=color --get-regexp '^color\.diff-highlight\.' 2>$NULL`;
> +		for my $line (split /\n/, $data) {
> +			my ($key, $color) = split ' ', $line, 2;

...another $key. I think it would be better to change the name here. 
What do you think?

> +			$key =~ s/^color\.diff-highlight\.// or next;
> +			$cached_config->{$key} = $color;
> +		}
> +	}
> +

...

> +	my $s = $cached_config->{$key};
>   	return length($s) ? $s : $default;
>   }
>  

Something doesn't feel quite right here.

If the user has not configured color.diff-highlight.*, the expression 
git config --type=color --get-regexp=^color\.diff-highlight\. will not 
find a match and should not output anything. In this case, 
%cached_config->{$key} becomes undef, length() returns 0, and a warning 
is issued.

But we have "use warnings FATAL => 'all'". This situation will result in 
a fatal error, which I don't think is what we want.


> @@ -172,16 +184,16 @@ sub load_color_config {
>   	# always be set if you want highlighting to do anything.
>   	if (!defined $OLD_HIGHLIGHT[1]) {
>   		@OLD_HIGHLIGHT = (
> -			color_config('color.diff-highlight.oldnormal'),
> -			color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
> -			color_config('color.diff-highlight.oldreset', "\x1b[27m")
> +			color_config('oldnormal'),
> +			color_config('oldhighlight', "\x1b[7m"),
> +			color_config('oldreset', "\x1b[27m")
>   		);
>   	}
>   	if (!defined $NEW_HIGHLIGHT[1]) {
>   		@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])
> +			color_config('newnormal', $OLD_HIGHLIGHT[0]),
> +			color_config('newhighlight', $OLD_HIGHLIGHT[1]),
> +			color_config('newreset', $OLD_HIGHLIGHT[2])
>   		);
>   	};
>   }

Regards,

Yuchen

Re: [PATCH 5/8] diff-highlight: use test_decode_color in tests

[Tian Yuchen]

On 3/20/26 08:44, Jeff King wrote:
> The diff-highlight tests use raw color bytes when comparing expected and
> actual output. Let's use test_decode_color, which is our usual technique
> in other tests. It makes reading test output diffs a bit easier, since
> you're not relying on your terminal to interpret the result (or worse,
> interpreting characters yourself via "cat -A").
> 
> This will also make it easier to add tests with new colors/attributes,
> without having to pre-define the byte sequences ourselves.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   .../diff-highlight/t/t9400-diff-highlight.sh  | 37 +++++++++----------
>   1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> index 42d331c6cd..ba80cda7c8 100755
> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -7,9 +7,6 @@ TEST_OUTPUT_DIRECTORY=$(pwd)
>   TEST_DIRECTORY="$CURR_DIR"/../../../t
>   DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
>   
> -CW="$(printf "\033[7m")"	# white
> -CR="$(printf "\033[27m")"	# reset
> -
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   . "$TEST_DIRECTORY"/test-lib.sh
> @@ -42,9 +39,9 @@ dh_test () {
>   	} >/dev/null &&
>   
>   	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
> -	test_strip_patch_header <diff.hi >diff.act
> +	test_strip_patch_header <diff.hi | test_decode_color >diff.act

Although this is just simple text filtering and leaving it as is 
wouldn’t cause any problems IMO, why not go ahead and add the && while 
you’re at it?

I've noticed that there are several missing &&.

>   	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
> -	test_strip_patch_header <commit.hi >commit.act &&
> +	test_strip_patch_header <commit.hi | test_decode_color >commit.act &&
>   	test_cmp patch.exp diff.act &&
>   	test_cmp patch.exp commit.act
>   }
> @@ -126,8 +123,8 @@ test_expect_success 'diff-highlight highlights the beginning of a line' '
>   	dh_test a b <<-EOF
>   		@@ -1,3 +1,3 @@
>   		 aaa
> -		-${CW}b${CR}bb
> -		+${CW}0${CR}bb
> +		-<REVERSE>b<NOREVERSE>bb
> +		+<REVERSE>0<NOREVERSE>bb
>   		 ccc
>   	EOF
>   '
> @@ -148,8 +145,8 @@ test_expect_success 'diff-highlight highlights the end of a line' '
>   	dh_test a b <<-EOF
>   		@@ -1,3 +1,3 @@
>   		 aaa
> -		-bb${CW}b${CR}
> -		+bb${CW}0${CR}
> +		-bb<REVERSE>b<NOREVERSE>
> +		+bb<REVERSE>0<NOREVERSE>
>   		 ccc
>   	EOF
>   '
> @@ -170,8 +167,8 @@ test_expect_success 'diff-highlight highlights the middle of a line' '
>   	dh_test a b <<-EOF
>   		@@ -1,3 +1,3 @@
>   		 aaa
> -		-b${CW}b${CR}b
> -		+b${CW}0${CR}b
> +		-b<REVERSE>b<NOREVERSE>b
> +		+b<REVERSE>0<NOREVERSE>b
>   		 ccc
>   	EOF
>   '
> @@ -213,8 +210,8 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
>   	dh_test a b <<-EOF
>   		@@ -1,3 +1,3 @@
>   		 aaa
> -		-b${CW}b${CR}b
> -		+b${CW}0${CR}b
> +		-b<REVERSE>b<NOREVERSE>b
> +		+b<REVERSE>0<NOREVERSE>b
>   		+ccc
>   	EOF
>   '
> @@ -232,8 +229,8 @@ test_expect_success 'diff-highlight treats multibyte utf-8 as a unit' '
>   	echo "unic${o_stroke}de" >b &&
>   	dh_test a b <<-EOF
>   		@@ -1 +1 @@
> -		-unic${CW}${o_accent}${CR}de
> -		+unic${CW}${o_stroke}${CR}de
> +		-unic<REVERSE>${o_accent}<NOREVERSE>de
> +		+unic<REVERSE>${o_stroke}<NOREVERSE>de
>   	EOF
>   '
>   
> @@ -250,8 +247,8 @@ test_expect_failure 'diff-highlight treats combining code points as a unit' '
>   	echo "unico${combine_circum}de" >b &&
>   	dh_test a b <<-EOF
>   		@@ -1 +1 @@
> -		-unic${CW}o${combine_accent}${CR}de
> -		+unic${CW}o${combine_circum}${CR}de
> +		-unic<REVERSE>o${combine_accent}<NOREVERSE>de
> +		+unic<REVERSE>o${combine_circum}<NOREVERSE>de
>   	EOF
>   '
>   
> @@ -333,12 +330,12 @@ test_expect_success 'diff-highlight handles --graph with leading dash' '
>   	+++ b/file
>   	@@ -1,3 +1,3 @@
>   	 before
> -	-the ${CW}old${CR} line
> -	+the ${CW}new${CR} line
> +	-the <REVERSE>old<NOREVERSE> line
> +	+the <REVERSE>new<NOREVERSE> line
>   	 -leading dash
>   	EOF
>   	git log --graph -p -1 | "$DIFF_HIGHLIGHT" >actual.raw &&
> -	trim_graph <actual.raw | sed -n "/^---/,\$p" >actual &&
> +	trim_graph <actual.raw | sed -n "/^---/,\$p" | test_decode_color >actual &&
>   	test_cmp expect actual
>   '
>   

Thanks,

Yuchen

Re: [PATCH 8/8] diff-highlight: fetch all config with one process

[Jeff King]

On Mon, Mar 23, 2026 at 01:18:30AM +0800, Tian Yuchen wrote:

> > -	my $s = `git config --get-color $key 2>$NULL`;
> > +
> > +	if (!defined $cached_config) {
> > +		$cached_config = {};
> > +		my $data = `git config --type=color --get-regexp '^color\.diff-highlight\.' 2>$NULL`;
> > +		for my $line (split /\n/, $data) {
> > +			my ($key, $color) = split ' ', $line, 2;
> 
> ...another $key. I think it would be better to change the name here. What do
> you think?

I noticed it, too, but didn't have a better name (in fact they are of
the same type, just two different contexts). Shadowing seemed less bad
to me than using a mis-matched name.

> > +	my $s = $cached_config->{$key};
> >   	return length($s) ? $s : $default;
> >   }
> 
> Something doesn't feel quite right here.
> 
> If the user has not configured color.diff-highlight.*, the expression git
> config --type=color --get-regexp=^color\.diff-highlight\. will not find a
> match and should not output anything. In this case, %cached_config->{$key}
> becomes undef, length() returns 0, and a warning is issued.

The length() of undef is also undef (and documented in "perldoc -f
length"). But either way, length($s) will be false, and we will return
$default, not $s.

I don't get any warning on perl 5.40.1. Are you seeing one on a
different version?

-Peff

Re: [PATCH 5/8] diff-highlight: use test_decode_color in tests

[Jeff King]

On Mon, Mar 23, 2026 at 01:24:00AM +0800, Tian Yuchen wrote:

> > @@ -42,9 +39,9 @@ dh_test () {
> >   	} >/dev/null &&
> >   	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
> > -	test_strip_patch_header <diff.hi >diff.act
> > +	test_strip_patch_header <diff.hi | test_decode_color >diff.act
> 
> Although this is just simple text filtering and leaving it as is wouldn’t
> cause any problems IMO, why not go ahead and add the && while you’re at it?

The bug is in an earlier commit (patch 3), which breaks apart the pipe
but doesn't add the necessary &&. And it's more than just text
filtering; it breaks the &&-chain, so we miss the exit code of
$DIFF_HIGHLIGHT (which was the whole point of patch 3).

chainlint doesn't find it because we're in a helper function, not
directinly in a test snippet.

I'll send a revised series to fix it, but...

> I've noticed that there are several missing &&.

Where else do you see?

Or do you mean that we should not pipe text filtering commands? There
I'd disagree. We are not likely to see a failure from 'sed', and if we
do, the fact that the output does not match would catch it. And the cost
of breaking every command down without pipes means having to manage lots
of intermediate files.

-Peff

Re: [PATCH 8/8] diff-highlight: fetch all config with one process

[Tian Yuchen]

On 3/23/26 04:45, Jeff King wrote:

> I noticed it, too, but didn't have a better name (in fact they are of
> the same type, just two different contexts). Shadowing seemed less bad
> to me than using a mis-matched name. 

Fair enough! Let's leave it as it is ;)

> The length() of undef is also undef (and documented in "perldoc -f
> length"). But either way, length($s) will be false, and we will return
> $default, not $s.
> 
> I don't get any warning on perl 5.40.1. Are you seeing one on a
> different version?

It's mainly because I saw that you changed the required version earlier：

-require v5.26;
+require v5.008;

I clearly remember that in older versions of Perl, the length function 
behaved differently than it does now.

	use strict;
	use warnings FATAL => 'uninitialized';

	my $x;
	print "length = ", length($x), "\n";

In 5.8.8 the output is:

	Use of uninitialized value in length at test.pl line 5.

And in 5.38.2 the output is:

	Use of uninitialized value in print at test.pl line 5. length =
	
In the first case, the code will throw an error and exit. Although I 
haven't compiled this patch under version 5.8.8 yet, I suspect there 
will be issues.
> 
> -Peff

Regards, Yuchen

Re: [PATCH 5/8] diff-highlight: use test_decode_color in tests

[Tian Yuchen]

On 3/23/26 04:47, Jeff King wrote:
> On Mon, Mar 23, 2026 at 01:24:00AM +0800, Tian Yuchen wrote:
> 
>>> @@ -42,9 +39,9 @@ dh_test () {
>>>    	} >/dev/null &&
>>>    	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
>>> -	test_strip_patch_header <diff.hi >diff.act
>>> +	test_strip_patch_header <diff.hi | test_decode_color >diff.act
>>
>> Although this is just simple text filtering and leaving it as is wouldn’t
>> cause any problems IMO, why not go ahead and add the && while you’re at it?
> 
> The bug is in an earlier commit (patch 3), which breaks apart the pipe
> but doesn't add the necessary &&. And it's more than just text
> filtering; it breaks the &&-chain, so we miss the exit code of
> $DIFF_HIGHLIGHT (which was the whole point of patch 3).
> 
> chainlint doesn't find it because we're in a helper function, not
> directinly in a test snippet.
> 
> I'll send a revised series to fix it, but...
> 
>> I've noticed that there are several missing &&.
> 
> Where else do you see?
> 
> Or do you mean that we should not pipe text filtering commands? There
> I'd disagree. We are not likely to see a failure from 'sed', and if we
> do, the fact that the output does not match would catch it. And the cost
> of breaking every command down without pipes means having to manage lots
> of intermediate files.
> 
> -Peff

Oh, I see.

To be honest, I didn't think about it in that much detail. I just 
noticed this tiny issue and wanted to take the opportunity to remind you 
to check other parts as well. I'm not saying we should remove those pipe 
commands :P

Thanks for the explanation about chainlint.

Regards, Yuchen

Re: [PATCH 5/8] diff-highlight: use test_decode_color in tests

[Jeff King]

On Mon, Mar 23, 2026 at 01:48:49PM +0800, Tian Yuchen wrote:

> To be honest, I didn't think about it in that much detail. I just noticed
> this tiny issue and wanted to take the opportunity to remind you to check
> other parts as well. I'm not saying we should remove those pipe commands :P

Ah, OK. I think that is the only spot, then. Thanks for pointing it out!
Without the fix it was missing the whole point of patch 3. ;)

-Peff

Re: [PATCH 8/8] diff-highlight: fetch all config with one process

[Jeff King]

On Mon, Mar 23, 2026 at 01:39:44PM +0800, Tian Yuchen wrote:

> > I don't get any warning on perl 5.40.1. Are you seeing one on a
> > different version?
> 
> It's mainly because I saw that you changed the required version earlier：
> 
> -require v5.26;
> +require v5.008;
> 
> I clearly remember that in older versions of Perl, the length function
> behaved differently than it does now.

Heh, it figures that dropping back the version requirement would bite me
immediately. ;)

> In 5.8.8 the output is:
> 
> 	Use of uninitialized value in length at test.pl line 5.

Yeah, looks like it changed in 5.12:

  https://www.effectiveperlprogramming.com/2010/09/in-perl-v5-12-lengthundef-returns-undef/

I had actually written it using exists() originally, but then dropped it
to keep the diff smaller. Which is a silly reason. I've switched it to
use:

  return defined($s) ? $s : $default;

which I think captures the intent pretty clearly. You could also use
"$s || $default", but I try to avoid that because of surprise-false
values. In modern perl you could just use "//", but that wasn't added
until v5.10.

Thanks for pointing it out!

-Peff

[PATCH v2 0/8] some diff-highlight tweaks

[Jeff King]

Here's a re-roll based on the review from Yuchen. The two changes are:

  1. Added a missing &&-chain in patch 3 (which cascades into patch 6).

  2. Avoid length(undef), since old perl versions will warn about it.

Patch list and range diff below.

 1:  c59dd0aac9 =  1:  c59dd0aac9 contrib/diff-highlight: do not highlight identical pairs
 2:  16aa04fc6d =  2:  16aa04fc6d diff-highlight: mention build instructions
 3:  55788fac3a =  3:  55788fac3a diff-highlight: drop perl version dependency back to 5.8
 4:  7c2af2348b !  4:  1101c94f65 diff-highlight: check diff-highlight exit status in tests
    @@ contrib/diff-highlight/t/t9400-diff-highlight.sh: dh_test () {
     -	"$DIFF_HIGHLIGHT" <diff.raw | test_strip_patch_header >diff.act &&
     -	"$DIFF_HIGHLIGHT" <commit.raw | test_strip_patch_header >commit.act &&
     +	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
    -+	test_strip_patch_header <diff.hi >diff.act
    ++	test_strip_patch_header <diff.hi >diff.act &&
     +	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
     +	test_strip_patch_header <commit.hi >commit.act &&
      	test_cmp patch.exp diff.act &&
 5:  52f0358329 =  5:  65420b8b79 t: add matching negative attributes to test_decode_color
 6:  0f1aacf264 !  6:  60977c32f6 diff-highlight: use test_decode_color in tests
    @@ contrib/diff-highlight/t/t9400-diff-highlight.sh: dh_test () {
      	} >/dev/null &&
      
      	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
    --	test_strip_patch_header <diff.hi >diff.act
    -+	test_strip_patch_header <diff.hi | test_decode_color >diff.act
    +-	test_strip_patch_header <diff.hi >diff.act &&
    ++	test_strip_patch_header <diff.hi | test_decode_color >diff.act &&
      	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
     -	test_strip_patch_header <commit.hi >commit.act &&
     +	test_strip_patch_header <commit.hi | test_decode_color >commit.act &&
 7:  8bad893f09 =  7:  bf33329640 diff-highlight: test color config
 8:  179f83d791 =  8:  ea71a8b648 diff-highlight: allow module callers to pass in color config
 9:  b8ff37b193 !  9:  aab7912ca2 diff-highlight: fetch all config with one process
    @@ contrib/diff-highlight/DiffHighlight.pm: sub highlight_stdin {
     +	our $cached_config;
      	my ($key, $default) = @_;
     -	my $s = `git config --get-color $key 2>$NULL`;
    +-	return length($s) ? $s : $default;
     +
     +	if (!defined $cached_config) {
     +		$cached_config = {};
    @@ contrib/diff-highlight/DiffHighlight.pm: sub highlight_stdin {
     +	}
     +
     +	my $s = $cached_config->{$key};
    - 	return length($s) ? $s : $default;
    ++	return defined($s) ? $s : $default;
      }
      
    + sub show_hunk {
     @@ contrib/diff-highlight/DiffHighlight.pm: sub load_color_config {
      	# always be set if you want highlighting to do anything.
      	if (!defined $OLD_HIGHLIGHT[1]) {

  [1/8]: diff-highlight: mention build instructions
  [2/8]: diff-highlight: drop perl version dependency back to 5.8
  [3/8]: diff-highlight: check diff-highlight exit status in tests
  [4/8]: t: add matching negative attributes to test_decode_color
  [5/8]: diff-highlight: use test_decode_color in tests
  [6/8]: diff-highlight: test color config
  [7/8]: diff-highlight: allow module callers to pass in color config
  [8/8]: diff-highlight: fetch all config with one process

 contrib/diff-highlight/DiffHighlight.pm       | 59 +++++++++++-----
 contrib/diff-highlight/README                 | 19 +++++-
 .../diff-highlight/t/t9400-diff-highlight.sh  | 67 +++++++++++++------
 t/test-lib-functions.sh                       |  3 +
 4 files changed, 112 insertions(+), 36 deletions(-)

[PATCH v2 1/8] diff-highlight: mention build instructions

[Jeff King]

Once upon a time, this was just a script in a directory that could be
run directly. That changed in 0c977dbc81 (diff-highlight: split code
into module, 2017-06-15). Let's update the README to make it more clear
that you need to run make.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/README | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 1db4440e68..9c89146fb0 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -39,10 +39,21 @@ visually distracting.  Non-diff lines and existing diff coloration is
 preserved; the intent is that the output should look exactly the same as
 the input, except for the occasional highlight.
 
+Build/Install
+-------------
+
+You can build the `diff-highlight` script by running `make` from within
+the diff-highlight directory. There is no `make install` target; you can
+copy the built script to your $PATH.
+
+You can run diff-highlight's internal tests by running `make test`. Note
+that you must also build Git itself first (by running `make` from the
+top-level of the project).
+
 Use
 ---
 
-You can try out the diff-highlight program with:
+You can try out the built diff-highlight program with:
 
 ---------------------------------------------
 git log -p --color | /path/to/diff-highlight
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 2/8] diff-highlight: drop perl version dependency back to 5.8

[Jeff King]

From: Scott Baker <scott@perturb.org>

The diff-highlight code does not rely on any perl features beyond what
perl 5.8 provides. We bumped it to v5.26 along with the rest of the
project's perl scripts in 702d8c1f3b (Require Perl 5.26.0, 2024-10-23).

There's some value in just having a uniform baseline for the project,
but I think diff-highlight is special here:

  - it's in a contrib/ directory that is not frequently touched, so
    there is little risk of Git developers getting annoyed that modern
    perl features are not available

  - it provides a module used by other projects. In particular,
    diff-so-fancy relies on DiffHighlight.pm but does not otherwise
    require a perl version more modern than 5.8.

Let's drop back to the more conservative requirement.

Signed-off-by: Scott Baker <scott@perturb.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/DiffHighlight.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index f0607a4b68..a5e5de3b18 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -1,6 +1,6 @@
 package DiffHighlight;
 
-require v5.26;
+require v5.008;
 use warnings FATAL => 'all';
 use strict;
 
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 3/8] diff-highlight: check diff-highlight exit status in tests

[Jeff King]

When testing diff-highlight, we pipe the output through a sanitizing
function. This loses the exit status of diff-highlight itself, which
could mean we are missing cases where it crashes or exits unexpectedly.
Use an extra tempfile to avoid the pipe.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 2a9b68cf3b..7ebff8b18f 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -41,8 +41,10 @@ dh_test () {
 		git show >commit.raw
 	} >/dev/null &&
 
-	"$DIFF_HIGHLIGHT" <diff.raw | test_strip_patch_header >diff.act &&
-	"$DIFF_HIGHLIGHT" <commit.raw | test_strip_patch_header >commit.act &&
+	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
+	test_strip_patch_header <diff.hi >diff.act &&
+	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
+	test_strip_patch_header <commit.hi >commit.act &&
 	test_cmp patch.exp diff.act &&
 	test_cmp patch.exp commit.act
 }
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 4/8] t: add matching negative attributes to test_decode_color

[Jeff King]

Most of the ANSI color attributes have an "off" variant. We don't use
these yet in our test suite, so we never bothered to decode them. Add
the ones that match the attributes we encode so we can make use of them.

There are even more attributes not covered on the positive side, so this
is meant to be useful but not all-inclusive.

Note that "nobold" and "nodim" are the same code, so I've decoded this
as "normal intensity".

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib-functions.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 14e238d24d..f3af10fb7e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -48,6 +48,9 @@ test_decode_color () {
 			if (n == 2) return "FAINT";
 			if (n == 3) return "ITALIC";
 			if (n == 7) return "REVERSE";
+			if (n == 22) return "NORMAL_INTENSITY";
+			if (n == 23) return "NOITALIC";
+			if (n == 27) return "NOREVERSE";
 			if (n == 30) return "BLACK";
 			if (n == 31) return "RED";
 			if (n == 32) return "GREEN";
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 5/8] diff-highlight: use test_decode_color in tests

[Jeff King]

The diff-highlight tests use raw color bytes when comparing expected and
actual output. Let's use test_decode_color, which is our usual technique
in other tests. It makes reading test output diffs a bit easier, since
you're not relying on your terminal to interpret the result (or worse,
interpreting characters yourself via "cat -A").

This will also make it easier to add tests with new colors/attributes,
without having to pre-define the byte sequences ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 .../diff-highlight/t/t9400-diff-highlight.sh  | 37 +++++++++----------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 7ebff8b18f..4f3d55a26e 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -7,9 +7,6 @@ TEST_OUTPUT_DIRECTORY=$(pwd)
 TEST_DIRECTORY="$CURR_DIR"/../../../t
 DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
 
-CW="$(printf "\033[7m")"	# white
-CR="$(printf "\033[27m")"	# reset
-
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . "$TEST_DIRECTORY"/test-lib.sh
@@ -42,9 +39,9 @@ dh_test () {
 	} >/dev/null &&
 
 	"$DIFF_HIGHLIGHT" <diff.raw >diff.hi &&
-	test_strip_patch_header <diff.hi >diff.act &&
+	test_strip_patch_header <diff.hi | test_decode_color >diff.act &&
 	"$DIFF_HIGHLIGHT" <commit.raw >commit.hi &&
-	test_strip_patch_header <commit.hi >commit.act &&
+	test_strip_patch_header <commit.hi | test_decode_color >commit.act &&
 	test_cmp patch.exp diff.act &&
 	test_cmp patch.exp commit.act
 }
@@ -126,8 +123,8 @@ test_expect_success 'diff-highlight highlights the beginning of a line' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-${CW}b${CR}bb
-		+${CW}0${CR}bb
+		-<REVERSE>b<NOREVERSE>bb
+		+<REVERSE>0<NOREVERSE>bb
 		 ccc
 	EOF
 '
@@ -148,8 +145,8 @@ test_expect_success 'diff-highlight highlights the end of a line' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-bb${CW}b${CR}
-		+bb${CW}0${CR}
+		-bb<REVERSE>b<NOREVERSE>
+		+bb<REVERSE>0<NOREVERSE>
 		 ccc
 	EOF
 '
@@ -170,8 +167,8 @@ test_expect_success 'diff-highlight highlights the middle of a line' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-b${CW}b${CR}b
-		+b${CW}0${CR}b
+		-b<REVERSE>b<NOREVERSE>b
+		+b<REVERSE>0<NOREVERSE>b
 		 ccc
 	EOF
 '
@@ -213,8 +210,8 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 	dh_test a b <<-EOF
 		@@ -1,3 +1,3 @@
 		 aaa
-		-b${CW}b${CR}b
-		+b${CW}0${CR}b
+		-b<REVERSE>b<NOREVERSE>b
+		+b<REVERSE>0<NOREVERSE>b
 		+ccc
 	EOF
 '
@@ -232,8 +229,8 @@ test_expect_success 'diff-highlight treats multibyte utf-8 as a unit' '
 	echo "unic${o_stroke}de" >b &&
 	dh_test a b <<-EOF
 		@@ -1 +1 @@
-		-unic${CW}${o_accent}${CR}de
-		+unic${CW}${o_stroke}${CR}de
+		-unic<REVERSE>${o_accent}<NOREVERSE>de
+		+unic<REVERSE>${o_stroke}<NOREVERSE>de
 	EOF
 '
 
@@ -250,8 +247,8 @@ test_expect_failure 'diff-highlight treats combining code points as a unit' '
 	echo "unico${combine_circum}de" >b &&
 	dh_test a b <<-EOF
 		@@ -1 +1 @@
-		-unic${CW}o${combine_accent}${CR}de
-		+unic${CW}o${combine_circum}${CR}de
+		-unic<REVERSE>o${combine_accent}<NOREVERSE>de
+		+unic<REVERSE>o${combine_circum}<NOREVERSE>de
 	EOF
 '
 
@@ -333,12 +330,12 @@ test_expect_success 'diff-highlight handles --graph with leading dash' '
 	+++ b/file
 	@@ -1,3 +1,3 @@
 	 before
-	-the ${CW}old${CR} line
-	+the ${CW}new${CR} line
+	-the <REVERSE>old<NOREVERSE> line
+	+the <REVERSE>new<NOREVERSE> line
 	 -leading dash
 	EOF
 	git log --graph -p -1 | "$DIFF_HIGHLIGHT" >actual.raw &&
-	trim_graph <actual.raw | sed -n "/^---/,\$p" >actual &&
+	trim_graph <actual.raw | sed -n "/^---/,\$p" | test_decode_color >actual &&
 	test_cmp expect actual
 '
 
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 6/8] diff-highlight: test color config

[Jeff King]

We added configurable colors long ago in bca45fbc1f (diff-highlight:
allow configurable colors, 2014-11-20), but never actually tested it.
Since we'll be touching the color code in a moment, this is a good time
to beef up the tests.

Note that we cover both the highlight/reset style used by the default
colors, as well as the normal/highlight style added by that commit
(which was previously totally untested).

Signed-off-by: Jeff King <peff@peff.net>
---
 .../diff-highlight/t/t9400-diff-highlight.sh  | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 4f3d55a26e..b38fe2196a 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -350,4 +350,32 @@ test_expect_success 'highlight diff that removes final newline' '
 	EOF
 '
 
+test_expect_success 'configure set/reset colors' '
+	test_config color.diff-highlight.oldhighlight bold &&
+	test_config color.diff-highlight.oldreset nobold &&
+	test_config color.diff-highlight.newhighlight italic &&
+	test_config color.diff-highlight.newreset noitalic &&
+	echo "prefix a suffix" >a &&
+	echo "prefix b suffix" >b &&
+	dh_test a b <<-\EOF
+	@@ -1 +1 @@
+	-prefix <BOLD>a<NORMAL_INTENSITY> suffix
+	+prefix <ITALIC>b<NOITALIC> suffix
+	EOF
+'
+
+test_expect_success 'configure normal/highlight colors' '
+	test_config color.diff-highlight.oldnormal red &&
+	test_config color.diff-highlight.oldhighlight magenta &&
+	test_config color.diff-highlight.newnormal green &&
+	test_config color.diff-highlight.newhighlight yellow &&
+	echo "prefix a suffix" >a &&
+	echo "prefix b suffix" >b &&
+	dh_test a b <<-\EOF
+	@@ -1 +1 @@
+	<RED>-prefix <RESET><MAGENTA>a<RESET><RED> suffix<RESET>
+	<GREEN>+prefix <RESET><YELLOW>b<RESET><GREEN> suffix<RESET>
+	EOF
+'
+
 test_done
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 7/8] diff-highlight: allow module callers to pass in color config

[Jeff King]

From: Scott Baker <scott@perturb.org>

Users of the module may want to pass in their own color config for a few
obvious reasons:

  - they are pulling the config from different variables than
    diff-highlight itself uses

  - they are loading the config in a more efficient way (say, by parsing
    git-config --list) and don't want to incur the six (!) git-config
    calls that DiffHighlight.pm runs to check all config

Let's allow users of the module to pass in the color config, and
lazy-load it when needed if they haven't.

Signed-off-by: Scott Baker <scott@perturb.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/DiffHighlight.pm | 41 +++++++++++++++++--------
 contrib/diff-highlight/README           |  6 ++++
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index a5e5de3b18..96369eadf9 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -9,18 +9,11 @@ package DiffHighlight;
 
 my $NULL = File::Spec->devnull();
 
-# Highlight by reversing foreground and background. You could do
-# other things like bold or underline if you prefer.
-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])
-);
+# The color theme is initially set to nothing here to allow outside callers
+# to set the colors for their application. If nothing is sent in we use
+# colors from git config in load_color_config().
+our @OLD_HIGHLIGHT = ();
+our @NEW_HIGHLIGHT = ();
 
 my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
@@ -170,6 +163,29 @@ sub show_hunk {
 	$line_cb->(@queue);
 }
 
+sub load_color_config {
+	# If the colors were NOT set from outside this module we load them on-demand
+	# from the git config. Note that only one of elements 0 and 2 in each
+	# array is used (depending on whether you are doing set/unset on an
+	# attribute, or specifying normal vs highlighted coloring). So we use
+	# element 1 as our check for whether colors were passed in; it should
+	# always be set if you want highlighting to do anything.
+	if (!defined $OLD_HIGHLIGHT[1]) {
+		@OLD_HIGHLIGHT = (
+			color_config('color.diff-highlight.oldnormal'),
+			color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
+			color_config('color.diff-highlight.oldreset', "\x1b[27m")
+		);
+	}
+	if (!defined $NEW_HIGHLIGHT[1]) {
+		@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])
+		);
+	};
+}
+
 sub highlight_pair {
 	my @a = split_line(shift);
 	my @b = split_line(shift);
@@ -218,6 +234,7 @@ sub highlight_pair {
 	}
 
 	if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
+		load_color_config();
 		return highlight_line(\@a, $pa, $sa, \@OLD_HIGHLIGHT),
 		       highlight_line(\@b, $pb, $sb, \@NEW_HIGHLIGHT);
 	}
diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 9c89146fb0..ed8d876a18 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -138,6 +138,12 @@ Your script may set up one or more of the following variables:
     processing a logical chunk of input). The default function flushes
     stdout.
 
+  - @DiffHighlight::OLD_HIGHLIGHT and @DiffHighlight::NEW_HIGHLIGHT - these
+    arrays specify the normal, highlighted, and reset colors (in that order)
+    for old/new lines. If unset, values will be retrieved by calling `git
+    config` (see "Color Config" above). Note that these should be the literal
+    color bytes (starting with an ANSI escape code), not color names.
+
 The script may then feed lines, one at a time, to DiffHighlight::handle_line().
 When lines are done processing, they will be fed to $line_cb. Note that
 DiffHighlight may queue up many input lines (to analyze a whole hunk)
-- 
2.53.0.1051.ga14e96f895

[PATCH v2 8/8] diff-highlight: fetch all config with one process

[Jeff King]

When diff-highlight was written, there was no way to fetch multiple
config keys _and_ have them interpreted as colors. So we were stuck
with either invoking git-config once for each config key, or fetching
them all and converting human-readable color names into ANSI codes
ourselves.

I chose the former, but it means that diff-highlight kicks off 6
git-config processes (even if you haven't configured anything, it has to
check each one).

But since Git 2.18.0, we can do:

   git config --type=color --get-regexp=^color\.diff-highlight\.

to get all of them in one shot.

Note that any callers which pass in colors directly to the module via
@OLD_HIGHLIGHT and @NEW_HIGHLIGHT (like diff-so-fancy plans to do) are
unaffected; those colors suppress any config lookup we'd do ourselves.

You can see the effect like:

  # diff-highlight suppresses git-config's stderr, so dump
  # trace through descriptor 3
  git show d1f33c753d | GIT_TRACE=3 diff-highlight 3>&2 >/dev/null

which drops from 6 lines down to 1.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/DiffHighlight.pm | 28 ++++++++++++++++++-------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index 96369eadf9..abe457882e 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -131,9 +131,21 @@ sub highlight_stdin {
 # 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 {
+	our $cached_config;
 	my ($key, $default) = @_;
-	my $s = `git config --get-color $key 2>$NULL`;
-	return length($s) ? $s : $default;
+
+	if (!defined $cached_config) {
+		$cached_config = {};
+		my $data = `git config --type=color --get-regexp '^color\.diff-highlight\.' 2>$NULL`;
+		for my $line (split /\n/, $data) {
+			my ($key, $color) = split ' ', $line, 2;
+			$key =~ s/^color\.diff-highlight\.// or next;
+			$cached_config->{$key} = $color;
+		}
+	}
+
+	my $s = $cached_config->{$key};
+	return defined($s) ? $s : $default;
 }
 
 sub show_hunk {
@@ -172,16 +184,16 @@ sub load_color_config {
 	# always be set if you want highlighting to do anything.
 	if (!defined $OLD_HIGHLIGHT[1]) {
 		@OLD_HIGHLIGHT = (
-			color_config('color.diff-highlight.oldnormal'),
-			color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
-			color_config('color.diff-highlight.oldreset', "\x1b[27m")
+			color_config('oldnormal'),
+			color_config('oldhighlight', "\x1b[7m"),
+			color_config('oldreset', "\x1b[27m")
 		);
 	}
 	if (!defined $NEW_HIGHLIGHT[1]) {
 		@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])
+			color_config('newnormal', $OLD_HIGHLIGHT[0]),
+			color_config('newhighlight', $OLD_HIGHLIGHT[1]),
+			color_config('newreset', $OLD_HIGHLIGHT[2])
 		);
 	};
 }
-- 
2.53.0.1051.ga14e96f895

Re: [PATCH v2 0/8] some diff-highlight tweaks

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a re-roll based on the review from Yuchen. The two changes are:
>
>   1. Added a missing &&-chain in patch 3 (which cascades into patch 6).
>
>   2. Avoid length(undef), since old perl versions will warn about it.
>
> Patch list and range diff below.

Everything looks as expected from watching the discussion from
the sideline.  Looking good.

Will queue and mark the topic for 'next'.

Thanks.

Re: [PATCH v2 0/8] some diff-highlight tweaks

[Patrick Steinhardt]

On Mon, Mar 23, 2026 at 09:38:56AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Here's a re-roll based on the review from Yuchen. The two changes are:
> >
> >   1. Added a missing &&-chain in patch 3 (which cascades into patch 6).
> >
> >   2. Avoid length(undef), since old perl versions will warn about it.
> >
> > Patch list and range diff below.
> 
> Everything looks as expected from watching the discussion from
> the sideline.  Looking good.
> 
> Will queue and mark the topic for 'next'.

I just read through the series and couldn't find anything wrong. That
being said, my Perl skills are severely lacking, so my assessment may
not be worth much :)

Thanks!

Patrick