From: Junio C Hamano <gitster@pobox.com>
To: "Gary V. Vaughan" <git@mlists.thewrittenword.com>
Cc: git@vger.kernel.org
Subject: Re: [patch 05/15] diff-test_cmp.patch
Date: Tue, 16 Mar 2010 00:21:01 -0700 [thread overview]
Message-ID: <7veijkohpe.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: 20100316054321.668397000@mlists.thewrittenword.com
"Gary V. Vaughan" <git@mlists.thewrittenword.com> writes:
> 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".
> 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.
> 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.
next prev parent reply other threads:[~2010-03-16 7:21 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-16 5:42 [patch 00/15] Portability Patches v3 Gary V. Vaughan
2010-03-16 5:42 ` [patch 01/15] user-cppflags.patch Gary V. Vaughan
2010-03-16 7:21 ` Junio C Hamano
2010-03-16 5:42 ` [patch 02/15] const-expr.patch Gary V. Vaughan
2010-03-16 7:31 ` Johannes Sixt
2010-03-16 8:23 ` Gary V. Vaughan
2010-03-16 23:43 ` Avery Pennarun
2010-04-25 8:38 ` Gary V. Vaughan
2010-03-16 5:42 ` [patch 03/15] pthread.patch Gary V. Vaughan
2010-03-16 7:21 ` Junio C Hamano
2010-03-16 5:42 ` [patch 04/15] diff-export.patch Gary V. Vaughan
2010-03-16 7:24 ` Junio C Hamano
2010-03-16 5:42 ` [patch 05/15] diff-test_cmp.patch Gary V. Vaughan
2010-03-16 7:21 ` Junio C Hamano [this message]
2010-04-25 8:38 ` Gary V. Vaughan
2010-03-16 5:42 ` [patch 06/15] diff-defaults.patch Gary V. Vaughan
2010-03-16 7:22 ` Junio C Hamano
2010-04-25 8:38 ` Gary V. Vaughan
2010-03-16 5:42 ` [patch 07/15] host-IRIX.patch Gary V. Vaughan
2010-03-16 7:24 ` Junio C Hamano
2010-04-25 8:39 ` Gary V. Vaughan
2010-03-16 15:27 ` Brandon Casey
2010-04-25 8:39 ` Gary V. Vaughan
2010-03-16 5:42 ` [patch 08/15] host-HPUX10.patch Gary V. Vaughan
2010-03-16 5:42 ` [patch 09/15] host-HPUX11.patch Gary V. Vaughan
2010-03-16 5:42 ` [patch 10/15] host-OSF1.patch Gary V. Vaughan
2010-03-16 5:42 ` [patch 11/15] no-hstrerror.patch Gary V. Vaughan
2010-03-16 7:24 ` Junio C Hamano
2010-04-25 8:39 ` Gary V. Vaughan
2010-03-16 5:42 ` [patch 12/15] no-inet_ntop.patch Gary V. Vaughan
2010-03-16 5:42 ` [patch 13/15] no-socklen_t.patch Gary V. Vaughan
2010-03-16 7:25 ` Junio C Hamano
2010-04-25 8:39 ` Gary V. Vaughan
2010-03-16 5:42 ` [patch 14/15] no-ss_family.patch Gary V. Vaughan
2010-03-16 5:42 ` [patch 15/15] no-inline.patch Gary V. Vaughan
2010-03-16 6:18 ` [patch 00/15] Portability Patches v3 Sverre Rabbelier
2010-03-16 16:03 ` Brandon Casey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7veijkohpe.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=git@mlists.thewrittenword.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).