# [PATCH v3 2/3] diff-highlight: add failing test for handling --graph output.

30 messages from 2016-08-19 to 2016-08-31. Participants: Brian Henderson, Junio C Hamano, Eric Wong, Jeff King.
Thread: https://gitlist.dev/t/43885

## Brian Henderson, 2016-08-19 17:08

Subject: [PATCH v3 2/3] diff-highlight: add failing test for handling --graph output.
Message-ID: <20160819170812.1676-3-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160819170812.1676-3-henderson.bj%40gmail.com
In-Reply-To: <20160819170812.1676-1-henderson.bj@gmail.com>

```
Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 54 ++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 6b8a461..3b3c831 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -85,10 +85,50 @@ dh_commit_test () {
 	test_cmp commit.exp commit.act
 }
 
+# dh_test_setup_history takes a series (3) of changes and generates a contrived graph
+# of commits and merges
+dh_test_setup_history () {
+	a="$1" b="$2" c="$3"
+
+	printf "$a" >file &&
+	git add file &&
+	git commit -m"Add a file" &&
+
+	printf "$b" >file &&
+	git commit -am"Update a file" &&
+
+	git checkout -b branch &&
+	printf "$c" >file &&
+	git commit -am"Update a file on branch" &&
+
+	git checkout master &&
+	printf "$a" >file &&
+	git commit -am"Update a file again" &&
+
+	git checkout branch &&
+	printf "$b" >file &&
+	git commit -am"Update a file similar to master" &&
+
+	git merge master &&
+	git checkout master &&
+	git merge branch --no-ff
+}
+
+
 test_chomp_eof () {
 	"$PERL_PATH" -pe 'chomp if eof'
 }
 
+left_trim () {
+	"$PERL_PATH" -pe 's/^\s+//'
+}
+
+trim_graph_el () {
+	# graphs start with * or |
+	# followed by a space or / or \
+	"$PERL_PATH" -pe 's@^((\*|\|)( |/|\\))+@@'
+}
+
 test_expect_success 'diff-highlight highlights the beginning of a line' '
 	dh_test \
 		"aaa\nbbb\nccc\n" \
@@ -145,6 +185,20 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 
 # TODO add multi-byte test
 
+test_expect_failure 'diff-highlight works with the --graph option' '
+	dh_test_setup_history \
+		"aaa\nbbb\nccc\n" \
+		"aaa\n0bb\nccc\n" \
+		"aaa\nb0b\nccc\n" &&
+
+	# topo-order so that the order of the commits is the same as with --graph
+	# trim graph elements so we can do a diff
+	# trim leading space because our trim_graph_el is not perfect
+	git log -p --topo-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
+	git log -p --graph | "$DIFF_HIGHLIGHT" | trim_graph_el | left_trim >graph.act &&
+	test_cmp graph.exp graph.act
+'
+
 test_done
 
 # vim: set noet
-- 
2.9.0


```

## Brian Henderson, 2016-08-19 17:08

Subject: [PATCH v3 0/3] diff-highlight: add support for git log --graph output.
Message-ID: <20160819170812.1676-1-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160819170812.1676-1-henderson.bj%40gmail.com
In-Reply-To: <20160819145123.73hf7ffysy53l3kz@sigill.intra.peff.net>

```
I cleaned up the graph test, hopefully it's better.

Brian Henderson (3):
  diff-highlight: add some tests.
  diff-highlight: add failing test for handling --graph output.
  diff-highlight: add support for --graph output.

 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/diff-highlight            |  19 ++-
 contrib/diff-highlight/t/Makefile                |  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 204 +++++++++++++++++++++++
 4 files changed, 244 insertions(+), 6 deletions(-)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

-- 
2.9.0


```

## Brian Henderson, 2016-08-19 17:08

Subject: [PATCH v3 1/3] diff-highlight: add some tests.
Message-ID: <20160819170812.1676-2-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160819170812.1676-2-henderson.bj%40gmail.com
In-Reply-To: <20160819170812.1676-1-henderson.bj@gmail.com>

```
Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/t/Makefile                |  22 ++++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 150 +++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+	$(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 0000000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+	@echo 'Run "$(MAKE) test" to launch test scripts'
+	@echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+	$(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 0000000..6b8a461
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,150 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+TEST_OUTPUT_DIRECTORY=$(pwd)
+TEST_DIRECTORY="$CURR_DIR"/../../../t
+DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
+
+CW="\033[7m"	# white
+CR="\033[27m" # reset
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all='skipping diff-highlight tests; perl not available'
+	test_done
+fi
+
+# dh_test is a test helper function which takes 1) some file data, 2) some
+# change of the file data, creates a diff and commit of the changes and passes
+# that through diff-highlight. The optional 3rd parameter is the expected
+# output of diff-highlight minus the diff/commit header. Don't include a 3rd
+# parameter if diff-highlight is stupposed to leave the input unmodified.
+dh_test () {
+	dh_diff_test "$@" &&
+	dh_commit_test "$@"
+}
+
+# see dh_test for usage
+dh_diff_test () {
+	a="$1" b="$2"
+
+	printf "$a" >file
+	git add file
+
+	printf "$b" >file
+	git diff file >diff.raw
+
+	if test $# -ge 3
+	then
+		# Add the diff header to the expected file
+		# we remove the trailing newline to make the test a little more readable
+		# this means $3 should start with a newline
+		head -n5 diff.raw | test_chomp_eof >diff.exp
+		printf "$3" >>diff.exp
+	else
+		cat diff.raw >diff.exp
+	fi
+
+	"$DIFF_HIGHLIGHT" <diff.raw >diff.act &&
+	# check that at least one of the files is not empty (any of the above
+	# commands could have failed resulting in an empty file)
+	test -s diff.act &&
+	test_cmp diff.exp diff.act
+}
+
+# see dh_test for usage
+dh_commit_test () {
+	a="$1" b="$2"
+
+	printf "$a" >file
+	git add file
+	git commit -m "Add a file" >/dev/null
+
+	printf "$b" >file
+	git commit -am "Update a file" >/dev/null
+
+	git show >commit.raw
+
+	if test $# -ge 3
+	then
+		# same as dh_diff_test
+		head -n11 commit.raw | test_chomp_eof >commit.exp
+		printf "$3" >>commit.exp
+	else
+		cat commit.raw >commit.exp
+	fi
+
+	"$DIFF_HIGHLIGHT" <commit.raw >commit.act &&
+	# check that at least one of the files is not empty (any of the above
+	# commands could have failed resulting in an empty file)
+	test -s commit.act &&
+	test_cmp commit.exp commit.act
+}
+
+test_chomp_eof () {
+	"$PERL_PATH" -pe 'chomp if eof'
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\n0bb\nccc\n" \
+"
+ aaa
+-${CW}b${CR}bb
++${CW}0${CR}bb
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\nbb0\nccc\n" \
+"
+ aaa
+-bb${CW}b${CR}
++bb${CW}0${CR}
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\n000\nccc\n"
+'
+
+test_expect_failure 'diff-highlight highlights mismatched hunk size' '
+	dh_test \
+		"aaa\nbbb\n" \
+		"aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
++ccc
+"
+'
+
+# TODO add multi-byte test
+
+test_done
+
+# vim: set noet
-- 
2.9.0


```

## Brian Henderson, 2016-08-19 17:08

Subject: [PATCH v3 3/3] diff-highlight: add support for --graph output.
Message-ID: <20160819170812.1676-4-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160819170812.1676-4-henderson.bj%40gmail.com
In-Reply-To: <20160819170812.1676-1-henderson.bj@gmail.com>

