[PATCH 0/3] diff-highlight: add support for git log --graph output.

[PATCH 0/3] diff-highlight: add support for git log --graph output.

[Brian Henderson]

(resending as thread instead of attachments take 3
I realized that I was adding the To field only to the cover letter and I wasn't
catching that git-send-email wasn't adding it to the other patches. Apologies.)

Hi, I've been working with Jeff King on this patch, but he encouraged me to
email the list.

I recently learned about the diff-highlight tool, and find it very helpful,
however, I frequently use the --graph option to git log which breaks the tool.
This patch looks to fix this.

I wanted to try and add some tests as well, but I was unsure what number to
pick for the second digit. As there were limited tests in the contrib directory
(only t93xx and one t7900) I just chose the next number in the 9xxx range.
Please let me know if I need to change it.

I'm also not super happy about my test case for the graph option. If anyone has
any better ideas, please let me know!

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            |  13 +--
 contrib/diff-highlight/t/Makefile                |  19 ++++
 contrib/diff-highlight/t/t9400-diff-highlight.sh |  76 ++++++++++++++++
 contrib/diff-highlight/t/test-diff-highlight.sh  | 111 +++++++++++++++++++++++
 5 files changed, 218 insertions(+), 6 deletions(-)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
 create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh

-- 
2.9.0

[PATCH 1/3] diff-highlight: add some tests.

[Brian Henderson]

---
 contrib/diff-highlight/Makefile                  |  5 ++
 contrib/diff-highlight/t/Makefile                | 19 +++++++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++
 contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
 4 files changed, 156 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
 create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..b866259
--- /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..ac81fc0
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,19 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+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 "*** $@ ***"; sh $@ $(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 100644
index 0000000..ca7605f
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+#
+# Copyright (C) 2016
+
+test_description='Test diff-highlight'
+
+. ./test-diff-highlight.sh
+. $TEST_DIRECTORY/test-lib.sh
+
+# PERL is required, but assumed to be present, although not necessarily modern
+# some tests require 5.8
+
+test_expect_success 'diff-highlight highlightes 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 highlightes 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 highlightes 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_success 'diff-highlight does not highlight mismatched hunk size' '
+  dh_test \
+    "aaa\nbbb\n" \
+    "aaa\nb0b\nccc\n"
+'
+
+# TODO add multi-byte test
+
+test_done
diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh
new file mode 100644
index 0000000..9b26271
--- /dev/null
+++ b/contrib/diff-highlight/t/test-diff-highlight.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+#
+# Copyright (C) 2016
+
+CURR_DIR=$(pwd)
+TEST_OUTPUT_DIRECTORY=$(pwd)
+TEST_DIRECTORY="$CURR_DIR"/../../../t
+cmd=diff-highlight
+CMD="$CURR_DIR"/../$cmd
+
+CW="\033[7m"
+CR="\033[27m"
+
+export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR
+
+dh_test() {
+  dh_diff_test "$@"
+  dh_commit_test "$@"
+}
+
+dh_diff_test() {
+  local a="$1" b="$2"
+
+  printf "$a" > file
+  git add file
+
+  printf "$b" > file
+
+  git diff file > diff.raw
+
+  if test "$#" = 3
+  then
+    # remove last newline
+    head -n5 diff.raw | head -c -1 > diff.act
+    printf "$3" >> diff.act
+  else
+    cat diff.raw > diff.act
+  fi
+
+  < diff.raw $CMD > diff.exp
+
+  diff diff.exp diff.act
+}
+
+dh_commit_test() {
+  local 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 "$#" = 3
+  then
+    # remove last newline
+    head -n11 commit.raw | head -c -1 > commit.act
+    printf "$3" >> commit.act
+  else
+    cat commit.raw > commit.act
+  fi
+
+  < commit.raw $CMD > commit.exp
+
+  diff commit.exp commit.act
+}
-- 
2.9.0

Re: [PATCH 1/3] diff-highlight: add some tests.

[Eric Wong]

Brian Henderson <henderson.bj@gmail.com> wrote:

Hi Brian,

A few minor portability/style nits below, but contrib/ probably
(still?) has laxer rules than the rest of git...

I think we still require Signed-off-by lines in contrib,
though...

> +++ b/contrib/diff-highlight/t/Makefile
> @@ -0,0 +1,19 @@
> +-include ../../../config.mak.autogen
> +-include ../../../config.mak
> +
> +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 "*** $@ ***"; sh $@ $(GIT_TEST_OPTS)

Probably:

	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)

like we do in t/Makefile in case we need to override 'sh'.

> +
> +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 100644
> index 0000000..ca7605f
> --- /dev/null
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2016

IANAL, but I think your name (or who you represent) needs to be
in the copyright.

> +
> +test_description='Test diff-highlight'
> +
> +. ./test-diff-highlight.sh
> +. $TEST_DIRECTORY/test-lib.sh

TEST_DIRECTORY ought to be quoted since it could contain
shell-unfriendly chars (we intentionally name 'trash directory'
this way to trigger errors).

> +
> +# PERL is required, but assumed to be present, although not necessarily modern
> +# some tests require 5.8
> +
> +test_expect_success 'diff-highlight highlightes the beginning of a line' '

You can declare a prereq for PERL::

	test_expect_success PERL 'name' 'true'

And spelling: "highlights" (there's other instances of this, too)

> +  dh_test \
> +    "aaa\nbbb\nccc\n" \
> +    "aaa\n0bb\nccc\n" \
> +"

We use tabs for shell indentation.

<snip>

> diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh
> new file mode 100644
> index 0000000..9b26271
> --- /dev/null
> +++ b/contrib/diff-highlight/t/test-diff-highlight.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2016
> +
> +CURR_DIR=$(pwd)
> +TEST_OUTPUT_DIRECTORY=$(pwd)
> +TEST_DIRECTORY="$CURR_DIR"/../../../t
> +cmd=diff-highlight
> +CMD="$CURR_DIR"/../$cmd

Same comments as above on quoting pathnames and empty copyright.

> +
> +CW="\033[7m"
> +CR="\033[27m"
> +
> +export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR
> +
> +dh_test() {
> +  dh_diff_test "$@"
> +  dh_commit_test "$@"
> +}
> +
> +dh_diff_test() {
> +  local a="$1" b="$2"

"local" is not a portable construct.  It's common for
Debian/Ubuntu systems to use dash as /bin/sh instead of bash;
(dash is faster, and mostly sticks to POSIX)

The "devscripts" package in Debian/Ubuntu-based systems has a
handy "checkbashisms" tool for checking portability of shell
scripts.

> +
> +  printf "$a" > file
> +  git add file
> +
> +  printf "$b" > file
> +
> +  git diff file > diff.raw

Commands should be '&&'-chained here since any of them could fail
("git add"/printf/etc could run out of space or fail on disk errors)

Also, our redirect style is:	command >file
without a space between '>' and destination.

See Documentation/CodingGuidelines for more details.
Unfortunately, the reasoning is not explained for '>NOSPACE'
and I'm not sure exactly why, either...

> +  if test "$#" = 3

Better to use -eq for numeric comparisons: test $# -eq 3
Quoting $# doesn't seem necessary unless your shell is
hopelessly buggy :)

> +  then
> +    # remove last newline
> +    head -n5 diff.raw | head -c -1 > diff.act

"head -c" isn't portable, fortunately Jeff hoisted it out for
use as test_copy_bytes in commit 48860819e8026
https://public-inbox.org/git/20160630090753.GA17463@sigill.intra.peff.net/

> +    printf "$3" >> diff.act
> +  else
> +    cat diff.raw > diff.act
> +  fi
> +
> +  < diff.raw $CMD > diff.exp

$CMD probably needs to be quoted.  However, by the time I got
here I had to ask myself:  What is $CMD again?
A: Oh, look up at the top!

*shrug*  My attention span is tiny and my fonts are gigantic.

Perhaps using:

	DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight

Would be more-readable?

> +
> +  diff diff.exp diff.act

Maybe use test_cmp to avoid depending external diff.
(or "git diff -b --no-index" in your later test)

Same comments for the rest of the series, I think.

Typically, we expect a reroll in a few days, and I guess there's
no rush (so you can squash your comment patch in addressing
Junio's concern into 3/3).

Thanks.

Re: [PATCH 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:

<snip>

> Typically, we expect a reroll in a few days, and I guess there's
> no rush (so you can squash your comment patch in addressing
> Junio's concern into 3/3).
> 
> Thanks.

thanks, (slowly) working on an update.

Re: [PATCH 1/3] diff-highlight: add some tests.

[Junio C Hamano]

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

> On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:
>
> <snip>
>
>> Typically, we expect a reroll in a few days, and I guess there's
>> no rush (so you can squash your comment patch in addressing
>> Junio's concern into 3/3).
>> 
>> Thanks.
>
> thanks, (slowly) working on an update.

Thanks, both, for keeping the ball rolling ;-)

[PATCH v2 0/3] diff-highlight: add support for git log --graph output.

[Brian Henderson]

Changes made per Eric.

On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:
> Brian Henderson <henderson.bj@gmail.com> wrote:
> 
> Hi Brian,
> 
> A few minor portability/style nits below, but contrib/ probably
> (still?) has laxer rules than the rest of git...
> 
> I think we still require Signed-off-by lines in contrib,
> though...

added

> 
> > +++ b/contrib/diff-highlight/t/Makefile
> > @@ -0,0 +1,19 @@
> > +-include ../../../config.mak.autogen
> > +-include ../../../config.mak
> > +
> > +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 "*** $@ ***"; sh $@ $(GIT_TEST_OPTS)
> 
> Probably:
> 
> 	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
> 
> like we do in t/Makefile in case we need to override 'sh'.
> 

done, although I basically had to copy the SHELL_PATH_SQ logic from t/Makefile

> > +
> > +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 100644
> > index 0000000..ca7605f
> > --- /dev/null
> > +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> > @@ -0,0 +1,63 @@
> > +#!/bin/sh
> > +#
> > +# Copyright (C) 2016
> 
> IANAL, but I think your name (or who you represent) needs to be
> in the copyright.

diff-highlight doesn't have a copyright, so I just removed it. ok?

> 
> > +
> > +test_description='Test diff-highlight'
> > +
> > +. ./test-diff-highlight.sh
> > +. $TEST_DIRECTORY/test-lib.sh
> 
> TEST_DIRECTORY ought to be quoted since it could contain
> shell-unfriendly chars (we intentionally name 'trash directory'
> this way to trigger errors).

done

> 
> > +
> > +# PERL is required, but assumed to be present, although not necessarily modern
> > +# some tests require 5.8
> > +
> > +test_expect_success 'diff-highlight highlightes the beginning of a line' '
> 
> You can declare a prereq for PERL::
> 
> 	test_expect_success PERL 'name' 'true'

done

> 
> And spelling: "highlights" (there's other instances of this, too)

oops, thanks

> 
> > +  dh_test \
> > +    "aaa\nbbb\nccc\n" \
> > +    "aaa\n0bb\nccc\n" \
> > +"
> 
> We use tabs for shell indentation.

done

<snip>

> > +
> > +dh_diff_test() {
> > +  local a="$1" b="$2"
> 
> "local" is not a portable construct.  It's common for
> Debian/Ubuntu systems to use dash as /bin/sh instead of bash;
> (dash is faster, and mostly sticks to POSIX)
> 
> The "devscripts" package in Debian/Ubuntu-based systems has a
> handy "checkbashisms" tool for checking portability of shell
> scripts.

checkbashisms didn't output anything, and I found other instances of local in some tests. but I removed anyway.

> 
> > +
> > +  printf "$a" > file
> > +  git add file
> > +
> > +  printf "$b" > file
> > +
> > +  git diff file > diff.raw
> 
> Commands should be '&&'-chained here since any of them could fail
> ("git add"/printf/etc could run out of space or fail on disk errors)

I wasn't totally sure what to do here. I added some checks for empty files and
&& chained them with the last command to verify I wasn't diffing 2 empty files.
Hope that is sufficient.

> 
> Also, our redirect style is:	command >file
> without a space between '>' and destination.

done

> 
> See Documentation/CodingGuidelines for more details.
> Unfortunately, the reasoning is not explained for '>NOSPACE'
> and I'm not sure exactly why, either...
> 
> > +  if test "$#" = 3

done

> 
> Better to use -eq for numeric comparisons: test $# -eq 3
> Quoting $# doesn't seem necessary unless your shell is
> hopelessly buggy :)
> 
> > +  then
> > +    # remove last newline
> > +    head -n5 diff.raw | head -c -1 > diff.act
> 
> "head -c" isn't portable, fortunately Jeff hoisted it out for
> use as test_copy_bytes in commit 48860819e8026
> https://public-inbox.org/git/20160630090753.GA17463@sigill.intra.peff.net/

It didn't look like his version supported a negative number, so I rolled my own.

> 
> > +    printf "$3" >> diff.act
> > +  else
> > +    cat diff.raw > diff.act
> > +  fi
> > +
> > +  < diff.raw $CMD > diff.exp
> 
> $CMD probably needs to be quoted.  However, by the time I got
> here I had to ask myself:  What is $CMD again?
> A: Oh, look up at the top!
> 
> *shrug*  My attention span is tiny and my fonts are gigantic.
> 
> Perhaps using:
> 
> 	DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
> 
> Would be more-readable?

good, done.

> 
> > +
> > +  diff diff.exp diff.act
> 
> Maybe use test_cmp to avoid depending external diff.
> (or "git diff -b --no-index" in your later test)
> 
> Same comments for the rest of the series, I think.
> 
> Typically, we expect a reroll in a few days, and I guess there's
> no rush (so you can squash your comment patch in addressing
> Junio's concern into 3/3).
> 
> Thanks.

thanks for the feedback.

I've rebased my changes. Unfortunately, having 3 commits made this somewhat
tedious. I also find it weird that my new patch set makes it difficult to see
what changes I've made from my first set. What's the standard workflow here?

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            |  16 ++--
 contrib/diff-highlight/t/Makefile                |  22 +++++
 contrib/diff-highlight/t/t9400-diff-highlight.sh |  75 +++++++++++++++
 contrib/diff-highlight/t/test-diff-highlight.sh  | 112 +++++++++++++++++++++++
 5 files changed, 224 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
 create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh

-- 
2.9.0

Re: [PATCH v2 0/3] diff-highlight: add support for git log --graph output.

[Eric Wong]

Brian Henderson <henderson.bj@gmail.com> wrote:
> On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:
> > "local" is not a portable construct.  It's common for
> > Debian/Ubuntu systems to use dash as /bin/sh instead of bash;
> > (dash is faster, and mostly sticks to POSIX)
> > 
> > The "devscripts" package in Debian/Ubuntu-based systems has a
> > handy "checkbashisms" tool for checking portability of shell
> > scripts.
> 
> checkbashisms didn't output anything, and I found other instances of
> local in some tests. but I removed anyway.

Ah, I guess "checkbashisms --posix" is required nowadays
since Debian policy deviated from POSIX, here
(we don't blindly follow POSIX, either).

Anyways, some people still care about ksh93 as of a few months
ago; so I think avoiding "local" is preferable:

  https://public-inbox.org/git/?q=ksh93&d:..20160801

I think all of our other "local" uses are limited
to bash-specific parts: bash completion, mingw tests

<snip>

> I've rebased my changes. Unfortunately, having 3 commits made this somewhat
> tedious. I also find it weird that my new patch set makes it difficult to see
> what changes I've made from my first set. What's the standard workflow here?

I check out a new branch with the same base as the previous series
and "git diff previous current"

(without git, I'd be using interdiff from the patchutils Debian package)

Sometimes I will rebase against both old+new against Junio's master
to avoid/reduce conflicts.

Re: [PATCH v2 0/3] diff-highlight: add support for git log --graph output.

[Jeff King]

On Fri, Aug 19, 2016 at 09:19:44PM +0000, Eric Wong wrote:

> > I've rebased my changes. Unfortunately, having 3 commits made this somewhat
> > tedious. I also find it weird that my new patch set makes it difficult to see
> > what changes I've made from my first set. What's the standard workflow here?
> 
> I check out a new branch with the same base as the previous series
> and "git diff previous current"
> 
> (without git, I'd be using interdiff from the patchutils Debian package)
> 
> Sometimes I will rebase against both old+new against Junio's master
> to avoid/reduce conflicts.

You might also try Thomas Rast's topic-branch diff:

  https://github.com/trast/tbdiff

It gives better patch-by-patch differences. And it handles the case that
the topics have different bases (which is handy if you've rebased, or if
Junio happened to apply on a different base than you did).

-Peff

[PATCH v2 1/3] diff-highlight: add some tests.

[Brian Henderson]

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 | 62 +++++++++++++++++++++
 contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
 4 files changed, 158 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
 create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 0000000..b866259
--- /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..8eff178
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+. ./test-diff-highlight.sh
+. "$TEST_DIRECTORY"/test-lib.sh
+
+# PERL is required, but assumed to be present, although not necessarily modern
+# some tests require 5.8
+test_expect_success PERL 'name' 'true'
+
+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_success 'diff-highlight does not highlight mismatched hunk size' '
+	dh_test \
+		"aaa\nbbb\n" \
+		"aaa\nb0b\nccc\n"
+'
+
+# TODO add multi-byte test
+
+test_done
diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh
new file mode 100644
index 0000000..38323e8
--- /dev/null
+++ b/contrib/diff-highlight/t/test-diff-highlight.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+CURR_DIR=$(pwd)
+TEST_OUTPUT_DIRECTORY=$(pwd)
+TEST_DIRECTORY="$CURR_DIR"/../../../t
+DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
+
+CW="\033[7m"
+CR="\033[27m"
+
+export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR
+
+dh_test() {
+	dh_diff_test "$@" &&
+	dh_commit_test "$@"
+}
+
+dh_diff_test() {
+	a="$1" b="$2"
+
+	printf "$a" >file
+	git add file
+
+	printf "$b" >file
+	git diff file >diff.raw
+
+	if test $# -eq 3
+	then
+		# remove last newline
+		head -n5 diff.raw | test_chomp_eof >diff.exp
+		printf "$3" >>diff.exp
+	else
+		cat diff.raw >diff.exp
+	fi
+
+	<diff.raw "$DIFF_HIGHLIGHT" >diff.act &&
+	test -s diff.act &&
+	diff diff.exp diff.act
+}
+
+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 "$#" = 3
+	then
+		# remove last newline
+		head -n11 commit.raw | test_chomp_eof >commit.exp
+		printf "$3" >>commit.exp
+	else
+		cat commit.raw >commit.exp
+	fi
+
+	<commit.raw "$DIFF_HIGHLIGHT" >commit.act &&
+	test -s commit.act &&
+	test_cmp commit.exp commit.act
+}
+
+test_chomp_eof() {
+	perl -pe 'chomp if eof'
+}
-- 
2.9.0

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Junio C Hamano]

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

> 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 | 62 +++++++++++++++++++++
>  contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
>  4 files changed, 158 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
>  create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh

I am not sure test-diff-highlight.sh should be there; the function
definitions would still be useful but move them to the beginning of
t9400-diff-highlight.sh perhaps?

> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
> new file mode 100644
> index 0000000..b866259
> --- /dev/null
> +++ b/contrib/diff-highlight/Makefile
> @@ -0,0 +1,5 @@
> +# nothing to build
> +all:;

Drop ';'.

> 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..8eff178
> --- /dev/null
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -0,0 +1,62 @@
> +#!/bin/sh
> +
> +test_description='Test diff-highlight'
> +
> +. ./test-diff-highlight.sh
> +. "$TEST_DIRECTORY"/test-lib.sh
> +
> +# PERL is required, but assumed to be present, although not necessarily modern
> +# some tests require 5.8
> +test_expect_success PERL 'name' 'true'

If the platform lacks PERL prerequisite, this will simply be
skipped, and if the platform has it, it will always succeed.

I am not sure what you are trying to achieve by having this line
here.

> +test_expect_success 'diff-highlight does not highlight whole line' '
> +	dh_test \
> +		"aaa\nbbb\nccc\n" \
> +		"aaa\n000\nccc\n"
> +'

Hmm, does this express the desired outcome, or just document the
current (possibly broken--I dunno) behaviour?  The same question for
the next one.

> +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
> +	dh_test \
> +		"aaa\nbbb\n" \
> +		"aaa\nb0b\nccc\n"
> +'

> +dh_test() {

Style: "dh_test () {"

The other functions in this file share the same.

> +	dh_diff_test "$@" &&
> +	dh_commit_test "$@"
> +}
> +
> +dh_diff_test() {
> +	a="$1" b="$2"
> +
> +	printf "$a" >file
> +	git add file
> +
> +	printf "$b" >file
> +	git diff file >diff.raw
> +
> +	if test $# -eq 3
> +	then
> +		# remove last newline
> +		head -n5 diff.raw | test_chomp_eof >diff.exp

A reader can see "remove last newline" by seeing test_chomp_eof and
what it does without a comment, but it is totally unclear why you
need to remove.  The comment that says what it does without saying
why it does it is useless.

> +		printf "$3" >>diff.exp
> +	else
> +		cat diff.raw >diff.exp
> +	fi
> +
> +	<diff.raw "$DIFF_HIGHLIGHT" >diff.act &&

Even though this is technically kosher, I do not see a merit of
deviating from the common practice of starting a line with the
command, i.e.

	"$DIFF_HIGHLIGHT" <diff.raw >diff.actual

would be much easier to read.

> +	test -s diff.act &&

Why?  If you always have the expected output that you are going to
compare with, wouldn't that sufficient to do that test without this?
Besides, having "test -s" means that you can never make sure that a
certain pair of input does not show any changes.  Perhaps drop it?

> +	diff diff.exp diff.act

Use test_cmp unless there is a strong reason why you shouldn't?

> +}
> +
> +dh_commit_test() {
> +	a="$1" b="$2"
> +
> +	printf "$a" >file
> +	git add file
> +	git commit -m"Add a file" >/dev/null

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" >/dev/null

Likewise.

    git commit -a -m "Update a file"

The remainder of the file invites the same set of questions and
comments you see for dh_diff_test() above, so I won't repeat them.

Thanks.

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Wed, Aug 17, 2016 at 12:09:25PM -0700, Junio C Hamano wrote:
> Brian Henderson <henderson.bj@gmail.com> writes:

<snip>

> 
> > +
> > +# PERL is required, but assumed to be present, although not necessarily modern
> > +# some tests require 5.8
> > +test_expect_success PERL 'name' 'true'
> 
> If the platform lacks PERL prerequisite, this will simply be
> skipped, and if the platform has it, it will always succeed.
> 
> I am not sure what you are trying to achieve by having this line
> here.

I originally didn't have this line, and my comment was referring to the
t/README which says

   Even without the PERL prerequisite, tests can assume there is a
   usable perl interpreter at $PERL_PATH, though it need not be
   particularly modern.

There is current functionality in diff-highlight which requires at least
perl 5.8 (the utf8 functions). I was going to add a test for this as
well, but I'm not super comfy with multibyte chars.

Eric recommended adding this line, what do you think?

would `test_set_prereq PERL` be better?

> 
> > +test_expect_success 'diff-highlight does not highlight whole line' '
> > +	dh_test \
> > +		"aaa\nbbb\nccc\n" \
> > +		"aaa\n000\nccc\n"
> > +'

This (at least to me) is desired. See comment for `sub
is_pair_interesting`

> 
> Hmm, does this express the desired outcome, or just document the
> current (possibly broken--I dunno) behaviour?  The same question for
> the next one.
> 
> > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
> > +	dh_test \
> > +		"aaa\nbbb\n" \
> > +		"aaa\nb0b\nccc\n"
> > +'

This is undesired behavior, but currently implemented for simplicity,
see `sub show_hunk`

Do they need comments or something?

<snip>

> 
> > +	test -s diff.act &&
> 
> Why?  If you always have the expected output that you are going to
> compare with, wouldn't that sufficient to do that test without this?
> Besides, having "test -s" means that you can never make sure that a
> certain pair of input does not show any changes.  Perhaps drop it?

I was trying to address Eric's concern for `printf` or `git commit` et
al failing. Also, this file will always be a diff, it just might not
having any highlighting (so not empty?).

I'll take another stab.

> 
> > +	diff diff.exp diff.act
> 
> Use test_cmp unless there is a strong reason why you shouldn't?
> 
> > +}
> > +
> > +dh_commit_test() {
> > +	a="$1" b="$2"
> > +
> > +	printf "$a" >file
> > +	git add file
> > +	git commit -m"Add a file" >/dev/null
> 
> 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" >/dev/null
> 
> Likewise.
> 
>     git commit -a -m "Update a file"
> 
> The remainder of the file invites the same set of questions and
> comments you see for dh_diff_test() above, so I won't repeat them.
> 
> Thanks.

thanks for the feedback.

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Jeff King]

On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:

> > > +# PERL is required, but assumed to be present, although not necessarily modern
> > > +# some tests require 5.8
> > > +test_expect_success PERL 'name' 'true'
> > 
> > If the platform lacks PERL prerequisite, this will simply be
> > skipped, and if the platform has it, it will always succeed.
> > 
> > I am not sure what you are trying to achieve by having this line
> > here.
> 
> I originally didn't have this line, and my comment was referring to the
> t/README which says
> 
>    Even without the PERL prerequisite, tests can assume there is a
>    usable perl interpreter at $PERL_PATH, though it need not be
>    particularly modern.
> 
> There is current functionality in diff-highlight which requires at least
> perl 5.8 (the utf8 functions). I was going to add a test for this as
> well, but I'm not super comfy with multibyte chars.

Yeah, I'd agree this test would want the PERL prereq. It is not just
using perl for one-liners in support of the script; it is testing major
perl functionality that should be skipped if we do not have a modern
perl available.

> Eric recommended adding this line, what do you think?
> 
> would `test_set_prereq PERL` be better?

test_set_prereq is for telling the test scripts that we _have_ perl, but
what I think this script wants to do is test "do we have perl?" and
abort otherwise. The way to do that is:

  if ! test_have_prereq PERL
  then
	skip_all='skipping diff-highlight tests; perl not available'
	test_done
  fi

> > > +test_expect_success 'diff-highlight does not highlight whole line' '
> > > +	dh_test \
> > > +		"aaa\nbbb\nccc\n" \
> > > +		"aaa\n000\nccc\n"
> > > +'
> 
> This (at least to me) is desired. See comment for `sub
> is_pair_interesting`

Yeah, that is an intentional behavior, and makes sense to test.

> > > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
> > > +	dh_test \
> > > +		"aaa\nbbb\n" \
> > > +		"aaa\nb0b\nccc\n"
> > > +'
> 
> This is undesired behavior, but currently implemented for simplicity,
> see `sub show_hunk`
> 
> Do they need comments or something?

Undesired behavior should generally not be tested for. It just makes
life harder for somebody when they make a change that violates it, and
they have to figure out "oh, but it's _good_ that I changed that, the
tests were wrong" (or more likely "I didn't fix it, but it's just broken
in a different way, and neither is preferable").

If you want to document known shortcomings, the best thing to do is show
what you'd _like_ to have happen, and mark it as test_expect_failure;
the test scripts show this as a known-breakage, and somebody later who
fixes it can flip the "failure" to "success".

-Peff

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Fri, Aug 19, 2016 at 10:51:23AM -0400, Jeff King wrote:
> On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:
> 
> > > > +# PERL is required, but assumed to be present, although not necessarily modern
> > > > +# some tests require 5.8
> > > > +test_expect_success PERL 'name' 'true'
> > > 
> > > If the platform lacks PERL prerequisite, this will simply be
> > > skipped, and if the platform has it, it will always succeed.
> > > 
> > > I am not sure what you are trying to achieve by having this line
> > > here.
> > 
> > I originally didn't have this line, and my comment was referring to the
> > t/README which says
> > 
> >    Even without the PERL prerequisite, tests can assume there is a
> >    usable perl interpreter at $PERL_PATH, though it need not be
> >    particularly modern.
> > 
> > There is current functionality in diff-highlight which requires at least
> > perl 5.8 (the utf8 functions). I was going to add a test for this as
> > well, but I'm not super comfy with multibyte chars.
> 
> Yeah, I'd agree this test would want the PERL prereq. It is not just
> using perl for one-liners in support of the script; it is testing major
> perl functionality that should be skipped if we do not have a modern
> perl available.
> 
> > Eric recommended adding this line, what do you think?
> > 
> > would `test_set_prereq PERL` be better?
> 
> test_set_prereq is for telling the test scripts that we _have_ perl, but
> what I think this script wants to do is test "do we have perl?" and
> abort otherwise. The way to do that is:
> 
>   if ! test_have_prereq PERL
>   then
> 	skip_all='skipping diff-highlight tests; perl not available'
> 	test_done
>   fi
> 
> > > > +test_expect_success 'diff-highlight does not highlight whole line' '
> > > > +	dh_test \
> > > > +		"aaa\nbbb\nccc\n" \
> > > > +		"aaa\n000\nccc\n"
> > > > +'
> > 
> > This (at least to me) is desired. See comment for `sub
> > is_pair_interesting`
> 
> Yeah, that is an intentional behavior, and makes sense to test.
> 
> > > > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
> > > > +	dh_test \
> > > > +		"aaa\nbbb\n" \
> > > > +		"aaa\nb0b\nccc\n"
> > > > +'
> > 
> > This is undesired behavior, but currently implemented for simplicity,
> > see `sub show_hunk`
> > 
> > Do they need comments or something?
> 
> Undesired behavior should generally not be tested for. It just makes
> life harder for somebody when they make a change that violates it, and
> they have to figure out "oh, but it's _good_ that I changed that, the
> tests were wrong" (or more likely "I didn't fix it, but it's just broken
> in a different way, and neither is preferable").
> 
> If you want to document known shortcomings, the best thing to do is show
> what you'd _like_ to have happen, and mark it as test_expect_failure;
> the test scripts show this as a known-breakage, and somebody later who
> fixes it can flip the "failure" to "success".
> 
> -Peff

all very helpful, thanks!

[PATCH v3 0/3] diff-highlight: add support for git log --graph output.

[Brian Henderson]

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

[PATCH v3 1/3] diff-highlight: add some tests.

[Brian Henderson]

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

Re: [PATCH v3 1/3] diff-highlight: add some tests.

[Junio C Hamano]

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

Re: [PATCH v3 1/3] diff-highlight: add some tests.

[Brian Henderson]

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?

Re: [PATCH v3 1/3] diff-highlight: add some tests.

[Eric Wong]

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.

[PATCH] diff-highlight: add some tests.

[Brian Henderson]

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

Re: [PATCH] diff-highlight: add some tests.

[Junio C Hamano]

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.

Re: [PATCH] diff-highlight: add some tests.

[Jeff King]

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

Re: [PATCH] diff-highlight: add some tests.

[Junio C Hamano]

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.

[PATCH] diff-highlight: add some tests.

[Brian Henderson]

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

Re: [PATCH] diff-highlight: add some tests.

[Jeff King]

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

[PATCH v4 0/3] diff-highlight: add support for --graph option

[Brian Henderson]

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

[PATCH v4 1/3] diff-highlight: add some tests.

[Brian Henderson]

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

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

[Brian Henderson]

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

[PATCH v4 3/3] diff-highlight: add support for --graph output.

[Brian Henderson]

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

Re: [PATCH v4 0/3] diff-highlight: add support for --graph option

[Junio C Hamano]

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

[PATCH v4 0/3] diff-highlight: add support for --graph option

[Brian Henderson]

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

[PATCH v4 1/3] diff-highlight: add some tests

[Brian Henderson]

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

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

[Brian Henderson]

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

[PATCH v4 3/3] diff-highlight: add support for --graph output

[Brian Henderson]

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

Re: [PATCH v4 0/3] diff-highlight: add support for --graph option

[Jeff King]

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

[PATCH 1/3] diff-highlight: ignore test cruft

[Jeff King]

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

[PATCH 2/3] diff-highlight: add multi-byte tests

[Jeff King]

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

[PATCH 3/3] diff-highlight: avoid highlighting combined diffs

[Jeff King]

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

Re: [PATCH v4 0/3] diff-highlight: add support for --graph option

[Jeff King]

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

Re: [PATCH] diff-highlight: add some tests.

[Junio C Hamano]

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.

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

[Brian Henderson]

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

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

[Junio C Hamano]

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

[PATCH v3 3/3] diff-highlight: add support for --graph output.

[Brian Henderson]

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

Re: [PATCH v3 3/3] diff-highlight: add support for --graph output.

[Junio C Hamano]

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.

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

[Brian Henderson]

Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 13 +++++++
 contrib/diff-highlight/t/test-diff-highlight.sh  | 43 ++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 8eff178..39707c6 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -59,4 +59,17 @@ test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
 
 # TODO add multi-byte test
 
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+	dh_graph_test \
+		"aaa\nbbb\nccc\n" \
+		"aaa\n0bb\nccc\n" \
+		"aaa\nb0b\nccc\n" \
+"
+ aaa
+-${CW}b${CR}bb
++${CW}0${CR}bb
+ ccc
+"
+'
+
 test_done
diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh
index 38323e8..67f742c 100644
--- a/contrib/diff-highlight/t/test-diff-highlight.sh
+++ b/contrib/diff-highlight/t/test-diff-highlight.sh
@@ -64,6 +64,49 @@ dh_commit_test() {
 	test_cmp commit.exp commit.act
 }
 
+dh_graph_test() {
+	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
+	} >/dev/null 2>&1
+
+	git log -p --graph --no-merges >graph.raw
+
+	# git log --graph orders the commits different than git log so we hack it by
+	# using sed to remove the graph part. We know from other tests, that DIFF_HIGHLIGHT
+	# works without the graph, so there should be no diff when running it with
+	# and without.
+	<graph.raw sed -e 's"^\(*\|| \||/\)\+""' -e 's"^  ""' | "$DIFF_HIGHLIGHT" >graph.exp
+	<graph.raw "$DIFF_HIGHLIGHT" | sed -e 's"^\(*\|| \||/\)\+""' -e 's"^  ""' >graph.act &&
+
+	test -s graph.act &&
+	# ignore whitespace since we're using a hacky sed command to remove the graph
+	# parts.
+	git diff -b --no-index graph.exp graph.act
+}
+
 test_chomp_eof() {
 	perl -pe 'chomp if eof'
 }
-- 
2.9.0

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

[Junio C Hamano]

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

> Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
> ---
>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 13 +++++++
>  contrib/diff-highlight/t/test-diff-highlight.sh  | 43 ++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> index 8eff178..39707c6 100755
> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -59,4 +59,17 @@ test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
>  
>  # TODO add multi-byte test
>  
> +test_expect_success 'diff-highlight highlights the beginning of a line' '
> +	dh_graph_test \
> +		"aaa\nbbb\nccc\n" \
> +		"aaa\n0bb\nccc\n" \
> +		"aaa\nb0b\nccc\n" \
> +"
> + aaa
> +-${CW}b${CR}bb
> ++${CW}0${CR}bb
> + ccc
> +"
> +'

Is this expected to pass after applying 1/3 and 2/3?  The title says
"add faililng test", so I am assuming this is expected to fail, in
which case the test should start out as "test_expect_failure".  A
later patch that makes it pass should turn "test_expect_failure"
into "test_expect_success".

>  test_done
> diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh
> index 38323e8..67f742c 100644
> --- a/contrib/diff-highlight/t/test-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/test-diff-highlight.sh
> @@ -64,6 +64,49 @@ dh_commit_test() {
>  	test_cmp commit.exp commit.act
>  }
>  
> +dh_graph_test() {
> +	a="$1" b="$2" c="$3"
> +
> +	{
> +		printf "$a" >file
> + ...
> +		git merge master
> +		git checkout master
> +		git merge branch --no-ff
> +	} >/dev/null 2>&1
> +
> +	git log -p --graph --no-merges >graph.raw

Hmph, when does it make sense to have "--no-merges" together with
"--graph".  Doesn't it result in a disconnected mess?  Is that an
interesting and most often used case?

> +
> +	# git log --graph orders the commits different than git log so we hack it by
> +	# using sed to remove the graph part.

Would it help if you knew "log --topo-order" to remove the "hack"?

[PATCH v2 3/3] diff-highlight: add support for --graph output.

[Brian Henderson]

Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
---
 contrib/diff-highlight/diff-highlight | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index ffefc31..9364423 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,
@@ -211,8 +215,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*$/;
 }
-- 
2.9.0

Re: [PATCH v2 3/3] diff-highlight: add support for --graph output.

[Jeff King]

On Wed, Aug 17, 2016 at 08:31:24AM -0700, Brian Henderson wrote:

>  contrib/diff-highlight/diff-highlight | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)

Junio already commented on the tests, and I think everything he said in
his review is sensible.

As for the code itself, this looks much simpler than some of the things
we discussed off-list, which is good. There is one place that you did
not touch that I think is interesting: split_line.

It splits on $COLOR to mark those bits in their own list elements (so they
can be skipped). But we don't do anything special for $GRAPH there.

Obviously it would be wrong to find $GRAPH in the middle of the line.
But I think we end up in highlight_pair() with a sequence like this for
each line:

  [
    color-graph,
    "|"
    color-reset,
    " ",
    ... possibly more graph pipes ...
    "-" (or "+")
    ... actual line data ...
  ]

We ignore "$COLOR" in the middle of that list, but not the other
syntactic bits. I think this just works because we have to skip forward
to the "-" or "+" element, and that happens to skip over all of the
graph cruft, as well.

In a more general sense, it might have been better for split_line() to
return a list of records, with each one containing a bit for "am I
interesting?" along with the actual token data. But I think because the
graph data cannot contain "-" or "+", it's acceptable to simply leave
this as it is. It might be worth a comment (either in-code, or
describing the strategy in the commit message).

-Peff

Re: [PATCH 1/3] diff-highlight: add some tests.

[Lars Schneider]

> On 30 Jul 2016, at 17:11, Brian Henderson <henderson.bj@gmail.com> wrote:
> 
> ---
> contrib/diff-highlight/Makefile                  |  5 ++
> contrib/diff-highlight/t/Makefile                | 19 +++++++
> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++
> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
> 4 files changed, 156 insertions(+)
> create mode 100644 contrib/diff-highlight/Makefile
> create mode 100644 contrib/diff-highlight/t/Makefile
> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh
> 
> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile

Would it make sense to add the contrib tests to the Travis-CI build, too?
https://travis-ci.org/git/git/branches

- Lars

Re: [PATCH 1/3] diff-highlight: add some tests.

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 30 Jul 2016, at 17:11, Brian Henderson <henderson.bj@gmail.com> wrote:
>> 
>> ---
>> contrib/diff-highlight/Makefile                  |  5 ++
>> contrib/diff-highlight/t/Makefile                | 19 +++++++
>> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++
>> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
>> 4 files changed, 156 insertions(+)
>> create mode 100644 contrib/diff-highlight/Makefile
>> create mode 100644 contrib/diff-highlight/t/Makefile
>> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
>> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh
>> 
>> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
>
> Would it make sense to add the contrib tests to the Travis-CI build, too?
> https://travis-ci.org/git/git/branches

The more the merrier ;-)?  I do not think of a downside, especially
if you are adding it as a separate thing that comes after the main
test, or for even better isolation, an entirely separate job.

By the way, how flaky are existing tests?  Are people actively
following breakage reports?

Re: [PATCH 1/3] diff-highlight: add some tests.

[Lars Schneider]

> On 16 Aug 2016, at 22:48, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>>> On 30 Jul 2016, at 17:11, Brian Henderson <henderson.bj@gmail.com> wrote:
>>> 
>>> ---
>>> contrib/diff-highlight/Makefile                  |  5 ++
>>> contrib/diff-highlight/t/Makefile                | 19 +++++++
>>> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++
>>> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
>>> 4 files changed, 156 insertions(+)
>>> create mode 100644 contrib/diff-highlight/Makefile
>>> create mode 100644 contrib/diff-highlight/t/Makefile
>>> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
>>> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh
>>> 
>>> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
>> 
>> Would it make sense to add the contrib tests to the Travis-CI build, too?
>> https://travis-ci.org/git/git/branches
> 
> The more the merrier ;-)?  I do not think of a downside, especially
> if you are adding it as a separate thing that comes after the main
> test, or for even better isolation, an entirely separate job.

OK, if I will post a patch (might take a while).


> By the way, how flaky are existing tests?  Are people actively
> following breakage reports?

Good question - I was curious about that, too. 
That's why I made a little website (only tested on Chrome, requires JS): 

https://larsxschneider.github.io/git-ci-stats/

There you can see all builds for all branches including their failure reason.
In general I would say flakiness is no issue anymore.

I don't know who else is looking at the breakage reports but I do. 
That's the reason for my "next" breakage reports in the past:

http://public-inbox.org/git/A6131C47-3230-4EC4-B3F6-B2507C937A22@gmail.com/
http://public-inbox.org/git/4877318E-3CBF-4C87-B24D-AAE35C427D66@gmail.com/
http://public-inbox.org/git/FB76544F-16F7-45CA-9649-FD62EE44B0DE@gmail.com/
...

Cheers,
Lars

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

[Brian Henderson]

---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 13 ++++++++
 contrib/diff-highlight/t/test-diff-highlight.sh  | 42 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index ca7605f..2fb4869 100644
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -60,4 +60,17 @@ test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
 
 # TODO add multi-byte test
 
+test_expect_success 'diff-highlight highlightes the beginning of a line' '
+  dh_graph_test \
+    "aaa\nbbb\nccc\n" \
+    "aaa\n0bb\nccc\n" \
+    "aaa\nb0b\nccc\n" \
+"
+ aaa
+-${CW}b${CR}bb
++${CW}0${CR}bb
+ ccc
+"
+'
+
 test_done
diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh
index 9b26271..162a694 100644
--- a/contrib/diff-highlight/t/test-diff-highlight.sh
+++ b/contrib/diff-highlight/t/test-diff-highlight.sh
@@ -67,3 +67,45 @@ dh_commit_test() {
 
   diff commit.exp commit.act
 }
+
+dh_graph_test() {
+  local 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
+  } >/dev/null 2>&1
+
+  git log -p --graph --no-merges > graph.raw
+
+  # git log --graph orders the commits different than git log so we hack it by
+  # using sed to remove the graph part. We know from other tests, that CMD
+  # works without the graph, so there should be no diff when running it with
+  # and without.
+  < graph.raw sed -e 's"^\(*\|| \||/\)\+""' -e 's"^  ""' | $CMD > graph.exp
+  < graph.raw $CMD | sed -e 's"^\(*\|| \||/\)\+""' -e 's"^  ""' > graph.act
+
+  # ignore whitespace since we're using a hacky sed command to remove the graph
+  # parts.
+  diff -b graph.exp graph.act
+}
-- 
2.9.0

[PATCH 3/3] diff-highlight: add support for --graph output.

[Brian Henderson]

---
 contrib/diff-highlight/diff-highlight | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index ffefc31..ec31356 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -20,6 +20,7 @@ my @NEW_HIGHLIGHT = (
 my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
+my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
 
 my @removed;
 my @added;
@@ -32,12 +33,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 +47,7 @@ while (<>) {
 		@added = ();
 
 		print;
-		$in_hunk = /^$COLOR*[\@ ]/;
+		$in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
 	}
 
 	# Most of the time there is enough output to keep things streaming,
@@ -211,8 +212,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*$/;
 }
-- 
2.9.0

Re: [PATCH 3/3] diff-highlight: add support for --graph output.

[Junio C Hamano]

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

> ---
>  contrib/diff-highlight/diff-highlight | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
> index ffefc31..ec31356 100755
> --- a/contrib/diff-highlight/diff-highlight
> +++ b/contrib/diff-highlight/diff-highlight
> @@ -20,6 +20,7 @@ my @NEW_HIGHLIGHT = (
>  my $RESET = "\x1b[m";
>  my $COLOR = qr/\x1b\[[0-9;]*m/;
>  my $BORING = qr/$COLOR|\s/;
> +my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;

I didn't read the other parts (or existing code this patch changes,
for that matter) of the series, but this looks like an attempt to
catch the leading "|" bar showing direct ancestry.  It makes a
reader wonder what happens in a mergy histroy, though.

I _think_ that the patch portion of "log -p" output would only have
"|" and never "\" or "/" that are used to adjust the number of
tracks to deal with forks and merges, but perhaps the fact that this
code relies on that assumption deserves to be written down here as
an in-code comment?

>  my @removed;
>  my @added;
> @@ -32,12 +33,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 +47,7 @@ while (<>) {
>  		@added = ();
>  
>  		print;
> -		$in_hunk = /^$COLOR*[\@ ]/;
> +		$in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
>  	}
>  
>  	# Most of the time there is enough output to keep things streaming,
> @@ -211,8 +212,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*$/;
>  }

[PATCH] diff-highlight: Add comment for our assumption about --graph output.

[Brian Henderson]

---
 contrib/diff-highlight/diff-highlight | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index ec31356..9364423 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -20,6 +20,9 @@ my @NEW_HIGHLIGHT = (
 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;
-- 
2.9.0