Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 14, 2022, 11:55 UTC
- Message-ID
- <221114.86tu31lnwr.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@gmail.com>
On Sat, Nov 12 2022, Johannes Schindelin via GitGitGadget wrote:
Show 11 quoted lines
> 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