```
Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/diff-highlight            | 19 +++++++++++++------
 contrib/diff-highlight/t/t9400-diff-highlight.sh |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index ffefc31..9280c88 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -21,6 +21,10 @@ my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
+# The patch portion of git log -p --graph should only ever have preceding | and
+# not / or \ as merge history only shows up on the commit line.
+my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
+
 my @removed;
 my @added;
 my $in_hunk;
@@ -32,12 +36,12 @@ $SIG{PIPE} = 'DEFAULT';
 while (<>) {
 	if (!$in_hunk) {
 		print;
-		$in_hunk = /^$COLOR*\@/;
+		$in_hunk = /^$GRAPH*$COLOR*\@/;
 	}
-	elsif (/^$COLOR*-/) {
+	elsif (/^$GRAPH*$COLOR*-/) {
 		push @removed, $_;
 	}
-	elsif (/^$COLOR*\+/) {
+	elsif (/^$GRAPH*$COLOR*\+/) {
 		push @added, $_;
 	}
 	else {
@@ -46,7 +50,7 @@ while (<>) {
 		@added = ();
 
 		print;
-		$in_hunk = /^$COLOR*[\@ ]/;
+		$in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
 	}
 
 	# Most of the time there is enough output to keep things streaming,
@@ -163,6 +167,9 @@ sub highlight_pair {
 	}
 }
 
+# we split either by $COLOR or by character. This has the side effect of
+# leaving in graph cruft. It works because the graph cruft does not contain "-"
+# or "+"
 sub split_line {
 	local $_ = shift;
 	return utf8::decode($_) ?
@@ -211,8 +218,8 @@ sub is_pair_interesting {
 	my $suffix_a = join('', @$a[($sa+1)..$#$a]);
 	my $suffix_b = join('', @$b[($sb+1)..$#$b]);
 
-	return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
-	       $prefix_b !~ /^$COLOR*\+$BORING*$/ ||
+	return $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||
+	       $prefix_b !~ /^$GRAPH*$COLOR*\+$BORING*$/ ||
 	       $suffix_a !~ /^$BORING*$/ ||
 	       $suffix_b !~ /^$BORING*$/;
 }
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 3b3c831..b88174e 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -185,7 +185,7 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 
 # TODO add multi-byte test
 
-test_expect_failure 'diff-highlight works with the --graph option' '
+test_expect_success 'diff-highlight works with the --graph option' '
 	dh_test_setup_history \
 		"aaa\nbbb\nccc\n" \
 		"aaa\n0bb\nccc\n" \
-- 
2.9.0


```

## Junio C Hamano, 2016-08-19 18:10

Subject: Re: [PATCH v3 1/3] diff-highlight: add some tests.
Message-ID: <xmqqh9ag39zk.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqh9ag39zk.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160819170812.1676-2-henderson.bj@gmail.com>

```

> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and passes
> +# that through diff-highlight. The optional 3rd parameter is the expected
> +# output of diff-highlight minus the diff/commit header. Don't include a 3rd
> +# parameter if diff-highlight is stupposed to leave the input unmodified.

Good explanation, but it fails to say one crucial thing.  The third
parameter is directly fed to printf and not to "printf '%s' $3",
hence any '%' you expect to see in diff output need to be doubled to
protect it.

> +# see dh_test for usage
> +dh_diff_test () {
> +	a="$1" b="$2"
> +
> +	printf "$a" >file
> +	git add file
> +
> +	printf "$b" >file
> +	git diff file >diff.raw
> +
> +	if test $# -ge 3
> +	then
> +		# Add the diff header to the expected file
> +		# we remove the trailing newline to make the test a little more readable
> +		# this means $3 should start with a newline
> +		head -n5 diff.raw | test_chomp_eof >diff.exp
> +		printf "$3" >>diff.exp
> +	else
> +		cat diff.raw >diff.exp
> +	fi

Sorry, but I do not understand why you hardcode -n5 or why you need
chomp-eof.

I _think_ you are expecting "git diff file" to start with

    diff --git a/file b/file
    index fffffff..fffffff 100644
    --- a/file
    +++ b/file
    @@ -l,k +m,n @@ comments

and want to grab everything before and including this first hunk
header.  A more future-proof way to do the "stop at the first
occurrence of this" (this comment applies to -n11 we see below) is

	sed -e '/^@@/q' diff.raw

As to chomp-eof, I still do not see the point, especially with the
new comment you added: "this means $3 should start with newline".

You are forcing the caller to have an extra empty line at the
beginnig ONLY because you do this "chomp" thing.  Otherwise the
callers do not need to.

Unless you actually mean something else by the new comment, e.g. "I
think the callers look prettier if it begins with a newline, so we
compensate for that by removing the end of line from what comes
before it", that is.  If "this means $3 should" is what it sounds
like, i.e. imposing an unnatural constraint on the callers of this
helper function, then the helper can do this

	printf "\n$3" >>diff.exp

so that the callers do not have to worry about it.

If "I think the caller looks prettier if it begins with a newline"
is the true motivation, then "this means $3 should start..." needs
to be rephrased.  And as to the implementation of the helper, I
think

	{
		sed -e '/^@@/q' diff.raw
		printf "$3" | sed -e 1d
	} >diff.expected

may be easier to see what is going on.

> +
> +	"$DIFF_HIGHLIGHT" <diff.raw >diff.act &&

> +	# check that at least one of the files is not empty (any of the above
> +	# commands could have failed resulting in an empty file)
> +	test -s diff.act &&

A more established way in our test suite to ensure that "any of the
above commands could have failed" is caught is to &&-chain all the
commands, like this:

	a=$1 b=$2 &&
	printf "$a" >file && git add file &&
        printf "$b" >file && git diff file >diff.raw &&
        if test $# = 3
        then
        	...
	else
        	...
	fi &&

> +test_done
> +
> +# vim: set noet

We tend to avoid cluttering the source with editor specific insns
like this.

Stepping back a bit.  Because you are only interested in making sure
the body of the diff match what is expected, it may be a better
alternative approach to make the comparison _after_ stripping the
headers from the actual output with the expected (which you do not
have headers for), rather than comparing the expected (with fake
headers added in as necessary) and the actual output with headers,
i.e.

	git diff file >diff.raw &&
	if test $# = 3
	then
		printf "$3"
	else
	        sed -e '1,/^@@/d' <diff.raw
	fi >diff.expected &&
	"$DIFF_HIGHLIGHT" <diff.raw >diff.highlight &&
	sed -e '1,/^@@/d' <diff.highlight >diff.actual &&
	test_cmp diff.expected diff.actual

or something like that.

That does not address "is it a good idea to require an empty line at
the beginning of $3"? at all, though.  If you want to require a
blank in front of "$3", the "printf" needs to be tweaked to somehow
strip it.

```

## Junio C Hamano, 2016-08-19 18:25

Subject: Re: [PATCH v3 2/3] diff-highlight: add failing test for handling --graph output.
Message-ID: <xmqqd1l439am.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqd1l439am.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160819170812.1676-3-henderson.bj@gmail.com>

