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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Nov 12 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is more performant to run `git diff --no-index` than running the
> `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for
> Windows uses. And a lot more readable.
>
> Note: Earlier attempts at fixing this involved a test helper that avoids
> the overhead of the diff machinery, in favor of implementing a behavior
> that is more in line with what `mingw_test_cmp` does now, but that
> attempt saw a lot of backlash and distractions during review and was
> therefore abandoned.

I don't know if you counted this under "backlash and distractions", or
if you just didn't see it, but I think part of the comments I left on
the v2[1][2] are still applicable.

Namely that before this we've been assuming that GIT_TEST_CMP is a
not-ours binary. I.e. "diff", "cmp" etc. If it is a "that's ours" this
change should be changing e.g. '! test_cmp' to 'test_cmp !', as the
former would now hide segfaults.

Which isn't a theoretical issue b.t.w., e.g. the recent d00113ec347
(t/Makefile: apply chainlint.pl to existing self-tests, 2022-09-01)
will invoke "git diff --no-index", which has some interesting results
(that I've run into) when testing a tree where "git diff" happens to
segfault.

From grepping our tests that doesn't seem to be too hard, certainly a
lot easier than a new "not-quite-diff" test helper.

Additionally: We don't *need* this for an initial implementation, but
having e.g. one of the Ubuntu CI targets run with "git diff --no-index"
would be a nice cherry on top, as:

 * It would show that the "--ignore-cr-at-eol" is only needed for MinGW,
   and is also the reason for why "GIT_TEST_CMP" is still
   unconditionally clobbered there, unlike other platforms. See
   4d715ac05cf (Windows: a test_cmp that is agnostic to random LF <>
   CRLF conversions, 2013-10-26).

 * We do pass this elsewhere, except for the one trivial case which you
   aren't running into on MINGW because it doesn't have SYMLINKS, but
   presumably would if the test were adjusted to use "SYMLINKS_WINDOWS".

 * If we're trusting "git diff --no-index" to run the tests, we could
   also get rid of "GIT_TEST_CMP_USE_COPIED_CONTEXT", whose only reason
   for existing is to support a system "diff" that doesn't understand
   "-u" (squashable diff below)

1. https://lore.kernel.org/git/220907.86v8pzl6jz.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/220907.86r10nl63s.gmgdl@evledraar.gmail.com/

diff --git a/Makefile b/Makefile
index 4927379184c..dea6069b5fe 100644
--- a/Makefile
+++ b/Makefile
@@ -1950,10 +1950,6 @@ ifdef OVERRIDE_STRDUP
 	COMPAT_OBJS += compat/strdup.o
 endif
 
-ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
-	export GIT_TEST_CMP_USE_COPIED_CONTEXT
-endif
-
 ifndef NO_MSGFMT_EXTENDED_OPTIONS
 	MSGFMT += --check
 endif
@@ -3008,9 +3004,6 @@ endif
 ifdef GIT_TEST_CMP
 	@echo GIT_TEST_CMP=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_CMP)))'\' >>$@+
 endif
-ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
-	@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+
-endif
 ifdef GIT_TEST_UTF8_LOCALE
 	@echo GIT_TEST_UTF8_LOCALE=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_UTF8_LOCALE)))'\' >>$@+
 endif
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4fab1c1984c..cd6e9f797b6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1503,12 +1503,7 @@ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
 
 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
+	GIT_TEST_CMP="$DIFF -u"
 fi
 
 GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib