Re: [PATCH v5 06/18] tests: use "test_cmp", not "diff", when verifying the result
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:53
Subsystem:
kernel build + files below scripts/ (unless maintained elsewhere), the rest · Maintainers:
Nathan Chancellor, Nicolas Schier, Linus Torvalds
"Gary V. Vaughan" [off-list ref] writes:
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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