```
Brian Henderson <henderson.bj@gmail.com> writes:

> Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
> ---
>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 54 ++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> index 6b8a461..3b3c831 100755
> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -85,10 +85,50 @@ dh_commit_test () {
>  	test_cmp commit.exp commit.act
>  }
>  
> +# dh_test_setup_history takes a series (3) of changes and generates a contrived graph
> +# of commits and merges

The first line look a bit overlong.

If the graph is truly "contrived" as the comment at the beginning
says (I haven't formed an opinion), it probably is a good idea to
draw a picture upfront.

> +dh_test_setup_history () {
> +	a="$1" b="$2" c="$3"

Even though assignment would not fail, it probably is a good idea to
&&-chain this line, too, for consistency.  I.e.

	a=$1 b=$2 c=$3 &&

Note that RHS of an assignment do not need dq; it does not hurt to
quote them, though.

> +	printf "$a" >file &&
> +	git add file &&
> +	git commit -m"Add a file" &&

Avoid sticking a short-option to its argument, i.e.

    git commit -m "Add a file"

> +	printf "$b" >file &&
> +	git commit -am"Update a file" &&

Likewise.

    git commit -a -m "Update a file".

> +left_trim () {
> +	"$PERL_PATH" -pe 's/^\s+//'
> +}
> +
> +trim_graph_el () {

What does "el" stand for?  Not E-lisp ;-)

If you meant "elements" or something, probably that is unnecessary
and "trim_graph" would be sufficient, but you may have meant
something else.

> +	# graphs start with * or |
> +	# followed by a space or / or \
> +	"$PERL_PATH" -pe 's@^((\*|\|)( |/|\\))+@@'
> +}
> +
>  test_expect_success 'diff-highlight highlights the beginning of a line' '
>  	dh_test \
>  		"aaa\nbbb\nccc\n" \
> @@ -145,6 +185,20 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
>  
>  # TODO add multi-byte test
>  
> +test_expect_failure 'diff-highlight works with the --graph option' '
> +	dh_test_setup_history \
> +		"aaa\nbbb\nccc\n" \
> +		"aaa\n0bb\nccc\n" \
> +		"aaa\nb0b\nccc\n" &&
> +
> +	# topo-order so that the order of the commits is the same as with --graph
> +	# trim graph elements so we can do a diff
> +	# trim leading space because our trim_graph_el is not perfect
> +	git log -p --topo-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
> +	git log -p --graph | "$DIFF_HIGHLIGHT" | trim_graph_el | left_trim >graph.act &&
> +	test_cmp graph.exp graph.act
> +'
> +
>  test_done
>  
>  # vim: set noet

```

## Junio C Hamano, 2016-08-19 18:27

Subject: Re: [PATCH v3 3/3] diff-highlight: add support for --graph output.
Message-ID: <xmqq8tvs397z.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqq8tvs397z.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160819170812.1676-4-henderson.bj@gmail.com>

```
Brian Henderson <henderson.bj@gmail.com> writes:

> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> index 3b3c831..b88174e 100755
> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -185,7 +185,7 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
>  
>  # TODO add multi-byte test
>  
> -test_expect_failure 'diff-highlight works with the --graph option' '
> +test_expect_success 'diff-highlight works with the --graph option' '
>  	dh_test_setup_history \
>  		"aaa\nbbb\nccc\n" \
>  		"aaa\n0bb\nccc\n" \

Yup, the standard "describe what we want and document a bug to be
fixed with _failure, and flip it to _success when you fix the bug"
pattern makes it very easy to understand what is going on.


```

## Brian Henderson, 2016-08-19 19:30

Subject: Re: [PATCH v3 1/3] diff-highlight: add some tests.
Message-ID: <20160819193045.GA9262@tci.corp.yp.com>
URL: https://gitlist.dev/e/20160819193045.GA9262%40tci.corp.yp.com
In-Reply-To: <xmqqh9ag39zk.fsf@gitster.mtv.corp.google.com>

```
On Fri, Aug 19, 2016 at 11:10:55AM -0700, Junio C Hamano wrote:
> 
> > +# vim: set noet
> 
> We tend to avoid cluttering the source with editor specific insns
> like this.

oops.

Anyone have any suggestions for project level vim settings?

```

## Brian Henderson, 2016-08-19 20:18

Subject: [PATCH] diff-highlight: add some tests.
Message-ID: <20160819201837.GA18918@tci.corp.yp.com>
URL: https://gitlist.dev/e/20160819201837.GA18918%40tci.corp.yp.com
In-Reply-To: <xmqqh9ag39zk.fsf@gitster.mtv.corp.google.com>

```
Junio, how does this look?

Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/t/Makefile                |  22 ++++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 122 +++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+	$(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 0000000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+	@echo 'Run "$(MAKE) test" to launch test scripts'
+	@echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+	$(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 0000000..3c04116
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,122 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+TEST_OUTPUT_DIRECTORY=$(pwd)
+TEST_DIRECTORY="$CURR_DIR"/../../../t
+DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
+
+CW="\033[7m"	# white
+CR="\033[27m" # reset
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all='skipping diff-highlight tests; perl not available'
+	test_done
+fi
+
+# dh_test is a test helper function which takes 1) some file data, 2) some
+# change of the file data, creates a diff and commit of the changes and passes
+# that through diff-highlight.
+# The optional 3rd parameter is the expected output of diff-highlight minus the
+# diff/commit header. This parameter is given directly to printf as the format
+# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
+# need to be doubled to protect it.
+# Don't include a 3rd parameter if diff-highlight is supposed to leave the
+# input unmodified.
+# For convienence, the 3rd parameter can begin with a newline which will be
+# stripped.
+dh_test () {
+	a="$1" b="$2" &&
+
+	{
+		printf "$a" >file &&
+		git add file &&
+		git commit -m "Add a file" &&
+
+		printf "$b" >file &&
+		git diff file >diff.raw &&
+		git commit -am "Update a file" &&
+		git show >commit.raw
+	} >/dev/null &&
+
+	if test $# -ge 3
+	then
+		# strip optional beginning newline
+		printf "$3" | perl -pe 's/^\n//'
+	else
+		test_strip_patch_header diff.raw
+	fi >patch.exp &&
+
+	"$DIFF_HIGHLIGHT" <diff.raw | test_strip_patch_header >diff.act &&
+	"$DIFF_HIGHLIGHT" <commit.raw | test_strip_patch_header >commit.act &&
+	test_cmp patch.exp diff.act &&
+	test_cmp patch.exp commit.act
+}
+
+test_strip_patch_header () {
+	sed -e '1,/^@@/d' "$@"
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\n0bb\nccc\n" \
+"
+ aaa
+-${CW}b${CR}bb
++${CW}0${CR}bb
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\nbb0\nccc\n" \
+"
+ aaa
+-bb${CW}b${CR}
++bb${CW}0${CR}
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+	dh_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\n000\nccc\n"
+'
+
+test_expect_failure 'diff-highlight highlights mismatched hunk size' '
+	dh_test \
+		"aaa\nbbb\n" \
+		"aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
++ccc
+"
+'
+
+# TODO add multi-byte test
+
+test_done
+
+# vim: set noet
-- 
2.9.0


```

## Junio C Hamano, 2016-08-19 20:44

Subject: Re: [PATCH] diff-highlight: add some tests.
Message-ID: <xmqqbn0o1ob7.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqbn0o1ob7.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160819201837.GA18918@tci.corp.yp.com>

