From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "René Scharfe" <l.s.r@web.de>,
"Johannes Schindelin" <johannes.schindelin@gmx.de>,
"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`
Date: Mon, 14 Nov 2022 12:55:16 +0100 [thread overview]
Message-ID: <221114.86tu31lnwr.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@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
next prev parent reply other threads:[~2022-11-14 12:29 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-29 14:53 [PATCH] tests: replace mingw_test_cmp with a helper in C Johannes Schindelin via GitGitGadget
2022-07-29 14:54 ` Johannes Schindelin
2022-07-29 16:44 ` Junio C Hamano
2022-09-06 13:10 ` Johannes Schindelin
2022-09-07 12:09 ` René Scharfe
2022-09-07 16:25 ` Junio C Hamano
2022-09-07 21:45 ` Re* " Junio C Hamano
2022-09-07 22:39 ` René Scharfe
2022-09-08 0:03 ` Junio C Hamano
2022-09-08 8:59 ` René Scharfe
2022-09-08 15:26 ` Ævar Arnfjörð Bjarmason
2022-09-08 20:54 ` Johannes Schindelin
2022-09-08 21:09 ` Junio C Hamano
2022-09-06 13:10 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget
2022-09-06 13:10 ` [PATCH v2 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-09-06 13:10 ` [PATCH v2 2/2] tests: replace mingw_test_cmp with a helper in C Johannes Schindelin via GitGitGadget
2022-09-07 11:57 ` Ævar Arnfjörð Bjarmason
2022-09-07 12:24 ` Ævar Arnfjörð Bjarmason
2022-09-07 19:45 ` Junio C Hamano
2022-09-07 9:04 ` [PATCH v2 0/2] " Johannes Schindelin
2022-11-12 22:07 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-11-12 22:07 ` [PATCH v3 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-11-12 22:07 ` [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp` Johannes Schindelin via GitGitGadget
2022-11-13 4:51 ` Taylor Blau
2022-11-14 13:34 ` Johannes Schindelin
2022-11-18 23:15 ` Junio C Hamano
2022-11-19 2:53 ` Taylor Blau
2022-11-19 12:03 ` Ævar Arnfjörð Bjarmason
2022-11-19 8:18 ` Johannes Sixt
2022-11-19 17:50 ` René Scharfe
2022-11-20 9:29 ` Torsten Bögershausen
2022-11-21 17:49 ` Johannes Sixt
2022-11-21 3:13 ` Junio C Hamano
2022-11-14 9:53 ` Phillip Wood
2022-11-14 13:47 ` Johannes Schindelin
2022-11-14 11:55 ` Ævar Arnfjörð Bjarmason [this message]
2022-11-14 14:02 ` Johannes Schindelin
2022-11-14 15:23 ` Ævar Arnfjörð Bjarmason
2022-11-18 23:19 ` Junio C Hamano
2022-11-19 2:56 ` Taylor Blau
2022-11-19 11:54 ` Ævar Arnfjörð Bjarmason
2022-11-21 3:17 ` Junio C Hamano
2022-11-14 14:06 ` [PATCH v4 0/2] tests(mingw): avoid super-slow mingw_test_cmp Johannes Schindelin via GitGitGadget
2022-11-14 14:06 ` [PATCH v4 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-11-14 14:06 ` [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp` Johannes Schindelin via GitGitGadget
2022-11-14 22:40 ` Taylor Blau
2022-11-18 13:32 ` Johannes Schindelin
2022-11-18 18:14 ` Taylor Blau
2022-11-20 23:36 ` Johannes Schindelin
2022-11-21 0:07 ` Taylor Blau
2022-12-06 15:07 ` [PATCH v5 0/2] tests(mingw): avoid super-slow mingw_test_cmp Johannes Schindelin via GitGitGadget
2022-12-06 15:07 ` [PATCH v5 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-12-06 15:07 ` [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp` Johannes Schindelin via GitGitGadget
2022-12-06 18:55 ` Ævar Arnfjörð Bjarmason
2022-12-06 21:52 ` Johannes Sixt
2022-12-06 21:54 ` René Scharfe
2022-12-07 4:33 ` Junio C Hamano
2022-12-07 1:31 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221114.86tu31lnwr.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
--cc=l.s.r@web.de \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).