tests: replace mingw_test_cmp with a helper in C

From: Johannes Schindelin <[email protected]>

This helper is more performant than running the mingw_test_cmp code with with MSYS2's Bash. And a lot more readable.

To accommodate t1050, which wants to compare files weighing in with 3MB (falling outside of t1050's malloc limit of 1.5MB), we simply lift the allocation limit by setting the environment variable GIT_ALLOC_LIMIT to zero when calling the helper.

---
tests: replace mingw_test_cmp with a helper in C

On the heels of sending a patch to fix a performance regression due to a mis-use of test_cmp
[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],
I was curious to see whether Git for Windows had the same issue. And it does not
[https://github.com/git-for-windows/git/runs/7556381815?check_suite_focus=true#step:5:127]:
it passes t5351 in 22 seconds, even while using test_cmp to compare pack files
[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].

The answer is of course that a test helper written in C is much faster than writing the same in Bash, especially on Windows. This is especially sad when said Bash code is only used on Windows. So I pulled out this helper from the years-long effort to let Git for Windows use BusyBox' ash to run the test suite. The result is this patch, which has been in Git for Windows since June 2018.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1309

 Makefile                |  1 +
 t/helper/test-cmp.c     | 73 +++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c    |  1 +
 t/helper/test-tool.h    |  1 +
 t/test-lib-functions.sh | 68 +-------------------------------------
 t/test-lib.sh           |  2 +-
 6 files changed, 78 insertions(+), 68 deletions(-)
 create mode 100644 t/helper/test-cmp.c

index 1624471badc..45f108e43a1 100644
@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o
 TEST_BUILTINS_OBJS += test-bitmap.o
 TEST_BUILTINS_OBJS += test-bloom.o
 TEST_BUILTINS_OBJS += test-chmtime.o
+TEST_BUILTINS_OBJS += test-cmp.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o

index 318fdbab0c3..3334de248a1 100644
@@ -18,6 +18,7 @@ static struct test_cmd cmds[] = {
 	{ "bitmap", cmd__bitmap },
 	{ "bloom", cmd__bloom },
 	{ "chmtime", cmd__chmtime },
+	{ "cmp", cmd__cmp },
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },

index bb799271631..e1104898cc3 100644
@@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);
 int cmd__bitmap(int argc, const char **argv);
 int cmd__bloom(int argc, const char **argv);
 int cmd__chmtime(int argc, const char **argv);
+int cmd__cmp(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);

index 7726d1da88a..220c259e796 100644
@@ -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=mingw_test_cmp
+	GIT_TEST_CMP="test-tool cmp"
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c

gitgitgadget

Hi,

Ooops... s/with with/with/

Ciao,
Dscho

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hi Junio,

Yes!

No, you're right, I've added a guard that prevents test-tool cmp - - from failing in obscure ways.

Right. I've added a clause that says that we cannot show the diff because stdin has been consumed already.

Fair point. The Unix tool cmp does not care about line endings at all, so when you come from a Unix background you will expect the same to be true for test-tool cmp.

On the other hand, you will expect the same to be true for test_cmp, too, which is not the case, and the root cause of why I had to come up with 32ed3314c10 (t5351: avoid using test_cmp for binary data, 2022-07-29).

Having said that, I agree that the test tool name should reflect better what the subcommand does.

I do dislike the proposed name mingwcmp. Not only because it is misleading, as the purpose is not to compare MINGW-specific files but instead the purpose is to compare text files (and, in fact, the tool works just fine on Linux and macOS, too). But also because it would contribute to just how much of a second-class citizen the MINGW-based build is in Git
land: From choosing to implement large parts, including the entire test suite as well as the performance benchmarks, in POSIX scripts (which plays to Windows' weaknesses in a big way) to massively favoring spawned processes over multi-threading (which plays to Linux' strengths and to Windows' weaknesses), to a still-inherent assumption that the underlying filesystem is case-sensitive (think: branch names), to an implicit agreement in the core Git community that patch contributions need not take care of working well on Windows (but that that's the job "of Windows folk" instead). This is kind of at odds with the fact that we must assume that half of Git's users are Windows-based (we can only assume, based on surveys, because we successfully avoid any kind of even opt-in telemetry that would give us hard data). I definitely want to stay away from making that second-citizenry even worse.

So I am going with the name test-tool text-cmp instead.

Thank you for your review,
Dscho

Am 06.09.22 um 15:10 schrieb Johannes Schindelin:

"git diff --no-index - -" also doesn't complain, by the way.

Why not use "git diff --no-index --ignore-cr-at-eol"? Do you even need to wrap it?

René

True, but in this case hopefully it is worth to call it out, as both this code that uses "diff --no-index" and "diff --no-index" itself came from the same author ;-)

I think "git diff --no-index - -" should just exit 0 after slurping all its input (i.e. allow it to be placed downstream of a pipe without blocking the upstream), but it is also fine to exit with 0 without reading a single byte from the standard input. Of course the latter is easier to implement ;-)

Hmph. That surely sounds sensible if it works, and I offhand do not see why it shouldn't work.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] diff: "--no-index - -" compares the same standard input

Conceptually, "git diff --no-index - A" slurps the standard input to the EOF and compares it with the contents of A, so it is a natural extension for "git diff --no-index - -" to slurp the standard input to the end, and compares it with itself, which should always yield "true".

There can be two plausible implementations. We could read and discard the input to the end and exit 0. It would allow us to avoid sigpipe on the upstream command

    $ dd if=/dev/zero bs=1m count=1 | git diff --no-index - -

As we can tell the outcome without even consuming a single byte from the standard input, we can just notice the two input files specified are "-" and immediately exit. It would allow us to give a correct answer to

    $ git diff --no-index - - </dev/full

but can kill the command on the upstream side of a pipe that feeds us.

We pick the latter (i.e. not touching the input at all), simply because it is more efficient. Data producer that can be placed on the upstream side of a pipe should always be prepared for a consumer that does not consume all its output (e.g. "head" and pager) anyway.

---
 diff-no-index.c          | 11 +++++++++++
 t/t4053-diff-no-index.sh | 11 +++++++++++
 2 files changed, 22 insertions(+)

index 18edbdf4b5..940b66b4c8 100644
@@ -278,6 +278,17 @@ int diff_no_index(struct rev_info *revs,
 			p = to_free[i] = prefix_filename(prefix, p);
 		paths[i] = p;
 	}
+	if (paths[0] == file_from_standard_input &&
+	    paths[1] == file_from_standard_input) {
+		/*
+		 * "git diff --no-index - -".  We are asked to compare
+		 * what came from the standard input with itself; we
+		 * know they are the same without even reading a
+		 * single byte.
+		 */
+		ret = 0;
+		goto out;
+	}
 
 	fixup_paths(paths, &replacement);
 

index 3feadf0e35..d089d9c2b1 100755
@@ -156,6 +156,16 @@ test_expect_success 'diff --no-index normalizes mode: no changes' '
 	test_must_be_empty out
 '
 
+test_expect_success 'diff --no-index with standard input' '
+	echo foo >x &&
+	echo bar >z &&
+	git diff --no-index - x <x &&
+	git diff --no-index x - <x &&
+	test_must_fail git diff --no-index - x <z &&
+	test_must_fail git diff --no-index x - <z &&
+	git diff --no-index - - <x
+'
+
 test_expect_success POSIXPERM 'diff --no-index normalizes mode: chmod +x' '
 	chmod +x y &&
 	cat >expected <<-\EOF &&

@@ -180,6 +190,7 @@ test_expect_success POSIXPERM 'diff --no-index normalizes: mode not like git mod
 '
 
 test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not like git mode (symlink)' '
+	rm -f z &&
 	ln -s y z &&
 	X_OID=$(git hash-object --stdin <x) &&
 	Z_OID=$(printf y | git hash-object --stdin) &&

2.37.3-732-ge93f299430

Am 07.09.2022 um 23:45 schrieb Junio C Hamano:

OK, that sounds like a happy accident ;-)

Am 07.09.22 um 18:25 schrieb Junio C Hamano:

Using git diff in test_cmp and using test_cmp to check if git diff works would become a cyclical dependency. Only doing that on one platform limits the potential blind spot to platform-specific bugs, though. Enough to go wrapper-less? Not sure.

René

I don't see how being wrapper-less is less of a "cyclical dependency" than using "git diff" directly.

If we are to postulate some bug where "git diff" thwarts us for the use of "test_cmp" it's going to be *very* broken.

I don't see how such a "git diff" would pass the rest of the test suite (some of which involves comparing its exact output), but still be functional enough to work as a GIT_TEST_CMP.

Even one where it just returns 0 unconditionally wouldn't pass, as we rely on "! test_cmp" in some cases.

And any such breakage we imagine might just as well affect a wrapper for it, and I'd think that would be the more likely of two unlikely possibilities, as that wrapper would only be used for the test suite, whereas "git diff" is more widely tested.

In any case, as long as we preserve the ability to set a GIT_TEST_CMP=cmp we can sanity check any such wrapper or dogfodding of "git diff" with an external program.

Needing to deal just with "git diff" and "cmp" would be a step forward, as we'd be able to drop the current shellscript "mingw" wrapper, as well as the special support for a "diff" that doesn't understand "-u".

Hi Junio,

Is this a reversal of the stance you took in your reply in
https://lore.kernel.org/git/[email protected]/ to my suggestion to replace cmp by diff --no-index (in that mail referred to as patch [7/8])?

If I recall correctly, you clarified outside of that thread that "I do not think it is a good enough reason to make the tests slower" was you being concerned about employing the entire diff machinery instead of doing a simple byte-for-byte comparison.

And while it is no longer _that_ simple a comparison (it now special-handles Carriage Returns), the speed and simplicity concern is still valid: test-tool text-cmp is vastly simpler (and provably faster) than diff --no-index.

Just because it is easier to review a one-liner to switch from essentially cmp to git diff --no-index --ignore-cr-at-eol does not mean that it is
reasonable: it would cause us to blast out that much more CO2, just for our one-time convenience.

Or for that matter, it would willfully slow down the windows-test jobs that already is mired by sloooow performance (mostly due to running a shell script-based test suite).

Can you please let me make the Windows situation better rather than worse?

Thank you,
Dscho

cox.net? It is a lifetime ago and the world has changed. Hopefully that "diff --no-index" has matured a lot to earn more confidence by us than it had back then.

Is it still relevant, now that we are talking about text-cmp that ignores cr-at-eol, to bring a random remark about byte-for-byte comparison from more than 10 years ago?

Measurement, in tons per year, is needed, or please stop talking about CO2. It is not that funny.

Developer cycle time is easier to measure and more meaningful.

It would be much faster to run a byte-for-byte-with-ignore-cr-at-eol comparison than running "diff --no-index --ignore-cr-at-eol" on files with meaningful sizes. But comparison between "expect" and "actual", which are typically at most several lines long? Wouldn't the overhead to spawn a process dwarf everything else, especially on Windows, whether it is your byte-for-byte-with-ignore-cr-at-eol program or "git diff"?