```
Brian Henderson <henderson.bj@gmail.com> writes:

> Junio, how does this look?
> ...
> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and passes
> +# that through diff-highlight.
> +# The optional 3rd parameter is the expected output of diff-highlight minus the
> +# diff/commit header. This parameter is given directly to printf as the format
> +# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
> +# need to be doubled to protect it.
> +# Don't include a 3rd parameter if diff-highlight is supposed to leave the
> +# input unmodified.
> +# For convienence, the 3rd parameter can begin with a newline which will be
> +# stripped.

You seem to be stripping any and all empty lines with "perl -pe"; I
am not sure if that is sensible.

I really do not see the point of being able to spell

"
aaa
bbb
"

when you can perfectly well read

"aaa
bbb"

or even "aaa\nbbb\n" for that matter.  I personally do not think the
difference is worth the cost of an extra invocation of Perl, but we
already saw how stubborn you are, so there is no point spending my
time on trying to convince you further.  Assuming that it is so
precious that the input can start with an extra blank line, what you
wrote is a sensible implementation.

> +dh_test () {
> +	a="$1" b="$2" &&
> +
> +	{
> +		printf "$a" >file &&
> +		git add file &&
> +		git commit -m "Add a file" &&
> +
> +		printf "$b" >file &&
> +		git diff file >diff.raw &&
> +		git commit -am "Update a file" &&
> +		git show >commit.raw
> +	} >/dev/null &&
> +
> +	if test $# -ge 3

I think

	if test $# = 3

is much better; you would want to catch bugs in a caller that gave
you fourth parameter by mistake, instead of silently ignoring it.

> +test_strip_patch_header () {
> +	sed -e '1,/^@@/d' "$@"
> +}

While I think it is a great idea to hide the sed command behind a
helper function for readability, I am not sure "strip patch header"
is a great name that describes what it does.  A natural expectation
for an operation with that name done to this input:

    diff --git a/file b/file
    index fffffff..fffffff 100644
    --- a/file
    +++ b/file
    @@ -l,k +m,n @@ comments
     common
    -old
    +new
     common
    @@ -l,k +m,n @@ more comments
     common
    +added more
    +added even more

would be to remove the first four lines, not five.


```

## Eric Wong, 2016-08-19 20:56

Subject: Re: [PATCH v3 1/3] diff-highlight: add some tests.
Message-ID: <20160819205641.GA31393@dcvr>
URL: https://gitlist.dev/e/20160819205641.GA31393%40dcvr
In-Reply-To: <20160819193045.GA9262@tci.corp.yp.com>

```
Brian Henderson <henderson.bj@gmail.com> wrote:
> On Fri, Aug 19, 2016 at 11:10:55AM -0700, Junio C Hamano wrote:
> > 
> > > +# vim: set noet
> > 
> > We tend to avoid cluttering the source with editor specific insns
> > like this.
> 
> oops.
> 
> Anyone have any suggestions for project level vim settings?

vim defaults work fine for me on FreeBSD and Debian:

	tabstop=8 shiftwidth=8 softtabstop=0 noexpandtab

Generally, we take after the Linux kernel:

	https://kernel.org/doc/Documentation/CodingStyle

Which I'm happy about and largely agree with.

```

## Jeff King, 2016-08-19 21:04

Subject: Re: [PATCH] diff-highlight: add some tests.
Message-ID: <20160819210431.usy6x3x6sdhe3vbp@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160819210431.usy6x3x6sdhe3vbp%40sigill.intra.peff.net
In-Reply-To: <xmqqbn0o1ob7.fsf@gitster.mtv.corp.google.com>

```
On Fri, Aug 19, 2016 at 01:44:28PM -0700, Junio C Hamano wrote:

> Brian Henderson <henderson.bj@gmail.com> writes:
> 
> > Junio, how does this look?
> > ...
> > +# dh_test is a test helper function which takes 1) some file data, 2) some
> > +# change of the file data, creates a diff and commit of the changes and passes
> > +# that through diff-highlight.
> > +# The optional 3rd parameter is the expected output of diff-highlight minus the
> > +# diff/commit header. This parameter is given directly to printf as the format
> > +# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
> > +# need to be doubled to protect it.
> > +# Don't include a 3rd parameter if diff-highlight is supposed to leave the
> > +# input unmodified.
> > +# For convienence, the 3rd parameter can begin with a newline which will be
> > +# stripped.
> 
> You seem to be stripping any and all empty lines with "perl -pe"; I
> am not sure if that is sensible.
> 
> I really do not see the point of being able to spell
> 
> "
> aaa
> bbb
> "
> 
> when you can perfectly well read
> 
> "aaa
> bbb"
> 
> or even "aaa\nbbb\n" for that matter.  I personally do not think the
> difference is worth the cost of an extra invocation of Perl, but we
> already saw how stubborn you are, so there is no point spending my
> time on trying to convince you further.  Assuming that it is so
> precious that the input can start with an extra blank line, what you
> wrote is a sensible implementation.

I didn't want to bikeshed, so I resisted saying so up until now, but I
actually think:

  dh_test \
    "aaa\nbbb\nccc\n" \
    "aaa\n0bb\nccc\n" \
    <<-EOF
  aaa
  -${CW}b${CR}bb
  +${CW}0${CR}bb
  EOF

might before readable, if only because it lets you indent the content to
match the rest of the test content. For that matter, I'm not sure that:

  cat >a <<-\EOF &&
  aaa
  bbb
  ccc
  EOF

  cat >b <<-\EOF &&
  aaa
  0bb
  ccc
  EOF

  dh_test a b <<\EOF
  aaa
  -${CW}b${CR}bb
  +${CW}0${CR}bb
  EOF

isn't more readable, too. It's more lines, certainly, but it makes it
very easy to see what the input files look like, rather than cramming
"\n" into the middle of a string (the existing code does make the diff
easy to see for _this_ case, because the pre- and post-image line up
vertically, but that is only the case for pure transliterations like
this).

Just my two cents.

-Peff

```

## Junio C Hamano, 2016-08-19 21:05

Subject: Re: [PATCH] diff-highlight: add some tests.
Message-ID: <xmqq37m01nci.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqq37m01nci.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160819201837.GA18918@tci.corp.yp.com>

```
Brian Henderson <henderson.bj@gmail.com> writes:

> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and passes
> +# that through diff-highlight.
> +# The optional 3rd parameter is the expected output of diff-highlight minus the
> +# diff/commit header. This parameter is given directly to printf as the format
> +# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
> +# need to be doubled to protect it.

My mistake.  All three are given directly to printf as the format
string, so it is misleading to single out $3 when warning the
callers about '%'.

Sorry about that.

```

## Junio C Hamano, 2016-08-19 22:31

Subject: Re: [PATCH] diff-highlight: add some tests.
Message-ID: <xmqqeg5kz8zq.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqeg5kz8zq.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160819210431.usy6x3x6sdhe3vbp@sigill.intra.peff.net>

```
Jeff King <peff@peff.net> writes:

> For that matter, I'm not sure that:
>
>   cat >a <<-\EOF &&
>   aaa
>   bbb
>   ccc
>   EOF
>
>   cat >b <<-\EOF &&
>   aaa
>   0bb
>   ccc
>   EOF
>
>   dh_test a b <<\EOF
>   aaa
>   -${CW}b${CR}bb
>   +${CW}0${CR}bb
>   EOF
>
> isn't more readable, too. It's more lines, certainly, but it makes it
> very easy to see what the input files look like, rather than cramming
> "\n" into the middle of a string (the existing code does make the diff
> easy to see for _this_ case, because the pre- and post-image line up
> vertically, but that is only the case for pure transliterations like
> this).

Yeah, the simplicity and obviousness certainly is very tempting.

With something like

CW=$(printf "\033[7m")	# white
CR=$(printf "\033[27m") # reset

upfront, the test helper does not even need to worry about feeding a
random string through printf as if it is a format string.


```

## Brian Henderson, 2016-08-22 15:55

Subject: [PATCH] diff-highlight: add some tests.
Message-ID: <20160822155539.25350-1-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160822155539.25350-1-henderson.bj%40gmail.com
In-Reply-To: <xmqqeg5kz8zq.fsf@gitster.mtv.corp.google.com>

