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

Changes made per Eric.

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

Brian Henderson [off-list ref] 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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

We use tabs for shell indentation.

done

<snip>

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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