Re: [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 6, 2022, 18:55 UTC
Message-ID: <221206.86sfhsbau4.gmgdl@evledraar.gmail.com>
In-Reply-To: <6a80fab7e3936ec56e1583d6136d47487327e907.1670339267.git.gitgitgadget@gmail.com>

On Tue, Dec 06 2022, Johannes Schindelin via GitGitGadget wrote:

Show 6 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
> The reason why this approach was not even considered in Git for Windows
> is that in 2007, there was already a motion on the table to use Git's
> own diff machinery to perform comparisons in Git's test suite, but it
> was dismissed in https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/

I think you mixed up your links there, that's the "no, that should be fine" from 2022, but it contains the link to the 2007 message you seem to be talking about.

Show 7 quoted lines

> as undesirable because tests might potentially succeed due to bugs in
> the diff machinery when they should not succeed, and those bugs could
> therefore hide regressions that the tests try to prevent.
>
> By the time Git for Windows' `mingw-test-cmp` in C was finally
> contributed to the Git mailing list, reviewers agreed that the diff
> machinery had matured enough and should be used instead.

I think it's fine to dogfood it like this, but I've been pointing out to you[1] that this will hide segfaults etc. in "git" as we negate the exit code of "test_cmp" in some places.

E.g. let's produce this stack overflow:

	diff --git a/diff-no-index.c b/diff-no-index.c
	index 9a8b09346bd..1c4a9e5c351 100644
	--- a/diff-no-index.c
	+++ b/diff-no-index.c
	@@ -279,6 +279,7 @@ int diff_no_index(struct rev_info *revs,
	 
	 	fixup_paths(paths, &replacement);
	 
	+	revs++;
	 	revs->diffopt.skip_stat_unmatch = 1;
	 	if (!revs->diffopt.output_format)
	 		revs->diffopt.output_format = DIFF_FORMAT_PATCH;

Then compiling with SANITIZE=address and running:

	GIT_TEST_CMP="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --" ./t9351-fast-export-anonymize.sh --run=1,17 -vixd

Will produce (less relevant bits elided):
	
	expecting success of 9351.17 'idents are shared': 
		[...]
		test_line_count = 1 unique &&
		! test_cmp authors committers
	
	[...]
	+ test 1 = 1
	+ test_cmp authors committers
	+ test 2 -ne 2
	+ eval GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- "$@"
	+ GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- authors committers
	=================================================================
	==2865782==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc6d64d0f4 at pc 0x00000077fa4d bp 0x7ffc6d64bcc0 sp 0x7ffc6d64bcb8
	WRITE of size 4 at 0x7ffc6d64d0f4 thread T0
	    #0 0x77fa4c in diff_no_index diff-no-index.c:292
	[...]
	ok 17 - idents are shared
	
	# passed all 17 test(s)
	1..17

I.e. we'll proclaim "all tests passed", even though we segfaulted. I think this direction will fix it for you:

	diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
	index 77047e250dc..29a2d3a03a7 100755
	--- a/t/t9351-fast-export-anonymize.sh
	+++ b/t/t9351-fast-export-anonymize.sh
	@@ -133,7 +133,7 @@ test_expect_success 'idents are shared' '
	 	git log --all --format="%cn <%ce>" >committers &&
	 	sort -u committers >unique &&
	 	test_line_count = 1 unique &&
	-	! test_cmp authors committers
	+	test_cmp ! authors committers
	 '
	 
	 test_done
	diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
	index 452fe9bc8aa..d640b4b7881 100644
	--- a/t/test-lib-functions.sh
	+++ b/t/test-lib-functions.sh
	@@ -1239,8 +1239,25 @@ test_expect_code () {
	 # - not all diff versions understand "-u"
	 
	 test_cmp () {
	+	local negate= &&
	+	local exit_code= &&
	+	if test "$1" = "!"
	+	then
	+		negate=t &&
	+		shift
	+	fi &&
	 	test "$#" -ne 2 && BUG "2 param"
	-	eval "$GIT_TEST_CMP" '"$@"'
	+	if test -n "$GIT_TEST_CMP_BUILTIN"
	+	then
	+		if test -n "$negate"
	+		then
	+			test_must_fail env GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- "$@"
	+		else
	+			GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- "$@"
	+		fi
	+	else
	+		eval "$GIT_TEST_CMP" '"$@"'
	+	fi
	 }
	 
	 # Check that the given config key has the expected value.
	diff --git a/t/test-lib.sh b/t/test-lib.sh
	index f8c6205e08f..0a5f8e7c7fc 100644
	--- a/t/test-lib.sh
	+++ b/t/test-lib.sh
	@@ -1546,7 +1546,7 @@ case $uname_s in
	 	test_set_prereq SED_STRIPS_CR
	 	test_set_prereq GREP_STRIPS_CR
	 	test_set_prereq WINDOWS
	-	GIT_TEST_CMP="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --"
	+	GIT_TEST_CMP_BUILTIN=t
	 	;;
	 *CYGWIN*)
	 	test_set_prereq POSIXPERM

I.e. it's fine to start invoking "git" in one of the test-lib-functions.sh, but it's not OK to do so without also ensuring that we're properly checking its exit code.

The above diff fixes it only for t9351-fast-export-anonymize.sh, we'd also need to change "! test_cmp " to "test_cmp ! " elsewhere, but that's a rather easy mechanical replacement.

1. https://lore.kernel.org/git/221119.86wn7rdugi.gmgdl@evledraar.gmail.com/