```
Jeff, I love your idea. how's this looking?

Junio, I wasn't meaning to be stubborn, although definitely a fault of mine. I
understand a lot better now, thanks for your patience.

---
 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/t/Makefile                |  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 163 +++++++++++++++++++++++
 3 files changed, 190 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+	$(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 0000000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+	@echo 'Run "$(MAKE) test" to launch test scripts'
+	@echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+	$(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 0000000..7c303f7
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,163 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+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
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all='skipping diff-highlight tests; perl not available'
+	test_done
+fi
+
+# dh_test is a test helper function which takes 3 file names as parameters. The
+# first 2 files are used to generate diff and commit output, which is then
+# piped through diff-highlight. The 3rd file should contain the expected output
+# of diff-highlight (minus the diff/commit header, ie. everything after and
+# including the first @@ line).
+dh_test () {
+	a="$1" b="$2" &&
+
+	cat >patch.exp &&
+
+	{
+		cat "$a" >file &&
+		git add file &&
+		git commit -m "Add a file" &&
+
+		cat "$b" >file &&
+		git diff file >diff.raw &&
+		git commit -am "Update a file" &&
+		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 &&
+	test_cmp patch.exp diff.act &&
+	test_cmp patch.exp commit.act
+}
+
+test_strip_patch_header () {
+	sed -n '/^@@/,$p' $*
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		0bb
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-${CW}b${CR}bb
+		+${CW}0${CR}bb
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		bb0
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-bb${CW}b${CR}
+		+bb${CW}0${CR}
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		b0b
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-b${CW}b${CR}b
+		+b${CW}0${CR}b
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		000
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-bbb
+		+000
+		 ccc
+	EOF
+'
+
+test_expect_failure 'diff-highlight highlights mismatched hunk size' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		b0b
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-b${CW}b${CR}b
+		+b${CW}0${CR}b
+		+ccc
+	EOF
+'
+
+# TODO add multi-byte test
+
+test_done
-- 
2.9.0


```

## Jeff King, 2016-08-23 04:12

Subject: Re: [PATCH] diff-highlight: add some tests.
Message-ID: <20160823041252.53ldwacgdey2euxt@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160823041252.53ldwacgdey2euxt%40sigill.intra.peff.net
In-Reply-To: <20160822155539.25350-1-henderson.bj@gmail.com>

```
On Mon, Aug 22, 2016 at 08:55:39AM -0700, Brian Henderson wrote:

> Jeff, I love your idea. how's this looking?

Much more readable, IMHO.

-Peff

```

## Brian Henderson, 2016-08-29 17:33

Subject: [PATCH v4 0/3] diff-highlight: add support for --graph option
Message-ID: <20160829173347.454-1-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160829173347.454-1-henderson.bj%40gmail.com
In-Reply-To: <20160823041252.53ldwacgdey2euxt@sigill.intra.peff.net>

```
How does this look?

Drawing the graph helped me a lot in figuring out what I was actually testing. thanks!

Brian Henderson (3):
  diff-highlight: add some tests.
  diff-highlight: add failing test for handling --graph output.
  diff-highlight: add support for --graph output.

 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/diff-highlight            |  19 +-
 contrib/diff-highlight/t/Makefile                |  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 223 +++++++++++++++++++++++
 4 files changed, 263 insertions(+), 6 deletions(-)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

-- 
2.9.3


```

## Brian Henderson, 2016-08-29 17:33

Subject: [PATCH v4 2/3] diff-highlight: add failing test for handling --graph output.
Message-ID: <20160829173347.454-3-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160829173347.454-3-henderson.bj%40gmail.com
In-Reply-To: <20160829173347.454-1-henderson.bj@gmail.com>

```
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 60 ++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 7c303f7..54e11fe 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -49,6 +49,55 @@ test_strip_patch_header () {
 	sed -n '/^@@/,$p' $*
 }
 
+# dh_test_setup_history generates a contrived graph such that we have at least
+# 1 nesting (E) and 2 nestings (F).
+#
+#	      A branch
+#	     /
+#	D---E---F master
+#
+#	git log --all --graph
+#	* commit
+#	|    A
+#	| * commit
+#	| |    F
+#	| * commit
+#	|/
+#	|    E
+#	* commit
+#	     D
+#
+dh_test_setup_history () {
+	echo "file1" >file1 &&
+	echo "file2" >file2 &&
+	echo "file3" >file3 &&
+
+	cat file1 >file &&
+	git add file &&
+	git commit -m "D" &&
+
+	git checkout -b branch &&
+	cat file2 >file &&
+	git commit -am "A" &&
+
+	git checkout master &&
+	cat file2 >file &&
+	git commit -am "E" &&
+
+	cat file3 >file &&
+	git commit -am "F"
+}
+
+left_trim () {
+	"$PERL_PATH" -pe 's/^\s+//'
+}
+
+trim_graph () {
+	# graphs start with * or |
+	# followed by a space or / or \
+	"$PERL_PATH" -pe 's@^((\*|\|)( |/|\\))+@@'
+}
+
 test_expect_success 'diff-highlight highlights the beginning of a line' '
 	cat >a <<-\EOF &&
 		aaa
@@ -160,4 +209,15 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 
 # TODO add multi-byte test
 
+test_expect_failure 'diff-highlight works with the --graph option' '
+	dh_test_setup_history &&
+
+	# topo-order so that the order of the commits is the same as with --graph
+	# trim graph elements so we can do a diff
+	# trim leading space because our trim_graph is not perfect
+	git log --branches -p --topo-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
+	git log --branches -p --graph | "$DIFF_HIGHLIGHT" | trim_graph | left_trim >graph.act &&
+	test_cmp graph.exp graph.act
+'
+
 test_done
-- 
2.9.3


```

## Brian Henderson, 2016-08-29 17:33

Subject: [PATCH v4 1/3] diff-highlight: add some tests.
Message-ID: <20160829173347.454-2-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160829173347.454-2-henderson.bj%40gmail.com
In-Reply-To: <20160829173347.454-1-henderson.bj@gmail.com>

```
---
 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/t/Makefile                |  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 163 +++++++++++++++++++++++
 3 files changed, 190 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+	$(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 0000000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+	@echo 'Run "$(MAKE) test" to launch test scripts'
+	@echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+	$(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 0000000..7c303f7
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,163 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+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
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all='skipping diff-highlight tests; perl not available'
+	test_done
+fi
+
+# dh_test is a test helper function which takes 3 file names as parameters. The
+# first 2 files are used to generate diff and commit output, which is then
+# piped through diff-highlight. The 3rd file should contain the expected output
+# of diff-highlight (minus the diff/commit header, ie. everything after and
+# including the first @@ line).
+dh_test () {
+	a="$1" b="$2" &&
+
+	cat >patch.exp &&
+
+	{
+		cat "$a" >file &&
+		git add file &&
+		git commit -m "Add a file" &&
+
+		cat "$b" >file &&
+		git diff file >diff.raw &&
+		git commit -am "Update a file" &&
+		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 &&
+	test_cmp patch.exp diff.act &&
+	test_cmp patch.exp commit.act
+}
+
+test_strip_patch_header () {
+	sed -n '/^@@/,$p' $*
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		0bb
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-${CW}b${CR}bb
+		+${CW}0${CR}bb
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		bb0
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-bb${CW}b${CR}
+		+bb${CW}0${CR}
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		b0b
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-b${CW}b${CR}b
+		+b${CW}0${CR}b
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		000
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-bbb
+		+000
+		 ccc
+	EOF
+'
+
+test_expect_failure 'diff-highlight highlights mismatched hunk size' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		b0b
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-b${CW}b${CR}b
+		+b${CW}0${CR}b
+		+ccc
+	EOF
+'
+
+# TODO add multi-byte test
+
+test_done
-- 
2.9.3


```

