Re: [patch 05/15] diff-test_cmp.patch

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2010, 07:21 UTC
Message-ID: <7veijkohpe.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100316054321.668397000@mlists.thewrittenword.com>

"Gary V. Vaughan" <git@mlists.thewrittenword.com> writes:

Show 10 quoted lines

> Index: b/t/t0000-basic.sh
> ===================================================================
> --- 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 says "compare ignoring whitespace changes" but the updated one says "they must match literally". Is this conversion safe?

For the purpose of debugging tests, I think it would be better to lose the redirection into /dev/null. If the test passes, we wouldn't see anything anyway, and if the test fails, we would see what's different, and that helps diagnosing the breakage. For systems with implementations of diff that is "-u" challenged, we could define test_cmp in terms of "diff -c" instead of "cmp".

Show 10 quoted lines

> Index: b/t/t9400-git-cvsserver-server.sh
> ===================================================================
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -226,7 +226,7 @@ test_expect_success 'gitcvs.ext.enabled 
>    'GIT_DIR="$SERVERDIR" git config --bool gitcvs.ext.enabled true &&
>     GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled false &&
>     GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 master >cvs.log 2>&1 &&
> -   diff -q cvswork cvswork2'
> +   test_cmp cvswork cvswork2' >/dev/null

If the -q in the original really matters, then please add the redirection to /dev/null only for test_cmp; never redirect the output from the entire test_expect_success. On the other hand, if -q does not matter the outcome of the test, simply lose the "quiet". We really should not care, and make sure it is available easily to people who broke cvsserver and need to see the difference between expected and actual results while debugging.

There are similar dubious conversions in your patch to this file.

Show 9 quoted lines

> Index: b/t/Makefile
> ===================================================================
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -6,6 +6,7 @@
>  -include ../config.mak
>  
>  #GIT_TEST_OPTS=--verbose --debug
> +GIT_TEST_CMP ?= $(DIFF)

If this forces plain diff not more readable "diff -u" to everybody, that sounds like a regression to me.

Other than that, the conversion in this patch looked sane.

Thanks.