Re: [patch 06/16] diff-test_cmp.patch

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 27, 2010, 17:15 UTC
Message-ID: <20100427171531.GA15553@progeny.tock>
In-Reply-To: <20100427135833.403548000@mlists.thewrittenword.com>

Hi,

Gary V. Vaughan wrote:

Show 7 quoted lines

> Subject: diff-test_cmp.patch
>
> In tests, call test_cmp rather than raw diff where possible (i.e. if
> the output does not go to a pipe), to allow the use of, say, 'cmp'
> when the default 'diff -u' is not compatible with a vendor diff.
> 
> When that is not possible, use $DIFF, as set in GIT-BUILD-OPTIONS.

Sign-off? (See SubmittingPatches for what I am asking about here.) And is it possible to change your mailing script to use more meaningful subject lines?

This patch makes a good change, but I do not think your description captures it. Most of the changes are from ‘diff’, not from ‘diff -u’. Is your bare ‘diff’ really incapable of distinguishing between identical and differing files?

But using test_cmp consistently would make debugging test scripts with -v much easier, since the output is more readable.

Show 8 quoted lines

> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -280,7 +280,7 @@ $expectfilter >expected <<\EOF
>  EOF
>  test_expect_success \
>      'validate git diff-files output for a know cache/work tree state.' \
> -    'git diff-files >current && diff >/dev/null -b current expected'
> +    'git diff-files >current && test_cmp current expected >/dev/null'

The original ignores whitespace changes; this version does not. It turns out that’s okay, but it’s worth mentioning in the commit message. (I think we do guarantee that diff-files will not change the whitespace it produces without good reason.)

The original suppressed its output, without any good reason to. Could you remove the >/dev/null while at it, to make it easier to debug?

Show 11 quoted lines

> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -44,7 +44,7 @@ test_fix () {
>  	apply_patch --whitespace=fix || return 1
>  
>  	# find touched lines
> -	diff file target | sed -n -e "s/^> //p" >fixed
> +	$DIFF file target | sed -n -e "s/^> //p" >fixed
>  
>  	# the changed lines are all expeced to change
>  	fixed_cnt=$(wc -l <fixed)

Is this needed?  I don’t mind it, just curious.

I hope some earlier patch takes care of setting DIFF in test-lib.sh. Tests need to be usable without running them through the makefile.

Show 16 quoted lines

> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -6,10 +6,14 @@
>  -include ../config.mak
>  
>  #GIT_TEST_OPTS=--verbose --debug
> +GIT_TEST_CMP ?= $(DIFF)
>  SHELL_PATH ?= $(SHELL)
>  TAR ?= $(TAR)
>  RM ?= rm -f
>  
> +# Make sure test-lib.sh uses make's value of GIT_TEST_CMP
> +export GIT_TEST_CMP
> +
>  # Shell quote;
>  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

If neither DIFF nor GIT_TEST_CMP is already set, this will export GIT_TEST_CMP as the empty string. Will t/test-lib.sh treat that as asking for the default? Yes --- phew.

Except for as commented above,

  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.