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

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

Probably:

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

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

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>

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.