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

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

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

Brian Henderson [off-list ref] writes:

quoted

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