## Brian Henderson, 2016-08-29 17:33

Subject: [PATCH v4 3/3] diff-highlight: add support for --graph output.
Message-ID: <20160829173347.454-4-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160829173347.454-4-henderson.bj%40gmail.com
In-Reply-To: <20160829173347.454-1-henderson.bj@gmail.com>

```
---
 contrib/diff-highlight/diff-highlight            | 19 +++++++++++++------
 contrib/diff-highlight/t/t9400-diff-highlight.sh |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index ffefc31..9280c88 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -21,6 +21,10 @@ my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
+# The patch portion of git log -p --graph should only ever have preceding | and
+# not / or \ as merge history only shows up on the commit line.
+my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
+
 my @removed;
 my @added;
 my $in_hunk;
@@ -32,12 +36,12 @@ $SIG{PIPE} = 'DEFAULT';
 while (<>) {
 	if (!$in_hunk) {
 		print;
-		$in_hunk = /^$COLOR*\@/;
+		$in_hunk = /^$GRAPH*$COLOR*\@/;
 	}
-	elsif (/^$COLOR*-/) {
+	elsif (/^$GRAPH*$COLOR*-/) {
 		push @removed, $_;
 	}
-	elsif (/^$COLOR*\+/) {
+	elsif (/^$GRAPH*$COLOR*\+/) {
 		push @added, $_;
 	}
 	else {
@@ -46,7 +50,7 @@ while (<>) {
 		@added = ();
 
 		print;
-		$in_hunk = /^$COLOR*[\@ ]/;
+		$in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
 	}
 
 	# Most of the time there is enough output to keep things streaming,
@@ -163,6 +167,9 @@ sub highlight_pair {
 	}
 }
 
+# we split either by $COLOR or by character. This has the side effect of
+# leaving in graph cruft. It works because the graph cruft does not contain "-"
+# or "+"
 sub split_line {
 	local $_ = shift;
 	return utf8::decode($_) ?
@@ -211,8 +218,8 @@ sub is_pair_interesting {
 	my $suffix_a = join('', @$a[($sa+1)..$#$a]);
 	my $suffix_b = join('', @$b[($sb+1)..$#$b]);
 
-	return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
-	       $prefix_b !~ /^$COLOR*\+$BORING*$/ ||
+	return $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||
+	       $prefix_b !~ /^$GRAPH*$COLOR*\+$BORING*$/ ||
 	       $suffix_a !~ /^$BORING*$/ ||
 	       $suffix_b !~ /^$BORING*$/;
 }
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 54e11fe..e42232d 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -209,7 +209,7 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 
 # TODO add multi-byte test
 
-test_expect_failure 'diff-highlight works with the --graph option' '
+test_expect_success 'diff-highlight works with the --graph option' '
 	dh_test_setup_history &&
 
 	# topo-order so that the order of the commits is the same as with --graph
-- 
2.9.3


```

## Junio C Hamano, 2016-08-29 21:37

Subject: Re: [PATCH v4 0/3] diff-highlight: add support for --graph option
Message-ID: <xmqq4m638ded.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqq4m638ded.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <20160829173347.454-1-henderson.bj@gmail.com>

```
Brian Henderson <henderson.bj@gmail.com> writes:

> How does this look?
>
> Drawing the graph helped me a lot in figuring out what I was
> actually testing. thanks!

Yeah, I also am pleased to see the picture of what is being tested
in the test script.

With your sign-off, they would have been almost perfect ;-).

> Brian Henderson (3):
>   diff-highlight: add some tests.
>   diff-highlight: add failing test for handling --graph output.
>   diff-highlight: add support for --graph output.
>
>  contrib/diff-highlight/Makefile                  |   5 +
>  contrib/diff-highlight/diff-highlight            |  19 +-
>  contrib/diff-highlight/t/Makefile                |  22 +++
>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 223 +++++++++++++++++++++++
>  4 files changed, 263 insertions(+), 6 deletions(-)
>  create mode 100644 contrib/diff-highlight/Makefile
>  create mode 100644 contrib/diff-highlight/t/Makefile
>  create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

```

## Jeff King, 2016-08-30 08:11

Subject: Re: [PATCH v4 0/3] diff-highlight: add support for --graph option
Message-ID: <20160830081152.rbvwexqqe6jwkue2@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160830081152.rbvwexqqe6jwkue2%40sigill.intra.peff.net
In-Reply-To: <20160829173347.454-1-henderson.bj@gmail.com>

```
On Mon, Aug 29, 2016 at 10:33:44AM -0700, Brian Henderson wrote:

> How does this look?
> 
> Drawing the graph helped me a lot in figuring out what I was actually testing. thanks!
> 
> Brian Henderson (3):
>   diff-highlight: add some tests.
>   diff-highlight: add failing test for handling --graph output.
>   diff-highlight: add support for --graph output.

These all look good to me. As Junio mentioned, you need to add a
signoff (see the section "Sign your work" in Documentation/SubmittingPatches).

Also, a minor nit, but we don't usually put a "." at the end of our
commit message subjects (not worth a re-roll on its own, but if you are
sending them again anyway...).

-Peff

```

## Brian Henderson, 2016-08-30 14:07

Subject: [PATCH v4 0/3] diff-highlight: add support for --graph option
Message-ID: <20160830140714.19048-1-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160830140714.19048-1-henderson.bj%40gmail.com
In-Reply-To: <xmqq4m638ded.fsf@gitster.mtv.corp.google.com>

```
On Mon, Aug 29, 2016 at 02:37:46PM -0700, Junio C Hamano wrote:
> Brian Henderson <henderson.bj@gmail.com> writes:
>
> > How does this look?
> >
> > Drawing the graph helped me a lot in figuring out what I was
> > actually testing. thanks!
>
> Yeah, I also am pleased to see the picture of what is being tested
> in the test script.
>
> With your sign-off, they would have been almost perfect ;-).

doh. fixed.

I left the subject as v4, probably mostly because I have this weird aversion to
increasing version numbers :) but I justified it by thinking that the actual
patch set isn't changing, I just added the sign-off (and updated the commit
messages per Jeff.) Hope that's ok.

thanks everyone for all your help!


Brian Henderson (3):
  diff-highlight: add some tests
  diff-highlight: add failing test for handling --graph output
  diff-highlight: add support for --graph output

 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/diff-highlight            |  19 +-
 contrib/diff-highlight/t/Makefile                |  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 223 +++++++++++++++++++++++
 4 files changed, 263 insertions(+), 6 deletions(-)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

-- 
2.9.3


```

## Brian Henderson, 2016-08-30 14:07

Subject: [PATCH v4 1/3] diff-highlight: add some tests
Message-ID: <20160830140714.19048-2-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160830140714.19048-2-henderson.bj%40gmail.com
In-Reply-To: <20160830140714.19048-1-henderson.bj@gmail.com>

