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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

may be easier to see what is going on.

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

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

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

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

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

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

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

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

or something like that.

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