Re: [PATCH v5 06/18] tests: use "test_cmp", not "diff", when verifying the result

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2010, 04:39 UTC
Message-ID: <7vy6eyoxoj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100514093751.825924000@mlists.thewrittenword.com>

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

Show 5 quoted lines

> 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.

Both are very worthy goal.

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'

... and I think we could lose >/dev/null redirection once we rewrite these using test_cmp, but that can be a separate patch.

Show 18 quoted lines

> Index: b/t/Makefile
> ===================================================================
> --- 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))

But isn't this a regression?  When GIT_TEST_CMP is not defined, we used to

    GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}

which in turn is used like this:

    test_cmp() {
            $GIT_TEST_CMP "$@"
    }

so people would get a more readable "diff -u" output when GIT_TEST_CMP is not defined and exported. With your patch we would lose -u everywhere, no?

Also even if your vendor diff lacks unified context format, I would presume that it would support good old copied context format with -c, and it would give us a better readability.

How about doing something like this on top of your patch?

Your 7/18 will instead be setting "GIT_TEST_CMP_USE_COPIED_CONTEXT = YesPlease" for (hopefully) most of the targets whose native "diff" knows copied context format, and others will set GIT_TEST_CMP to cmp, perhaps?

---
 Makefile      |    4 ++++
 t/Makefile    |    4 ----
 t/test-lib.sh |   11 ++++++++++-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 668dbc9..c8cc9e2 100644
--- a/Makefile
+++ b/Makefile
@@ -1374,6 +1374,10 @@ ifdef USE_NED_ALLOCATOR
        COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 endif
 
+ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
+	export GIT_TEST_CMP_USE_COPIED_CONTEXT
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK=NoThanks
 endif
diff --git a/t/Makefile b/t/Makefile
index 93a6475..25c559b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -6,14 +6,10 @@
 -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))
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c582964..a290011 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -63,7 +63,16 @@ export GIT_MERGE_VERBOSITY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
 export EDITOR
-GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
+
+if test -z "$GIT_TEST_CMP"
+then
+	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
+	then
+		GIT_TEST_CMP="$DIFF -c"
+	else
+		GIT_TEST_CMP="$DIFF -u"
+	fi
+fi
 
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment