Hi,
Gary V. Vaughan wrote:
> 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.
> --- 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?
> --- 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.
> --- 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@xxxxxxxxx>
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html