```
Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/Makefile                  |   5 +
 contrib/diff-highlight/t/Makefile                |  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 163 +++++++++++++++++++++++
 3 files changed, 190 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+	$(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 0000000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+	@echo 'Run "$(MAKE) test" to launch test scripts'
+	@echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+	$(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 0000000..7c303f7
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,163 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+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
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all='skipping diff-highlight tests; perl not available'
+	test_done
+fi
+
+# dh_test is a test helper function which takes 3 file names as parameters. The
+# first 2 files are used to generate diff and commit output, which is then
+# piped through diff-highlight. The 3rd file should contain the expected output
+# of diff-highlight (minus the diff/commit header, ie. everything after and
+# including the first @@ line).
+dh_test () {
+	a="$1" b="$2" &&
+
+	cat >patch.exp &&
+
+	{
+		cat "$a" >file &&
+		git add file &&
+		git commit -m "Add a file" &&
+
+		cat "$b" >file &&
+		git diff file >diff.raw &&
+		git commit -am "Update a file" &&
+		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 &&
+	test_cmp patch.exp diff.act &&
+	test_cmp patch.exp commit.act
+}
+
+test_strip_patch_header () {
+	sed -n '/^@@/,$p' $*
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		0bb
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-${CW}b${CR}bb
+		+${CW}0${CR}bb
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		bb0
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-bb${CW}b${CR}
+		+bb${CW}0${CR}
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		b0b
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-b${CW}b${CR}b
+		+b${CW}0${CR}b
+		 ccc
+	EOF
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+		ccc
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		000
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-bbb
+		+000
+		 ccc
+	EOF
+'
+
+test_expect_failure 'diff-highlight highlights mismatched hunk size' '
+	cat >a <<-\EOF &&
+		aaa
+		bbb
+	EOF
+
+	cat >b <<-\EOF &&
+		aaa
+		b0b
+		ccc
+	EOF
+
+	dh_test a b <<-EOF
+		@@ -1,3 +1,3 @@
+		 aaa
+		-b${CW}b${CR}b
+		+b${CW}0${CR}b
+		+ccc
+	EOF
+'
+
+# TODO add multi-byte test
+
+test_done
-- 
2.9.3


```

## Brian Henderson, 2016-08-30 14:07

Subject: [PATCH v4 3/3] diff-highlight: add support for --graph output
Message-ID: <20160830140714.19048-4-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160830140714.19048-4-henderson.bj%40gmail.com
In-Reply-To: <20160830140714.19048-1-henderson.bj@gmail.com>

```
Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/diff-highlight            | 19 +++++++++++++------
 contrib/diff-highlight/t/t9400-diff-highlight.sh |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index ffefc31..9280c88 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -21,6 +21,10 @@ my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
+# The patch portion of git log -p --graph should only ever have preceding | and
+# not / or \ as merge history only shows up on the commit line.
+my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
+
 my @removed;
 my @added;
 my $in_hunk;
@@ -32,12 +36,12 @@ $SIG{PIPE} = 'DEFAULT';
 while (<>) {
 	if (!$in_hunk) {
 		print;
-		$in_hunk = /^$COLOR*\@/;
+		$in_hunk = /^$GRAPH*$COLOR*\@/;
 	}
-	elsif (/^$COLOR*-/) {
+	elsif (/^$GRAPH*$COLOR*-/) {
 		push @removed, $_;
 	}
-	elsif (/^$COLOR*\+/) {
+	elsif (/^$GRAPH*$COLOR*\+/) {
 		push @added, $_;
 	}
 	else {
@@ -46,7 +50,7 @@ while (<>) {
 		@added = ();
 
 		print;
-		$in_hunk = /^$COLOR*[\@ ]/;
+		$in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
 	}
 
 	# Most of the time there is enough output to keep things streaming,
@@ -163,6 +167,9 @@ sub highlight_pair {
 	}
 }
 
+# we split either by $COLOR or by character. This has the side effect of
+# leaving in graph cruft. It works because the graph cruft does not contain "-"
+# or "+"
 sub split_line {
 	local $_ = shift;
 	return utf8::decode($_) ?
@@ -211,8 +218,8 @@ sub is_pair_interesting {
 	my $suffix_a = join('', @$a[($sa+1)..$#$a]);
 	my $suffix_b = join('', @$b[($sb+1)..$#$b]);
 
-	return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
-	       $prefix_b !~ /^$COLOR*\+$BORING*$/ ||
+	return $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||
+	       $prefix_b !~ /^$GRAPH*$COLOR*\+$BORING*$/ ||
 	       $suffix_a !~ /^$BORING*$/ ||
 	       $suffix_b !~ /^$BORING*$/;
 }
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 54e11fe..e42232d 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -209,7 +209,7 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 
 # TODO add multi-byte test
 
-test_expect_failure 'diff-highlight works with the --graph option' '
+test_expect_success 'diff-highlight works with the --graph option' '
 	dh_test_setup_history &&
 
 	# topo-order so that the order of the commits is the same as with --graph
-- 
2.9.3


```

## Brian Henderson, 2016-08-30 14:07

Subject: [PATCH v4 2/3] diff-highlight: add failing test for handling --graph output
Message-ID: <20160830140714.19048-3-henderson.bj@gmail.com>
URL: https://gitlist.dev/e/20160830140714.19048-3-henderson.bj%40gmail.com
In-Reply-To: <20160830140714.19048-1-henderson.bj@gmail.com>

```
Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 60 ++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 7c303f7..54e11fe 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -49,6 +49,55 @@ test_strip_patch_header () {
 	sed -n '/^@@/,$p' $*
 }
 
+# dh_test_setup_history generates a contrived graph such that we have at least
+# 1 nesting (E) and 2 nestings (F).
+#
+#	      A branch
+#	     /
+#	D---E---F master
+#
+#	git log --all --graph
+#	* commit
+#	|    A
+#	| * commit
+#	| |    F
+#	| * commit
+#	|/
+#	|    E
+#	* commit
+#	     D
+#
+dh_test_setup_history () {
+	echo "file1" >file1 &&
+	echo "file2" >file2 &&
+	echo "file3" >file3 &&
+
+	cat file1 >file &&
+	git add file &&
+	git commit -m "D" &&
+
+	git checkout -b branch &&
+	cat file2 >file &&
+	git commit -am "A" &&
+
+	git checkout master &&
+	cat file2 >file &&
+	git commit -am "E" &&
+
+	cat file3 >file &&
+	git commit -am "F"
+}
+
+left_trim () {
+	"$PERL_PATH" -pe 's/^\s+//'
+}
+
+trim_graph () {
+	# graphs start with * or |
+	# followed by a space or / or \
+	"$PERL_PATH" -pe 's@^((\*|\|)( |/|\\))+@@'
+}
+
 test_expect_success 'diff-highlight highlights the beginning of a line' '
 	cat >a <<-\EOF &&
 		aaa
@@ -160,4 +209,15 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 
 # TODO add multi-byte test
 
+test_expect_failure 'diff-highlight works with the --graph option' '
+	dh_test_setup_history &&
+
+	# topo-order so that the order of the commits is the same as with --graph
+	# trim graph elements so we can do a diff
+	# trim leading space because our trim_graph is not perfect
+	git log --branches -p --topo-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
+	git log --branches -p --graph | "$DIFF_HIGHLIGHT" | trim_graph | left_trim >graph.act &&
+	test_cmp graph.exp graph.act
+'
+
 test_done
-- 
2.9.3


```

## Jeff King, 2016-08-31 05:02

Subject: Re: [PATCH v4 0/3] diff-highlight: add support for --graph option
Message-ID: <20160831050229.cabhfzqcpcpvkugl@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160831050229.cabhfzqcpcpvkugl%40sigill.intra.peff.net
In-Reply-To: <20160830140714.19048-1-henderson.bj@gmail.com>