On the heels of sending a patch to fix a performance regression due to a mis-use of test_cmp
[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],
I was curious to see whether Git for Windows had the same issue. And it does not
[https://github.com/git-for-windows/git/runs/7556381815?check_suite_focus=true#step:5:127]:
it passes t5351 in 22 seconds, even while using test_cmp to compare pack files
[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].

The answer is of course that a test helper written in C is much faster than writing the same in Bash, especially on Windows. This is especially sad when said Bash code is only used on Windows. So I pulled out this helper from the years-long effort to let Git for Windows use BusyBox' ash to run the test suite. The result is this patch, which has been in Git for Windows since June 2018.

Changes since v1:

* Fixed double "with" in the commit message.

Johannes Schindelin (2):
t0021: use Windows-friendly pwd
tests: replace mingw_test_cmp with a helper in C

 Makefile                 |  1 +
 t/helper/test-text-cmp.c | 78 ++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c     |  1 +
 t/helper/test-tool.h     |  1 +
 t/t0021-conversion.sh    |  4 +--
 t/test-lib-functions.sh  | 68 +----------------------------------
 t/test-lib.sh            |  2 +-
 7 files changed, 85 insertions(+), 70 deletions(-)
 create mode 100644 t/helper/test-text-cmp.c

base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1309

Range-diff vs v1:

 -:  ----------- > 1:  ad7c41401ee t0021: use Windows-friendly `pwd`
 1:  eacbbc53700 ! 2:  1f5366f1379 tests: replace mingw_test_cmp with a helper in C
     @@ Commit message
          tests: replace mingw_test_cmp with a helper in C
      
          This helper is more performant than running the `mingw_test_cmp` code
     -    with with MSYS2's Bash. And a lot more readable.
     +    with MSYS2's Bash. And a lot more readable.
      
          To accommodate t1050, which wants to compare files weighing in with 3MB
          (falling outside of t1050's malloc limit of 1.5MB), we simply lift the
     @@ Commit message
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## Makefile ##
     -@@ Makefile: TEST_BUILTINS_OBJS += test-advise.o
     - TEST_BUILTINS_OBJS += test-bitmap.o
     - TEST_BUILTINS_OBJS += test-bloom.o
     - TEST_BUILTINS_OBJS += test-chmtime.o
     -+TEST_BUILTINS_OBJS += test-cmp.o
     - TEST_BUILTINS_OBJS += test-config.o
     - TEST_BUILTINS_OBJS += test-crontab.o
     - TEST_BUILTINS_OBJS += test-csprng.o
     +@@ Makefile: TEST_BUILTINS_OBJS += test-string-list.o
     + TEST_BUILTINS_OBJS += test-submodule-config.o
     + TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
     + TEST_BUILTINS_OBJS += test-subprocess.o
     ++TEST_BUILTINS_OBJS += test-text-cmp.o
     + TEST_BUILTINS_OBJS += test-trace2.o
     + TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
     + TEST_BUILTINS_OBJS += test-userdiff.o
      
     - ## t/helper/test-cmp.c (new) ##
     + ## t/helper/test-text-cmp.c (new) ##
      @@
      +#include "test-tool.h"
      +#include "git-compat-util.h"
     @@ t/helper/test-cmp.c (new)
      +static int run_diff(const char *path1, const char *path2)
      +{
      +	const char *argv[] = {
     -+		"diff", "--no-index", NULL, NULL, NULL
     ++		"diff", "--no-index", "--", NULL, NULL, NULL
      +	};
      +	const char *env[] = {
      +		"GIT_PAGER=cat",
     @@ t/helper/test-cmp.c (new)
      +		NULL
      +	};
      +
     -+	argv[2] = path1;
     -+	argv[3] = path2;
     ++	argv[3] = path1;
     ++	argv[4] = path2;
      +	return run_command_v_opt_cd_env(argv,
      +					RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
      +					NULL, env);
      +}
      +
     -+int cmd__cmp(int argc, const char **argv)
     ++int cmd__text_cmp(int argc, const char **argv)
      +{
      +	FILE *f0, *f1;
      +	struct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;
     @@ t/helper/test-cmp.c (new)
      +	if (argc != 3)
      +		die("Require exactly 2 arguments, got %d", argc);
      +
     ++	if (!strcmp(argv[1], "-") && !strcmp(argv[2], "-"))
     ++		die("only one parameter can refer to `stdin` but not both");
     ++
      +	if (!(f0 = !strcmp(argv[1], "-") ? stdin : fopen(argv[1], "r")))
      +		return error_errno("could not open '%s'", argv[1]);
      +	if (!(f1 = !strcmp(argv[2], "-") ? stdin : fopen(argv[2], "r"))) {
     @@ t/helper/test-cmp.c (new)
      +			if (r1 == EOF)
      +				return 0;
      +cmp_failed:
     -+			if (!run_diff(argv[1], argv[2]))
     ++			if (!strcmp(argv[1], "-") || !strcmp(argv[2], "-"))
     ++				warning("cannot show diff because `stdin` was already consumed");
     ++			else if (!run_diff(argv[1], argv[2]))
      +				die("Huh? 'diff --no-index %s %s' succeeded",
      +				    argv[1], argv[2]);
      +			return 1;
     @@ t/helper/test-cmp.c (new)
      
       ## t/helper/test-tool.c ##
      @@ t/helper/test-tool.c: static struct test_cmd cmds[] = {
     - 	{ "bitmap", cmd__bitmap },
     - 	{ "bloom", cmd__bloom },
     - 	{ "chmtime", cmd__chmtime },
     -+	{ "cmp", cmd__cmp },
     - 	{ "config", cmd__config },
     - 	{ "crontab", cmd__crontab },
     - 	{ "csprng", cmd__csprng },
     + 	{ "submodule-config", cmd__submodule_config },
     + 	{ "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
     + 	{ "subprocess", cmd__subprocess },
     ++	{ "text-cmp", cmd__text_cmp },
     + 	{ "trace2", cmd__trace2 },
     + 	{ "userdiff", cmd__userdiff },
     + 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
      
       ## t/helper/test-tool.h ##
     -@@ t/helper/test-tool.h: int cmd__advise_if_enabled(int argc, const char **argv);
     - int cmd__bitmap(int argc, const char **argv);
     - int cmd__bloom(int argc, const char **argv);
     - int cmd__chmtime(int argc, const char **argv);
     -+int cmd__cmp(int argc, const char **argv);
     - int cmd__config(int argc, const char **argv);
     - int cmd__crontab(int argc, const char **argv);
     - int cmd__csprng(int argc, const char **argv);
     +@@ t/helper/test-tool.h: int cmd__string_list(int argc, const char **argv);
     + int cmd__submodule_config(int argc, const char **argv);
     + int cmd__submodule_nested_repo_config(int argc, const char **argv);
     + int cmd__subprocess(int argc, const char **argv);
     ++int cmd__text_cmp(int argc, const char **argv);
     + int cmd__trace2(int argc, const char **argv);
     + int cmd__userdiff(int argc, const char **argv);
     + int cmd__urlmatch_normalization(int argc, const char **argv);
      
       ## t/test-lib-functions.sh ##
      @@ t/test-lib-functions.sh: test_expect_code () {
     @@ t/test-lib.sh: case $uname_s in
       	test_set_prereq GREP_STRIPS_CR
       	test_set_prereq WINDOWS
      -	GIT_TEST_CMP=mingw_test_cmp
     -+	GIT_TEST_CMP="test-tool cmp"
     ++	GIT_TEST_CMP="test-tool text-cmp"
       	;;
       *CYGWIN*)
       	test_set_prereq POSIXPERM

gitgitgadget

From: Johannes Schindelin <[email protected]>

In Git for Windows, when passing paths from shell scripts to regular Win32 executables, thanks to the MSYS2 runtime a somewhat magic path conversion happens that lets the shell script think that there is a file at /git/Makefile and the Win32 process it spawned thinks that the shell script said C:/git-sdk-64/git/Makefile instead.

This conversion is documented in detail over here:
https://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion

As all automatic conversions, there are gaps. For example, to avoid mistaking command-line options like /LOG=log.txt (which are quite common in the Windows world) from being mistaken for a Unix-style absolute path, the MSYS2 runtime specifically exempts arguments containing a = character from that conversion.

We are about to change test_cmp to use a test helper, which is such a Win32 process.

In combination, this would cause a failure in t0021-conversion.sh where we pass an absolute path containing an equal character to the test_cmp function.

Seeing as the Unix tools like cp and diff that are used by Git's test suite in the Git for Windows SDK (thanks to the MSYS2 project) understand both Unix-style as well as Windows-style paths, we can stave off this problem by simply switching to Windows-style paths and side-stepping the need for any automatic path conversion.

Note: The PATH variable is obviously special, as it is colon-separated in the MSYS2 Bash used by Git for Windows, and therefore _cannot_ contain absolute Windows-style paths, lest the colon after the drive letter is mistaken for a path separator. Therefore, we need to be careful to keep the Unix-style when modifying the PATH variable.

---
 t/t0021-conversion.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index bad37abad2c..15482fa78e3 100755
@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-TEST_ROOT="$PWD"
-PATH=$TEST_ROOT:$PATH
+PATH=$PWD:$PATH
+TEST_ROOT="$(pwd)"
 
 write_script <<\EOF "$TEST_ROOT/rot13.sh"
 tr \

gitgitgadget

From: Johannes Schindelin <[email protected]>

This helper is more performant than running the mingw_test_cmp code with MSYS2's Bash. And a lot more readable.

To accommodate t1050, which wants to compare files weighing in with 3MB (falling outside of t1050's malloc limit of 1.5MB), we simply lift the allocation limit by setting the environment variable GIT_ALLOC_LIMIT to zero when calling the helper.

---
 Makefile                 |  1 +
 t/helper/test-text-cmp.c | 78 ++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c     |  1 +
 t/helper/test-tool.h     |  1 +
 t/test-lib-functions.sh  | 68 +----------------------------------
 t/test-lib.sh            |  2 +-
 6 files changed, 83 insertions(+), 68 deletions(-)
 create mode 100644 t/helper/test-text-cmp.c

index 1624471badc..73db55bba0f 100644
@@ -786,6 +786,7 @@ TEST_BUILTINS_OBJS += test-string-list.o
 TEST_BUILTINS_OBJS += test-submodule-config.o
 TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
 TEST_BUILTINS_OBJS += test-subprocess.o
+TEST_BUILTINS_OBJS += test-text-cmp.o
 TEST_BUILTINS_OBJS += test-trace2.o
 TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
 TEST_BUILTINS_OBJS += test-userdiff.o

index 318fdbab0c3..c6654ebc48b 100644
@@ -81,6 +81,7 @@ static struct test_cmd cmds[] = {
 	{ "submodule-config", cmd__submodule_config },
 	{ "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
 	{ "subprocess", cmd__subprocess },
+	{ "text-cmp", cmd__text_cmp },
 	{ "trace2", cmd__trace2 },
 	{ "userdiff", cmd__userdiff },
 	{ "urlmatch-normalization", cmd__urlmatch_normalization },

index bb799271631..2acfd2bcabc 100644
@@ -71,6 +71,7 @@ int cmd__string_list(int argc, const char **argv);
 int cmd__submodule_config(int argc, const char **argv);
 int cmd__submodule_nested_repo_config(int argc, const char **argv);
 int cmd__subprocess(int argc, const char **argv);
+int cmd__text_cmp(int argc, const char **argv);
 int cmd__trace2(int argc, const char **argv);
 int cmd__userdiff(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);

index 7726d1da88a..0be25ecbd59 100644
@@ -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=mingw_test_cmp
+	GIT_TEST_CMP="test-tool text-cmp"
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

gitgitgadget

Ah (partially?) I spoke too soon on this part. I.e. the GIT_DIR=/dev/null precludes reading the filter/repo in this case. So I *think* we're out of the woods as far as this is concerned.

Still, it would be nice to document in a code comment or commit message that the "not read the local repo's filter" is absolutely critical here.

But I think that re-raises the point René had in:
https://lore.kernel.org/git/[email protected]/

I ran the full test suite with:

	GIT_TEST_CMP='GIT_DIR=/dev/null HOME=/dev/null /usr/bin/git diff --no-index --ignore-cr-at-eol --'

And all of it passes, except for a test in t0001-init.sh which we could fix up as:

	diff --git a/t/t0001-init.sh b/t/t0001-init.sh
	index d479303efa0..d65afe7cceb 100755
	--- a/t/t0001-init.sh
	+++ b/t/t0001-init.sh
	@@ -426,7 +426,7 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '
	 	git init --separate-git-dir ../realgitdir
	 	) &&
	 	echo "gitdir: $(pwd)/realgitdir" >expected &&
	-	test_cmp expected newdir/.git &&
	+	test "$(test_readlink newdir/.git)" = here &&
	 	test_cmp expected newdir/here &&
	 	test_path_is_dir realgitdir/refs
	 '

Which without this series is more correct, as all we're re-testing there is whether the symlink is pointing to what we expect. A hypothetical "--dereference" to "git diff" would also take care of it (the equivalent of "--no-dereference" being the default).

But with that all tests pass for me, so I'm puzzled as to the need for the new helper, as opposed to just constructing the command above and sticking it in GIT_TEST_CMP ...

As the primary point of no-index mode was to expose fancy options "git diff" has to comparisons of files outside version control, without having to go through the trouble of upstreaming changes to GNU diff, I do think "--ignore-cr-at-eol" should work fine with it, and René's idea sounds like the best implementation for the test-text-cmp helper command.

Thanks.

Hi,

Whoops, I totally forgot to mention these two things:

- Renamed the test helper to text-cmp.
- Made the diff --no-index call more robust by using a double-dash separator.

Ciao,
Dscho

A few months ago, directly after sending a patch to fix a performance regression due to a mis-use of test_cmp
[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],
I got curious to see whether Git for Windows had the same issue. And it did
not: it passes t5351 in 22 seconds, even while using test_cmp to compare pack files
[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].

The explanation is of course that Git for Windows uses a test helper for test_cmp that is written in C, instead of the Bash function. And C is much faster than a Bash function, especially on Windows. This is especially sad when said Bash code is only used on Windows. So I originally had pulled out this helper from the years-long effort to let Git for Windows use BusyBox' ash to run the test suite. The result is this patch, which has been in Git for Windows since June 2018.

Unfortunately, this tried-and-tested code was rejected by the Git maintainer.

Let's fall back to the next-best solution: git diff --no-index, which the Git maintainer seems to like. The downside is that the diff machinery does a lot more than a simple cmp clone, and therefore a lot more things can go wrong that might make it look like a test case is failing when the fault is somewhere else entirely. There is one way to find out whether this is a valid concern.

Changes since v2:

* Dropped the test helper, using diff --no-index instead.

Changes since v1:

* Fixed double "with" in the commit message.
* Renamed the test helper to text-cmp.
* Made the diff --no-index call more robust by using a double-dash separator.

Johannes Schindelin (2):
t0021: use Windows-friendly pwd
tests(mingw): avoid very slow mingw_test_cmp

 t/t0021-conversion.sh   |  4 +--
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 3 files changed, 3 insertions(+), 69 deletions(-)

base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1309

Range-diff vs v2:

1: ad7c41401ee ! 1: b38b8fb5a85 t0021: use Windows-friendly pwd
@@ Commit message
absolute path, the MSYS2 runtime specifically exempts arguments containing a = character from that conversion.

- We are about to change test_cmp to use a test helper, which is such a
- Win32 process.
+ We are about to change test_cmp to use git diff --no-index, which
+ involves spawning precisely such a Win32 process.

In combination, this would cause a failure in t0021-conversion.sh where we pass an absolute path containing an equal character to the
2: 1f5366f1379 ! 2: a7f4265ceb2 tests: replace mingw_test_cmp with a helper in C

     @@ Metadata
      Author: Johannes Schindelin <[email protected]>
      
       ## Commit message ##
     -    tests: replace mingw_test_cmp with a helper in C
     +    tests(mingw): avoid very slow `mingw_test_cmp`
      
     -    This helper is more performant than running the `mingw_test_cmp` code
     -    with MSYS2's Bash. And a lot more readable.
     +    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.
      
     -    To accommodate t1050, which wants to compare files weighing in with 3MB
     -    (falling outside of t1050's malloc limit of 1.5MB), we simply lift the
     -    allocation limit by setting the environment variable GIT_ALLOC_LIMIT to
     -    zero when calling the helper.
     +    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.
      
          Signed-off-by: Johannes Schindelin <[email protected]>
      
     - ## Makefile ##
     -@@ Makefile: TEST_BUILTINS_OBJS += test-string-list.o
     - TEST_BUILTINS_OBJS += test-submodule-config.o
     - TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
     - TEST_BUILTINS_OBJS += test-subprocess.o
     -+TEST_BUILTINS_OBJS += test-text-cmp.o
     - TEST_BUILTINS_OBJS += test-trace2.o
     - TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
     - TEST_BUILTINS_OBJS += test-userdiff.o
     -
     - ## t/helper/test-text-cmp.c (new) ##
     -@@
     -+#include "test-tool.h"
     -+#include "git-compat-util.h"
     -+#include "strbuf.h"
     -+#include "gettext.h"
     -+#include "parse-options.h"
     -+#include "run-command.h"
     -+
     -+#ifdef WIN32
     -+#define NO_SUCH_DIR "\\\\.\\GLOBALROOT\\invalid"
     -+#else
     -+#define NO_SUCH_DIR "/dev/null"
     -+#endif
     -+
     -+static int run_diff(const char *path1, const char *path2)
     -+{
     -+	const char *argv[] = {
     -+		"diff", "--no-index", "--", NULL, NULL, NULL
     -+	};
     -+	const char *env[] = {
     -+		"GIT_PAGER=cat",
     -+		"GIT_DIR=" NO_SUCH_DIR,
     -+		"HOME=" NO_SUCH_DIR,
     -+		NULL
     -+	};
     -+
     -+	argv[3] = path1;
     -+	argv[4] = path2;
     -+	return run_command_v_opt_cd_env(argv,
     -+					RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
     -+					NULL, env);
     -+}
     -+
     -+int cmd__text_cmp(int argc, const char **argv)
     -+{
     -+	FILE *f0, *f1;
     -+	struct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;
     -+
     -+	if (argc != 3)
     -+		die("Require exactly 2 arguments, got %d", argc);
     -+
     -+	if (!strcmp(argv[1], "-") && !strcmp(argv[2], "-"))
     -+		die("only one parameter can refer to `stdin` but not both");
     -+
     -+	if (!(f0 = !strcmp(argv[1], "-") ? stdin : fopen(argv[1], "r")))
     -+		return error_errno("could not open '%s'", argv[1]);
     -+	if (!(f1 = !strcmp(argv[2], "-") ? stdin : fopen(argv[2], "r"))) {
     -+		fclose(f0);
     -+		return error_errno("could not open '%s'", argv[2]);
     -+	}
     -+
     -+	for (;;) {
     -+		int r0 = strbuf_getline(&b0, f0);
     -+		int r1 = strbuf_getline(&b1, f1);
     -+
     -+		if (r0 == EOF) {
     -+			fclose(f0);
     -+			fclose(f1);
     -+			strbuf_release(&b0);
     -+			strbuf_release(&b1);
     -+			if (r1 == EOF)
     -+				return 0;
     -+cmp_failed:
     -+			if (!strcmp(argv[1], "-") || !strcmp(argv[2], "-"))
     -+				warning("cannot show diff because `stdin` was already consumed");
     -+			else if (!run_diff(argv[1], argv[2]))
     -+				die("Huh? 'diff --no-index %s %s' succeeded",
     -+				    argv[1], argv[2]);
     -+			return 1;
     -+		}
     -+		if (r1 == EOF || strbuf_cmp(&b0, &b1)) {
     -+			fclose(f0);
     -+			fclose(f1);
     -+			strbuf_release(&b0);
     -+			strbuf_release(&b1);
     -+			goto cmp_failed;
     -+		}
     -+	}
     -+}
     -
     - ## t/helper/test-tool.c ##
     -@@ t/helper/test-tool.c: static struct test_cmd cmds[] = {
     - 	{ "submodule-config", cmd__submodule_config },
     - 	{ "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
     - 	{ "subprocess", cmd__subprocess },
     -+	{ "text-cmp", cmd__text_cmp },
     - 	{ "trace2", cmd__trace2 },
     - 	{ "userdiff", cmd__userdiff },
     - 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
     -
     - ## t/helper/test-tool.h ##
     -@@ t/helper/test-tool.h: int cmd__string_list(int argc, const char **argv);
     - int cmd__submodule_config(int argc, const char **argv);
     - int cmd__submodule_nested_repo_config(int argc, const char **argv);
     - int cmd__subprocess(int argc, const char **argv);
     -+int cmd__text_cmp(int argc, const char **argv);
     - int cmd__trace2(int argc, const char **argv);
     - int cmd__userdiff(int argc, const char **argv);
     - int cmd__urlmatch_normalization(int argc, const char **argv);
     -
       ## t/test-lib-functions.sh ##
     -@@ t/test-lib-functions.sh: test_expect_code () {
     - 
     - test_cmp () {
     - 	test "$#" -ne 2 && BUG "2 param"
     --	eval "$GIT_TEST_CMP" '"$@"'
     -+	GIT_ALLOC_LIMIT=0 eval "$GIT_TEST_CMP" '"$@"'
     - }
     - 
     - # Check that the given config key has the expected value.
      @@ t/test-lib-functions.sh: test_skip_or_die () {
       	error "$2"
       }
     @@ t/test-lib.sh: case $uname_s in
       	test_set_prereq GREP_STRIPS_CR
       	test_set_prereq WINDOWS
      -	GIT_TEST_CMP=mingw_test_cmp
     -+	GIT_TEST_CMP="test-tool text-cmp"
     ++	GIT_TEST_CMP="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --"
       	;;
       *CYGWIN*)
       	test_set_prereq POSIXPERM

gitgitgadget

From: Johannes Schindelin <[email protected]>

In Git for Windows, when passing paths from shell scripts to regular Win32 executables, thanks to the MSYS2 runtime a somewhat magic path conversion happens that lets the shell script think that there is a file at /git/Makefile and the Win32 process it spawned thinks that the shell script said C:/git-sdk-64/git/Makefile instead.

This conversion is documented in detail over here:
https://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion

As all automatic conversions, there are gaps. For example, to avoid mistaking command-line options like /LOG=log.txt (which are quite common in the Windows world) from being mistaken for a Unix-style absolute path, the MSYS2 runtime specifically exempts arguments containing a = character from that conversion.

We are about to change test_cmp to use git diff --no-index, which involves spawning precisely such a Win32 process.

In combination, this would cause a failure in t0021-conversion.sh where we pass an absolute path containing an equal character to the test_cmp function.

Seeing as the Unix tools like cp and diff that are used by Git's test suite in the Git for Windows SDK (thanks to the MSYS2 project) understand both Unix-style as well as Windows-style paths, we can stave off this problem by simply switching to Windows-style paths and side-stepping the need for any automatic path conversion.

Note: The PATH variable is obviously special, as it is colon-separated in the MSYS2 Bash used by Git for Windows, and therefore _cannot_ contain absolute Windows-style paths, lest the colon after the drive letter is mistaken for a path separator. Therefore, we need to be careful to keep the Unix-style when modifying the PATH variable.

---
 t/t0021-conversion.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index bad37abad2c..15482fa78e3 100755
@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-TEST_ROOT="$PWD"
-PATH=$TEST_ROOT:$PATH
+PATH=$PWD:$PATH
+TEST_ROOT="$(pwd)"
 
 write_script <<\EOF "$TEST_ROOT/rot13.sh"
 tr \

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 2 files changed, 1 insertion(+), 67 deletions(-)

index 8c44856eaec..452fe9bc8aa 100644
@@ -1541,72 +1541,6 @@ test_skip_or_die () {
 	error "$2"
 }
 
-# The following mingw_* functions obey POSIX shell syntax, but are actually
-# bash scripts, and are meant to be used only with bash on Windows.
-
-# A test_cmp function that treats LF and CRLF equal and avoids to fork
-# diff when possible.
-mingw_test_cmp () {
-	# Read text into shell variables and compare them. If the results
-	# are different, use regular diff to report the difference.
-	local test_cmp_a= test_cmp_b=
-
-	# When text came from stdin (one argument is '-') we must feed it
-	# to diff.
-	local stdin_for_diff=
-
-	# Since it is difficult to detect the difference between an
-	# empty input file and a failure to read the files, we go straight
-	# to diff if one of the inputs is empty.
-	if test -s "$1" && test -s "$2"
-	then
-		# regular case: both files non-empty
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-	elif test -s "$1" && test "$2" = -
-	then
-		# read 2nd file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b
-		stdin_for_diff='<<<"$test_cmp_b"'
-	elif test "$1" = - && test -s "$2"
-	then
-		# read 1st file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-		stdin_for_diff='<<<"$test_cmp_a"'
-	fi
-	test -n "$test_cmp_a" &&
-	test -n "$test_cmp_b" &&
-	test "$test_cmp_a" = "$test_cmp_b" ||
-	eval "diff -u \"\$@\" $stdin_for_diff"
-}
-
-# $1 is the name of the shell variable to fill in
-mingw_read_file_strip_cr_ () {
-	# Read line-wise using LF as the line separator
-	# and use IFS to strip CR.
-	local line
-	while :
-	do
-		if IFS=$'\r' read -r -d $'\n' line
-		then
-			# good
-			line=$line$'\n'
-		else
-			# we get here at EOF, but also if the last line
-			# was not terminated by LF; in the latter case,
-			# some text was read
-			if test -z "$line"
-			then
-				# EOF, really
-				break
-			fi
-		fi
-		eval "$1=\$$1\$line"
-	done
-}
-
 # Like "env FOO=BAR some-program", but run inside a subshell, which means
 # it also works for shell functions (though those functions cannot impact
 # the environment outside of the test_env invocation).

index 7726d1da88a..f8c6205e08f 100644
@@ -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=mingw_test_cmp
+	GIT_TEST_CMP="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --"
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

gitgitgadget

This makes sense, and motivates the changes below.

But I do not think that this paragraph adds as much as we'd like to the historical record.

What about the original approach did you have trouble pushing forward? Perhaps framing that as an alternative approach, along with why it didn't ultimately get merged would be a more useful recollection of previous attempts here.

One thing that the commit message doesn't allude to (that is covered in the earlier discussion) is why it is important to pass --ignore-cr-at-eol. I think that is worth mentioning here.

Thanks,
Taylor

Hi Taylor,

It explains why we replace a relatively simple logic with a relatively complex one.

I elaborated on both in the new commit message.

Ciao,
Dscho

Isn't it because Git on the platform is expected to use CRLF in certain places, unlike on other platforms where LF is used, but the platform port hasn't adjusted tests to match that expectation? And vice versa, where Git is expected to produce LF terminated text everywhere but the expected output is not "ported" to force LF termination and instead produces CRLF terminated text on platforms whose native line ending is CRLF?

Use of "ignore-cr-at-eol" may allow such tests that are not ported correctly to prepare expected output with a "wrong" line ending and still pass, and I do think it may be an expedite way to make tests appear to pass.

But I worry that it may not be a good thing for the health of the Windows port in the longer term.

When Git is expected to produce platform-native line endings, if a test is not adjusted to expect platform-native output and instead uses the ignore-cr-at-eol workaround, a future breakage that makes Git to produce LF terminated lines everywhere would not be caught, would it?

Yes, I think that's right. My suggestion to Johannes was to (a) make sure that your and my understanding is correct, and (b) to memorialize that understanding in the commit message itself.

I share your concerns, too.

Thanks,
Taylor

It's not right, but mostly right. I.e. the "--ignore-cr-at-eol" is surely needed to avoid a deluge of errors on Windows, but as I noted in
[1] you'll also find that the tests don't pass on a *nix box without it.

Which is apparently because "--ignore-cr-at-eol" is also conflating "is binary?" with "should I replace \r\n", or something.

I don't really care about that, and would tend to defer to one or more Johannes in this thread on that question.

But as noted in [1] the upthread patch seems to be replacing some existing newline munging with not-quite-the-same "diff" behavior.

Which I think is something that needs addressing, either by the commit explaining why that's desired & OK.

Which is also a good reason to pick one of our *nix CI jobs and run it with "git diff --no-index", but without "--ignore-cr-at-eol", as I suggested. It would tease out exactly that difference, both for current and future tests.

1. https://lore.kernel.org/git/[email protected]/

Am 19.11.22 um 00:15 schrieb Junio C Hamano:

The reason that mingw_test_cmp exists is not that Git isn't ported correctly, or that tests aren't ported correctly. The reason is that tests assume Unix LF line endings everywhere, but there are some tools that are outside our control that randomly -- to the layman's eye -- produce CRLF line endings even when their input has LF style.

For example, when we post-process Git output with sed, the result suddenly has CRLF line endings instead of LF that the input had.

There is no such requirement or expectation on Windows. LF style is acceptable.

-- Hannes

Am 19.11.2022 um 09:18 schrieb Johannes Sixt:

Actually I see the opposite behavior -- sed eats CRs on an up-to-date Git for Windows SDK:

   $ uname -s
   MINGW64_NT-10.0-22621

   $ printf 'a\r\n' | hexdump.exe -C
   00000000  61 0d 0a                                          |a..|
   00000003

   $ printf 'a\r\n' | sed '' | hexdump.exe -C
   00000000  61 0a                                             |a.|
   00000002

And with the following patch on top of eea7033409 (The twelfth batch, 2022-11-14) the test suite passes for me -- just one case of grep stealing CRs seems to need adjustment to make mingw_test_cmp unnecessary:

 t/t3920-crlf-messages.sh | 2 +-
 t/test-lib.sh            | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

index 4c661d4d54..353b1a550e 100755
@@ -12,7 +12,7 @@ create_crlf_ref () {
 	cat >.crlf-orig-$branch.txt &&
 	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
 	grep 'Subject' .crlf-orig-$branch.txt | tr '\n' ' ' | sed 's/[ ]*$//' | tr -d '\n' >.crlf-subject-$branch.txt &&
-	grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&
+	grep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt || true &&
 	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}" &&
 	test_tick &&
 	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&

index 6db377f68b..af5ec357e5 100644
@@ -1721,7 +1721,6 @@ case $uname_s in
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
 	test_set_prereq WINDOWS
-	GIT_TEST_CMP=mingw_test_cmp
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

There is a "-b" option for sed under MINGW;
-b, --binary
open files in binary mode (CR+LFs are not processed specially)

The CRLF handling for sed (and probably grep and awk) had beed changed in cygwin
https://cygwin.com/pipermail/cygwin/2017-June/233133.html

(And I suspect that this rippled into MINGW some day)

Am 19.11.22 um 18:50 schrieb René Scharfe:

Wow, nice catch! Just let enough time pass, and a liability such as mingw_test_cmp becomes obsolete by mere magic. ;)

That's exactly the kind of thing I meant by "porting the tests".

If we _know_ that the tests want to munge something (either the hardcoded expectation or command output) with LF line ending, and produce output with LF line ending, then platform tools like 'sed' used for that task should be told not to turn LF into CRLF, with something like:

    case $uname_s in
    *MINGW*)
        sed () {
            command sed --binary "$@"
        }
	... other workarounds come here ...
	;;
    easc

before we can call that we ported our tests to the platform.

Hi Dscho

This makes me wonder about the implications for our diff tests. We have ~200 calls to test_cmp in t*-diff-*.sh. I had a look at t4053-diff-no-index.sh and non of the tests that use test_cmp look critical for functionality that is used by test_cmp and there are tests for the exit code. I suspect that if diff --no-index breaks we'll end up with confusing test failures rather than misleading passes.

A side effect of this change is that on windows test_cmp will now except directories as well as files but I don't think that matters.

Hopefully most of the files we feed into test_cmp are small enough that the absolute difference in run-time is not too big. There is an optimization for -U0 that trims the common tail from the files before calling xdl_diff() but that does not help here because we need to use --ignore-cr-at-eol (otherwise git diff --no-index -U0 || git diff --no-index might speed up the common case of matching files).

Best Wishes

Phillip

Hi Phillip,

I was surprised how little of a dent the overhead makes (it was not even measurable within the noise) when I tested this a couple of weeks ago (but then ran out of time and motivation to send a new iteration).

It would probably still be better to use less complex code, but Junio made his preference abundantly clear, and if we wanted to save on CI time, there are much bigger construction sites that we have to open. Point in
case: our CI runtime goes up, up, up, with no real restraint going on. A randomly picked run from 6 weeks ago took 7h14m of build time
(https://github.com/git/git/actions/runs/3175983767/usage), the current seen run took 8h48m
(https://github.com/git/git/actions/runs/3448435246/usage). If there is considerate use of resources going on then please help me because I cannot see it.

Indeed, I would believe that the non-Windows test jobs would catch that failure, so we do not need any extra code to check specifically for that.

There is similar code in xdiff to "shrink the box" to avoid unnecessary work, i.e. skipping identical lines at both ends of the line range:
https://github.com/git/git/blob/v2.38.1/xdiff/xdiffi.c#L261-L265

I share your concern, but my current concern is to cut the loss of time spent on this patch series on my part, and that concern is currently even bigger.

Ciao,
Dscho

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/[email protected]/
2. https://lore.kernel.org/git/[email protected]/

index 4927379184c..dea6069b5fe 100644
@@ -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

index 4fab1c1984c..cd6e9f797b6 100644
@@ -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

Hi Ævar,

The implied assumption of test_cmp is that the command is reliable, i.e. we are not expecting to test the code that runs the comparison itself.

Originally, Junio shared your concern that diff --no-index might not be reliable enough, but that was "a lifetime ago". In
https://lore.kernel.org/git/[email protected]/ he hinted that the diff --no-index code has matured enough to be considered reliable. That is far from the endorsement I would wish to have received (it would have been nice to see "I consider git diff --no-index to be reliable enough to serve as mingw_test_cmp", for example), but I believe it is the clearest statement I will get in that regard.

Why would this be a nice cherry on top?

From my perspective, it would co-opt unrelated test cases into the task of validating diff --no-index' correctness.

Such a loss of focus in test cases makes it harder to diagnose, debug and fix breakages. And it would mean that a single bug could make gazillions of test cases fail. That would be bad practice, of course.

Be my guest to contribute this once the current patch has made it to next. But please only then, we have enough friction on the Git mailing list and do not need to go out of our way to add more.

Thanks,
Johannes

I don't see a mention of reliability in the original 2007 thread, just optimization v.s. "cmp". But the list archive seems to only carry the CL, perhaps it was in that 7/8 or 8/8 patch?

In any case, that's different than what I'm pointing out, which is:

I fully trust "git diff --no-index" these days *in general* to serve as a "/usr/bin/diff -u" replacement, but *only* as long as it's the "/usr/bin/git" version of "git diff", and not the "git" we just built in-tree.

IOW this bit in t/README:

- Don't use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling "die()".[...]

So, before this series a test like this on Windows, with GIT_TEST_CMP=mingw_test_cmp:

! test_cmp foo bar

Would only succeed if "foo" and "bar" weren't identical, now it'll also succeed if 'git diff --no-index' happens to segfault on those inputs.

Now, the good thing is that the fallout of this should be relatively contained in this case. We'd only get those cases sneaking in if it's in the Windows-only code, as we use "/usr/bin/diff -u" elsewhere.

I think the diff here at the end (on top af master, but you get the idea) is what you'd need to stop conflating those. Most of it's just 's/! test_cmp/test_cmp !/g'.

The "test_cmp" then learns to distinguish between "a git" command and "not a git". I guess we could use test_must_fail_acceptable() there to to make this DWYM...

Yeah, probably not worth it. It would illustrate which --ignore-cr-at-eol is just due to test_cmp v.s. test_cmp_bin semantics on *nix though. E.g. in addition to the below this makes things pass without --ignore-cr-at-eol for me (aside from the t0001-init.sh failure I noted in the v2 feedback):

	diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
	index 7b5423eebda..f2e6b7a078c 100755
	--- a/t/t0061-run-command.sh
	+++ b/t/t0061-run-command.sh
	@@ -72,7 +72,7 @@ test_expect_success 'run_command does not try to execute a directory' '
	 
	 	PATH=$PWD/bin1:$PWD/bin2:$PATH \
	 		test-tool run-command run-command greet >actual 2>err &&
	-	test_cmp bin2/greet actual &&
	+	test_cmp_bin bin2/greet actual &&
	 	test_must_be_empty err
	 '
	 
	@@ -89,7 +89,7 @@ test_expect_success POSIXPERM 'run_command passes over non-executable file' '
	 
	 	PATH=$PWD/bin1:$PWD/bin2:$PATH \
	 		test-tool run-command run-command greet >actual 2>err &&
	-	test_cmp bin2/greet actual &&
	+	test_cmp_bin bin2/greet actual &&
	 	test_must_be_empty err
	 '
	 
	diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
	index 497b62868d4..4001ac24adf 100755
	--- a/t/t4120-apply-popt.sh
	+++ b/t/t4120-apply-popt.sh
	@@ -83,7 +83,7 @@ test_expect_success 'apply (-p2) diff, rename' '
	 	cp file1.saved file1 &&
	 	rm -f file2 &&
	 	git apply -p2 patch.rename &&
	-	test_cmp expected file2
	+	test_cmp_bin expected file2
	 '
	 
	 test_done
	diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
	index 5d853225a93..4c64396ed15 100644
	--- a/t/test-lib-functions.sh
	+++ b/t/test-lib-functions.sh
	@@ -1163,9 +1163,9 @@ test_cmp () {
	 		if test -n "$negate"
	 		then
	 			test_expect_code 1 env GIT_DIR=/dev/null \
	-					 git diff --no-index --ignore-cr-at-eol -- "$@"
	+					 git diff --no-index -- "$@"
	 		else
	-			GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- "$@"
	+			GIT_DIR=/dev/null git diff --no-index -- "$@"
	 		fi
	 	else
	 		if test -n "$negate"

Yeah, we can leave the nice-to-haves for later, a "conflates files being different with segfaults" really should be fixed though. And hopefully the below helps.

index 1f6b9b08d1d..d26ed200cbf 100644
@@ -304,7 +304,7 @@ check_access_log() {
 	sort "$1" >"$1".sorted &&
 	strip_access_log >access.log.stripped &&
 	sort access.log.stripped >access.log.sorted &&
-	if ! test_cmp "$1".sorted access.log.sorted
+	if test_cmp ! "$1".sorted access.log.sorted
 	then
 		test_cmp "$1" access.log.stripped
 	fi

index abecd75e4e4..6e1e6c056ff 100755
@@ -751,7 +751,7 @@ test_expect_success 'process filter should restart after unexpected write failur
 		test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
 
 		# Smudge failed
-		! test_cmp smudge-write-fail.o smudge-write-fail.r &&
+		test_cmp ! smudge-write-fail.o smudge-write-fail.r &&
 		rot13.sh <smudge-write-fail.o >expected &&
 		git cat-file blob :smudge-write-fail.r >actual &&
 		test_cmp expected actual

index cd3969e852b..dfab275dbe6 100755
@@ -14,7 +14,7 @@ test_expect_success 'setup: list of builtins' '
 
 test_expect_success 'list of txt and help mismatches is sorted' '
 	sort -u "$TEST_DIRECTORY"/t0450/txt-help-mismatches >expect &&
-	if ! test_cmp expect "$TEST_DIRECTORY"/t0450/txt-help-mismatches
+	if test_cmp ! expect "$TEST_DIRECTORY"/t0450/txt-help-mismatches
 	then
 		BUG "please keep the list of txt and help mismatches sorted"
 	fi

index 801919009e1..d11980916b0 100755
@@ -1214,7 +1214,7 @@ test_expect_success 'checkout-index with folders' '
 	# entry is a sparse directory.
 	run_on_all test_must_fail git checkout-index -f -- folder1/ &&
 	test_cmp full-checkout-err sparse-checkout-err &&
-	! test_cmp full-checkout-err sparse-index-err &&
+	test_cmp ! full-checkout-err sparse-index-err &&
 	grep "is a sparse directory" sparse-index-err
 '
 

index cf58cf025cd..14b6b58f531 100755
@@ -1403,7 +1403,7 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '
 	test_must_fail git rev-parse refs/bisect/something &&
 	git update-ref refs/bisect/something HEAD &&
 	git rev-parse refs/bisect/something >main-head &&
-	! test_cmp main-head worktree-head
+	test_cmp ! main-head worktree-head
 '
 
 test_expect_success 'transaction handles empty commit' '

index 7f605f865b3..dafb7860451 100755
@@ -778,7 +778,7 @@ test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
 
 	git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
 	git rev-parse --absolute-git-dir >our.dir &&
-	! test_cmp subdir.dir our.dir &&
+	test_cmp ! subdir.dir our.dir &&
 
 	git -C subdir log &&
 	git -C subdir branch rename-src &&

index fe72b247164..d2fba236a19 100755
@@ -155,7 +155,7 @@ test_expect_success 'should create branches if branch exists and --force is give
 		test_cmp expected actual-new-branch-a &&
 		# assert that branch --force actually moved the sub
 		# branch
-		! test_cmp expected actual-old-branch-a
+		test_cmp ! expected actual-old-branch-a
 	)
 '
 

index de1da4673da..3e7ceb27f3c 100755
@@ -2136,8 +2136,8 @@ test_expect_success 'format-patch --base' '
 	awk "{print \"prerequisite-patch-id:\", \$1}" <patch.id.raw >>fail &&
 
 	signature >>fail &&
-	! test_cmp fail actual1 &&
-	! test_cmp fail actual2
+	test_cmp ! fail actual1 &&
+	test_cmp ! fail actual2
 '
 
 test_expect_success 'format-patch --base errors out when base commit is in revision list' '

index 090987c89b2..d2e61579d0a 100755
@@ -93,7 +93,7 @@ test_expect_success 'withfix (backward)' '
 	sed -n -e /h/p file-fixed >fixed-tail &&
 	sed -n -e /h/p file >file-tail &&
 
-	! test_cmp fixed-tail file-tail
+	test_cmp ! fixed-tail file-tail
 
 '
 

index cdad4b68807..8028aa2b712 100755
@@ -837,7 +837,7 @@ test_expect_success 'am without --committer-date-is-author-date' '
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
 	sed -ne "/^author /s/.*> //p" head1 >at &&
 	sed -ne "/^committer /s/.*> //p" head1 >ct &&
-	! test_cmp at ct
+	test_cmp ! at ct
 '
 
 # This checks for +0000 because TZ is set to UTC and that should

index a7fa94ce0a2..edc11213a10 100755
@@ -74,7 +74,7 @@ test_expect_success 'patch-id detects equality' '
 test_expect_success 'patch-id detects inequality' '
 	get_patch_id main &&
 	get_patch_id notsame &&
-	! test_cmp patch-id_main patch-id_notsame
+	test_cmp ! patch-id_main patch-id_notsame
 '
 test_expect_success 'patch-id detects equality binary' '
 	cat >.gitattributes <<-\EOF &&

@@ -100,7 +100,7 @@ test_expect_success 'patch-id detects inequality binary' '
 	EOF
 	get_patch_id main &&
 	get_patch_id notsame &&
-	! test_cmp patch-id_main patch-id_notsame &&
+	test_cmp ! patch-id_main patch-id_notsame &&
 	test_when_finished "rm .gitattributes"
 '
 

@@ -126,7 +126,7 @@ cmp_patch_id () {
 	if
 		test "$1" = "relevant"
 	then
-		! test_cmp patch-id_"$2" patch-id_"$3"
+		test_cmp ! patch-id_"$2" patch-id_"$3"
 	else
 		test_cmp patch-id_"$2" patch-id_"$3"
 	fi

@@ -279,7 +279,7 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' '
 	test_cmp patch-id_nonl patch-id_withnl &&
 	calc_patch_id nonl-inc-ws --verbatim <nonl &&
 	calc_patch_id withnl-inc-ws --verbatim <withnl &&
-	! test_cmp patch-id_nonl-inc-ws patch-id_withnl-inc-ws
+	test_cmp ! patch-id_nonl-inc-ws patch-id_withnl-inc-ws
 '
 
 test_expect_success 'patch-id handles diffs with one line of before/after' '

index ed9dfd624c7..06e4ca7bdca 100755
@@ -26,7 +26,7 @@ test_expect_success 'info/refs is not needlessly overwritten' '
 test_expect_success 'info/refs can be forced to update' '
 	git update-server-info -f &&
 	test-tool chmtime --get .git/info/refs >b &&
-	! test_cmp a b
+	test_cmp ! a b
 '
 
 test_expect_success 'info/refs updates when changes are made' '

@@ -36,7 +36,7 @@ test_expect_success 'info/refs updates when changes are made' '
 	git update-ref refs/heads/foo HEAD &&
 	git update-server-info &&
 	test-tool chmtime --get .git/info/refs >b &&
-	! test_cmp a b
+	test_cmp ! a b
 '
 
 test_done

index 6d693eef82f..9718f0ab12a 100755
@@ -400,7 +400,7 @@ test_bitmap_cases () {
 			test-tool bitmap list-commits | sort >bitmaps &&
 			comm -13 bitmaps commits >after &&
 
-			! test_cmp before after
+			test_cmp ! before after
 		)
 	'
 

index 0882cbb6e4a..338ccd32fac 100755
@@ -186,7 +186,7 @@ test_midx_bitmap_cases () {
 			test-tool bitmap list-commits | sort >bitmaps &&
 			comm -13 bitmaps commits >after &&
 
-			! test_cmp before after
+			test_cmp ! before after
 		)
 	'
 

@@ -268,7 +268,7 @@ test_midx_bitmap_cases () {
 			test-tool bitmap list-commits | sort >bitmaps &&
 			comm -13 bitmaps commits >after &&
 
-			! test_cmp before after
+			test_cmp ! before after
 		)
 	'
 

index 20d063fb9ae..7037335533c 100755
@@ -111,7 +111,7 @@ test_expect_success 'use "origin" when no remote specified and multiple found' '
 
 test_expect_success 'suppress "From <url>" with -q' '
 	git ls-remote -q 2>actual_err &&
-	! test_cmp exp_err actual_err
+	test_cmp ! exp_err actual_err
 '
 
 test_expect_success 'use branch.<name>.remote if possible' '

index 3126cfd7e9d..eeef68c9f3d 100755
@@ -90,7 +90,7 @@ test_expect_success 'by default no tags will be kept updated' '
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-	! test_cmp expect actual &&
+	test_cmp ! expect actual &&
 	test_line_count = 2 actual
 '
 

index 41d0ca00b1c..c927b0d9e62 100755
@@ -538,7 +538,7 @@ test_expect_success '--abbrev' '
 	sed -e "s/$OID_REGEX/LONG/g" -e "s/$_x05/SHORT/g" <actual3 >fuzzy3 &&
 	test_cmp expect2 fuzzy2 &&
 	test_cmp expect3 fuzzy3 &&
-	! test_cmp actual1 actual2
+	test_cmp ! actual1 actual2
 '
 
 test_expect_success '%H is not affected by --abbrev-commit' '

index 523efbecde1..d4819317649 100755
@@ -24,7 +24,7 @@ verify_expect () {
 	git status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual &&
 	if test "x$1" = 'x!'
 	then
-		! test_cmp expect actual
+		test_cmp ! expect actual
 	else
 		test_cmp expect actual
 	fi

index eae6a46ef3d..904f91fc4bc 100755
@@ -451,7 +451,7 @@ test_expect_success 'status from subdirectory should have the same SHA1' '
 		git submodule status >output &&
 		awk "{print \$1}" <output >expect2 &&
 		test_cmp expect2 actual2 &&
-		! test_cmp actual actual2
+		test_cmp ! actual actual2
 	)
 '
 

index 2b7ef6c41a4..fb3d51de13d 100755
@@ -1510,7 +1510,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
 test_expect_success '"status.branch=true" different from "--no-branch"' '
 	git status -s --no-branch  >expected_nobranch &&
 	git -c status.branch=true status -s >actual &&
-	! test_cmp expected_nobranch actual
+	test_cmp ! expected_nobranch actual
 '
 
 test_expect_success '"status.branch=true" weaker than "--no-branch"' '

index 7c3f6ed9943..acdf91a77a2 100755
@@ -1059,7 +1059,7 @@ test_expect_success 'merge --quit' '
 		test_path_is_missing .git/MERGE_MSG &&
 		git rerere status >rerere.after &&
 		test_must_be_empty rerere.after &&
-		! test_cmp rerere.after rerere.before
+		test_cmp ! rerere.after rerere.before
 	)
 '
 

index 81fb7c474c1..687dbcc9d23 100755
@@ -55,7 +55,7 @@ test_expect_success 'merge c2 with a custom strategy' '
 	git diff --exit-code c2 HEAD &&
 	git diff --exit-code c2 &&
 
-	! test_cmp head.old head.new &&
+	test_cmp ! head.old head.new &&
 	test_cmp head.old first-parent &&
 	test_cmp second-parent.expected second-parent &&
 	test_cmp tree.expected tree &&

@@ -81,7 +81,7 @@ test_expect_success 'trivial merge with custom strategy' '
 	git diff --exit-code c3 HEAD &&
 	git diff --exit-code c3 &&
 
-	! test_cmp head.old head.new &&
+	test_cmp ! head.old head.new &&
 	test_cmp head.old first-parent &&
 	test_cmp second-parent.expected second-parent &&
 	test_cmp tree.expected tree &&

index 5be483bf887..a1d62e8566c 100755
@@ -365,7 +365,7 @@ test_expect_success '--write-midx with preferred bitmap tips' '
 		test-tool bitmap list-commits | sort >bitmaps &&
 		comm -13 bitmaps commits >after &&
 
-		! test_cmp before after
+		test_cmp ! before after
 	)
 '
 

index b18633dee1b..d8f26d1197b 100755
@@ -184,10 +184,10 @@ test_expect_success mark_ignored_lines '
 	test_cmp expect actual &&
 
 	sed -n "3p" blame_raw | cut -c1 >actual &&
-	! test_cmp expect actual &&
+	test_cmp ! expect actual &&
 
 	sed -n "4p" blame_raw | cut -c1 >actual &&
-	! test_cmp expect actual
+	test_cmp ! expect actual
 '
 
 # For ignored revs that added 'unblamable' lines and more recent commits changed

index 1465156072e..53f240e45cf 100755
@@ -94,7 +94,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
 		echo 1 >> file &&
 		svn_cmd commit -m "changing file" &&
 		svn_cmd up &&
-		! test_cmp auto_updated_file au_file_saved
+		test_cmp ! auto_updated_file au_file_saved
 	)
 '
 

index 77047e250dc..29a2d3a03a7 100755
@@ -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

index 5fe83315ecd..6fdfa7d5855 100755
@@ -68,7 +68,7 @@ scrub_k_check () {
 	file="$1" &&
 	scrub="$TRASH_DIRECTORY/$file" &&
 	"$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" <"$git/$file" >"$scrub" &&
-	! test_cmp "$cli/$file" "$scrub" &&
+	test_cmp ! "$cli/$file" "$scrub" &&
 	test_cmp "$git/$file" "$scrub" &&
 	rm "$scrub"
 }

@@ -76,7 +76,7 @@ scrub_ko_check () {
 	file="$1" &&
 	scrub="$TRASH_DIRECTORY/$file" &&
 	"$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" <"$git/$file" >"$scrub" &&
-	! test_cmp "$cli/$file" "$scrub" &&
+	test_cmp ! "$cli/$file" "$scrub" &&
 	test_cmp "$git/$file" "$scrub" &&
 	rm "$scrub"
 }

index 796093a7b32..5d853225a93 100644
@@ -1148,8 +1148,33 @@ test_expect_code () {
 # - not all diff versions understand "-u"
 
 test_cmp () {
-	test "$#" -ne 2 && BUG "2 param"
-	eval "$GIT_TEST_CMP" '"$@"'
+	test "$#" -lt 2 && BUG "2..3 param"
+	test "$#" -gt 3 && BUG "2..3 param"
+
+	local negate=
+	if test "$#" = 3 && test "$1" = "!"
+	then
+		negate=t
+		shift
+	fi
+
+	if test "$GIT_TEST_CMP" = "builtin:git-no-index"
+	then
+		if test -n "$negate"
+		then
+			test_expect_code 1 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
+		if test -n "$negate"
+		then
+			! "$GIT_TEST_CMP" "$@"
+		else
+			"$GIT_TEST_CMP" "$@"
+		fi
+	fi
 }
 
 # Check that the given config key has the expected value.

index 6db377f68b8..3028283f307 100644
@@ -1721,7 +1721,7 @@ case $uname_s in
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
 	test_set_prereq WINDOWS
-	GIT_TEST_CMP=mingw_test_cmp
+	GIT_TEST_CMP=builtin:git-no-index
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

Well "! test_cmp" is wrong anyway, because it _expects_ two files are the same and gives more detailed diagnosis when they differ by giving "diff" output.

If you expect them to be different, "! test_cmp" would give "detailed diagnosis" in the wrong case, i.e. the outcome is what we expect.

So the caller must do "test_cmp !" whether the underlying implementation of test_cmp uses "diff -u" or "diff --no-index".

I agree that "! test_cmp foo bar" will give output about how "foo" is different from "bar" (and halt the test only when the two have the same contents).

But this confuses me. "git grep 'test_cmp !'" turns up no results, and furthermore, test_cmp() itself begins with:

    test "$#" -ne 2 && BUG "2 param"

So I am not sure what you are referring to.

Thanks,
Taylor

I think we've got got a bit sidetracked here.

Junio's pointing out that a test that would do:

! diff a b

Is pretty stupid, why would you want to show a diff of differences, if you're expecting it to be different? Just use:

! cmp a b

Or something. That's correct. So I think probably those "! test_cmp" uses would be good candidates for some clean-up to make the test suite pretty.

But what I'm pointing out is that by having "test_cmp" be something that invokes "git" (or a helper) we *must not* negate it using the shell's negation, we have to use "test_must_fail" for anything we build ourselves.

So, the proposed patch would make the existing cases where we do:

! test_cmp

Cases where we'd hide a segfault, whereas before it was maybe a bit odd, but harmless, and working as intended in the sense of reliably passing if we had differences.

So no, nothing currently does "test_cmp !", I was suggesting that upthread as the least invasive way of having "test_cmp" safe'd under negation, now that we were having it call out to "git diff" (or a helper...). It's the pattern we use for our own test helpers that invoke our own binaries (and if we don't, that's a bug).

Yes.

Besides, "the outcome must be different from this other file" is a very sloppy thing to say in a test. It rarely is OK for our output to be merely different from one "wrong" output. Tests should specify in what way the output must be different from the "wrong" example, at least.

A few months ago, directly after sending a patch to fix a performance regression due to a mis-use of test_cmp
[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],
I got curious to see whether Git for Windows had the same issue. And it did
not: it passes t5351 in 22 seconds, even while using test_cmp to compare pack files
[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].

The explanation is of course that Git for Windows uses a test helper for test_cmp that is written in C, instead of the Bash function. And C is much faster than a Bash function, especially on Windows. This is especially sad when said Bash code is only used on Windows. So I originally had pulled out this helper from the years-long effort to let Git for Windows use BusyBox' ash to run the test suite. The result was a single-patch contribution of a change that had been in Git for Windows since June 2018. Unfortunately, this tried-and-tested code was rejected by the Git maintainer.

Let's fall back to the next-best solution: git diff --no-index, which the Git maintainer seems to like. The downside is that the diff machinery does a lot more than a simple cmp clone, and therefore a lot more things can go wrong that might make it look like a test case is failing when the fault is somewhere else entirely. There is one way to find out whether this is a valid concern.

Changes since v3:

* Fixed the subject of the cover letter (which should have been adjusted in v3)
* Elaborated the paragraph about the historical context of this patch

Changes since v2:

* Dropped the test helper, using diff --no-index instead.

Changes since v1:

* Fixed double "with" in the commit message.
* Renamed the test helper to text-cmp.
* Made the diff --no-index call more robust by using a double-dash separator.

Johannes Schindelin (2):
t0021: use Windows-friendly pwd
tests(mingw): avoid very slow mingw_test_cmp

 t/t0021-conversion.sh   |  4 +--
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 3 files changed, 3 insertions(+), 69 deletions(-)

base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1309

Range-diff vs v3:

 1:  b38b8fb5a85 = 1:  b38b8fb5a85 t0021: use Windows-friendly `pwd`
 2:  a7f4265ceb2 ! 2:  128b1f348d8 tests(mingw): avoid very slow `mingw_test_cmp`

@@ Commit message
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.
+ The original reason why Git's test suite needs the mingw_test_cmp
+ function at all (and why cmp is not good enough) is that Git's test
+ suite is not actually trying to compare binary files when it calls
+ test_cmp, but it compares text files. And those text files can contain
+ CR/LF line endings depending on the circumstances.
+
+ Note: The original fix in the Git for Windows project implemented 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. This was done to minimize the risk in using something as
+ complex as the diff machinery to perform something as simple as
+ determining whether text output is identical to the expected output or
+ not. This approach has served Git for Windows well for years, but the
+ attempt to upstream this saw a lot of backlash and distractions during
+ the review, was disliked by the Git maintainer and was therefore
+ abandoned. For full details, see the thread at
+ https://lore.kernel.org/git/[email protected]/t

gitgitgadget

From: Johannes Schindelin <[email protected]>

In Git for Windows, when passing paths from shell scripts to regular Win32 executables, thanks to the MSYS2 runtime a somewhat magic path conversion happens that lets the shell script think that there is a file at /git/Makefile and the Win32 process it spawned thinks that the shell script said C:/git-sdk-64/git/Makefile instead.

This conversion is documented in detail over here:
https://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion

As all automatic conversions, there are gaps. For example, to avoid mistaking command-line options like /LOG=log.txt (which are quite common in the Windows world) from being mistaken for a Unix-style absolute path, the MSYS2 runtime specifically exempts arguments containing a = character from that conversion.

We are about to change test_cmp to use git diff --no-index, which involves spawning precisely such a Win32 process.

In combination, this would cause a failure in t0021-conversion.sh where we pass an absolute path containing an equal character to the test_cmp function.

Seeing as the Unix tools like cp and diff that are used by Git's test suite in the Git for Windows SDK (thanks to the MSYS2 project) understand both Unix-style as well as Windows-style paths, we can stave off this problem by simply switching to Windows-style paths and side-stepping the need for any automatic path conversion.

Note: The PATH variable is obviously special, as it is colon-separated in the MSYS2 Bash used by Git for Windows, and therefore _cannot_ contain absolute Windows-style paths, lest the colon after the drive letter is mistaken for a path separator. Therefore, we need to be careful to keep the Unix-style when modifying the PATH variable.

---
 t/t0021-conversion.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index bad37abad2c..15482fa78e3 100755
@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-TEST_ROOT="$PWD"
-PATH=$TEST_ROOT:$PATH
+PATH=$PWD:$PATH
+TEST_ROOT="$(pwd)"
 
 write_script <<\EOF "$TEST_ROOT/rot13.sh"
 tr \

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

The original reason why Git's test suite needs the mingw_test_cmp function at all (and why cmp is not good enough) is that Git's test suite is not actually trying to compare binary files when it calls test_cmp, but it compares text files. And those text files can contain CR/LF line endings depending on the circumstances.

Note: The original fix in the Git for Windows project implemented 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. This was done to minimize the risk in using something as complex as the diff machinery to perform something as simple as determining whether text output is identical to the expected output or not. This approach has served Git for Windows well for years, but the attempt to upstream this saw a lot of backlash and distractions during the review, was disliked by the Git maintainer and was therefore abandoned. For full details, see the thread at
https://lore.kernel.org/git/[email protected]/t

---
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 2 files changed, 1 insertion(+), 67 deletions(-)

index 8c44856eaec..452fe9bc8aa 100644
@@ -1541,72 +1541,6 @@ test_skip_or_die () {
 	error "$2"
 }
 
-# The following mingw_* functions obey POSIX shell syntax, but are actually
-# bash scripts, and are meant to be used only with bash on Windows.
-
-# A test_cmp function that treats LF and CRLF equal and avoids to fork
-# diff when possible.
-mingw_test_cmp () {
-	# Read text into shell variables and compare them. If the results
-	# are different, use regular diff to report the difference.
-	local test_cmp_a= test_cmp_b=
-
-	# When text came from stdin (one argument is '-') we must feed it
-	# to diff.
-	local stdin_for_diff=
-
-	# Since it is difficult to detect the difference between an
-	# empty input file and a failure to read the files, we go straight
-	# to diff if one of the inputs is empty.
-	if test -s "$1" && test -s "$2"
-	then
-		# regular case: both files non-empty
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-	elif test -s "$1" && test "$2" = -
-	then
-		# read 2nd file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b
-		stdin_for_diff='<<<"$test_cmp_b"'
-	elif test "$1" = - && test -s "$2"
-	then
-		# read 1st file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-		stdin_for_diff='<<<"$test_cmp_a"'
-	fi
-	test -n "$test_cmp_a" &&
-	test -n "$test_cmp_b" &&
-	test "$test_cmp_a" = "$test_cmp_b" ||
-	eval "diff -u \"\$@\" $stdin_for_diff"
-}
-
-# $1 is the name of the shell variable to fill in
-mingw_read_file_strip_cr_ () {
-	# Read line-wise using LF as the line separator
-	# and use IFS to strip CR.
-	local line
-	while :
-	do
-		if IFS=$'\r' read -r -d $'\n' line
-		then
-			# good
-			line=$line$'\n'
-		else
-			# we get here at EOF, but also if the last line
-			# was not terminated by LF; in the latter case,
-			# some text was read
-			if test -z "$line"
-			then
-				# EOF, really
-				break
-			fi
-		fi
-		eval "$1=\$$1\$line"
-	done
-}
-
 # Like "env FOO=BAR some-program", but run inside a subshell, which means
 # it also works for shell functions (though those functions cannot impact
 # the environment outside of the test_env invocation).

index 7726d1da88a..f8c6205e08f 100644
@@ -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=mingw_test_cmp
+	GIT_TEST_CMP="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --"
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

gitgitgadget

In the previous round, you wrote that this paragraph:

It explains why we replace a relatively simple logic with a relatively complex one.

But I'm not sure the rewritten version does what you claim, at least in my own personal opinion.

It is not helpful to say the original approach "saw a lot of backlash". It is helpful, on the other hand, to say what about the original approach gave reviewers pause, and why that alternative approach isn't pursued here.

Maybe I'm splitting hairs here, but I really do stand by my original suggestion from back in [1].

Thanks,
Taylor

[1]: https://lore.kernel.org/git/[email protected]/

Hi Taylor,

Yes. And I obviously got myself misunderstood.

The simple logic I refer to is the t/helper/test-cmp.c that I abandoned.

The complex logic is the diff machinery we now call, which is a lot more involved and goes through a lot more code paths.

It is the nicest thing I can say about it.

And I want people who are curious what happened to Git for Windows' custom code to have an explanation in the commit messages.

We can also keep hitting a dead horse, but I don't think that will make anything any better.

Thanks,
Johannes

I don't think you have to or should refer to the earlier round of review at all.

Instead, it would be much more helpful to explain what you tried before, and why you are taking a different approach now.

I am simply not comfortable with taking the patch with the way the body is currently written.

Thanks,
Taylor

Hi Taylor,

You misunderstand.

I am referring to the fact that Git for Windows has run with a very different solution for this problem, for years, yet it was rejected upon upstreaming, and had to be replaced by a completely different workaround.

It's not just a simple "earlier round of review" at all that is the issue I am describing.

It is a very real concern of future readers who know what patches are currently in Git for Windows and who all of a sudden do not find the git test-tool cmp code anymore in Git for Windows and then see that git diff --no-index is used and naturally want to know what the heck happened.

This is context relevant to understand why the particular approach implemented in the patch was chosen and another one was discarded (when that other approach has served Git for Windows very well for several years), for which the commit message is precisely the appropriate place. I am quite lost trying to understand why I am asked to remove said context, leaving future readers puzzled e.g. in the case that it should turn out to have been a terrible idea to use the quite complex diff machinery for as simple a task as test_cmp. It sounds to me like I am asked to make my contribution worse ("worsimprove" is the term I recently learned to describe this) instead of helping me to improve it.

The advice you provided directly contradicts what is written in
https://git-scm.com/docs/SubmittingPatches#describe-changes, after all (ignore the funny grammar for now unless you want to add a tangent to this already long thread):

The body should provide a meaningful commit message, which:

[...]

3. alternate solutions considered but discarded, if any.

Thanks,
Johannes

Right. Having read the earlier thread myself, I am familiar with the context. So I'm not trying to dismiss it as just another round of review, but instead try to steer the commit message in a more constructive direction.

No. I am not suggesting you remove context at all. But what I am saying is that describing the last attempt at upstreaming by saying it "saw a lot of backlash and distractions during review and was therefore abandoned" is not helpful.

If it saw backlash and distraction: why? What about the approach caused backlash? Describing that thing as an alternative approach and explaining concretely why it was disliked is the sort of context that I think we should aim for in our commit messages.

But characterizing the review outright as full of backlash and distractions is not helpful to future readers, and it is not a kind way to treat others on the list who may have participated in that review.

I am saying that, as written, the commit message does not explain what the alternative approaches were in great detail.

Thanks,
Taylor

Half a year ago, directly after sending a patch to fix a performance regression due to a mis-use of test_cmp
[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],
I got curious to see whether Git for Windows had the same issue. And it did
not: it passed t5351 in 22 seconds, even while using test_cmp to compare pack files
[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].
This motivated me to upstream Git for Windows' changes.

Changes since v4:

* The commit message was rewritten almost completely.

Changes since v3:

* Fixed the subject of the cover letter (which should have been adjusted in v3)
* Elaborated the paragraph about the historical context of this patch

Changes since v2:

* Dropped the test helper, using diff --no-index instead.

Changes since v1:

* Fixed double "with" in the commit message.
* Renamed the test helper to text-cmp.
* Made the diff --no-index call more robust by using a double-dash separator.

Johannes Schindelin (2):
t0021: use Windows-friendly pwd
tests(mingw): avoid very slow mingw_test_cmp

 t/t0021-conversion.sh   |  4 +--
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 3 files changed, 3 insertions(+), 69 deletions(-)

base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1309

Range-diff vs v4:

 1:  b38b8fb5a85 = 1:  b38b8fb5a85 t0021: use Windows-friendly `pwd`
 2:  128b1f348d8 ! 2:  6a80fab7e39 tests(mingw): avoid very slow `mingw_test_cmp`

@@ Metadata
## Commit message ##
tests(mingw): avoid very slow mingw_test_cmp

- 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.
+ When Git's test suite uses test_cmp, it is not actually trying to
+ compare binary files as the name cmp would suggest to users familiar
+ with Unix' tools, but the tests instead verify that actual output
+ matches the expected text.

- The original reason why Git's test suite needs the mingw_test_cmp
- function at all (and why cmp is not good enough) is that Git's test
- suite is not actually trying to compare binary files when it calls
- test_cmp, but it compares text files. And those text files can contain
- CR/LF line endings depending on the circumstances.
+ On Unix, cmp works well enough for Git's purposes because only Line
+ Feed characters are used as line endings. However, on Windows, while
+ most tools accept Line Feeds as line endings, many tools produce
+ Carriage Return + Line Feed line endings, including some of the tools
+ used by the test suite (which are therefore provided via Git for Windows
+ SDK). Therefore, cmp would frequently fail merely due to different
+ line endings.

- Note: The original fix in the Git for Windows project implemented 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. This was done to minimize the risk in using something as
- complex as the diff machinery to perform something as simple as
- determining whether text output is identical to the expected output or
- not. This approach has served Git for Windows well for years, but the
- attempt to upstream this saw a lot of backlash and distractions during
- the review, was disliked by the Git maintainer and was therefore
- abandoned. For full details, see the thread at
- https://lore.kernel.org/git/[email protected]/t
+ To accommodate for that, the mingw_test_cmp function was introduced
+ into Git's test suite to perform a line-by-line comparison that ignores
+ line endings. This function is a Bash function that is only used on
+ Windows, everywhere else cmp is used.
+
+ This is a double whammy because cmp is fast, and mingw_test_cmp is
+ slow, even more so on Windows because it is a Bash script function, and
+ Bash scripts are known to run particularly slowly on Windows due to
+ Bash's need for the POSIX emulation layer provided by the MSYS2 runtime.
+
+ The commit message of 32ed3314c104 (t5351: avoid using test_cmp for
+ binary data, 2022-07-29) provides an illuminating account of the
+ consequences: On Windows, the platform on which Git could really use all
+ the help it can get to improve its performance, the time spent on one
+ entire test script was reduced from half an hour to less than half a
+ minute merely by avoiding a single call to mingw_test_cmp in but a
+ single test case.
+
+ Learning the lesson to avoid shell scripting wherever possible, the Git
+ for Windows project implemented a minimal replacement for
+ mingw_test_cmp in the form of a test-tool subcommand that parses the
+ input files line by line, ignoring line endings, and compares them.
+ Essentially the same thing as mingw_test_cmp, but implemented in
+ C instead of Bash. This solution served the Git for Windows project
+ well, over years.
+
+ However, when this solution was finally upstreamed, the conclusion was
+ reached that a change to use git diff --no-index instead of
+ mingw_test_cmp was more easily reviewed and hence should be used
+ instead.
+
+ 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/[email protected]/
+ 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.
+
+ When the concern was raised that the diff machinery, due to its
+ complexity, would perform substantially worse than the test helper
+ originally implemented in the Git for Windows project, a test
+ demonstrated that these performance differences are well lost within the
+ 100+ minutes it takes to run Git's test suite on Windows.

gitgitgadget

From: Johannes Schindelin <[email protected]>

In Git for Windows, when passing paths from shell scripts to regular Win32 executables, thanks to the MSYS2 runtime a somewhat magic path conversion happens that lets the shell script think that there is a file at /git/Makefile and the Win32 process it spawned thinks that the shell script said C:/git-sdk-64/git/Makefile instead.

This conversion is documented in detail over here:
https://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion

As all automatic conversions, there are gaps. For example, to avoid mistaking command-line options like /LOG=log.txt (which are quite common in the Windows world) from being mistaken for a Unix-style absolute path, the MSYS2 runtime specifically exempts arguments containing a = character from that conversion.

We are about to change test_cmp to use git diff --no-index, which involves spawning precisely such a Win32 process.

In combination, this would cause a failure in t0021-conversion.sh where we pass an absolute path containing an equal character to the test_cmp function.

Seeing as the Unix tools like cp and diff that are used by Git's test suite in the Git for Windows SDK (thanks to the MSYS2 project) understand both Unix-style as well as Windows-style paths, we can stave off this problem by simply switching to Windows-style paths and side-stepping the need for any automatic path conversion.

Note: The PATH variable is obviously special, as it is colon-separated in the MSYS2 Bash used by Git for Windows, and therefore _cannot_ contain absolute Windows-style paths, lest the colon after the drive letter is mistaken for a path separator. Therefore, we need to be careful to keep the Unix-style when modifying the PATH variable.

---
 t/t0021-conversion.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index bad37abad2c..15482fa78e3 100755
@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-TEST_ROOT="$PWD"
-PATH=$TEST_ROOT:$PATH
+PATH=$PWD:$PATH
+TEST_ROOT="$(pwd)"
 
 write_script <<\EOF "$TEST_ROOT/rot13.sh"
 tr \

gitgitgadget

From: Johannes Schindelin <[email protected]>

When Git's test suite uses test_cmp, it is not actually trying to compare binary files as the name cmp would suggest to users familiar with Unix' tools, but the tests instead verify that actual output matches the expected text.

On Unix, cmp works well enough for Git's purposes because only Line Feed characters are used as line endings. However, on Windows, while most tools accept Line Feeds as line endings, many tools produce Carriage Return + Line Feed line endings, including some of the tools used by the test suite (which are therefore provided via Git for Windows SDK). Therefore, cmp would frequently fail merely due to different line endings.

To accommodate for that, the mingw_test_cmp function was introduced into Git's test suite to perform a line-by-line comparison that ignores line endings. This function is a Bash function that is only used on Windows, everywhere else cmp is used.

This is a double whammy because cmp is fast, and mingw_test_cmp is slow, even more so on Windows because it is a Bash script function, and Bash scripts are known to run particularly slowly on Windows due to Bash's need for the POSIX emulation layer provided by the MSYS2 runtime.

The commit message of 32ed3314c104 (t5351: avoid using test_cmp for binary data, 2022-07-29) provides an illuminating account of the
consequences: On Windows, the platform on which Git could really use all the help it can get to improve its performance, the time spent on one entire test script was reduced from half an hour to less than half a minute merely by avoiding a single call to mingw_test_cmp in but a single test case.

Learning the lesson to avoid shell scripting wherever possible, the Git for Windows project implemented a minimal replacement for mingw_test_cmp in the form of a test-tool subcommand that parses the input files line by line, ignoring line endings, and compares them. Essentially the same thing as mingw_test_cmp, but implemented in C instead of Bash. This solution served the Git for Windows project well, over years.

However, when this solution was finally upstreamed, the conclusion was reached that a change to use git diff --no-index instead of mingw_test_cmp was more easily reviewed and hence should be used instead.

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/[email protected]/
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.

When the concern was raised that the diff machinery, due to its complexity, would perform substantially worse than the test helper originally implemented in the Git for Windows project, a test demonstrated that these performance differences are well lost within the 100+ minutes it takes to run Git's test suite on Windows.

---
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 2 files changed, 1 insertion(+), 67 deletions(-)

index 8c44856eaec..452fe9bc8aa 100644
@@ -1541,72 +1541,6 @@ test_skip_or_die () {
 	error "$2"
 }
 
-# The following mingw_* functions obey POSIX shell syntax, but are actually
-# bash scripts, and are meant to be used only with bash on Windows.
-
-# A test_cmp function that treats LF and CRLF equal and avoids to fork
-# diff when possible.
-mingw_test_cmp () {
-	# Read text into shell variables and compare them. If the results
-	# are different, use regular diff to report the difference.
-	local test_cmp_a= test_cmp_b=
-
-	# When text came from stdin (one argument is '-') we must feed it
-	# to diff.
-	local stdin_for_diff=
-
-	# Since it is difficult to detect the difference between an
-	# empty input file and a failure to read the files, we go straight
-	# to diff if one of the inputs is empty.
-	if test -s "$1" && test -s "$2"
-	then
-		# regular case: both files non-empty
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-	elif test -s "$1" && test "$2" = -
-	then
-		# read 2nd file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b
-		stdin_for_diff='<<<"$test_cmp_b"'
-	elif test "$1" = - && test -s "$2"
-	then
-		# read 1st file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-		stdin_for_diff='<<<"$test_cmp_a"'
-	fi
-	test -n "$test_cmp_a" &&
-	test -n "$test_cmp_b" &&
-	test "$test_cmp_a" = "$test_cmp_b" ||
-	eval "diff -u \"\$@\" $stdin_for_diff"
-}
-
-# $1 is the name of the shell variable to fill in
-mingw_read_file_strip_cr_ () {
-	# Read line-wise using LF as the line separator
-	# and use IFS to strip CR.
-	local line
-	while :
-	do
-		if IFS=$'\r' read -r -d $'\n' line
-		then
-			# good
-			line=$line$'\n'
-		else
-			# we get here at EOF, but also if the last line
-			# was not terminated by LF; in the latter case,
-			# some text was read
-			if test -z "$line"
-			then
-				# EOF, really
-				break
-			fi
-		fi
-		eval "$1=\$$1\$line"
-	done
-}
-
 # Like "env FOO=BAR some-program", but run inside a subshell, which means
 # it also works for shell functions (though those functions cannot impact
 # the environment outside of the test_env invocation).

index 7726d1da88a..f8c6205e08f 100644
@@ -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=mingw_test_cmp
+	GIT_TEST_CMP="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --"
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM

gitgitgadget

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.

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/[email protected]/

Am 06.12.22 um 16:07 schrieb Johannes Schindelin via GitGitGadget:

Am 06.12.2022 um 16:07 schrieb Johannes Schindelin via GitGitGadget:

Only t3920 needs mingw_test_cmp on my system. [2] on top of [1] removes that dependency. Does that work for you as well?

diff -u and git diff are slower than mingw_test_cmp when running "make test" on my system ("uname -rs" says "MINGW64_NT-10.0-22621 3.3.6-341.x86_64"). Timings (sorry, only a single run each):

2.39.0-rc2:
All tests successful.
Files=983, Tests=26154, 2082 wallclock secs ( 7.86 usr 15.97 sys + 776.73 cusr 2352.96 csys = 3153.52 CPU)
Result: PASS

2.39.0-rc2 + [1] + [2] + removal of "GIT_TEST_CMP=mingw_test_cmp"
from t/test-lib.sh:
All tests successful.
Files=983, Tests=26154, 2174 wallclock secs ( 7.75 usr 17.27 sys + 813.53 cusr 2699.12 csys = 3537.66 CPU)
Result: PASS

2.39.0-rc2 + your v5:
All tests successful.
Files=983, Tests=26154, 2160 wallclock secs ( 4.16 usr 10.03 sys + 647.75 cusr 2038.99 csys = 2700.92 CPU)
Result: PASS

2.39.0-rc2 + [1] + [2] + the patch below:
All tests successful.
Files=983, Tests=26154, 2026 wallclock secs ( 4.91 usr 9.91 sys + 608.91 cusr 1881.98 csys = 2505.70 CPU)
Result: PASS

This is noisy, running "make test" multiple times takes too long. Running a single test using hyperfine is feasible. Here are my results for the last test that needed a CR-agnostic test_cmp:

2.39.0-rc2:
Benchmark 1: sh.exe t3920-crlf-messages.sh
Time (mean ± σ): 5.666 s ± 0.085 s [User: 0.000 s, System: 0.007 s]
Range (min … max): 5.583 s … 5.858 s 10 runs

2.39.0-rc2 + [1] + [2] + removal of "GIT_TEST_CMP=mingw_test_cmp" from t/test-lib.sh:
Benchmark 1: sh.exe t3920-crlf-messages.sh
Time (mean ± σ): 6.540 s ± 0.065 s [User: 0.000 s, System: 0.004 s]
Range (min … max): 6.454 s … 6.681 s 10 runs

2.39.0-rc2 + your v5:
Benchmark 1: sh.exe t3920-crlf-messages.sh
Time (mean ± σ): 6.632 s ± 0.090 s [User: 0.000 s, System: 0.004 s]
Range (min … max): 6.550 s … 6.791 s 10 runs

2.39.0-rc2 + [1] + [2] + the patch below:
Benchmark 1: sh.exe t3920-crlf-messages.sh
Time (mean ± σ): 5.743 s ± 0.065 s [User: 0.002 s, System: 0.001 s]
Range (min … max): 5.684 s … 5.870 s 10 runs

Still noisy, but avoiding to fork out to a comparison program seems worth it, i.e. the shell function wins for the typically short inputs in test scripts. Do you get different numbers?

I'm a bit disappointed by the performance of the patch below, but we'd need something like this to compare precisely (no longer ignoring CRs), I suppose.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

index 796093a7b3..bf10746a08 100644
@@ -1450,70 +1450,21 @@ test_skip_or_die () {
 	error "$2"
 }
 
-# The following mingw_* functions obey POSIX shell syntax, but are actually
-# bash scripts, and are meant to be used only with bash on Windows.
-
-# A test_cmp function that treats LF and CRLF equal and avoids to fork
-# diff when possible.
+# A test_cmp function that avoids to fork diff when possible. It's only
+# meant to be used with bash on Windows.
 mingw_test_cmp () {
 	# Read text into shell variables and compare them. If the results
 	# are different, use regular diff to report the difference.
-	local test_cmp_a= test_cmp_b=
-
-	# When text came from stdin (one argument is '-') we must feed it
-	# to diff.
-	local stdin_for_diff=
+	local test_cmp_a=a test_cmp_b=b
 
-	# Since it is difficult to detect the difference between an
-	# empty input file and a failure to read the files, we go straight
-	# to diff if one of the inputs is empty.
-	if test -s "$1" && test -s "$2"
-	then
-		# regular case: both files non-empty
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-	elif test -s "$1" && test "$2" = -
-	then
-		# read 2nd file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a <"$1"
-		mingw_read_file_strip_cr_ test_cmp_b
-		stdin_for_diff='<<<"$test_cmp_b"'
-	elif test "$1" = - && test -s "$2"
+	# Leave the uncommon case of reading from stdin to diff.
+	if test "$1" != "-" && test "$2" != "-"
 	then
-		# read 1st file from stdin
-		mingw_read_file_strip_cr_ test_cmp_a
-		mingw_read_file_strip_cr_ test_cmp_b <"$2"
-		stdin_for_diff='<<<"$test_cmp_a"'
+		IFS= read -r -d '' test_cmp_a <"$1"
+		IFS= read -r -d '' test_cmp_b <"$2"
 	fi
-	test -n "$test_cmp_a" &&
-	test -n "$test_cmp_b" &&
 	test "$test_cmp_a" = "$test_cmp_b" ||
-	eval "diff -u \"\$@\" $stdin_for_diff"
-}
-
-# $1 is the name of the shell variable to fill in
-mingw_read_file_strip_cr_ () {
-	# Read line-wise using LF as the line separator
-	# and use IFS to strip CR.
-	local line
-	while :
-	do
-		if IFS=$'\r' read -r -d $'\n' line
-		then
-			# good
-			line=$line$'\n'
-		else
-			# we get here at EOF, but also if the last line
-			# was not terminated by LF; in the latter case,
-			# some text was read
-			if test -z "$line"
-			then
-				# EOF, really
-				break
-			fi
-		fi
-		eval "$1=\$$1\$line"
-	done
+	diff -u "$@"
 }
 
 # Like "env FOO=BAR some-program", but run inside a subshell, which means

This is on js/t3920-shell-and-or-fix topic and in 'next'.

I forgot to pick this up, which I just did.

Hi Johannes,

Thanks for this update. I think these five paragraphs are an accurate and helpful record of the history behind this patch and approach. It is very much appreciated.

Thanks,
Taylor