```
On Tue, Aug 30, 2016 at 07:07:11AM -0700, Brian Henderson wrote:

> On Mon, Aug 29, 2016 at 02:37:46PM -0700, Junio C Hamano wrote:
> > Brian Henderson <henderson.bj@gmail.com> writes:
> >
> > > How does this look?
> > >
> > > Drawing the graph helped me a lot in figuring out what I was
> > > actually testing. thanks!
> >
> > Yeah, I also am pleased to see the picture of what is being tested
> > in the test script.
> >
> > With your sign-off, they would have been almost perfect ;-).
> 
> doh. fixed.
> 
> I left the subject as v4, probably mostly because I have this weird aversion to
> increasing version numbers :) but I justified it by thinking that the actual
> patch set isn't changing, I just added the sign-off (and updated the commit
> messages per Jeff.) Hope that's ok.

Thanks. Here are a few patches to go on top. The first one could
arguably be squashed into your first patch (and I don't mind if Junio
wants to do so while applying, but I don't think it's worth you
re-sending).

The second one fleshes out the test scripts a bit, now that we have them
(yay!).

And the third fixes a bug that was reported to me off-list. I held back
because it touches the same lines as your topic (and as a bonus, I was
now able to write a test for it). It could be its own topic branch that
graduates separately, but seeing as it's contrib, I don't mind one big
diff-highlight potpourri topic if it makes things simpler.

  [1/3]: diff-highlight: ignore test cruft
  [2/3]: diff-highlight: add multi-byte tests
  [3/3]: diff-highlight: avoid highlighting combined diffs

-Peff

```

## Jeff King, 2016-08-31 05:02

Subject: [PATCH 1/3] diff-highlight: ignore test cruft
Message-ID: <20160831050253.5diq5coyedgpaiil@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160831050253.5diq5coyedgpaiil%40sigill.intra.peff.net
In-Reply-To: <20160831050229.cabhfzqcpcpvkugl@sigill.intra.peff.net>

```
These are the same as in the normal t/.gitignore, with the
exception of ".prove", as our Makefile does not support it.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/t/.gitignore | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 contrib/diff-highlight/t/.gitignore

diff --git a/contrib/diff-highlight/t/.gitignore b/contrib/diff-highlight/t/.gitignore
new file mode 100644
index 0000000..7dcbb23
--- /dev/null
+++ b/contrib/diff-highlight/t/.gitignore
@@ -0,0 +1,2 @@
+/trash directory*
+/test-results
-- 
2.10.0.rc2.125.gcfb3d08


```

## Jeff King, 2016-08-31 05:03

Subject: [PATCH 2/3] diff-highlight: add multi-byte tests
Message-ID: <20160831050309.3k43nn737ztcajiz@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160831050309.3k43nn737ztcajiz%40sigill.intra.peff.net
In-Reply-To: <20160831050229.cabhfzqcpcpvkugl@sigill.intra.peff.net>

```
Now that we have a test suite for diff highlight, we can
show off the improvements from 8d00662 (diff-highlight: do
not split multibyte characters, 2015-04-03).

While we're at it, we can also add another case that
_doesn't_ work: combining code points are treated as their
own unit, which means that we may stick colors between them
and the character they are modifying (with the result that
the color is not shown in an xterm, though it's possible
that other terminals err the other way, and show the color
but not the accent).  There's no fix here, but let's
document it as a failure.

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

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index e42232d..7d034aa 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -207,7 +207,41 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '
 	EOF
 '
 
-# TODO add multi-byte test
+# These two code points share the same leading byte in UTF-8 representation;
+# a naive byte-wise diff would highlight only the second byte.
+#
+#   - U+00f3 ("o" with acute)
+o_accent=$(printf '\303\263')
+#   - U+00f8 ("o" with stroke)
+o_stroke=$(printf '\303\270')
+
+test_expect_success 'diff-highlight treats multibyte utf-8 as a unit' '
+	echo "unic${o_accent}de" >a &&
+	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
+	EOF
+'
+
+# Unlike the UTF-8 above, these are combining code points which are meant
+# to modify the character preceding them:
+#
+#   - U+0301 (combining acute accent)
+combine_accent=$(printf '\314\201')
+#   - U+0302 (combining circumflex)
+combine_circum=$(printf '\314\202')
+
+test_expect_failure 'diff-highlight treats combining code points as a unit' '
+	echo "unico${combine_accent}de" >a &&
+	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
+	EOF
+'
 
 test_expect_success 'diff-highlight works with the --graph option' '
 	dh_test_setup_history &&
-- 
2.10.0.rc2.125.gcfb3d08


```

## Jeff King, 2016-08-31 05:05

Subject: [PATCH 3/3] diff-highlight: avoid highlighting combined diffs
Message-ID: <20160831050538.53fki52cvwlmkd6y@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20160831050538.53fki52cvwlmkd6y%40sigill.intra.peff.net
In-Reply-To: <20160831050229.cabhfzqcpcpvkugl@sigill.intra.peff.net>

```
The algorithm in diff-highlight only understands how to look
at two sides of a diff; it cannot correctly handle combined
diffs with multiple preimages. Often highlighting does not
trigger at all for these diffs because the line counts do
not match up.  E.g., if we see:

  - ours
   -theirs
  ++resolved

we would not bother highlighting; it naively looks like a
single line went away, and then a separate hunk added
another single line.

But of course there are exceptions. E.g., if the other side
deleted the line, we might see:

  - ours
  ++resolved

which looks like we dropped " ours" and added "+resolved".
This is only a small highlighting glitch (we highlight the
space and the "+" along with the content), but it's also the
tip of the iceberg. Even if we learned to find the true
content here (by noticing we are in a 3-way combined diff
and marking _two_ characters from the front of the line as
uninteresting), there are other more complicated cases where
we really do need to handle a 3-way hunk.

Let's just punt for now; we can recognize combined diffs by
the presence of extra "@" symbols in the hunk header, and
treat them as non-diff content.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/diff-highlight            |  2 +-
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 37 ++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 9280c88..81bd804 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -36,7 +36,7 @@ $SIG{PIPE} = 'DEFAULT';
 while (<>) {
 	if (!$in_hunk) {
 		print;
-		$in_hunk = /^$GRAPH*$COLOR*\@/;
+		$in_hunk = /^$GRAPH*$COLOR*\@\@ /;
 	}
 	elsif (/^$GRAPH*$COLOR*-/) {
 		push @removed, $_;
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 7d034aa..64dd9f7 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -254,4 +254,41 @@ test_expect_success 'diff-highlight works with the --graph option' '
 	test_cmp graph.exp graph.act
 '
 
+# Most combined diffs won't meet diff-highlight's line-number filter. So we
+# create one here where one side drops a line and the other modifies it. That
+# should result in a diff like:
+#
+#    - modified content
+#    ++resolved content
+#
+# which naively looks like one side added "+resolved".
+test_expect_success 'diff-highlight ignores combined diffs' '
+	echo "content" >file &&
+	git add file &&
+	git commit -m base &&
+
+	>file &&
+	git commit -am master &&
+
+	git checkout -b other HEAD^ &&
+	echo "modified content" >file &&
+	git commit -am other &&
+
+	test_must_fail git merge master &&
+	echo "resolved content" >file &&
+	git commit -am resolved &&
+
+	cat >expect <<-\EOF &&
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,0 +1,1 @@@
+	- modified content
+	++resolved content
+	EOF
+
+	git show -c | "$DIFF_HIGHLIGHT" >actual.raw &&
+	sed -n "/^---/,\$p" <actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.0.rc2.125.gcfb3d08

```