{"thread":{"id":"58243","subject":"[PATCH] tests: replace mingw_test_cmp with a helper in C","startedAt":"2022-07-29T14:53:08Z","lastAt":"2022-12-07T04:34:18Z","messageCount":58,"participants":["Johannes Schindelin via GitGitGadget","Johannes Schindelin","Junio C Hamano","René Scharfe","Ævar Arnfjörð Bjarmason","Taylor Blau","Phillip Wood","Johannes Sixt","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"460219","messageId":"pull.1309.git.1659106382128.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":null,"subject":"[PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T14:53:01Z","receivedAt":"2022-07-29T14:53:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis helper is more performant than running the `mingw_test_cmp` code\nwith with MSYS2's Bash. And a lot more readable.\n\nTo accommodate t1050, which wants to compare files weighing in with 3MB\n(falling outside of t1050's malloc limit of 1.5MB), we simply lift the\nallocation limit by setting the environment variable GIT_ALLOC_LIMIT to\nzero when calling the helper.\n\nSigned-off-by: Johannes Schindelin \n---\n tests: replace mingw_test_cmp with a helper in C\n \n On the heels of sending a patch to fix a performance regression due to a\n mis-use of test_cmp\n [https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],\n I was curious to see whether Git for Windows had the same issue. And it\n does not\n [https://github.com/git-for-windows/git/runs/7556381815?check_suite_focus=true#step:5:127]:\n it passes t5351 in 22 seconds, even while using test_cmp to compare pack\n files\n [https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].\n \n The answer is of course that a test helper written in C is much faster\n than writing the same in Bash, especially on Windows. This is especially\n sad when said Bash code is only used on Windows. So I pulled out this\n helper from the years-long effort to let Git for Windows use BusyBox'\n ash to run the test suite. The result is this patch, which has been in\n Git for Windows since June 2018.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1309\n\n Makefile | 1 +\n t/helper/test-cmp.c | 73 +++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/test-lib-functions.sh | 68 +-------------------------------------\n t/test-lib.sh | 2 +-\n 6 files changed, 78 insertions(+), 68 deletions(-)\n create mode 100644 t/helper/test-cmp.c\n\ndiff --git a/Makefile b/Makefile\nindex 1624471badc..45f108e43a1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o\n TEST_BUILTINS_OBJS += test-bitmap.o\n TEST_BUILTINS_OBJS += test-bloom.o\n TEST_BUILTINS_OBJS += test-chmtime.o\n+TEST_BUILTINS_OBJS += test-cmp.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\ndiff --git a/t/helper/test-cmp.c b/t/helper/test-cmp.c\nnew file mode 100644\nindex 00000000000..1c646a54bf6\n--- /dev/null\n+++ b/t/helper/test-cmp.c\n@@ -0,0 +1,73 @@\n+#include \"test-tool.h\"\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"gettext.h\"\n+#include \"parse-options.h\"\n+#include \"run-command.h\"\n+\n+#ifdef WIN32\n+#define NO_SUCH_DIR \"\\\\\\\\.\\\\GLOBALROOT\\\\invalid\"\n+#else\n+#define NO_SUCH_DIR \"/dev/null\"\n+#endif\n+\n+static int run_diff(const char *path1, const char *path2)\n+{\n+\tconst char *argv[] = {\n+\t\t\"diff\", \"--no-index\", NULL, NULL, NULL\n+\t};\n+\tconst char *env[] = {\n+\t\t\"GIT_PAGER=cat\",\n+\t\t\"GIT_DIR=\" NO_SUCH_DIR,\n+\t\t\"HOME=\" NO_SUCH_DIR,\n+\t\tNULL\n+\t};\n+\n+\targv[2] = path1;\n+\targv[3] = path2;\n+\treturn run_command_v_opt_cd_env(argv,\n+\t\t\t\t\tRUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n+\t\t\t\t\tNULL, env);\n+}\n+\n+int cmd__cmp(int argc, const char **argv)\n+{\n+\tFILE *f0, *f1;\n+\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n+\n+\tif (argc != 3)\n+\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n+\n+\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n+\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n+\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n+\t\tfclose(f0);\n+\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n+\t}\n+\n+\tfor (;;) {\n+\t\tint r0 = strbuf_getline(&b0, f0);\n+\t\tint r1 = strbuf_getline(&b1, f1);\n+\n+\t\tif (r0 == EOF) {\n+\t\t\tfclose(f0);\n+\t\t\tfclose(f1);\n+\t\t\tstrbuf_release(&b0);\n+\t\t\tstrbuf_release(&b1);\n+\t\t\tif (r1 == EOF)\n+\t\t\t\treturn 0;\n+cmp_failed:\n+\t\t\tif (!run_diff(argv[1], argv[2]))\n+\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n+\t\t\t\t argv[1], argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (r1 == EOF || strbuf_cmp(&b0, &b1)) {\n+\t\t\tfclose(f0);\n+\t\t\tfclose(f1);\n+\t\t\tstrbuf_release(&b0);\n+\t\t\tstrbuf_release(&b1);\n+\t\t\tgoto cmp_failed;\n+\t\t}\n+\t}\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 318fdbab0c3..3334de248a1 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -18,6 +18,7 @@ static struct test_cmd cmds[] = {\n \t{ \"bitmap\", cmd__bitmap },\n \t{ \"bloom\", cmd__bloom },\n \t{ \"chmtime\", cmd__chmtime },\n+\t{ \"cmp\", cmd__cmp },\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n \t{ \"csprng\", cmd__csprng },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex bb799271631..e1104898cc3 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);\n int cmd__bitmap(int argc, const char **argv);\n int cmd__bloom(int argc, const char **argv);\n int cmd__chmtime(int argc, const char **argv);\n+int cmd__cmp(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8c44856eaec..28eddbc8e36 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1240,7 +1240,7 @@ test_expect_code () {\n \n test_cmp () {\n \ttest \"$#\" -ne 2 && BUG \"2 param\"\n-\teval \"$GIT_TEST_CMP\" '\"$@\"'\n+\tGIT_ALLOC_LIMIT=0 eval \"$GIT_TEST_CMP\" '\"$@\"'\n }\n \n # Check that the given config key has the expected value.\n@@ -1541,72 +1541,6 @@ test_skip_or_die () {\n \terror \"$2\"\n }\n \n-# The following mingw_* functions obey POSIX shell syntax, but are actually\n-# bash scripts, and are meant to be used only with bash on Windows.\n-\n-# A test_cmp function that treats LF and CRLF equal and avoids to fork\n-# diff when possible.\n-mingw_test_cmp () {\n-\t# Read text into shell variables and compare them. If the results\n-\t# are different, use regular diff to report the difference.\n-\tlocal test_cmp_a= test_cmp_b=\n-\n-\t# When text came from stdin (one argument is '-') we must feed it\n-\t# to diff.\n-\tlocal stdin_for_diff=\n-\n-\t# Since it is difficult to detect the difference between an\n-\t# empty input file and a failure to read the files, we go straight\n-\t# to diff if one of the inputs is empty.\n-\tif test -s \"$1\" && test -s \"$2\"\n-\tthen\n-\t\t# regular case: both files non-empty\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\telif test -s \"$1\" && test \"$2\" = -\n-\tthen\n-\t\t# read 2nd file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b\n-\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n-\telif test \"$1\" = - && test -s \"$2\"\n-\tthen\n-\t\t# read 1st file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n-\tfi\n-\ttest -n \"$test_cmp_a\" &&\n-\ttest -n \"$test_cmp_b\" &&\n-\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n-\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n-}\n-\n-# $1 is the name of the shell variable to fill in\n-mingw_read_file_strip_cr_ () {\n-\t# Read line-wise using LF as the line separator\n-\t# and use IFS to strip CR.\n-\tlocal line\n-\twhile :\n-\tdo\n-\t\tif IFS=$'\\r' read -r -d $'\\n' line\n-\t\tthen\n-\t\t\t# good\n-\t\t\tline=$line$'\\n'\n-\t\telse\n-\t\t\t# we get here at EOF, but also if the last line\n-\t\t\t# was not terminated by LF; in the latter case,\n-\t\t\t# some text was read\n-\t\t\tif test -z \"$line\"\n-\t\t\tthen\n-\t\t\t\t# EOF, really\n-\t\t\t\tbreak\n-\t\t\tfi\n-\t\tfi\n-\t\teval \"$1=\\$$1\\$line\"\n-\tdone\n-}\n-\n # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n # it also works for shell functions (though those functions cannot impact\n # the environment outside of the test_env invocation).\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7726d1da88a..220c259e796 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1546,7 +1546,7 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n+\tGIT_TEST_CMP=\"test-tool cmp\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\n-- \ngitgitgadget\n"},{"id":"460220","messageId":"06n1s2n7-o827-0244-50s2-rrq50s42oqn5@tzk.qr","threadId":"58243","inReplyTo":"pull.1309.git.1659106382128.gitgitgadget@gmail.com","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-07-29T14:54:47Z","receivedAt":"2022-07-29T14:54:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 29 Jul 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> This helper is more performant than running the `mingw_test_cmp` code\n> with with MSYS2's Bash. And a lot more readable.\n\nOoops... s/with with/with/\n\nCiao,\nDscho\n\n>\n> To accommodate t1050, which wants to compare files weighing in with 3MB\n> (falling outside of t1050's malloc limit of 1.5MB), we simply lift the\n> allocation limit by setting the environment variable GIT_ALLOC_LIMIT to\n> zero when calling the helper.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> tests: replace mingw_test_cmp with a helper in C\n>\n> On the heels of sending a patch to fix a performance regression due to a\n> mis-use of test_cmp\n> [https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],\n> I was curious to see whether Git for Windows had the same issue. And it\n> does not\n> [https://github.com/git-for-windows/git/runs/7556381815?check_suite_focus=true#step:5:127]:\n> it passes t5351 in 22 seconds, even while using test_cmp to compare pack\n> files\n> [https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].\n>\n> The answer is of course that a test helper written in C is much faster\n> than writing the same in Bash, especially on Windows. This is especially\n> sad when said Bash code is only used on Windows. So I pulled out this\n> helper from the years-long effort to let Git for Windows use BusyBox'\n> ash to run the test suite. The result is this patch, which has been in\n> Git for Windows since June 2018.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1309\n>\n> Makefile | 1 +\n> t/helper/test-cmp.c | 73 +++++++++++++++++++++++++++++++++++++++++\n> t/helper/test-tool.c | 1 +\n> t/helper/test-tool.h | 1 +\n> t/test-lib-functions.sh | 68 +-------------------------------------\n> t/test-lib.sh | 2 +-\n> 6 files changed, 78 insertions(+), 68 deletions(-)\n> create mode 100644 t/helper/test-cmp.c\n>\n> diff --git a/Makefile b/Makefile\n> index 1624471badc..45f108e43a1 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o\n> TEST_BUILTINS_OBJS += test-bitmap.o\n> TEST_BUILTINS_OBJS += test-bloom.o\n> TEST_BUILTINS_OBJS += test-chmtime.o\n> +TEST_BUILTINS_OBJS += test-cmp.o\n> TEST_BUILTINS_OBJS += test-config.o\n> TEST_BUILTINS_OBJS += test-crontab.o\n> TEST_BUILTINS_OBJS += test-csprng.o\n> diff --git a/t/helper/test-cmp.c b/t/helper/test-cmp.c\n> new file mode 100644\n> index 00000000000..1c646a54bf6\n> --- /dev/null\n> +++ b/t/helper/test-cmp.c\n> @@ -0,0 +1,73 @@\n> +#include \"test-tool.h\"\n> +#include \"git-compat-util.h\"\n> +#include \"strbuf.h\"\n> +#include \"gettext.h\"\n> +#include \"parse-options.h\"\n> +#include \"run-command.h\"\n> +\n> +#ifdef WIN32\n> +#define NO_SUCH_DIR \"\\\\\\\\.\\\\GLOBALROOT\\\\invalid\"\n> +#else\n> +#define NO_SUCH_DIR \"/dev/null\"\n> +#endif\n> +\n> +static int run_diff(const char *path1, const char *path2)\n> +{\n> +\tconst char *argv[] = {\n> +\t\t\"diff\", \"--no-index\", NULL, NULL, NULL\n> +\t};\n> +\tconst char *env[] = {\n> +\t\t\"GIT_PAGER=cat\",\n> +\t\t\"GIT_DIR=\" NO_SUCH_DIR,\n> +\t\t\"HOME=\" NO_SUCH_DIR,\n> +\t\tNULL\n> +\t};\n> +\n> +\targv[2] = path1;\n> +\targv[3] = path2;\n> +\treturn run_command_v_opt_cd_env(argv,\n> +\t\t\t\t\tRUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n> +\t\t\t\t\tNULL, env);\n> +}\n> +\n> +int cmd__cmp(int argc, const char **argv)\n> +{\n> +\tFILE *f0, *f1;\n> +\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n> +\n> +\tif (argc != 3)\n> +\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n> +\n> +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n> +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n> +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n> +\t\tfclose(f0);\n> +\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n> +\t}\n> +\n> +\tfor (;;) {\n> +\t\tint r0 = strbuf_getline(&b0, f0);\n> +\t\tint r1 = strbuf_getline(&b1, f1);\n> +\n> +\t\tif (r0 == EOF) {\n> +\t\t\tfclose(f0);\n> +\t\t\tfclose(f1);\n> +\t\t\tstrbuf_release(&b0);\n> +\t\t\tstrbuf_release(&b1);\n> +\t\t\tif (r1 == EOF)\n> +\t\t\t\treturn 0;\n> +cmp_failed:\n> +\t\t\tif (!run_diff(argv[1], argv[2]))\n> +\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n> +\t\t\t\t argv[1], argv[2]);\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t\tif (r1 == EOF || strbuf_cmp(&b0, &b1)) {\n> +\t\t\tfclose(f0);\n> +\t\t\tfclose(f1);\n> +\t\t\tstrbuf_release(&b0);\n> +\t\t\tstrbuf_release(&b1);\n> +\t\t\tgoto cmp_failed;\n> +\t\t}\n> +\t}\n> +}\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 318fdbab0c3..3334de248a1 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -18,6 +18,7 @@ static struct test_cmd cmds[] = {\n> \t{ \"bitmap\", cmd__bitmap },\n> \t{ \"bloom\", cmd__bloom },\n> \t{ \"chmtime\", cmd__chmtime },\n> +\t{ \"cmp\", cmd__cmp },\n> \t{ \"config\", cmd__config },\n> \t{ \"crontab\", cmd__crontab },\n> \t{ \"csprng\", cmd__csprng },\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index bb799271631..e1104898cc3 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);\n> int cmd__bitmap(int argc, const char **argv);\n> int cmd__bloom(int argc, const char **argv);\n> int cmd__chmtime(int argc, const char **argv);\n> +int cmd__cmp(int argc, const char **argv);\n> int cmd__config(int argc, const char **argv);\n> int cmd__crontab(int argc, const char **argv);\n> int cmd__csprng(int argc, const char **argv);\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 8c44856eaec..28eddbc8e36 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1240,7 +1240,7 @@ test_expect_code () {\n>\n> test_cmp () {\n> \ttest \"$#\" -ne 2 && BUG \"2 param\"\n> -\teval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\tGIT_ALLOC_LIMIT=0 eval \"$GIT_TEST_CMP\" '\"$@\"'\n> }\n>\n> # Check that the given config key has the expected value.\n> @@ -1541,72 +1541,6 @@ test_skip_or_die () {\n> \terror \"$2\"\n> }\n>\n> -# The following mingw_* functions obey POSIX shell syntax, but are actually\n> -# bash scripts, and are meant to be used only with bash on Windows.\n> -\n> -# A test_cmp function that treats LF and CRLF equal and avoids to fork\n> -# diff when possible.\n> -mingw_test_cmp () {\n> -\t# Read text into shell variables and compare them. If the results\n> -\t# are different, use regular diff to report the difference.\n> -\tlocal test_cmp_a= test_cmp_b=\n> -\n> -\t# When text came from stdin (one argument is '-') we must feed it\n> -\t# to diff.\n> -\tlocal stdin_for_diff=\n> -\n> -\t# Since it is difficult to detect the difference between an\n> -\t# empty input file and a failure to read the files, we go straight\n> -\t# to diff if one of the inputs is empty.\n> -\tif test -s \"$1\" && test -s \"$2\"\n> -\tthen\n> -\t\t# regular case: both files non-empty\n> -\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n> -\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n> -\telif test -s \"$1\" && test \"$2\" = -\n> -\tthen\n> -\t\t# read 2nd file from stdin\n> -\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n> -\t\tmingw_read_file_strip_cr_ test_cmp_b\n> -\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n> -\telif test \"$1\" = - && test -s \"$2\"\n> -\tthen\n> -\t\t# read 1st file from stdin\n> -\t\tmingw_read_file_strip_cr_ test_cmp_a\n> -\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n> -\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n> -\tfi\n> -\ttest -n \"$test_cmp_a\" &&\n> -\ttest -n \"$test_cmp_b\" &&\n> -\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n> -\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n> -}\n> -\n> -# $1 is the name of the shell variable to fill in\n> -mingw_read_file_strip_cr_ () {\n> -\t# Read line-wise using LF as the line separator\n> -\t# and use IFS to strip CR.\n> -\tlocal line\n> -\twhile :\n> -\tdo\n> -\t\tif IFS=$'\\r' read -r -d $'\\n' line\n> -\t\tthen\n> -\t\t\t# good\n> -\t\t\tline=$line$'\\n'\n> -\t\telse\n> -\t\t\t# we get here at EOF, but also if the last line\n> -\t\t\t# was not terminated by LF; in the latter case,\n> -\t\t\t# some text was read\n> -\t\t\tif test -z \"$line\"\n> -\t\t\tthen\n> -\t\t\t\t# EOF, really\n> -\t\t\t\tbreak\n> -\t\t\tfi\n> -\t\tfi\n> -\t\teval \"$1=\\$$1\\$line\"\n> -\tdone\n> -}\n> -\n> # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n> # it also works for shell functions (though those functions cannot impact\n> # the environment outside of the test_env invocation).\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..220c259e796 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1546,7 +1546,7 @@ case $uname_s in\n> \ttest_set_prereq SED_STRIPS_CR\n> \ttest_set_prereq GREP_STRIPS_CR\n> \ttest_set_prereq WINDOWS\n> -\tGIT_TEST_CMP=mingw_test_cmp\n> +\tGIT_TEST_CMP=\"test-tool cmp\"\n> \t;;\n> *CYGWIN*)\n> \ttest_set_prereq POSIXPERM\n>\n> base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\n> --\n> gitgitgadget\n>\n>\n"},{"id":"460233","messageId":"xmqqwnbv7trp.fsf@gitster.g","threadId":"58243","inReplyTo":"pull.1309.git.1659106382128.gitgitgadget@gmail.com","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-29T16:44:58Z","receivedAt":"2022-07-29T16:45:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +\tconst char *argv[] = {\n> +\t\t\"diff\", \"--no-index\", NULL, NULL, NULL\n> +\t};\n\nDon't we want to have \"--\" before the two paths?\n\n> +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n> +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n> +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n> +\t\tfclose(f0);\n> +\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n> +\t}\n\nIt is tricky that you need to take \"-\" and treat it as the standard\ninput stream in either argv[1] or argv[2] (but not both). If would\nbe a different story in an end-user facing program, but because this\nis a test helper, feeding wrong input is developer's fault, and I do\nnot mind lack of attention to detail of error checking to make sure\nwe avoid comparing alternating lines of the standard input.\n\n> +\tfor (;;) {\n> +\t\tint r0 = strbuf_getline(&b0, f0);\n> +\t\tint r1 = strbuf_getline(&b1, f1);\n> +\n> +\t\tif (r0 == EOF) {\n> +\t\t\tfclose(f0);\n> +\t\t\tfclose(f1);\n> +\t\t\tstrbuf_release(&b0);\n> +\t\t\tstrbuf_release(&b1);\n> +\t\t\tif (r1 == EOF)\n> +\t\t\t\treturn 0;\n\nIf both hit the EOF at the same time, we know they are the same, OK.\n\n> +cmp_failed:\n> +\t\t\tif (!run_diff(argv[1], argv[2]))\n\nIf one of argv[] was \"-\", then this wouldn't work correctly, as the\nother file is read from the beginning but the \"-\" side have consumed\nthe initial part of the input and we cannot unseek it. This bug\nneeds to be fixed only if we expect a useful and reliable output\nfrom the helper.\n\nBut otherwise the idea is sound. We compare them line by line,\nusing strbuf_getline() to ignore differences in CRLF and LF that\noriginates at 4d715ac0 (Windows: a test_cmp that is agnostic to\nrandom LF <> CRLF conversions, 2013-10-26). Only when we find the\ninput different, we use \"git diff --no-index\" to make the difference\n(and unfortunately more, as it does not ignore CRLF <> LF\ndifferences) visible.\n\n> +\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n> +\t\t\t\t argv[1], argv[2]);\n\nNice attention to (possibly irrelevant) detail here. I would have\nignored the return value and reported \"they are different\" at this\npoint, though. The line-by-line comparison we did was the\nauthoritative one, and \"git diff --no-index\" is merely used for\nhuman readable output.\n\nIn any case, \"test-tool mingwcmp\" would be a better name that\nhighlights the spirit of 4d715ac0 to ignore CRLF <> LF issues. IOW,\nit does a lot more than \"cmp\" replacement, and we shouldn't mislead\nusers/developers into thinking it is a plain \"cmp\" replacement.\n\nThanks.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..220c259e796 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1546,7 +1546,7 @@ case $uname_s in\n> \ttest_set_prereq SED_STRIPS_CR\n> \ttest_set_prereq GREP_STRIPS_CR\n> \ttest_set_prereq WINDOWS\n> -\tGIT_TEST_CMP=mingw_test_cmp\n> +\tGIT_TEST_CMP=\"test-tool cmp\"\n> \t;;\n> *CYGWIN*)\n> \ttest_set_prereq POSIXPERM\n>\n> base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\n"},{"id":"462649","messageId":"354qp59q-r4r3-1971-5o09-71q224911orp@tzk.qr","threadId":"58243","inReplyTo":"xmqqwnbv7trp.fsf@gitster.g","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-06T13:10:29Z","receivedAt":"2022-09-06T13:10:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Jul 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +\tconst char *argv[] = {\n> > +\t\t\"diff\", \"--no-index\", NULL, NULL, NULL\n> > +\t};\n>\n> Don't we want to have \"--\" before the two paths?\n\nYes!\n\n> > +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n> > +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n> > +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n> > +\t\tfclose(f0);\n> > +\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n> > +\t}\n>\n> It is tricky that you need to take \"-\" and treat it as the standard\n> input stream in either argv[1] or argv[2] (but not both). If would\n> be a different story in an end-user facing program, but because this\n> is a test helper, feeding wrong input is developer's fault, and I do\n> not mind lack of attention to detail of error checking to make sure\n> we avoid comparing alternating lines of the standard input.\n\nNo, you're right, I've added a guard that prevents `test-tool cmp - -`\nfrom failing in obscure ways.\n\n> > +\tfor (;;) {\n> > +\t\tint r0 = strbuf_getline(&b0, f0);\n> > +\t\tint r1 = strbuf_getline(&b1, f1);\n> > +\n> > +\t\tif (r0 == EOF) {\n> > +\t\t\tfclose(f0);\n> > +\t\t\tfclose(f1);\n> > +\t\t\tstrbuf_release(&b0);\n> > +\t\t\tstrbuf_release(&b1);\n> > +\t\t\tif (r1 == EOF)\n> > +\t\t\t\treturn 0;\n>\n> If both hit the EOF at the same time, we know they are the same, OK.\n>\n> > +cmp_failed:\n> > +\t\t\tif (!run_diff(argv[1], argv[2]))\n>\n> If one of argv[] was \"-\", then this wouldn't work correctly, as the\n> other file is read from the beginning but the \"-\" side have consumed\n> the initial part of the input and we cannot unseek it. This bug\n> needs to be fixed only if we expect a useful and reliable output\n> from the helper.\n\nRight. I've added a clause that says that we cannot show the diff because\n`stdin` has been consumed already.\n\n> But otherwise the idea is sound. We compare them line by line,\n> using strbuf_getline() to ignore differences in CRLF and LF that\n> originates at 4d715ac0 (Windows: a test_cmp that is agnostic to\n> random LF <> CRLF conversions, 2013-10-26). Only when we find the\n> input different, we use \"git diff --no-index\" to make the difference\n> (and unfortunately more, as it does not ignore CRLF <> LF\n> differences) visible.\n>\n> > +\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n> > +\t\t\t\t argv[1], argv[2]);\n>\n> Nice attention to (possibly irrelevant) detail here. I would have\n> ignored the return value and reported \"they are different\" at this\n> point, though. The line-by-line comparison we did was the\n> authoritative one, and \"git diff --no-index\" is merely used for\n> human readable output.\n>\n> In any case, \"test-tool mingwcmp\" would be a better name that\n> highlights the spirit of 4d715ac0 to ignore CRLF <> LF issues. IOW,\n> it does a lot more than \"cmp\" replacement, and we shouldn't mislead\n> users/developers into thinking it is a plain \"cmp\" replacement.\n\nFair point. The Unix tool `cmp` does not care about line endings at all,\nso when you come from a Unix background you will expect the same to be\ntrue for `test-tool cmp`.\n\nOn the other hand, you will expect the same to be true for `test_cmp`,\ntoo, which is not the case, and the root cause of why I had to come up\nwith 32ed3314c10 (t5351: avoid using `test_cmp` for binary data,\n2022-07-29).\n\nHaving said that, I agree that the test tool name should reflect better\nwhat the subcommand does.\n\nI do dislike the proposed name `mingwcmp`. Not only because it is\nmisleading, as the purpose is not to compare MINGW-specific files but\ninstead the purpose is to compare text files (and, in fact, the tool works\njust fine on Linux and macOS, too). But also because it would contribute\nto just how much of a second-class citizen the MINGW-based build is in Git\nland: From choosing to implement large parts, including the entire test\nsuite as well as the performance benchmarks, in POSIX scripts (which plays\nto Windows' weaknesses in a big way) to massively favoring spawned\nprocesses over multi-threading (which plays to Linux' strengths and to\nWindows' weaknesses), to a still-inherent assumption that the underlying\nfilesystem is case-sensitive (think: branch names), to an implicit\nagreement in the core Git community that patch contributions need not take\ncare of working well on Windows (but that that's the job \"of Windows folk\"\ninstead). This is kind of at odds with the fact that we must assume that\nhalf of Git's users are Windows-based (we can only assume, based on\nsurveys, because we successfully avoid any kind of even opt-in telemetry\nthat would give us hard data). I definitely want to stay away from making\nthat second-citizenry even worse.\n\nSo I am going with the name `test-tool text-cmp` instead.\n\nThank you for your review,\nDscho\n\n>\n> Thanks.\n>\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 7726d1da88a..220c259e796 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -1546,7 +1546,7 @@ case $uname_s in\n> > \ttest_set_prereq SED_STRIPS_CR\n> > \ttest_set_prereq GREP_STRIPS_CR\n> > \ttest_set_prereq WINDOWS\n> > -\tGIT_TEST_CMP=mingw_test_cmp\n> > +\tGIT_TEST_CMP=\"test-tool cmp\"\n> > \t;;\n> > *CYGWIN*)\n> > \ttest_set_prereq POSIXPERM\n> >\n> > base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\n>\n"},{"id":"462650","messageId":"pull.1309.v2.git.1662469859.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.git.1659106382128.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-06T13:10:56Z","receivedAt":"2022-09-06T13:11:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On the heels of sending a patch to fix a performance regression due to a\nmis-use of test_cmp\n[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],\nI was curious to see whether Git for Windows had the same issue. And it does\nnot\n[https://github.com/git-for-windows/git/runs/7556381815?check_suite_focus=true#step:5:127]:\nit passes t5351 in 22 seconds, even while using test_cmp to compare pack\nfiles\n[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].\n\nThe answer is of course that a test helper written in C is much faster than\nwriting the same in Bash, especially on Windows. This is especially sad when\nsaid Bash code is only used on Windows. So I pulled out this helper from the\nyears-long effort to let Git for Windows use BusyBox' ash to run the test\nsuite. The result is this patch, which has been in Git for Windows since\nJune 2018.\n\nChanges since v1:\n\n * Fixed double \"with\" in the commit message.\n\nJohannes Schindelin (2):\n t0021: use Windows-friendly `pwd`\n tests: replace mingw_test_cmp with a helper in C\n\n Makefile | 1 +\n t/helper/test-text-cmp.c | 78 ++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0021-conversion.sh | 4 +--\n t/test-lib-functions.sh | 68 +----------------------------------\n t/test-lib.sh | 2 +-\n 7 files changed, 85 insertions(+), 70 deletions(-)\n create mode 100644 t/helper/test-text-cmp.c\n\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1309\n\nRange-diff vs v1:\n\n -: ----------- > 1: ad7c41401ee t0021: use Windows-friendly `pwd`\n 1: eacbbc53700 ! 2: 1f5366f1379 tests: replace mingw_test_cmp with a helper in C\n @@ Commit message\n tests: replace mingw_test_cmp with a helper in C\n \n This helper is more performant than running the `mingw_test_cmp` code\n - with with MSYS2's Bash. And a lot more readable.\n + with MSYS2's Bash. And a lot more readable.\n \n To accommodate t1050, which wants to compare files weighing in with 3MB\n (falling outside of t1050's malloc limit of 1.5MB), we simply lift the\n @@ Commit message\n Signed-off-by: Johannes Schindelin \n \n ## Makefile ##\n -@@ Makefile: TEST_BUILTINS_OBJS += test-advise.o\n - TEST_BUILTINS_OBJS += test-bitmap.o\n - TEST_BUILTINS_OBJS += test-bloom.o\n - TEST_BUILTINS_OBJS += test-chmtime.o\n -+TEST_BUILTINS_OBJS += test-cmp.o\n - TEST_BUILTINS_OBJS += test-config.o\n - TEST_BUILTINS_OBJS += test-crontab.o\n - TEST_BUILTINS_OBJS += test-csprng.o\n +@@ Makefile: TEST_BUILTINS_OBJS += test-string-list.o\n + TEST_BUILTINS_OBJS += test-submodule-config.o\n + TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o\n + TEST_BUILTINS_OBJS += test-subprocess.o\n ++TEST_BUILTINS_OBJS += test-text-cmp.o\n + TEST_BUILTINS_OBJS += test-trace2.o\n + TEST_BUILTINS_OBJS += test-urlmatch-normalization.o\n + TEST_BUILTINS_OBJS += test-userdiff.o\n \n - ## t/helper/test-cmp.c (new) ##\n + ## t/helper/test-text-cmp.c (new) ##\n @@\n +#include \"test-tool.h\"\n +#include \"git-compat-util.h\"\n @@ t/helper/test-cmp.c (new)\n +static int run_diff(const char *path1, const char *path2)\n +{\n +\tconst char *argv[] = {\n -+\t\t\"diff\", \"--no-index\", NULL, NULL, NULL\n ++\t\t\"diff\", \"--no-index\", \"--\", NULL, NULL, NULL\n +\t};\n +\tconst char *env[] = {\n +\t\t\"GIT_PAGER=cat\",\n @@ t/helper/test-cmp.c (new)\n +\t\tNULL\n +\t};\n +\n -+\targv[2] = path1;\n -+\targv[3] = path2;\n ++\targv[3] = path1;\n ++\targv[4] = path2;\n +\treturn run_command_v_opt_cd_env(argv,\n +\t\t\t\t\tRUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n +\t\t\t\t\tNULL, env);\n +}\n +\n -+int cmd__cmp(int argc, const char **argv)\n ++int cmd__text_cmp(int argc, const char **argv)\n +{\n +\tFILE *f0, *f1;\n +\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n @@ t/helper/test-cmp.c (new)\n +\tif (argc != 3)\n +\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n +\n ++\tif (!strcmp(argv[1], \"-\") && !strcmp(argv[2], \"-\"))\n ++\t\tdie(\"only one parameter can refer to `stdin` but not both\");\n ++\n +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n @@ t/helper/test-cmp.c (new)\n +\t\t\tif (r1 == EOF)\n +\t\t\t\treturn 0;\n +cmp_failed:\n -+\t\t\tif (!run_diff(argv[1], argv[2]))\n ++\t\t\tif (!strcmp(argv[1], \"-\") || !strcmp(argv[2], \"-\"))\n ++\t\t\t\twarning(\"cannot show diff because `stdin` was already consumed\");\n ++\t\t\telse if (!run_diff(argv[1], argv[2]))\n +\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n +\t\t\t\t argv[1], argv[2]);\n +\t\t\treturn 1;\n @@ t/helper/test-cmp.c (new)\n \n ## t/helper/test-tool.c ##\n @@ t/helper/test-tool.c: static struct test_cmd cmds[] = {\n - \t{ \"bitmap\", cmd__bitmap },\n - \t{ \"bloom\", cmd__bloom },\n - \t{ \"chmtime\", cmd__chmtime },\n -+\t{ \"cmp\", cmd__cmp },\n - \t{ \"config\", cmd__config },\n - \t{ \"crontab\", cmd__crontab },\n - \t{ \"csprng\", cmd__csprng },\n + \t{ \"submodule-config\", cmd__submodule_config },\n + \t{ \"submodule-nested-repo-config\", cmd__submodule_nested_repo_config },\n + \t{ \"subprocess\", cmd__subprocess },\n ++\t{ \"text-cmp\", cmd__text_cmp },\n + \t{ \"trace2\", cmd__trace2 },\n + \t{ \"userdiff\", cmd__userdiff },\n + \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n \n ## t/helper/test-tool.h ##\n -@@ t/helper/test-tool.h: int cmd__advise_if_enabled(int argc, const char **argv);\n - int cmd__bitmap(int argc, const char **argv);\n - int cmd__bloom(int argc, const char **argv);\n - int cmd__chmtime(int argc, const char **argv);\n -+int cmd__cmp(int argc, const char **argv);\n - int cmd__config(int argc, const char **argv);\n - int cmd__crontab(int argc, const char **argv);\n - int cmd__csprng(int argc, const char **argv);\n +@@ t/helper/test-tool.h: int cmd__string_list(int argc, const char **argv);\n + int cmd__submodule_config(int argc, const char **argv);\n + int cmd__submodule_nested_repo_config(int argc, const char **argv);\n + int cmd__subprocess(int argc, const char **argv);\n ++int cmd__text_cmp(int argc, const char **argv);\n + int cmd__trace2(int argc, const char **argv);\n + int cmd__userdiff(int argc, const char **argv);\n + int cmd__urlmatch_normalization(int argc, const char **argv);\n \n ## t/test-lib-functions.sh ##\n @@ t/test-lib-functions.sh: test_expect_code () {\n @@ t/test-lib.sh: case $uname_s in\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n -\tGIT_TEST_CMP=mingw_test_cmp\n -+\tGIT_TEST_CMP=\"test-tool cmp\"\n ++\tGIT_TEST_CMP=\"test-tool text-cmp\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n\n-- \ngitgitgadget\n"},{"id":"462651","messageId":"ad7c41401ee942152b525de27b809b6458f2159a.1662469859.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v2.git.1662469859.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t0021: use Windows-friendly `pwd`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-06T13:10:57Z","receivedAt":"2022-09-06T13:11:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows, when passing paths from shell scripts to regular\nWin32 executables, thanks to the MSYS2 runtime a somewhat magic path\nconversion happens that lets the shell script think that there is a file\nat `/git/Makefile` and the Win32 process it spawned thinks that the\nshell script said `C:/git-sdk-64/git/Makefile` instead.\n\nThis conversion is documented in detail over here:\nhttps://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion\n\nAs all automatic conversions, there are gaps. For example, to avoid\nmistaking command-line options like `/LOG=log.txt` (which are quite\ncommon in the Windows world) from being mistaken for a Unix-style\nabsolute path, the MSYS2 runtime specifically exempts arguments\ncontaining a `=` character from that conversion.\n\nWe are about to change `test_cmp` to use a test helper, which is such a\nWin32 process.\n\nIn combination, this would cause a failure in `t0021-conversion.sh`\nwhere we pass an absolute path containing an equal character to the\n`test_cmp` function.\n\nSeeing as the Unix tools like `cp` and `diff` that are used by Git's\ntest suite in the Git for Windows SDK (thanks to the MSYS2 project)\nunderstand both Unix-style as well as Windows-style paths, we can stave\noff this problem by simply switching to Windows-style paths and\nside-stepping the need for any automatic path conversion.\n\nNote: The `PATH` variable is obviously special, as it is colon-separated\nin the MSYS2 Bash used by Git for Windows, and therefore _cannot_\ncontain absolute Windows-style paths, lest the colon after the drive\nletter is mistaken for a path separator. Therefore, we need to be\ncareful to keep the Unix-style when modifying the `PATH` variable.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0021-conversion.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bad37abad2c..15482fa78e3 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-TEST_ROOT=\"$PWD\"\n-PATH=$TEST_ROOT:$PATH\n+PATH=$PWD:$PATH\n+TEST_ROOT=\"$(pwd)\"\n \n write_script <<\\EOF \"$TEST_ROOT/rot13.sh\"\n tr \\\n-- \ngitgitgadget\n\n"},{"id":"462652","messageId":"1f5366f137967cbec30041b40eedd86ce5f6e953.1662469859.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v2.git.1662469859.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-06T13:10:58Z","receivedAt":"2022-09-06T13:11:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis helper is more performant than running the `mingw_test_cmp` code\nwith MSYS2's Bash. And a lot more readable.\n\nTo accommodate t1050, which wants to compare files weighing in with 3MB\n(falling outside of t1050's malloc limit of 1.5MB), we simply lift the\nallocation limit by setting the environment variable GIT_ALLOC_LIMIT to\nzero when calling the helper.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 1 +\n t/helper/test-text-cmp.c | 78 ++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/test-lib-functions.sh | 68 +----------------------------------\n t/test-lib.sh | 2 +-\n 6 files changed, 83 insertions(+), 68 deletions(-)\n create mode 100644 t/helper/test-text-cmp.c\n\ndiff --git a/Makefile b/Makefile\nindex 1624471badc..73db55bba0f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -786,6 +786,7 @@ TEST_BUILTINS_OBJS += test-string-list.o\n TEST_BUILTINS_OBJS += test-submodule-config.o\n TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o\n TEST_BUILTINS_OBJS += test-subprocess.o\n+TEST_BUILTINS_OBJS += test-text-cmp.o\n TEST_BUILTINS_OBJS += test-trace2.o\n TEST_BUILTINS_OBJS += test-urlmatch-normalization.o\n TEST_BUILTINS_OBJS += test-userdiff.o\ndiff --git a/t/helper/test-text-cmp.c b/t/helper/test-text-cmp.c\nnew file mode 100644\nindex 00000000000..7c26d925086\n--- /dev/null\n+++ b/t/helper/test-text-cmp.c\n@@ -0,0 +1,78 @@\n+#include \"test-tool.h\"\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"gettext.h\"\n+#include \"parse-options.h\"\n+#include \"run-command.h\"\n+\n+#ifdef WIN32\n+#define NO_SUCH_DIR \"\\\\\\\\.\\\\GLOBALROOT\\\\invalid\"\n+#else\n+#define NO_SUCH_DIR \"/dev/null\"\n+#endif\n+\n+static int run_diff(const char *path1, const char *path2)\n+{\n+\tconst char *argv[] = {\n+\t\t\"diff\", \"--no-index\", \"--\", NULL, NULL, NULL\n+\t};\n+\tconst char *env[] = {\n+\t\t\"GIT_PAGER=cat\",\n+\t\t\"GIT_DIR=\" NO_SUCH_DIR,\n+\t\t\"HOME=\" NO_SUCH_DIR,\n+\t\tNULL\n+\t};\n+\n+\targv[3] = path1;\n+\targv[4] = path2;\n+\treturn run_command_v_opt_cd_env(argv,\n+\t\t\t\t\tRUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n+\t\t\t\t\tNULL, env);\n+}\n+\n+int cmd__text_cmp(int argc, const char **argv)\n+{\n+\tFILE *f0, *f1;\n+\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n+\n+\tif (argc != 3)\n+\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n+\n+\tif (!strcmp(argv[1], \"-\") && !strcmp(argv[2], \"-\"))\n+\t\tdie(\"only one parameter can refer to `stdin` but not both\");\n+\n+\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n+\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n+\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n+\t\tfclose(f0);\n+\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n+\t}\n+\n+\tfor (;;) {\n+\t\tint r0 = strbuf_getline(&b0, f0);\n+\t\tint r1 = strbuf_getline(&b1, f1);\n+\n+\t\tif (r0 == EOF) {\n+\t\t\tfclose(f0);\n+\t\t\tfclose(f1);\n+\t\t\tstrbuf_release(&b0);\n+\t\t\tstrbuf_release(&b1);\n+\t\t\tif (r1 == EOF)\n+\t\t\t\treturn 0;\n+cmp_failed:\n+\t\t\tif (!strcmp(argv[1], \"-\") || !strcmp(argv[2], \"-\"))\n+\t\t\t\twarning(\"cannot show diff because `stdin` was already consumed\");\n+\t\t\telse if (!run_diff(argv[1], argv[2]))\n+\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n+\t\t\t\t argv[1], argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (r1 == EOF || strbuf_cmp(&b0, &b1)) {\n+\t\t\tfclose(f0);\n+\t\t\tfclose(f1);\n+\t\t\tstrbuf_release(&b0);\n+\t\t\tstrbuf_release(&b1);\n+\t\t\tgoto cmp_failed;\n+\t\t}\n+\t}\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 318fdbab0c3..c6654ebc48b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -81,6 +81,7 @@ static struct test_cmd cmds[] = {\n \t{ \"submodule-config\", cmd__submodule_config },\n \t{ \"submodule-nested-repo-config\", cmd__submodule_nested_repo_config },\n \t{ \"subprocess\", cmd__subprocess },\n+\t{ \"text-cmp\", cmd__text_cmp },\n \t{ \"trace2\", cmd__trace2 },\n \t{ \"userdiff\", cmd__userdiff },\n \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex bb799271631..2acfd2bcabc 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -71,6 +71,7 @@ int cmd__string_list(int argc, const char **argv);\n int cmd__submodule_config(int argc, const char **argv);\n int cmd__submodule_nested_repo_config(int argc, const char **argv);\n int cmd__subprocess(int argc, const char **argv);\n+int cmd__text_cmp(int argc, const char **argv);\n int cmd__trace2(int argc, const char **argv);\n int cmd__userdiff(int argc, const char **argv);\n int cmd__urlmatch_normalization(int argc, const char **argv);\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8c44856eaec..28eddbc8e36 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1240,7 +1240,7 @@ test_expect_code () {\n \n test_cmp () {\n \ttest \"$#\" -ne 2 && BUG \"2 param\"\n-\teval \"$GIT_TEST_CMP\" '\"$@\"'\n+\tGIT_ALLOC_LIMIT=0 eval \"$GIT_TEST_CMP\" '\"$@\"'\n }\n \n # Check that the given config key has the expected value.\n@@ -1541,72 +1541,6 @@ test_skip_or_die () {\n \terror \"$2\"\n }\n \n-# The following mingw_* functions obey POSIX shell syntax, but are actually\n-# bash scripts, and are meant to be used only with bash on Windows.\n-\n-# A test_cmp function that treats LF and CRLF equal and avoids to fork\n-# diff when possible.\n-mingw_test_cmp () {\n-\t# Read text into shell variables and compare them. If the results\n-\t# are different, use regular diff to report the difference.\n-\tlocal test_cmp_a= test_cmp_b=\n-\n-\t# When text came from stdin (one argument is '-') we must feed it\n-\t# to diff.\n-\tlocal stdin_for_diff=\n-\n-\t# Since it is difficult to detect the difference between an\n-\t# empty input file and a failure to read the files, we go straight\n-\t# to diff if one of the inputs is empty.\n-\tif test -s \"$1\" && test -s \"$2\"\n-\tthen\n-\t\t# regular case: both files non-empty\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\telif test -s \"$1\" && test \"$2\" = -\n-\tthen\n-\t\t# read 2nd file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b\n-\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n-\telif test \"$1\" = - && test -s \"$2\"\n-\tthen\n-\t\t# read 1st file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n-\tfi\n-\ttest -n \"$test_cmp_a\" &&\n-\ttest -n \"$test_cmp_b\" &&\n-\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n-\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n-}\n-\n-# $1 is the name of the shell variable to fill in\n-mingw_read_file_strip_cr_ () {\n-\t# Read line-wise using LF as the line separator\n-\t# and use IFS to strip CR.\n-\tlocal line\n-\twhile :\n-\tdo\n-\t\tif IFS=$'\\r' read -r -d $'\\n' line\n-\t\tthen\n-\t\t\t# good\n-\t\t\tline=$line$'\\n'\n-\t\telse\n-\t\t\t# we get here at EOF, but also if the last line\n-\t\t\t# was not terminated by LF; in the latter case,\n-\t\t\t# some text was read\n-\t\t\tif test -z \"$line\"\n-\t\t\tthen\n-\t\t\t\t# EOF, really\n-\t\t\t\tbreak\n-\t\t\tfi\n-\t\tfi\n-\t\teval \"$1=\\$$1\\$line\"\n-\tdone\n-}\n-\n # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n # it also works for shell functions (though those functions cannot impact\n # the environment outside of the test_env invocation).\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7726d1da88a..0be25ecbd59 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1546,7 +1546,7 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n+\tGIT_TEST_CMP=\"test-tool text-cmp\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n-- \ngitgitgadget\n"},{"id":"462694","messageId":"s8625r17-6p9q-n28o-31sr-o90ps09948rs@tzk.qr","threadId":"58243","inReplyTo":"pull.1309.v2.git.1662469859.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-07T09:04:56Z","receivedAt":"2022-09-07T09:05:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 6 Sep 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> Changes since v1:\n>\n> * Fixed double \"with\" in the commit message.\n\nWhoops, I totally forgot to mention these two things:\n\n\n- Renamed the test helper to `text-cmp`.\n- Made the `diff --no-index` call more robust by using a double-dash\n separator.\n\nCiao,\nDscho\n"},{"id":"462707","messageId":"b21d2b60-428f-58ec-28b6-3c617b9f2e45@web.de","threadId":"58243","inReplyTo":"354qp59q-r4r3-1971-5o09-71q224911orp@tzk.qr","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T12:09:27Z","receivedAt":"2022-09-07T12:09:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.09.22 um 15:10 schrieb Johannes Schindelin:\n> Hi Junio,\n>\n> On Fri, 29 Jul 2022, Junio C Hamano wrote:\n>\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>\n>>> +\tconst char *argv[] = {\n>>> +\t\t\"diff\", \"--no-index\", NULL, NULL, NULL\n>>> +\t};\n>>\n>> Don't we want to have \"--\" before the two paths?\n>\n> Yes!\n>\n>>> +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n>>> +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n>>> +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n>>> +\t\tfclose(f0);\n>>> +\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n>>> +\t}\n>>\n>> It is tricky that you need to take \"-\" and treat it as the standard\n>> input stream in either argv[1] or argv[2] (but not both). If would\n>> be a different story in an end-user facing program, but because this\n>> is a test helper, feeding wrong input is developer's fault, and I do\n>> not mind lack of attention to detail of error checking to make sure\n>> we avoid comparing alternating lines of the standard input.\n\n\"git diff --no-index - -\" also doesn't complain, by the way.\n\n> No, you're right, I've added a guard that prevents `test-tool cmp - -`\n> from failing in obscure ways.\n>\n>>> +\tfor (;;) {\n>>> +\t\tint r0 = strbuf_getline(&b0, f0);\n>>> +\t\tint r1 = strbuf_getline(&b1, f1);\n>>> +\n>>> +\t\tif (r0 == EOF) {\n>>> +\t\t\tfclose(f0);\n>>> +\t\t\tfclose(f1);\n>>> +\t\t\tstrbuf_release(&b0);\n>>> +\t\t\tstrbuf_release(&b1);\n>>> +\t\t\tif (r1 == EOF)\n>>> +\t\t\t\treturn 0;\n>>\n>> If both hit the EOF at the same time, we know they are the same, OK.\n>>\n>>> +cmp_failed:\n>>> +\t\t\tif (!run_diff(argv[1], argv[2]))\n>>\n>> If one of argv[] was \"-\", then this wouldn't work correctly, as the\n>> other file is read from the beginning but the \"-\" side have consumed\n>> the initial part of the input and we cannot unseek it. This bug\n>> needs to be fixed only if we expect a useful and reliable output\n>> from the helper.\n>\n> Right. I've added a clause that says that we cannot show the diff because\n> `stdin` has been consumed already.\n>\n>> But otherwise the idea is sound. We compare them line by line,\n>> using strbuf_getline() to ignore differences in CRLF and LF that\n>> originates at 4d715ac0 (Windows: a test_cmp that is agnostic to\n>> random LF <> CRLF conversions, 2013-10-26). Only when we find the\n>> input different, we use \"git diff --no-index\" to make the difference\n>> (and unfortunately more, as it does not ignore CRLF <> LF\n>> differences) visible.\n\nWhy not use \"git diff --no-index --ignore-cr-at-eol\"? Do you even need\nto wrap it?\n\nRené\n\n"},{"id":"462708","messageId":"220907.86v8pzl6jz.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"1f5366f137967cbec30041b40eedd86ce5f6e953.1662469859.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-07T11:57:59Z","receivedAt":"2022-09-07T12:20:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 06 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> [...]\n> +++ b/t/helper/test-text-cmp.c\n> @@ -0,0 +1,78 @@\n> +#include \"test-tool.h\"\n> +#include \"git-compat-util.h\"\n> +#include \"strbuf.h\"\n> +#include \"gettext.h\"\n\nSuperflous header? Compiles without gettext.h for me (and we shouldn't\nuse i18n in test helpers).\n\n> [...]\n> +int cmd__text_cmp(int argc, const char **argv)\n> +{\n> +\tFILE *f0, *f1;\n> +\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n> +\n> +\tif (argc != 3)\n> +\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n\nHere you conflate the argc v.s. arguments minus the \"text-cmp\",\nresulting in:\n\n\thelper/test-tool text-cmp 2\n fatal: Require exactly 2 arguments, got 2\n\nAn argc-- argv++ at the beginning seems like the easiest way out of\nthis. Also s/Require/require/ per CodingGuidelines.\n\n> +\tif (!strcmp(argv[1], \"-\") && !strcmp(argv[2], \"-\"))\n> +\t\tdie(\"only one parameter can refer to `stdin` but not both\");\n> +\n> +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n> +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n> +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n> +\t\tfclose(f0);\n> +\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n> +\t}\n\nFaithfully emulating the old version. I do wonder if we couldn't simply\nadjust the handful of tests that actually make use of the \"-\" diff(1)\nfeature. AFAICT there's around 10 of those at most, and they all seem\nlike cases where it would be easy to change:\n\n\t(echo foo) | test_cmp - actual\n\nOr whatever, to:\n\n\techo foo >expected &&\n\ttest_cmp expected actual\n\n...\n\n> +\t\t\tif (!strcmp(argv[1], \"-\") || !strcmp(argv[2], \"-\"))\n> +\t\t\t\twarning(\"cannot show diff because `stdin` was already consumed\");\n\n...\n\nWhich means we wouldn't need to punt on this.\n\n> +\t\t\telse if (!run_diff(argv[1], argv[2]))\n> +\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n> +\t\t\t\t argv[1], argv[2]);\n\nI tried manually testing this with:\n\n\tGIT_TRACE=1 GIT_TEST_CMP=\"/home/avar/g/git/git diff --no-index --\" ./t0021-conversion.sh -vixd\n\nv.s.:\n\n\tGIT_TRACE=1 GIT_TEST_CMP=\"$PWD/helper/test-tool text-cmp\" ./t0021-conversion.sh -vixd\n\nYour version doesn't get confused by the same, but AFAICT this is by\nfragile accident.\n\nI.e. you run your own equivalent of \"cmp\", so because the files are the\nsame in that case we don't run the \"diff --no-index\".\n\nBut the \"diff --no-index\" in that t0021*.sh case *would* report\ndifferences, even though the files are byte-for-byte identical.\n\nSo the \"cmp\"-a-like here isn't just an optimization to avoid forking the\n\"git diff\" process, it's an entirely different comparison method in\ncases where we have a \"filter\".\n\nIt just so happens that our test suite doesn't currently combine them in\na way that causes a current failure.\n\n> test_cmp () {\n> \ttest \"$#\" -ne 2 && BUG \"2 param\"\n> -\teval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\tGIT_ALLOC_LIMIT=0 eval \"$GIT_TEST_CMP\" '\"$@\"'\n> }\n\nFurther, we have a clear boundary in the test suite between \"git\" and\n\"test-tool\" things we invoke, and third party tools. The former we put\nin \"test_must_fail_acceptable\".\n\nWhen using this new helper we'd hide potential segfaults and BUGs in any\n\"! test_cmp\" invocation..\n\nTo avoid the introduction of such a blindspot we'd need to change\n\"test_cmp\" to take an optional \"!\" as the 1st argument, and convert the\nexisting \"! test_cmp\" to \"test_cmp !\", then carry some flag to indicate\nthat our \"GIT_TEST_CMP\" is a git or test-tool invocation, and check it\nappropriately.\n\n> [...]\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..0be25ecbd59 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1546,7 +1546,7 @@ case $uname_s in\n> \ttest_set_prereq SED_STRIPS_CR\n> \ttest_set_prereq GREP_STRIPS_CR\n> \ttest_set_prereq WINDOWS\n> -\tGIT_TEST_CMP=mingw_test_cmp\n> +\tGIT_TEST_CMP=\"test-tool text-cmp\"\n> \t;;\n> *CYGWIN*)\n> \ttest_set_prereq POSIXPERM\n\nNot a new problem, but this is incompatible with\nGIT_TEST_CMP_USE_COPIED_CONTEXT.\n\nWhat is new though is that with this series there's no longer a good\nreason AFAICT to carry GIT_TEST_CMP_USE_COPIED_CONTEXT at all. I.e. we\nhave it for a \"diff\" that doesn't understand \"-u\".\n\nIf (after getting past tho caveats noted above) we could simply invoke\nour own test-tool we could drop that special-casing & just always invoke\nour own test_cmp helper.\n\n"},{"id":"462709","messageId":"220907.86r10nl63s.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"220907.86v8pzl6jz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-07T12:24:26Z","receivedAt":"2022-09-07T12:30:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 07 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Sep 06 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n>> From: Johannes Schindelin \n>> [...]\n>> +++ b/t/helper/test-text-cmp.c\n>> @@ -0,0 +1,78 @@\n>> +#include \"test-tool.h\"\n>> +#include \"git-compat-util.h\"\n>> +#include \"strbuf.h\"\n>> +#include \"gettext.h\"\n>\n> Superflous header? Compiles without gettext.h for me (and we shouldn't\n> use i18n in test helpers).\n>\n>> [...]\n>> +int cmd__text_cmp(int argc, const char **argv)\n>> +{\n>> +\tFILE *f0, *f1;\n>> +\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n>> +\n>> +\tif (argc != 3)\n>> +\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n>\n> Here you conflate the argc v.s. arguments minus the \"text-cmp\",\n> resulting in:\n>\n> \thelper/test-tool text-cmp 2\n> fatal: Require exactly 2 arguments, got 2\n>\n> An argc-- argv++ at the beginning seems like the easiest way out of\n> this. Also s/Require/require/ per CodingGuidelines.\n>\n>> +\tif (!strcmp(argv[1], \"-\") && !strcmp(argv[2], \"-\"))\n>> +\t\tdie(\"only one parameter can refer to `stdin` but not both\");\n>> +\n>> +\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n>> +\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n>> +\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n>> +\t\tfclose(f0);\n>> +\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n>> +\t}\n>\n> Faithfully emulating the old version. I do wonder if we couldn't simply\n> adjust the handful of tests that actually make use of the \"-\" diff(1)\n> feature. AFAICT there's around 10 of those at most, and they all seem\n> like cases where it would be easy to change:\n>\n> \t(echo foo) | test_cmp - actual\n>\n> Or whatever, to:\n>\n> \techo foo >expected &&\n> \ttest_cmp expected actual\n>\n> ...\n>\n>> +\t\t\tif (!strcmp(argv[1], \"-\") || !strcmp(argv[2], \"-\"))\n>> +\t\t\t\twarning(\"cannot show diff because `stdin` was already consumed\");\n>\n> ...\n>\n> Which means we wouldn't need to punt on this.\n>\n>> +\t\t\telse if (!run_diff(argv[1], argv[2]))\n>> +\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n>> +\t\t\t\t argv[1], argv[2]);\n>\n> I tried manually testing this with:\n>\n> \tGIT_TRACE=1 GIT_TEST_CMP=\"/home/avar/g/git/git diff --no-index --\" ./t0021-conversion.sh -vixd\n>\n> v.s.:\n>\n> \tGIT_TRACE=1 GIT_TEST_CMP=\"$PWD/helper/test-tool text-cmp\" ./t0021-conversion.sh -vixd\n>\n> Your version doesn't get confused by the same, but AFAICT this is by\n> fragile accident.\n>\n> I.e. you run your own equivalent of \"cmp\", so because the files are the\n> same in that case we don't run the \"diff --no-index\".\n>\n> But the \"diff --no-index\" in that t0021*.sh case *would* report\n> differences, even though the files are byte-for-byte identical.\n>\n> So the \"cmp\"-a-like here isn't just an optimization to avoid forking the\n> \"git diff\" process, it's an entirely different comparison method in\n> cases where we have a \"filter\".\n>\n> It just so happens that our test suite doesn't currently combine them in\n> a way that causes a current failure.\n\nAh (partially?) I spoke too soon on this part. I.e. the\nGIT_DIR=/dev/null precludes reading the filter/repo in this case. So I\n*think* we're out of the woods as far as this is concerned.\n\nStill, it would be nice to document in a code comment or commit message\nthat the \"not read the local repo's filter\" is absolutely critical here.\n\nBut I think that re-raises the point René had in:\nhttps://lore.kernel.org/git/b21d2b60-428f-58ec-28b6-3c617b9f2e45@web.de/\n\nI ran the full test suite with:\n\n\tGIT_TEST_CMP='GIT_DIR=/dev/null HOME=/dev/null /usr/bin/git diff --no-index --ignore-cr-at-eol --'\n\nAnd all of it passes, except for a test in t0001-init.sh which we could\nfix up as:\n\t\n\tdiff --git a/t/t0001-init.sh b/t/t0001-init.sh\n\tindex d479303efa0..d65afe7cceb 100755\n\t--- a/t/t0001-init.sh\n\t+++ b/t/t0001-init.sh\n\t@@ -426,7 +426,7 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '\n\t \tgit init --separate-git-dir ../realgitdir\n\t \t) &&\n\t \techo \"gitdir: $(pwd)/realgitdir\" >expected &&\n\t-\ttest_cmp expected newdir/.git &&\n\t+\ttest \"$(test_readlink newdir/.git)\" = here &&\n\t \ttest_cmp expected newdir/here &&\n\t \ttest_path_is_dir realgitdir/refs\n\t '\n\nWhich without this series is more correct, as all we're re-testing there\nis whether the symlink is pointing to what we expect. A hypothetical\n\"--dereference\" to \"git diff\" would also take care of it (the equivalent\nof \"--no-dereference\" being the default).\n\nBut with that all tests pass for me, so I'm puzzled as to the need for\nthe new helper, as opposed to just constructing the command above and\nsticking it in GIT_TEST_CMP ...\n\t\n"},{"id":"462733","messageId":"xmqq7d2fywvr.fsf@gitster.g","threadId":"58243","inReplyTo":"b21d2b60-428f-58ec-28b6-3c617b9f2e45@web.de","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T16:25:44Z","receivedAt":"2022-09-07T16:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> \"git diff --no-index - -\" also doesn't complain, by the way.\n\nTrue, but in this case hopefully it is worth to call it out, as both\nthis code that uses \"diff --no-index\" and \"diff --no-index\" itself\ncame from the same author ;-)\n\nI think \"git diff --no-index - -\" should just exit 0 after slurping\nall its input (i.e. allow it to be placed downstream of a pipe\nwithout blocking the upstream), but it is also fine to exit with 0\nwithout reading a single byte from the standard input. Of course\nthe latter is easier to implement ;-)\n\n>>> But otherwise the idea is sound. We compare them line by line,\n>>> using strbuf_getline() to ignore differences in CRLF and LF that\n>>> originates at 4d715ac0 (Windows: a test_cmp that is agnostic to\n>>> random LF <> CRLF conversions, 2013-10-26). Only when we find the\n>>> input different, we use \"git diff --no-index\" to make the difference\n>>> (and unfortunately more, as it does not ignore CRLF <> LF\n>>> differences) visible.\n>\n> Why not use \"git diff --no-index --ignore-cr-at-eol\"? Do you even need\n> to wrap it?\n\nHmph. That surely sounds sensible if it works, and I offhand do not\nsee why it shouldn't work.\n"},{"id":"462753","messageId":"xmqqillzt1cb.fsf@gitster.g","threadId":"58243","inReplyTo":"220907.86r10nl63s.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T19:45:56Z","receivedAt":"2022-09-07T19:46:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But I think that re-raises the point René had in:\n> https://lore.kernel.org/git/b21d2b60-428f-58ec-28b6-3c617b9f2e45@web.de/\n\nAs the primary point of no-index mode was to expose fancy options\n\"git diff\" has to comparisons of files outside version control,\nwithout having to go through the trouble of upstreaming changes to\nGNU diff, I do think \"--ignore-cr-at-eol\" should work fine with it,\nand René's idea sounds like the best implementation for the\ntest-text-cmp helper command.\n\nThanks.\n"},{"id":"462766","messageId":"xmqqlequsvt4.fsf_-_@gitster.g","threadId":"58243","inReplyTo":"xmqq7d2fywvr.fsf@gitster.g","subject":"Re* [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T21:45:27Z","receivedAt":"2022-09-07T21:45:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> René Scharfe writes:\n>\n>> \"git diff --no-index - -\" also doesn't complain, by the way.\n>\n> True, but in this case hopefully it is worth to call it out, as both\n> this code that uses \"diff --no-index\" and \"diff --no-index\" itself\n> came from the same author ;-)\n>\n> I think \"git diff --no-index - -\" should just exit 0 after slurping\n> all its input (i.e. allow it to be placed downstream of a pipe\n> without blocking the upstream), but it is also fine to exit with 0\n> without reading a single byte from the standard input. Of course\n> the latter is easier to implement ;-)\n\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] diff: \"--no-index - -\" compares the same standard input\n\nConceptually, \"git diff --no-index - A\" slurps the standard input to\nthe EOF and compares it with the contents of A, so it is a natural\nextension for \"git diff --no-index - -\" to slurp the standard input\nto the end, and compares it with itself, which should always yield\n\"true\".\n\nThere can be two plausible implementations. We could read and\ndiscard the input to the end and exit 0. It would allow us to avoid\nsigpipe on the upstream command\n\n $ dd if=/dev/zero bs=1m count=1 | git diff --no-index - -\n\nAs we can tell the outcome without even consuming a single byte from\nthe standard input, we can just notice the two input files specified\nare \"-\" and immediately exit. It would allow us to give a correct\nanswer to\n\n $ git diff --no-index - - \n---\n diff-no-index.c | 11 +++++++++++\n t/t4053-diff-no-index.sh | 11 +++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 18edbdf4b5..940b66b4c8 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -278,6 +278,17 @@ int diff_no_index(struct rev_info *revs,\n \t\t\tp = to_free[i] = prefix_filename(prefix, p);\n \t\tpaths[i] = p;\n \t}\n+\tif (paths[0] == file_from_standard_input &&\n+\t paths[1] == file_from_standard_input) {\n+\t\t/*\n+\t\t * \"git diff --no-index - -\". We are asked to compare\n+\t\t * what came from the standard input with itself; we\n+\t\t * know they are the same without even reading a\n+\t\t * single byte.\n+\t\t */\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tfixup_paths(paths, &replacement);\n \ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 3feadf0e35..d089d9c2b1 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -156,6 +156,16 @@ test_expect_success 'diff --no-index normalizes mode: no changes' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'diff --no-index with standard input' '\n+\techo foo >x &&\n+\techo bar >z &&\n+\tgit diff --no-index - x expected <<-\\EOF &&\n@@ -180,6 +190,7 @@ test_expect_success POSIXPERM 'diff --no-index normalizes: mode not like git mod\n '\n \n test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not like git mode (symlink)' '\n+\trm -f z &&\n \tln -s y z &&\n \tX_OID=$(git hash-object --stdin Junio C Hamano writes:\n>\n>> René Scharfe writes:\n>>\n>>> \"git diff --no-index - -\" also doesn't complain, by the way.\n>>\n>> True, but in this case hopefully it is worth to call it out, as both\n>> this code that uses \"diff --no-index\" and \"diff --no-index\" itself\n>> came from the same author ;-)\n>>\n>> I think \"git diff --no-index - -\" should just exit 0 after slurping\n>> all its input (i.e. allow it to be placed downstream of a pipe\n>> without blocking the upstream), but it is also fine to exit with 0\n>> without reading a single byte from the standard input. Of course\n>> the latter is easier to implement ;-)\n>\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] diff: \"--no-index - -\" compares the same standard input\n>\n> Conceptually, \"git diff --no-index - A\" slurps the standard input to\n> the EOF and compares it with the contents of A, so it is a natural\n> extension for \"git diff --no-index - -\" to slurp the standard input\n> to the end, and compares it with itself, which should always yield\n> \"true\".\n>\n> There can be two plausible implementations. We could read and\n> discard the input to the end and exit 0. It would allow us to avoid\n> sigpipe on the upstream command\n>\n> $ dd if=/dev/zero bs=1m count=1 | git diff --no-index - -\n>\n> As we can tell the outcome without even consuming a single byte from\n> the standard input, we can just notice the two input files specified\n> are \"-\" and immediately exit. It would allow us to give a correct\n> answer to\n>\n> $ git diff --no-index - - \n> but can kill the command on the upstream side of a pipe that feeds\n> us.\n>\n> We pick the latter (i.e. not touching the input at all), simply\n> because it is more efficient. Data producer that can be placed on\n> the upstream side of a pipe should always be prepared for a consumer\n> that does not consume all its output (e.g. \"head\" and pager) anyway.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff-no-index.c | 11 +++++++++++\n> t/t4053-diff-no-index.sh | 11 +++++++++++\n> 2 files changed, 22 insertions(+)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 18edbdf4b5..940b66b4c8 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -278,6 +278,17 @@ int diff_no_index(struct rev_info *revs,\n> \t\t\tp = to_free[i] = prefix_filename(prefix, p);\n> \t\tpaths[i] = p;\n> \t}\n> +\tif (paths[0] == file_from_standard_input &&\n> +\t paths[1] == file_from_standard_input) {\n> +\t\t/*\n> +\t\t * \"git diff --no-index - -\". We are asked to compare\n> +\t\t * what came from the standard input with itself; we\n> +\t\t * know they are the same without even reading a\n> +\t\t * single byte.\n> +\t\t */\n> +\t\tret = 0;\n> +\t\tgoto out;\n> +\t}\n>\n> \tfixup_paths(paths, &replacement);\n>\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 3feadf0e35..d089d9c2b1 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -156,6 +156,16 @@ test_expect_success 'diff --no-index normalizes mode: no changes' '\n> \ttest_must_be_empty out\n> '\n>\n> +test_expect_success 'diff --no-index with standard input' '\n> +\techo foo >x &&\n> +\techo bar >z &&\n> +\tgit diff --no-index - x +\tgit diff --no-index x - +\ttest_must_fail git diff --no-index - x +\ttest_must_fail git diff --no-index x - +\tgit diff --no-index - - +'\n\nThis test passes even without the changes to diff-no-index.c above.\nBecause diff.c::diff_fill_oid_info() sets an all-zero object ID for\nstdin on both sides, I guess, so the comparison is skipped even though\nthe data is read already for the first dash and the data size of the\nsecond one is thus zero?\n\n> +\n> test_expect_success POSIXPERM 'diff --no-index normalizes mode: chmod +x' '\n> \tchmod +x y &&\n> \tcat >expected <<-\\EOF &&\n> @@ -180,6 +190,7 @@ test_expect_success POSIXPERM 'diff --no-index normalizes: mode not like git mod\n> '\n>\n> test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not like git mode (symlink)' '\n> +\trm -f z &&\n> \tln -s y z &&\n> \tX_OID=$(git hash-object --stdin \tZ_OID=$(printf y | git hash-object --stdin) &&\n"},{"id":"462774","messageId":"xmqq4jxispe9.fsf@gitster.g","threadId":"58243","inReplyTo":"22402f5d-c463-acee-f57d-e507f23822ca@web.de","subject":"Re: Re* [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-08T00:03:58Z","receivedAt":"2022-09-08T00:04:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> This test passes even without the changes to diff-no-index.c above.\n> Because diff.c::diff_fill_oid_info() sets an all-zero object ID for\n> stdin on both sides, I guess, so the comparison is skipped even though\n> the data is read already for the first dash and the data size of the\n> second one is thus zero?\n\nOK, that sounds like a happy accident ;-)\n"},{"id":"462803","messageId":"2d839209-21e2-b05c-125b-57a9c1dad1ea@web.de","threadId":"58243","inReplyTo":"xmqq7d2fywvr.fsf@gitster.g","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-08T08:59:46Z","receivedAt":"2022-09-08T08:59:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.09.22 um 18:25 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>>> But otherwise the idea is sound. We compare them line by line,\n>>>> using strbuf_getline() to ignore differences in CRLF and LF that\n>>>> originates at 4d715ac0 (Windows: a test_cmp that is agnostic to\n>>>> random LF <> CRLF conversions, 2013-10-26). Only when we find the\n>>>> input different, we use \"git diff --no-index\" to make the difference\n>>>> (and unfortunately more, as it does not ignore CRLF <> LF\n>>>> differences) visible.\n>>\n>> Why not use \"git diff --no-index --ignore-cr-at-eol\"? Do you even need\n>> to wrap it?\n>\n> Hmph. That surely sounds sensible if it works, and I offhand do not\n> see why it shouldn't work.\n\nUsing git diff in test_cmp and using test_cmp to check if git diff works\nwould become a cyclical dependency. Only doing that on one platform\nlimits the potential blind spot to platform-specific bugs, though.\nEnough to go wrapper-less? Not sure.\n\nRené\n"},{"id":"462808","messageId":"220908.861qsllw5x.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"2d839209-21e2-b05c-125b-57a9c1dad1ea@web.de","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-08T15:26:41Z","receivedAt":"2022-09-08T15:32:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 08 2022, René Scharfe wrote:\n\n> Am 07.09.22 um 18:25 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>>>> But otherwise the idea is sound. We compare them line by line,\n>>>>> using strbuf_getline() to ignore differences in CRLF and LF that\n>>>>> originates at 4d715ac0 (Windows: a test_cmp that is agnostic to\n>>>>> random LF <> CRLF conversions, 2013-10-26). Only when we find the\n>>>>> input different, we use \"git diff --no-index\" to make the difference\n>>>>> (and unfortunately more, as it does not ignore CRLF <> LF\n>>>>> differences) visible.\n>>>\n>>> Why not use \"git diff --no-index --ignore-cr-at-eol\"? Do you even need\n>>> to wrap it?\n>>\n>> Hmph. That surely sounds sensible if it works, and I offhand do not\n>> see why it shouldn't work.\n>\n> Using git diff in test_cmp and using test_cmp to check if git diff works\n> would become a cyclical dependency. Only doing that on one platform\n> limits the potential blind spot to platform-specific bugs, though.\n> Enough to go wrapper-less? Not sure.\n\nI don't see how being wrapper-less is less of a \"cyclical dependency\"\nthan using \"git diff\" directly.\n\nIf we are to postulate some bug where \"git diff\" thwarts us for the use\nof \"test_cmp\" it's going to be *very* broken.\n\nI don't see how such a \"git diff\" would pass the rest of the test suite\n(some of which involves comparing its exact output), but still be\nfunctional enough to work as a GIT_TEST_CMP.\n\nEven one where it just returns 0 unconditionally wouldn't pass, as we\nrely on \"! test_cmp\" in some cases.\n\nAnd any such breakage we imagine might just as well affect a wrapper for\nit, and I'd think that would be the more likely of two unlikely\npossibilities, as that wrapper would only be used for the test suite,\nwhereas \"git diff\" is more widely tested.\n\nIn any case, as long as we preserve the ability to set a\nGIT_TEST_CMP=cmp we can sanity check any such wrapper or dogfodding of\n\"git diff\" with an external program.\n\nNeeding to deal just with \"git diff\" and \"cmp\" would be a step forward,\nas we'd be able to drop the current shellscript \"mingw\" wrapper, as well\nas the special support for a \"diff\" that doesn't understand \"-u\".\n"},{"id":"462830","messageId":"q044qs1r-n8p3-1617-so32-02s1r88186sp@tzk.qr","threadId":"58243","inReplyTo":"xmqq7d2fywvr.fsf@gitster.g","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-08T20:54:40Z","receivedAt":"2022-09-08T20:54:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 7 Sep 2022, Junio C Hamano wrote:\n\n> René Scharfe writes:\n>\n> > \"git diff --no-index - -\" also doesn't complain, by the way.\n>\n> True, but in this case hopefully it is worth to call it out, as both\n> this code that uses \"diff --no-index\" and \"diff --no-index\" itself\n> came from the same author ;-)\n>\n> I think \"git diff --no-index - -\" should just exit 0 after slurping\n> all its input (i.e. allow it to be placed downstream of a pipe\n> without blocking the upstream), but it is also fine to exit with 0\n> without reading a single byte from the standard input. Of course\n> the latter is easier to implement ;-)\n>\n> >>> But otherwise the idea is sound. We compare them line by line,\n> >>> using strbuf_getline() to ignore differences in CRLF and LF that\n> >>> originates at 4d715ac0 (Windows: a test_cmp that is agnostic to\n> >>> random LF <> CRLF conversions, 2013-10-26). Only when we find the\n> >>> input different, we use \"git diff --no-index\" to make the difference\n> >>> (and unfortunately more, as it does not ignore CRLF <> LF\n> >>> differences) visible.\n> >\n> > Why not use \"git diff --no-index --ignore-cr-at-eol\"? Do you even need\n> > to wrap it?\n>\n> Hmph. That surely sounds sensible if it works, and I offhand do not\n> see why it shouldn't work.\n\nIs this a reversal of the stance you took in your reply in\nhttps://lore.kernel.org/git/7vps7xrfxa.fsf@assigned-by-dhcp.cox.net/ to my\nsuggestion to replace `cmp` by `diff --no-index` (in that mail referred to\nas patch [7/8])?\n\nIf I recall correctly, you clarified outside of that thread that \"I do not\nthink it is a good enough reason to make the tests slower\" was you being\nconcerned about employing the entire diff machinery instead of doing a\nsimple byte-for-byte comparison.\n\nAnd while it is no longer _that_ simple a comparison (it now\nspecial-handles Carriage Returns), the speed and simplicity concern is\nstill valid: `test-tool text-cmp` is vastly simpler (and provably faster)\nthan `diff --no-index`.\n\nJust because it is easier to review a one-liner to switch from essentially\n`cmp` to `git diff --no-index --ignore-cr-at-eol` does not mean that it is\nreasonable: it would cause us to blast out that much more CO2, just for\nour one-time convenience.\n\nOr for that matter, it would willfully slow down the `windows-test` jobs\nthat already is mired by sloooow performance (mostly due to running a\nshell script-based test suite).\n\nCan you please let me make the Windows situation better rather than worse?\n\nThank you,\nDscho\n"},{"id":"462835","messageId":"xmqqbkrpo9or.fsf@gitster.g","threadId":"58243","inReplyTo":"q044qs1r-n8p3-1617-so32-02s1r88186sp@tzk.qr","subject":"Re: [PATCH] tests: replace mingw_test_cmp with a helper in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-08T21:09:08Z","receivedAt":"2022-09-08T21:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > Why not use \"git diff --no-index --ignore-cr-at-eol\"? Do you even need\n>> > to wrap it?\n>>\n>> Hmph. That surely sounds sensible if it works, and I offhand do not\n>> see why it shouldn't work.\n>\n> Is this a reversal of the stance you took in your reply in\n> https://lore.kernel.org/git/7vps7xrfxa.fsf@assigned-by-dhcp.cox.net/ to my\n> suggestion to replace `cmp` by `diff --no-index` (in that mail referred to\n> as patch [7/8])?\n\ncox.net? It is a lifetime ago and the world has changed. Hopefully\nthat \"diff --no-index\" has matured a lot to earn more confidence by\nus than it had back then.\n\n> If I recall correctly, you clarified outside of that thread that \"I do not\n> think it is a good enough reason to make the tests slower\" was you being\n> concerned about employing the entire diff machinery instead of doing a\n> simple byte-for-byte comparison.\n\nIs it still relevant, now that we are talking about text-cmp that\nignores cr-at-eol, to bring a random remark about byte-for-byte\ncomparison from more than 10 years ago?\n\n> Just because it is easier to review a one-liner to switch from essentially\n> `cmp` to `git diff --no-index --ignore-cr-at-eol` does not mean that it is\n> reasonable: it would cause us to blast out that much more CO2, just for\n> our one-time convenience.\n\nMeasurement, in tons per year, is needed, or please stop talking\nabout CO2. It is not that funny.\n\nDeveloper cycle time is easier to measure and more meaningful. \n\nIt would be much faster to run a byte-for-byte-with-ignore-cr-at-eol\ncomparison than running \"diff --no-index --ignore-cr-at-eol\" on\nfiles with meaningful sizes. But comparison between \"expect\" and\n\"actual\", which are typically at most several lines long? Wouldn't\nthe overhead to spawn a process dwarf everything else, especially on\nWindows, whether it is your byte-for-byte-with-ignore-cr-at-eol\nprogram or \"git diff\"?\n\n\n"},{"id":"467207","messageId":"pull.1309.v3.git.1668290855.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v2.git.1662469859.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] tests: replace mingw_test_cmp with a helper in C","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-12T22:07:32Z","receivedAt":"2022-11-12T22:07:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A few months ago, directly after sending a patch to fix a performance\nregression due to a mis-use of test_cmp\n[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],\nI got curious to see whether Git for Windows had the same issue. And it did\nnot: it passes t5351 in 22 seconds, even while using test_cmp to compare\npack files\n[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].\n\nThe explanation is of course that Git for Windows uses a test helper for\ntest_cmp that is written in C, instead of the Bash function. And C is much\nfaster than a Bash function, especially on Windows. This is especially sad\nwhen said Bash code is only used on Windows. So I originally had pulled out\nthis helper from the years-long effort to let Git for Windows use BusyBox'\nash to run the test suite. The result is this patch, which has been in Git\nfor Windows since June 2018.\n\nUnfortunately, this tried-and-tested code was rejected by the Git\nmaintainer.\n\nLet's fall back to the next-best solution: git diff --no-index, which the\nGit maintainer seems to like. The downside is that the diff machinery does a\nlot more than a simple cmp clone, and therefore a lot more things can go\nwrong that might make it look like a test case is failing when the fault is\nsomewhere else entirely. There is one way to find out whether this is a\nvalid concern.\n\nChanges since v2:\n\n * Dropped the test helper, using diff --no-index instead.\n\nChanges since v1:\n\n * Fixed double \"with\" in the commit message.\n * Renamed the test helper to text-cmp.\n * Made the diff --no-index call more robust by using a double-dash\n separator.\n\nJohannes Schindelin (2):\n t0021: use Windows-friendly `pwd`\n tests(mingw): avoid very slow `mingw_test_cmp`\n\n t/t0021-conversion.sh | 4 +--\n t/test-lib-functions.sh | 66 -----------------------------------------\n t/test-lib.sh | 2 +-\n 3 files changed, 3 insertions(+), 69 deletions(-)\n\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1309\n\nRange-diff vs v2:\n\n 1: ad7c41401ee ! 1: b38b8fb5a85 t0021: use Windows-friendly `pwd`\n @@ Commit message\n absolute path, the MSYS2 runtime specifically exempts arguments\n containing a `=` character from that conversion.\n \n - We are about to change `test_cmp` to use a test helper, which is such a\n - Win32 process.\n + We are about to change `test_cmp` to use `git diff --no-index`, which\n + involves spawning precisely such a Win32 process.\n \n In combination, this would cause a failure in `t0021-conversion.sh`\n where we pass an absolute path containing an equal character to the\n 2: 1f5366f1379 ! 2: a7f4265ceb2 tests: replace mingw_test_cmp with a helper in C\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - tests: replace mingw_test_cmp with a helper in C\n + tests(mingw): avoid very slow `mingw_test_cmp`\n \n - This helper is more performant than running the `mingw_test_cmp` code\n - with MSYS2's Bash. And a lot more readable.\n + It is more performant to run `git diff --no-index` than running the\n + `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n + Windows uses. And a lot more readable.\n \n - To accommodate t1050, which wants to compare files weighing in with 3MB\n - (falling outside of t1050's malloc limit of 1.5MB), we simply lift the\n - allocation limit by setting the environment variable GIT_ALLOC_LIMIT to\n - zero when calling the helper.\n + Note: Earlier attempts at fixing this involved a test helper that avoids\n + the overhead of the diff machinery, in favor of implementing a behavior\n + that is more in line with what `mingw_test_cmp` does now, but that\n + attempt saw a lot of backlash and distractions during review and was\n + therefore abandoned.\n \n Signed-off-by: Johannes Schindelin \n \n - ## Makefile ##\n -@@ Makefile: TEST_BUILTINS_OBJS += test-string-list.o\n - TEST_BUILTINS_OBJS += test-submodule-config.o\n - TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o\n - TEST_BUILTINS_OBJS += test-subprocess.o\n -+TEST_BUILTINS_OBJS += test-text-cmp.o\n - TEST_BUILTINS_OBJS += test-trace2.o\n - TEST_BUILTINS_OBJS += test-urlmatch-normalization.o\n - TEST_BUILTINS_OBJS += test-userdiff.o\n -\n - ## t/helper/test-text-cmp.c (new) ##\n -@@\n -+#include \"test-tool.h\"\n -+#include \"git-compat-util.h\"\n -+#include \"strbuf.h\"\n -+#include \"gettext.h\"\n -+#include \"parse-options.h\"\n -+#include \"run-command.h\"\n -+\n -+#ifdef WIN32\n -+#define NO_SUCH_DIR \"\\\\\\\\.\\\\GLOBALROOT\\\\invalid\"\n -+#else\n -+#define NO_SUCH_DIR \"/dev/null\"\n -+#endif\n -+\n -+static int run_diff(const char *path1, const char *path2)\n -+{\n -+\tconst char *argv[] = {\n -+\t\t\"diff\", \"--no-index\", \"--\", NULL, NULL, NULL\n -+\t};\n -+\tconst char *env[] = {\n -+\t\t\"GIT_PAGER=cat\",\n -+\t\t\"GIT_DIR=\" NO_SUCH_DIR,\n -+\t\t\"HOME=\" NO_SUCH_DIR,\n -+\t\tNULL\n -+\t};\n -+\n -+\targv[3] = path1;\n -+\targv[4] = path2;\n -+\treturn run_command_v_opt_cd_env(argv,\n -+\t\t\t\t\tRUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n -+\t\t\t\t\tNULL, env);\n -+}\n -+\n -+int cmd__text_cmp(int argc, const char **argv)\n -+{\n -+\tFILE *f0, *f1;\n -+\tstruct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;\n -+\n -+\tif (argc != 3)\n -+\t\tdie(\"Require exactly 2 arguments, got %d\", argc);\n -+\n -+\tif (!strcmp(argv[1], \"-\") && !strcmp(argv[2], \"-\"))\n -+\t\tdie(\"only one parameter can refer to `stdin` but not both\");\n -+\n -+\tif (!(f0 = !strcmp(argv[1], \"-\") ? stdin : fopen(argv[1], \"r\")))\n -+\t\treturn error_errno(\"could not open '%s'\", argv[1]);\n -+\tif (!(f1 = !strcmp(argv[2], \"-\") ? stdin : fopen(argv[2], \"r\"))) {\n -+\t\tfclose(f0);\n -+\t\treturn error_errno(\"could not open '%s'\", argv[2]);\n -+\t}\n -+\n -+\tfor (;;) {\n -+\t\tint r0 = strbuf_getline(&b0, f0);\n -+\t\tint r1 = strbuf_getline(&b1, f1);\n -+\n -+\t\tif (r0 == EOF) {\n -+\t\t\tfclose(f0);\n -+\t\t\tfclose(f1);\n -+\t\t\tstrbuf_release(&b0);\n -+\t\t\tstrbuf_release(&b1);\n -+\t\t\tif (r1 == EOF)\n -+\t\t\t\treturn 0;\n -+cmp_failed:\n -+\t\t\tif (!strcmp(argv[1], \"-\") || !strcmp(argv[2], \"-\"))\n -+\t\t\t\twarning(\"cannot show diff because `stdin` was already consumed\");\n -+\t\t\telse if (!run_diff(argv[1], argv[2]))\n -+\t\t\t\tdie(\"Huh? 'diff --no-index %s %s' succeeded\",\n -+\t\t\t\t argv[1], argv[2]);\n -+\t\t\treturn 1;\n -+\t\t}\n -+\t\tif (r1 == EOF || strbuf_cmp(&b0, &b1)) {\n -+\t\t\tfclose(f0);\n -+\t\t\tfclose(f1);\n -+\t\t\tstrbuf_release(&b0);\n -+\t\t\tstrbuf_release(&b1);\n -+\t\t\tgoto cmp_failed;\n -+\t\t}\n -+\t}\n -+}\n -\n - ## t/helper/test-tool.c ##\n -@@ t/helper/test-tool.c: static struct test_cmd cmds[] = {\n - \t{ \"submodule-config\", cmd__submodule_config },\n - \t{ \"submodule-nested-repo-config\", cmd__submodule_nested_repo_config },\n - \t{ \"subprocess\", cmd__subprocess },\n -+\t{ \"text-cmp\", cmd__text_cmp },\n - \t{ \"trace2\", cmd__trace2 },\n - \t{ \"userdiff\", cmd__userdiff },\n - \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n -\n - ## t/helper/test-tool.h ##\n -@@ t/helper/test-tool.h: int cmd__string_list(int argc, const char **argv);\n - int cmd__submodule_config(int argc, const char **argv);\n - int cmd__submodule_nested_repo_config(int argc, const char **argv);\n - int cmd__subprocess(int argc, const char **argv);\n -+int cmd__text_cmp(int argc, const char **argv);\n - int cmd__trace2(int argc, const char **argv);\n - int cmd__userdiff(int argc, const char **argv);\n - int cmd__urlmatch_normalization(int argc, const char **argv);\n -\n ## t/test-lib-functions.sh ##\n -@@ t/test-lib-functions.sh: test_expect_code () {\n - \n - test_cmp () {\n - \ttest \"$#\" -ne 2 && BUG \"2 param\"\n --\teval \"$GIT_TEST_CMP\" '\"$@\"'\n -+\tGIT_ALLOC_LIMIT=0 eval \"$GIT_TEST_CMP\" '\"$@\"'\n - }\n - \n - # Check that the given config key has the expected value.\n @@ t/test-lib-functions.sh: test_skip_or_die () {\n \terror \"$2\"\n }\n @@ t/test-lib.sh: case $uname_s in\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n -\tGIT_TEST_CMP=mingw_test_cmp\n -+\tGIT_TEST_CMP=\"test-tool text-cmp\"\n ++\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n\n-- \ngitgitgadget\n"},{"id":"467208","messageId":"b38b8fb5a85e10d6189d68b6633adf23fca7b65d.1668290855.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v3.git.1668290855.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] t0021: use Windows-friendly `pwd`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-12T22:07:33Z","receivedAt":"2022-11-12T22:07:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows, when passing paths from shell scripts to regular\nWin32 executables, thanks to the MSYS2 runtime a somewhat magic path\nconversion happens that lets the shell script think that there is a file\nat `/git/Makefile` and the Win32 process it spawned thinks that the\nshell script said `C:/git-sdk-64/git/Makefile` instead.\n\nThis conversion is documented in detail over here:\nhttps://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion\n\nAs all automatic conversions, there are gaps. For example, to avoid\nmistaking command-line options like `/LOG=log.txt` (which are quite\ncommon in the Windows world) from being mistaken for a Unix-style\nabsolute path, the MSYS2 runtime specifically exempts arguments\ncontaining a `=` character from that conversion.\n\nWe are about to change `test_cmp` to use `git diff --no-index`, which\ninvolves spawning precisely such a Win32 process.\n\nIn combination, this would cause a failure in `t0021-conversion.sh`\nwhere we pass an absolute path containing an equal character to the\n`test_cmp` function.\n\nSeeing as the Unix tools like `cp` and `diff` that are used by Git's\ntest suite in the Git for Windows SDK (thanks to the MSYS2 project)\nunderstand both Unix-style as well as Windows-style paths, we can stave\noff this problem by simply switching to Windows-style paths and\nside-stepping the need for any automatic path conversion.\n\nNote: The `PATH` variable is obviously special, as it is colon-separated\nin the MSYS2 Bash used by Git for Windows, and therefore _cannot_\ncontain absolute Windows-style paths, lest the colon after the drive\nletter is mistaken for a path separator. Therefore, we need to be\ncareful to keep the Unix-style when modifying the `PATH` variable.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0021-conversion.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bad37abad2c..15482fa78e3 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-TEST_ROOT=\"$PWD\"\n-PATH=$TEST_ROOT:$PATH\n+PATH=$PWD:$PATH\n+TEST_ROOT=\"$(pwd)\"\n \n write_script <<\\EOF \"$TEST_ROOT/rot13.sh\"\n tr \\\n-- \ngitgitgadget\n\n"},{"id":"467209","messageId":"a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v3.git.1668290855.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-12T22:07:34Z","receivedAt":"2022-11-12T22:07:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is more performant to run `git diff --no-index` than running the\n`mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\nWindows uses. And a lot more readable.\n\nNote: Earlier attempts at fixing this involved a test helper that avoids\nthe overhead of the diff machinery, in favor of implementing a behavior\nthat is more in line with what `mingw_test_cmp` does now, but that\nattempt saw a lot of backlash and distractions during review and was\ntherefore abandoned.\n\nSigned-off-by: Johannes Schindelin \n---\n t/test-lib-functions.sh | 66 -----------------------------------------\n t/test-lib.sh | 2 +-\n 2 files changed, 1 insertion(+), 67 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8c44856eaec..452fe9bc8aa 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1541,72 +1541,6 @@ test_skip_or_die () {\n \terror \"$2\"\n }\n \n-# The following mingw_* functions obey POSIX shell syntax, but are actually\n-# bash scripts, and are meant to be used only with bash on Windows.\n-\n-# A test_cmp function that treats LF and CRLF equal and avoids to fork\n-# diff when possible.\n-mingw_test_cmp () {\n-\t# Read text into shell variables and compare them. If the results\n-\t# are different, use regular diff to report the difference.\n-\tlocal test_cmp_a= test_cmp_b=\n-\n-\t# When text came from stdin (one argument is '-') we must feed it\n-\t# to diff.\n-\tlocal stdin_for_diff=\n-\n-\t# Since it is difficult to detect the difference between an\n-\t# empty input file and a failure to read the files, we go straight\n-\t# to diff if one of the inputs is empty.\n-\tif test -s \"$1\" && test -s \"$2\"\n-\tthen\n-\t\t# regular case: both files non-empty\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\telif test -s \"$1\" && test \"$2\" = -\n-\tthen\n-\t\t# read 2nd file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b\n-\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n-\telif test \"$1\" = - && test -s \"$2\"\n-\tthen\n-\t\t# read 1st file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n-\tfi\n-\ttest -n \"$test_cmp_a\" &&\n-\ttest -n \"$test_cmp_b\" &&\n-\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n-\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n-}\n-\n-# $1 is the name of the shell variable to fill in\n-mingw_read_file_strip_cr_ () {\n-\t# Read line-wise using LF as the line separator\n-\t# and use IFS to strip CR.\n-\tlocal line\n-\twhile :\n-\tdo\n-\t\tif IFS=$'\\r' read -r -d $'\\n' line\n-\t\tthen\n-\t\t\t# good\n-\t\t\tline=$line$'\\n'\n-\t\telse\n-\t\t\t# we get here at EOF, but also if the last line\n-\t\t\t# was not terminated by LF; in the latter case,\n-\t\t\t# some text was read\n-\t\t\tif test -z \"$line\"\n-\t\t\tthen\n-\t\t\t\t# EOF, really\n-\t\t\t\tbreak\n-\t\t\tfi\n-\t\tfi\n-\t\teval \"$1=\\$$1\\$line\"\n-\tdone\n-}\n-\n # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n # it also works for shell functions (though those functions cannot impact\n # the environment outside of the test_env invocation).\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7726d1da88a..f8c6205e08f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1546,7 +1546,7 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n+\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n-- \ngitgitgadget\n"},{"id":"467212","messageId":"Y3B36HjDJhIY5jNz@nand.local","threadId":"58243","inReplyTo":"a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-13T04:51:52Z","receivedAt":"2022-11-13T04:54:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Nov 12, 2022 at 10:07:34PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> It is more performant to run `git diff --no-index` than running the\n> `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> Windows uses. And a lot more readable.\n\nThis makes sense, and motivates the changes below.\n\n> Note: Earlier attempts at fixing this involved a test helper that avoids\n> the overhead of the diff machinery, in favor of implementing a behavior\n> that is more in line with what `mingw_test_cmp` does now, but that\n> attempt saw a lot of backlash and distractions during review and was\n> therefore abandoned.\n\nBut I do not think that this paragraph adds as much as we'd like to the\nhistorical record.\n\nWhat about the original approach did you have trouble pushing forward?\nPerhaps framing that as an alternative approach, along with why it\ndidn't ultimately get merged would be a more useful recollection of\nprevious attempts here.\n\nOne thing that the commit message doesn't allude to (that is covered in\nthe earlier discussion) is why it is important to pass\n`--ignore-cr-at-eol`. I think that is worth mentioning here.\n\nThanks,\nTaylor\n"},{"id":"467233","messageId":"8ec8f3f5-0827-25a3-464d-638378b65ddb@dunelm.org.uk","threadId":"58243","inReplyTo":"a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-14T09:53:43Z","receivedAt":"2022-11-14T09:54:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 12/11/2022 22:07, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> It is more performant to run `git diff --no-index` than running the\n> `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> Windows uses. And a lot more readable.\n\nThis makes me wonder about the implications for our diff tests. We have \n~200 calls to `test_cmp` in t*-diff-*.sh. I had a look at \nt4053-diff-no-index.sh and non of the tests that use `test_cmp` look \ncritical for functionality that is used by `test_cmp` and there are \ntests for the exit code. I suspect that if `diff --no-index` breaks \nwe'll end up with confusing test failures rather than misleading passes.\n\nA side effect of this change is that on windows `test_cmp` will now \nexcept directories as well as files but I don't think that matters.\n\n> Note: Earlier attempts at fixing this involved a test helper that avoids\n> the overhead of the diff machinery, in favor of implementing a behavior\n> that is more in line with what `mingw_test_cmp` does now, but that\n> attempt saw a lot of backlash and distractions during review and was\n> therefore abandoned.\n\nHopefully most of the files we feed into `test_cmp` are small enough \nthat the absolute difference in run-time is not too big. There is an \noptimization for -U0 that trims the common tail from the files before \ncalling xdl_diff() but that does not help here because we need to use \n--ignore-cr-at-eol (otherwise `git diff --no-index -U0 || git diff \n--no-index` might speed up the common case of matching files).\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/test-lib-functions.sh | 66 -----------------------------------------\n> t/test-lib.sh | 2 +-\n> 2 files changed, 1 insertion(+), 67 deletions(-)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 8c44856eaec..452fe9bc8aa 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1541,72 +1541,6 @@ test_skip_or_die () {\n> \terror \"$2\"\n> }\n> \n> -# The following mingw_* functions obey POSIX shell syntax, but are actually\n> -# bash scripts, and are meant to be used only with bash on Windows.\n> -\n> -# A test_cmp function that treats LF and CRLF equal and avoids to fork\n> -# diff when possible.\n> -mingw_test_cmp () {\n> -\t# Read text into shell variables and compare them. If the results\n> -\t# are different, use regular diff to report the difference.\n> -\tlocal test_cmp_a= test_cmp_b=\n> -\n> -\t# When text came from stdin (one argument is '-') we must feed it\n> -\t# to diff.\n> -\tlocal stdin_for_diff=\n> -\n> -\t# Since it is difficult to detect the difference between an\n> -\t# empty input file and a failure to read the files, we go straight\n> -\t# to diff if one of the inputs is empty.\n> -\tif test -s \"$1\" && test -s \"$2\"\n> -\tthen\n> -\t\t# regular case: both files non-empty\n> -\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n> -\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n> -\telif test -s \"$1\" && test \"$2\" = -\n> -\tthen\n> -\t\t# read 2nd file from stdin\n> -\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n> -\t\tmingw_read_file_strip_cr_ test_cmp_b\n> -\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n> -\telif test \"$1\" = - && test -s \"$2\"\n> -\tthen\n> -\t\t# read 1st file from stdin\n> -\t\tmingw_read_file_strip_cr_ test_cmp_a\n> -\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n> -\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n> -\tfi\n> -\ttest -n \"$test_cmp_a\" &&\n> -\ttest -n \"$test_cmp_b\" &&\n> -\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n> -\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n> -}\n> -\n> -# $1 is the name of the shell variable to fill in\n> -mingw_read_file_strip_cr_ () {\n> -\t# Read line-wise using LF as the line separator\n> -\t# and use IFS to strip CR.\n> -\tlocal line\n> -\twhile :\n> -\tdo\n> -\t\tif IFS=$'\\r' read -r -d $'\\n' line\n> -\t\tthen\n> -\t\t\t# good\n> -\t\t\tline=$line$'\\n'\n> -\t\telse\n> -\t\t\t# we get here at EOF, but also if the last line\n> -\t\t\t# was not terminated by LF; in the latter case,\n> -\t\t\t# some text was read\n> -\t\t\tif test -z \"$line\"\n> -\t\t\tthen\n> -\t\t\t\t# EOF, really\n> -\t\t\t\tbreak\n> -\t\t\tfi\n> -\t\tfi\n> -\t\teval \"$1=\\$$1\\$line\"\n> -\tdone\n> -}\n> -\n> # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n> # it also works for shell functions (though those functions cannot impact\n> # the environment outside of the test_env invocation).\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..f8c6205e08f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1546,7 +1546,7 @@ case $uname_s in\n> \ttest_set_prereq SED_STRIPS_CR\n> \ttest_set_prereq GREP_STRIPS_CR\n> \ttest_set_prereq WINDOWS\n> -\tGIT_TEST_CMP=mingw_test_cmp\n> +\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n> \t;;\n> *CYGWIN*)\n> \ttest_set_prereq POSIXPERM\n"},{"id":"467246","messageId":"221114.86tu31lnwr.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-14T11:55:16Z","receivedAt":"2022-11-14T12:29:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 12 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> It is more performant to run `git diff --no-index` than running the\n> `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> Windows uses. And a lot more readable.\n>\n> Note: Earlier attempts at fixing this involved a test helper that avoids\n> the overhead of the diff machinery, in favor of implementing a behavior\n> that is more in line with what `mingw_test_cmp` does now, but that\n> attempt saw a lot of backlash and distractions during review and was\n> therefore abandoned.\n\nI don't know if you counted this under \"backlash and distractions\", or\nif you just didn't see it, but I think part of the comments I left on\nthe v2[1][2] are still applicable.\n\nNamely that before this we've been assuming that GIT_TEST_CMP is a\nnot-ours binary. I.e. \"diff\", \"cmp\" etc. If it is a \"that's ours\" this\nchange should be changing e.g. '! test_cmp' to 'test_cmp !', as the\nformer would now hide segfaults.\n\nWhich isn't a theoretical issue b.t.w., e.g. the recent d00113ec347\n(t/Makefile: apply chainlint.pl to existing self-tests, 2022-09-01)\nwill invoke \"git diff --no-index\", which has some interesting results\n(that I've run into) when testing a tree where \"git diff\" happens to\nsegfault.\n\nFrom grepping our tests that doesn't seem to be too hard, certainly a\nlot easier than a new \"not-quite-diff\" test helper.\n\nAdditionally: We don't *need* this for an initial implementation, but\nhaving e.g. one of the Ubuntu CI targets run with \"git diff --no-index\"\nwould be a nice cherry on top, as:\n\n * It would show that the \"--ignore-cr-at-eol\" is only needed for MinGW,\n and is also the reason for why \"GIT_TEST_CMP\" is still\n unconditionally clobbered there, unlike other platforms. See\n 4d715ac05cf (Windows: a test_cmp that is agnostic to random LF <>\n CRLF conversions, 2013-10-26).\n\n * We do pass this elsewhere, except for the one trivial case which you\n aren't running into on MINGW because it doesn't have SYMLINKS, but\n presumably would if the test were adjusted to use \"SYMLINKS_WINDOWS\".\n\n * If we're trusting \"git diff --no-index\" to run the tests, we could\n also get rid of \"GIT_TEST_CMP_USE_COPIED_CONTEXT\", whose only reason\n for existing is to support a system \"diff\" that doesn't understand\n \"-u\" (squashable diff below)\n\n1. https://lore.kernel.org/git/220907.86v8pzl6jz.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/220907.86r10nl63s.gmgdl@evledraar.gmail.com/\n\ndiff --git a/Makefile b/Makefile\nindex 4927379184c..dea6069b5fe 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1950,10 +1950,6 @@ ifdef OVERRIDE_STRDUP\n \tCOMPAT_OBJS += compat/strdup.o\n endif\n \n-ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n-\texport GIT_TEST_CMP_USE_COPIED_CONTEXT\n-endif\n-\n ifndef NO_MSGFMT_EXTENDED_OPTIONS\n \tMSGFMT += --check\n endif\n@@ -3008,9 +3004,6 @@ endif\n ifdef GIT_TEST_CMP\n \t@echo GIT_TEST_CMP=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_CMP)))'\\' >>$@+\n endif\n-ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n-\t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n-endif\n ifdef GIT_TEST_UTF8_LOCALE\n \t@echo GIT_TEST_UTF8_LOCALE=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_UTF8_LOCALE)))'\\' >>$@+\n endif\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4fab1c1984c..cd6e9f797b6 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1503,12 +1503,7 @@ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM\n \n if test -z \"$GIT_TEST_CMP\"\n then\n-\tif test -n \"$GIT_TEST_CMP_USE_COPIED_CONTEXT\"\n-\tthen\n-\t\tGIT_TEST_CMP=\"$DIFF -c\"\n-\telse\n-\t\tGIT_TEST_CMP=\"$DIFF -u\"\n-\tfi\n+\tGIT_TEST_CMP=\"$DIFF -u\"\n fi\n \n GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/build/lib\n"},{"id":"467247","messageId":"1s951161-s35o-59sr-7988-q90qr6n28n6n@tzk.qr","threadId":"58243","inReplyTo":"Y3B36HjDJhIY5jNz@nand.local","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-14T13:34:09Z","receivedAt":"2022-11-14T13:34:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Sat, 12 Nov 2022, Taylor Blau wrote:\n\n> On Sat, Nov 12, 2022 at 10:07:34PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > It is more performant to run `git diff --no-index` than running the\n> > `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> > Windows uses. And a lot more readable.\n>\n> This makes sense, and motivates the changes below.\n>\n> > Note: Earlier attempts at fixing this involved a test helper that avoids\n> > the overhead of the diff machinery, in favor of implementing a behavior\n> > that is more in line with what `mingw_test_cmp` does now, but that\n> > attempt saw a lot of backlash and distractions during review and was\n> > therefore abandoned.\n>\n> But I do not think that this paragraph adds as much as we'd like to the\n> historical record.\n\nIt explains why we replace a relatively simple logic with a relatively\ncomplex one.\n\n> What about the original approach did you have trouble pushing forward?\n> Perhaps framing that as an alternative approach, along with why it\n> didn't ultimately get merged would be a more useful recollection of\n> previous attempts here.\n>\n> One thing that the commit message doesn't allude to (that is covered in\n> the earlier discussion) is why it is important to pass\n> `--ignore-cr-at-eol`. I think that is worth mentioning here.\n\nI elaborated on both in the new commit message.\n\nCiao,\nDscho\n"},{"id":"467248","messageId":"998821n6-6sp3-30q9-95s6-p7rp5o8nr5rr@tzk.qr","threadId":"58243","inReplyTo":"8ec8f3f5-0827-25a3-464d-638378b65ddb@dunelm.org.uk","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-14T13:47:09Z","receivedAt":"2022-11-14T13:47:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Mon, 14 Nov 2022, Phillip Wood wrote:\n\n> On 12/11/2022 22:07, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > It is more performant to run `git diff --no-index` than running the\n> > `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> > Windows uses. And a lot more readable.\n>\n> This makes me wonder about the implications for our diff tests. We have\n> ~200 calls to `test_cmp` in t*-diff-*.sh. I had a look at\n> t4053-diff-no-index.sh and non of the tests that use `test_cmp` look critical\n> for functionality that is used by `test_cmp` and there are tests for the exit\n> code. I suspect that if `diff --no-index` breaks we'll end up with confusing\n> test failures rather than misleading passes.\n\nI was surprised how little of a dent the overhead makes (it was not even\nmeasurable within the noise) when I tested this a couple of weeks ago (but\nthen ran out of time and motivation to send a new iteration).\n\nIt would probably still be better to use less complex code, but Junio made\nhis preference abundantly clear, and if we wanted to save on CI time,\nthere are much bigger construction sites that we have to open. Point in\ncase: our CI runtime goes up, up, up, with no real restraint going on. A\nrandomly picked run from 6 weeks ago took 7h14m of build time\n(https://github.com/git/git/actions/runs/3175983767/usage), the current\n`seen` run took 8h48m\n(https://github.com/git/git/actions/runs/3448435246/usage). If there is\nconsiderate use of resources going on then please help me because I cannot\nsee it.\n\n> A side effect of this change is that on windows `test_cmp` will now except\n> directories as well as files but I don't think that matters.\n\nIndeed, I would believe that the non-Windows test jobs would catch that\nfailure, so we do not need any extra code to check specifically for that.\n\n> > Note: Earlier attempts at fixing this involved a test helper that avoids\n> > the overhead of the diff machinery, in favor of implementing a behavior\n> > that is more in line with what `mingw_test_cmp` does now, but that\n> > attempt saw a lot of backlash and distractions during review and was\n> > therefore abandoned.\n>\n> Hopefully most of the files we feed into `test_cmp` are small enough that the\n> absolute difference in run-time is not too big. There is an optimization for\n> -U0 that trims the common tail from the files before calling xdl_diff() but\n> that does not help here because we need to use --ignore-cr-at-eol (otherwise\n> `git diff --no-index -U0 || git diff --no-index` might speed up the common\n> case of matching files).\n\nThere is similar code in xdiff to \"shrink the box\" to avoid unnecessary\nwork, i.e. skipping identical lines at both ends of the line range:\nhttps://github.com/git/git/blob/v2.38.1/xdiff/xdiffi.c#L261-L265\n\nI share your concern, but my current concern is to cut the loss of time\nspent on this patch series on my part, and that concern is currently even\nbigger.\n\nCiao,\nDscho\n"},{"id":"467249","messageId":"pss2rp96-qr48-21p4-36on-ns04nnp0944q@tzk.qr","threadId":"58243","inReplyTo":"221114.86tu31lnwr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-14T14:02:47Z","receivedAt":"2022-11-14T14:03:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 14 Nov 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> [...] before this we've been assuming that GIT_TEST_CMP is a not-ours\n> binary. I.e. \"diff\", \"cmp\" etc. If it is a \"that's ours\" this change\n> should be changing e.g. '! test_cmp' to 'test_cmp !', as the former\n> would now hide segfaults.\n\nThe implied assumption of `test_cmp` is that the command is reliable, i.e.\nwe are not expecting to test the code that runs the comparison itself.\n\nOriginally, Junio shared your concern that `diff --no-index` might not be\nreliable enough, but that was \"a lifetime ago\". In\nhttps://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/ he hinted that the\n`diff --no-index` code has matured enough to be considered reliable. That\nis far from the endorsement I would wish to have received (it would have\nbeen nice to see \"I consider `git diff --no-index` to be reliable enough\nto serve as `mingw_test_cmp`\", for example), but I believe it is the\nclearest statement I will get in that regard.\n\n> Additionally: We don't *need* this for an initial implementation, but\n> having e.g. one of the Ubuntu CI targets run with \"git diff --no-index\"\n> would be a nice cherry on top,\n\nWhy would this be a nice cherry on top?\n\nFrom my perspective, it would co-opt unrelated test cases into the task of\nvalidating `diff --no-index`' correctness.\n\nSuch a loss of focus in test cases makes it harder to diagnose, debug and\nfix breakages. And it would mean that a single bug could make gazillions\nof test cases fail. That would be bad practice, of course.\n\n> * If we're trusting \"git diff --no-index\" to run the tests, we could\n> also get rid of \"GIT_TEST_CMP_USE_COPIED_CONTEXT\", whose only reason\n> for existing is to support a system \"diff\" that doesn't understand\n> \"-u\" (squashable diff below)\n\nBe my guest to contribute this once the current patch has made it to\n`next`. But please only then, we have enough friction on the Git mailing\nlist and do not need to go out of our way to add more.\n\nThanks,\nJohannes\n\n>\n> 1. https://lore.kernel.org/git/220907.86v8pzl6jz.gmgdl@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/220907.86r10nl63s.gmgdl@evledraar.gmail.com/\n>\n> diff --git a/Makefile b/Makefile\n> index 4927379184c..dea6069b5fe 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1950,10 +1950,6 @@ ifdef OVERRIDE_STRDUP\n> \tCOMPAT_OBJS += compat/strdup.o\n> endif\n>\n> -ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n> -\texport GIT_TEST_CMP_USE_COPIED_CONTEXT\n> -endif\n> -\n> ifndef NO_MSGFMT_EXTENDED_OPTIONS\n> \tMSGFMT += --check\n> endif\n> @@ -3008,9 +3004,6 @@ endif\n> ifdef GIT_TEST_CMP\n> \t@echo GIT_TEST_CMP=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_CMP)))'\\' >>$@+\n> endif\n> -ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n> -\t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n> -endif\n> ifdef GIT_TEST_UTF8_LOCALE\n> \t@echo GIT_TEST_UTF8_LOCALE=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_UTF8_LOCALE)))'\\' >>$@+\n> endif\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 4fab1c1984c..cd6e9f797b6 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1503,12 +1503,7 @@ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM\n>\n> if test -z \"$GIT_TEST_CMP\"\n> then\n> -\tif test -n \"$GIT_TEST_CMP_USE_COPIED_CONTEXT\"\n> -\tthen\n> -\t\tGIT_TEST_CMP=\"$DIFF -c\"\n> -\telse\n> -\t\tGIT_TEST_CMP=\"$DIFF -u\"\n> -\tfi\n> +\tGIT_TEST_CMP=\"$DIFF -u\"\n> fi\n>\n> GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/build/lib\n>\n"},{"id":"467250","messageId":"pull.1309.v4.git.1668434812.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v3.git.1668290855.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] tests(mingw): avoid super-slow mingw_test_cmp","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-14T14:06:50Z","receivedAt":"2022-11-14T14:07:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A few months ago, directly after sending a patch to fix a performance\nregression due to a mis-use of test_cmp\n[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],\nI got curious to see whether Git for Windows had the same issue. And it did\nnot: it passes t5351 in 22 seconds, even while using test_cmp to compare\npack files\n[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].\n\nThe explanation is of course that Git for Windows uses a test helper for\ntest_cmp that is written in C, instead of the Bash function. And C is much\nfaster than a Bash function, especially on Windows. This is especially sad\nwhen said Bash code is only used on Windows. So I originally had pulled out\nthis helper from the years-long effort to let Git for Windows use BusyBox'\nash to run the test suite. The result was a single-patch contribution of a\nchange that had been in Git for Windows since June 2018. Unfortunately, this\ntried-and-tested code was rejected by the Git maintainer.\n\nLet's fall back to the next-best solution: git diff --no-index, which the\nGit maintainer seems to like. The downside is that the diff machinery does a\nlot more than a simple cmp clone, and therefore a lot more things can go\nwrong that might make it look like a test case is failing when the fault is\nsomewhere else entirely. There is one way to find out whether this is a\nvalid concern.\n\nChanges since v3:\n\n * Fixed the subject of the cover letter (which should have been adjusted in\n v3)\n * Elaborated the paragraph about the historical context of this patch\n\nChanges since v2:\n\n * Dropped the test helper, using diff --no-index instead.\n\nChanges since v1:\n\n * Fixed double \"with\" in the commit message.\n * Renamed the test helper to text-cmp.\n * Made the diff --no-index call more robust by using a double-dash\n separator.\n\nJohannes Schindelin (2):\n t0021: use Windows-friendly `pwd`\n tests(mingw): avoid very slow `mingw_test_cmp`\n\n t/t0021-conversion.sh | 4 +--\n t/test-lib-functions.sh | 66 -----------------------------------------\n t/test-lib.sh | 2 +-\n 3 files changed, 3 insertions(+), 69 deletions(-)\n\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1309\n\nRange-diff vs v3:\n\n 1: b38b8fb5a85 = 1: b38b8fb5a85 t0021: use Windows-friendly `pwd`\n 2: a7f4265ceb2 ! 2: 128b1f348d8 tests(mingw): avoid very slow `mingw_test_cmp`\n @@ Commit message\n `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n Windows uses. And a lot more readable.\n \n - Note: Earlier attempts at fixing this involved a test helper that avoids\n - the overhead of the diff machinery, in favor of implementing a behavior\n - that is more in line with what `mingw_test_cmp` does now, but that\n - attempt saw a lot of backlash and distractions during review and was\n - therefore abandoned.\n + The original reason why Git's test suite needs the `mingw_test_cmp`\n + function at all (and why `cmp` is not good enough) is that Git's test\n + suite is not actually trying to compare binary files when it calls\n + `test_cmp`, but it compares text files. And those text files can contain\n + CR/LF line endings depending on the circumstances.\n +\n + Note: The original fix in the Git for Windows project implemented a test\n + helper that avoids the overhead of the diff machinery, in favor of\n + implementing a behavior that is more in line with what `mingw_test_cmp`\n + does now. This was done to minimize the risk in using something as\n + complex as the diff machinery to perform something as simple as\n + determining whether text output is identical to the expected output or\n + not. This approach has served Git for Windows well for years, but the\n + attempt to upstream this saw a lot of backlash and distractions during\n + the review, was disliked by the Git maintainer and was therefore\n + abandoned. For full details, see the thread at\n + https://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t\n \n Signed-off-by: Johannes Schindelin \n \n\n-- \ngitgitgadget\n"},{"id":"467251","messageId":"b38b8fb5a85e10d6189d68b6633adf23fca7b65d.1668434812.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v4.git.1668434812.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] t0021: use Windows-friendly `pwd`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-14T14:06:51Z","receivedAt":"2022-11-14T14:07:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows, when passing paths from shell scripts to regular\nWin32 executables, thanks to the MSYS2 runtime a somewhat magic path\nconversion happens that lets the shell script think that there is a file\nat `/git/Makefile` and the Win32 process it spawned thinks that the\nshell script said `C:/git-sdk-64/git/Makefile` instead.\n\nThis conversion is documented in detail over here:\nhttps://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion\n\nAs all automatic conversions, there are gaps. For example, to avoid\nmistaking command-line options like `/LOG=log.txt` (which are quite\ncommon in the Windows world) from being mistaken for a Unix-style\nabsolute path, the MSYS2 runtime specifically exempts arguments\ncontaining a `=` character from that conversion.\n\nWe are about to change `test_cmp` to use `git diff --no-index`, which\ninvolves spawning precisely such a Win32 process.\n\nIn combination, this would cause a failure in `t0021-conversion.sh`\nwhere we pass an absolute path containing an equal character to the\n`test_cmp` function.\n\nSeeing as the Unix tools like `cp` and `diff` that are used by Git's\ntest suite in the Git for Windows SDK (thanks to the MSYS2 project)\nunderstand both Unix-style as well as Windows-style paths, we can stave\noff this problem by simply switching to Windows-style paths and\nside-stepping the need for any automatic path conversion.\n\nNote: The `PATH` variable is obviously special, as it is colon-separated\nin the MSYS2 Bash used by Git for Windows, and therefore _cannot_\ncontain absolute Windows-style paths, lest the colon after the drive\nletter is mistaken for a path separator. Therefore, we need to be\ncareful to keep the Unix-style when modifying the `PATH` variable.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0021-conversion.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bad37abad2c..15482fa78e3 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-TEST_ROOT=\"$PWD\"\n-PATH=$TEST_ROOT:$PATH\n+PATH=$PWD:$PATH\n+TEST_ROOT=\"$(pwd)\"\n \n write_script <<\\EOF \"$TEST_ROOT/rot13.sh\"\n tr \\\n-- \ngitgitgadget\n\n"},{"id":"467252","messageId":"128b1f348d8fad730cac1c36d3082fab49904b2c.1668434812.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v4.git.1668434812.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-14T14:06:52Z","receivedAt":"2022-11-14T14:07:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is more performant to run `git diff --no-index` than running the\n`mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\nWindows uses. And a lot more readable.\n\nThe original reason why Git's test suite needs the `mingw_test_cmp`\nfunction at all (and why `cmp` is not good enough) is that Git's test\nsuite is not actually trying to compare binary files when it calls\n`test_cmp`, but it compares text files. And those text files can contain\nCR/LF line endings depending on the circumstances.\n\nNote: The original fix in the Git for Windows project implemented a test\nhelper that avoids the overhead of the diff machinery, in favor of\nimplementing a behavior that is more in line with what `mingw_test_cmp`\ndoes now. This was done to minimize the risk in using something as\ncomplex as the diff machinery to perform something as simple as\ndetermining whether text output is identical to the expected output or\nnot. This approach has served Git for Windows well for years, but the\nattempt to upstream this saw a lot of backlash and distractions during\nthe review, was disliked by the Git maintainer and was therefore\nabandoned. For full details, see the thread at\nhttps://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t\n\nSigned-off-by: Johannes Schindelin \n---\n t/test-lib-functions.sh | 66 -----------------------------------------\n t/test-lib.sh | 2 +-\n 2 files changed, 1 insertion(+), 67 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8c44856eaec..452fe9bc8aa 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1541,72 +1541,6 @@ test_skip_or_die () {\n \terror \"$2\"\n }\n \n-# The following mingw_* functions obey POSIX shell syntax, but are actually\n-# bash scripts, and are meant to be used only with bash on Windows.\n-\n-# A test_cmp function that treats LF and CRLF equal and avoids to fork\n-# diff when possible.\n-mingw_test_cmp () {\n-\t# Read text into shell variables and compare them. If the results\n-\t# are different, use regular diff to report the difference.\n-\tlocal test_cmp_a= test_cmp_b=\n-\n-\t# When text came from stdin (one argument is '-') we must feed it\n-\t# to diff.\n-\tlocal stdin_for_diff=\n-\n-\t# Since it is difficult to detect the difference between an\n-\t# empty input file and a failure to read the files, we go straight\n-\t# to diff if one of the inputs is empty.\n-\tif test -s \"$1\" && test -s \"$2\"\n-\tthen\n-\t\t# regular case: both files non-empty\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\telif test -s \"$1\" && test \"$2\" = -\n-\tthen\n-\t\t# read 2nd file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b\n-\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n-\telif test \"$1\" = - && test -s \"$2\"\n-\tthen\n-\t\t# read 1st file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n-\tfi\n-\ttest -n \"$test_cmp_a\" &&\n-\ttest -n \"$test_cmp_b\" &&\n-\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n-\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n-}\n-\n-# $1 is the name of the shell variable to fill in\n-mingw_read_file_strip_cr_ () {\n-\t# Read line-wise using LF as the line separator\n-\t# and use IFS to strip CR.\n-\tlocal line\n-\twhile :\n-\tdo\n-\t\tif IFS=$'\\r' read -r -d $'\\n' line\n-\t\tthen\n-\t\t\t# good\n-\t\t\tline=$line$'\\n'\n-\t\telse\n-\t\t\t# we get here at EOF, but also if the last line\n-\t\t\t# was not terminated by LF; in the latter case,\n-\t\t\t# some text was read\n-\t\t\tif test -z \"$line\"\n-\t\t\tthen\n-\t\t\t\t# EOF, really\n-\t\t\t\tbreak\n-\t\t\tfi\n-\t\tfi\n-\t\teval \"$1=\\$$1\\$line\"\n-\tdone\n-}\n-\n # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n # it also works for shell functions (though those functions cannot impact\n # the environment outside of the test_env invocation).\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7726d1da88a..f8c6205e08f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1546,7 +1546,7 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n+\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n-- \ngitgitgadget\n"},{"id":"467255","messageId":"221114.86pmdplbs5.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"pss2rp96-qr48-21p4-36on-ns04nnp0944q@tzk.qr","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-14T15:23:24Z","receivedAt":"2022-11-14T16:52:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 14 2022, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Mon, 14 Nov 2022, Ævar Arnfjörð Bjarmason wrote:\n>\n>> [...] before this we've been assuming that GIT_TEST_CMP is a not-ours\n>> binary. I.e. \"diff\", \"cmp\" etc. If it is a \"that's ours\" this change\n>> should be changing e.g. '! test_cmp' to 'test_cmp !', as the former\n>> would now hide segfaults.\n>\n> The implied assumption of `test_cmp` is that the command is reliable, i.e.\n> we are not expecting to test the code that runs the comparison itself.\n>\n> Originally, Junio shared your concern that `diff --no-index` might not be\n> reliable enough, but that was \"a lifetime ago\". In\n> https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/ he hinted that the\n> `diff --no-index` code has matured enough to be considered reliable. That\n> is far from the endorsement I would wish to have received (it would have\n> been nice to see \"I consider `git diff --no-index` to be reliable enough\n> to serve as `mingw_test_cmp`\", for example), but I believe it is the\n> clearest statement I will get in that regard.\n\nI don't see a mention of reliability in the original 2007 thread, just\noptimization v.s. \"cmp\". But the list archive seems to only carry the\nCL, perhaps it was in that 7/8 or 8/8 patch?\n\nIn any case, that's different than what I'm pointing out, which is:\n\nI fully trust \"git diff --no-index\" these days *in general* to serve as\na \"/usr/bin/diff -u\" replacement, but *only* as long as it's the\n\"/usr/bin/git\" version of \"git diff\", and not the \"git\" we just built\nin-tree.\n\nIOW this bit in t/README:\n\n - Don't use '! git cmd' when you want to make sure the git command\n exits with failure in a controlled way by calling \"die()\".[...]\n\nSo, before this series a test like this on Windows, with\nGIT_TEST_CMP=mingw_test_cmp:\n\n\t! test_cmp foo bar\n\nWould only succeed if \"foo\" and \"bar\" weren't identical, now it'll also\nsucceed if 'git diff --no-index' happens to segfault on those inputs.\n\nNow, the good thing is that the fallout of this should be relatively\ncontained in this case. We'd only get those cases sneaking in if it's in\nthe Windows-only code, as we use \"/usr/bin/diff -u\" elsewhere.\n\nI think the diff here at the end (on top af master, but you get the\nidea) is what you'd need to stop conflating those. Most of it's just\n's/! test_cmp/test_cmp !/g'.\n\nThe \"test_cmp\" then learns to distinguish between \"a git\" command and\n\"not a git\". I guess we could use test_must_fail_acceptable() there to\nto make this DWYM...\n\n>> Additionally: We don't *need* this for an initial implementation, but\n>> having e.g. one of the Ubuntu CI targets run with \"git diff --no-index\"\n>> would be a nice cherry on top,\n>\n> Why would this be a nice cherry on top?\n>\n> From my perspective, it would co-opt unrelated test cases into the task of\n> validating `diff --no-index`' correctness.\n>\n> Such a loss of focus in test cases makes it harder to diagnose, debug and\n> fix breakages. And it would mean that a single bug could make gazillions\n> of test cases fail. That would be bad practice, of course.\n\nYeah, probably not worth it. It would illustrate which\n--ignore-cr-at-eol is just due to test_cmp v.s. test_cmp_bin semantics\non *nix though. E.g. in addition to the below this makes things pass\nwithout --ignore-cr-at-eol for me (aside from the t0001-init.sh failure\nI noted in the v2 feedback):\n\n\tdiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n\tindex 7b5423eebda..f2e6b7a078c 100755\n\t--- a/t/t0061-run-command.sh\n\t+++ b/t/t0061-run-command.sh\n\t@@ -72,7 +72,7 @@ test_expect_success 'run_command does not try to execute a directory' '\n\t \n\t \tPATH=$PWD/bin1:$PWD/bin2:$PATH \\\n\t \t\ttest-tool run-command run-command greet >actual 2>err &&\n\t-\ttest_cmp bin2/greet actual &&\n\t+\ttest_cmp_bin bin2/greet actual &&\n\t \ttest_must_be_empty err\n\t '\n\t \n\t@@ -89,7 +89,7 @@ test_expect_success POSIXPERM 'run_command passes over non-executable file' '\n\t \n\t \tPATH=$PWD/bin1:$PWD/bin2:$PATH \\\n\t \t\ttest-tool run-command run-command greet >actual 2>err &&\n\t-\ttest_cmp bin2/greet actual &&\n\t+\ttest_cmp_bin bin2/greet actual &&\n\t \ttest_must_be_empty err\n\t '\n\t \n\tdiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\n\tindex 497b62868d4..4001ac24adf 100755\n\t--- a/t/t4120-apply-popt.sh\n\t+++ b/t/t4120-apply-popt.sh\n\t@@ -83,7 +83,7 @@ test_expect_success 'apply (-p2) diff, rename' '\n\t \tcp file1.saved file1 &&\n\t \trm -f file2 &&\n\t \tgit apply -p2 patch.rename &&\n\t-\ttest_cmp expected file2\n\t+\ttest_cmp_bin expected file2\n\t '\n\t \n\t test_done\n\tdiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n\tindex 5d853225a93..4c64396ed15 100644\n\t--- a/t/test-lib-functions.sh\n\t+++ b/t/test-lib-functions.sh\n\t@@ -1163,9 +1163,9 @@ test_cmp () {\n\t \t\tif test -n \"$negate\"\n\t \t\tthen\n\t \t\t\ttest_expect_code 1 env GIT_DIR=/dev/null \\\n\t-\t\t\t\t\t git diff --no-index --ignore-cr-at-eol -- \"$@\"\n\t+\t\t\t\t\t git diff --no-index -- \"$@\"\n\t \t\telse\n\t-\t\t\tGIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- \"$@\"\n\t+\t\t\tGIT_DIR=/dev/null git diff --no-index -- \"$@\"\n\t \t\tfi\n\t \telse\n\t \t\tif test -n \"$negate\"\n\n\n>> * If we're trusting \"git diff --no-index\" to run the tests, we could\n>> also get rid of \"GIT_TEST_CMP_USE_COPIED_CONTEXT\", whose only reason\n>> for existing is to support a system \"diff\" that doesn't understand\n>> \"-u\" (squashable diff below)\n>\n> Be my guest to contribute this once the current patch has made it to\n> `next`. But please only then, we have enough friction on the Git mailing\n> list and do not need to go out of our way to add more.\n\nYeah, we can leave the nice-to-haves for later, a \"conflates files being\ndifferent with segfaults\" really should be fixed though. And hopefully\nthe below helps.\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 1f6b9b08d1d..d26ed200cbf 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -304,7 +304,7 @@ check_access_log() {\n \tsort \"$1\" >\"$1\".sorted &&\n \tstrip_access_log >access.log.stripped &&\n \tsort access.log.stripped >access.log.sorted &&\n-\tif ! test_cmp \"$1\".sorted access.log.sorted\n+\tif test_cmp ! \"$1\".sorted access.log.sorted\n \tthen\n \t\ttest_cmp \"$1\" access.log.stripped\n \tfi\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex abecd75e4e4..6e1e6c056ff 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -751,7 +751,7 @@ test_expect_success 'process filter should restart after unexpected write failur\n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n \n \t\t# Smudge failed\n-\t\t! test_cmp smudge-write-fail.o smudge-write-fail.r &&\n+\t\ttest_cmp ! smudge-write-fail.o smudge-write-fail.r &&\n \t\trot13.sh expected &&\n \t\tgit cat-file blob :smudge-write-fail.r >actual &&\n \t\ttest_cmp expected actual\ndiff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh\nindex cd3969e852b..dfab275dbe6 100755\n--- a/t/t0450-txt-doc-vs-help.sh\n+++ b/t/t0450-txt-doc-vs-help.sh\n@@ -14,7 +14,7 @@ test_expect_success 'setup: list of builtins' '\n \n test_expect_success 'list of txt and help mismatches is sorted' '\n \tsort -u \"$TEST_DIRECTORY\"/t0450/txt-help-mismatches >expect &&\n-\tif ! test_cmp expect \"$TEST_DIRECTORY\"/t0450/txt-help-mismatches\n+\tif test_cmp ! expect \"$TEST_DIRECTORY\"/t0450/txt-help-mismatches\n \tthen\n \t\tBUG \"please keep the list of txt and help mismatches sorted\"\n \tfi\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e1..d11980916b0 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1214,7 +1214,7 @@ test_expect_success 'checkout-index with folders' '\n \t# entry is a sparse directory.\n \trun_on_all test_must_fail git checkout-index -f -- folder1/ &&\n \ttest_cmp full-checkout-err sparse-checkout-err &&\n-\t! test_cmp full-checkout-err sparse-index-err &&\n+\ttest_cmp ! full-checkout-err sparse-index-err &&\n \tgrep \"is a sparse directory\" sparse-index-err\n '\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex cf58cf025cd..14b6b58f531 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1403,7 +1403,7 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '\n \ttest_must_fail git rev-parse refs/bisect/something &&\n \tgit update-ref refs/bisect/something HEAD &&\n \tgit rev-parse refs/bisect/something >main-head &&\n-\t! test_cmp main-head worktree-head\n+\ttest_cmp ! main-head worktree-head\n '\n \n test_expect_success 'transaction handles empty commit' '\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7f605f865b3..dafb7860451 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -778,7 +778,7 @@ test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n \n \tgit -C subdir rev-parse --absolute-git-dir >subdir.dir &&\n \tgit rev-parse --absolute-git-dir >our.dir &&\n-\t! test_cmp subdir.dir our.dir &&\n+\ttest_cmp ! subdir.dir our.dir &&\n \n \tgit -C subdir log &&\n \tgit -C subdir branch rename-src &&\ndiff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh\nindex fe72b247164..d2fba236a19 100755\n--- a/t/t3207-branch-submodule.sh\n+++ b/t/t3207-branch-submodule.sh\n@@ -155,7 +155,7 @@ test_expect_success 'should create branches if branch exists and --force is give\n \t\ttest_cmp expected actual-new-branch-a &&\n \t\t# assert that branch --force actually moved the sub\n \t\t# branch\n-\t\t! test_cmp expected actual-old-branch-a\n+\t\ttest_cmp ! expected actual-old-branch-a\n \t)\n '\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex de1da4673da..3e7ceb27f3c 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2136,8 +2136,8 @@ test_expect_success 'format-patch --base' '\n \tawk \"{print \\\"prerequisite-patch-id:\\\", \\$1}\" >fail &&\n \n \tsignature >>fail &&\n-\t! test_cmp fail actual1 &&\n-\t! test_cmp fail actual2\n+\ttest_cmp ! fail actual1 &&\n+\ttest_cmp ! fail actual2\n '\n \n test_expect_success 'format-patch --base errors out when base commit is in revision list' '\ndiff --git a/t/t4125-apply-ws-fuzz.sh b/t/t4125-apply-ws-fuzz.sh\nindex 090987c89b2..d2e61579d0a 100755\n--- a/t/t4125-apply-ws-fuzz.sh\n+++ b/t/t4125-apply-ws-fuzz.sh\n@@ -93,7 +93,7 @@ test_expect_success 'withfix (backward)' '\n \tsed -n -e /h/p file-fixed >fixed-tail &&\n \tsed -n -e /h/p file >file-tail &&\n \n-\t! test_cmp fixed-tail file-tail\n+\ttest_cmp ! fixed-tail file-tail\n \n '\n \ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex cdad4b68807..8028aa2b712 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -837,7 +837,7 @@ test_expect_success 'am without --committer-date-is-author-date' '\n \tgit cat-file commit HEAD | sed -e \"/^\\$/q\" >head1 &&\n \tsed -ne \"/^author /s/.*> //p\" head1 >at &&\n \tsed -ne \"/^committer /s/.*> //p\" head1 >ct &&\n-\t! test_cmp at ct\n+\ttest_cmp ! at ct\n '\n \n # This checks for +0000 because TZ is set to UTC and that should\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a7fa94ce0a2..edc11213a10 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -74,7 +74,7 @@ test_expect_success 'patch-id detects equality' '\n test_expect_success 'patch-id detects inequality' '\n \tget_patch_id main &&\n \tget_patch_id notsame &&\n-\t! test_cmp patch-id_main patch-id_notsame\n+\ttest_cmp ! patch-id_main patch-id_notsame\n '\n test_expect_success 'patch-id detects equality binary' '\n \tcat >.gitattributes <<-\\EOF &&\n@@ -100,7 +100,7 @@ test_expect_success 'patch-id detects inequality binary' '\n \tEOF\n \tget_patch_id main &&\n \tget_patch_id notsame &&\n-\t! test_cmp patch-id_main patch-id_notsame &&\n+\ttest_cmp ! patch-id_main patch-id_notsame &&\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n@@ -126,7 +126,7 @@ cmp_patch_id () {\n \tif\n \t\ttest \"$1\" = \"relevant\"\n \tthen\n-\t\t! test_cmp patch-id_\"$2\" patch-id_\"$3\"\n+\t\ttest_cmp ! patch-id_\"$2\" patch-id_\"$3\"\n \telse\n \t\ttest_cmp patch-id_\"$2\" patch-id_\"$3\"\n \tfi\n@@ -279,7 +279,7 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' '\n \ttest_cmp patch-id_nonl patch-id_withnl &&\n \tcalc_patch_id nonl-inc-ws --verbatim b &&\n-\t! test_cmp a b\n+\ttest_cmp ! a b\n '\n \n test_expect_success 'info/refs updates when changes are made' '\n@@ -36,7 +36,7 @@ test_expect_success 'info/refs updates when changes are made' '\n \tgit update-ref refs/heads/foo HEAD &&\n \tgit update-server-info &&\n \ttest-tool chmtime --get .git/info/refs >b &&\n-\t! test_cmp a b\n+\ttest_cmp ! a b\n '\n \n test_done\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6d693eef82f..9718f0ab12a 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -400,7 +400,7 @@ test_bitmap_cases () {\n \t\t\ttest-tool bitmap list-commits | sort >bitmaps &&\n \t\t\tcomm -13 bitmaps commits >after &&\n \n-\t\t\t! test_cmp before after\n+\t\t\ttest_cmp ! before after\n \t\t)\n \t'\n \ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 0882cbb6e4a..338ccd32fac 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -186,7 +186,7 @@ test_midx_bitmap_cases () {\n \t\t\ttest-tool bitmap list-commits | sort >bitmaps &&\n \t\t\tcomm -13 bitmaps commits >after &&\n \n-\t\t\t! test_cmp before after\n+\t\t\ttest_cmp ! before after\n \t\t)\n \t'\n \n@@ -268,7 +268,7 @@ test_midx_bitmap_cases () {\n \t\t\ttest-tool bitmap list-commits | sort >bitmaps &&\n \t\t\tcomm -13 bitmaps commits >after &&\n \n-\t\t\t! test_cmp before after\n+\t\t\ttest_cmp ! before after\n \t\t)\n \t'\n \ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 20d063fb9ae..7037335533c 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -111,7 +111,7 @@ test_expect_success 'use \"origin\" when no remote specified and multiple found' '\n \n test_expect_success 'suppress \"From \" with -q' '\n \tgit ls-remote -q 2>actual_err &&\n-\t! test_cmp exp_err actual_err\n+\ttest_cmp ! exp_err actual_err\n '\n \n test_expect_success 'use branch..remote if possible' '\ndiff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh\nindex 3126cfd7e9d..eeef68c9f3d 100755\n--- a/t/t5612-clone-refspec.sh\n+++ b/t/t5612-clone-refspec.sh\n@@ -90,7 +90,7 @@ test_expect_success 'by default no tags will be kept updated' '\n \t\tgit for-each-ref refs/tags >../actual\n \t) &&\n \tgit for-each-ref refs/tags >expect &&\n-\t! test_cmp expect actual &&\n+\ttest_cmp ! expect actual &&\n \ttest_line_count = 2 actual\n '\n \ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex 41d0ca00b1c..c927b0d9e62 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -538,7 +538,7 @@ test_expect_success '--abbrev' '\n \tsed -e \"s/$OID_REGEX/LONG/g\" -e \"s/$_x05/SHORT/g\" fuzzy3 &&\n \ttest_cmp expect2 fuzzy2 &&\n \ttest_cmp expect3 fuzzy3 &&\n-\t! test_cmp actual1 actual2\n+\ttest_cmp ! actual1 actual2\n '\n \n test_expect_success '%H is not affected by --abbrev-commit' '\ndiff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh\nindex 523efbecde1..d4819317649 100755\n--- a/t/t7107-reset-pathspec-file.sh\n+++ b/t/t7107-reset-pathspec-file.sh\n@@ -24,7 +24,7 @@ verify_expect () {\n \tgit status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual &&\n \tif test \"x$1\" = 'x!'\n \tthen\n-\t\t! test_cmp expect actual\n+\t\ttest_cmp ! expect actual\n \telse\n \t\ttest_cmp expect actual\n \tfi\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eae6a46ef3d..904f91fc4bc 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -451,7 +451,7 @@ test_expect_success 'status from subdirectory should have the same SHA1' '\n \t\tgit submodule status >output &&\n \t\tawk \"{print \\$1}\" expect2 &&\n \t\ttest_cmp expect2 actual2 &&\n-\t\t! test_cmp actual actual2\n+\t\ttest_cmp ! actual actual2\n \t)\n '\n \ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 2b7ef6c41a4..fb3d51de13d 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1510,7 +1510,7 @@ test_expect_success '\"status.branch=true\" same as \"-b\"' '\n test_expect_success '\"status.branch=true\" different from \"--no-branch\"' '\n \tgit status -s --no-branch >expected_nobranch &&\n \tgit -c status.branch=true status -s >actual &&\n-\t! test_cmp expected_nobranch actual\n+\ttest_cmp ! expected_nobranch actual\n '\n \n test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 7c3f6ed9943..acdf91a77a2 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -1059,7 +1059,7 @@ test_expect_success 'merge --quit' '\n \t\ttest_path_is_missing .git/MERGE_MSG &&\n \t\tgit rerere status >rerere.after &&\n \t\ttest_must_be_empty rerere.after &&\n-\t\t! test_cmp rerere.after rerere.before\n+\t\ttest_cmp ! rerere.after rerere.before\n \t)\n '\n \ndiff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh\nindex 81fb7c474c1..687dbcc9d23 100755\n--- a/t/t7606-merge-custom.sh\n+++ b/t/t7606-merge-custom.sh\n@@ -55,7 +55,7 @@ test_expect_success 'merge c2 with a custom strategy' '\n \tgit diff --exit-code c2 HEAD &&\n \tgit diff --exit-code c2 &&\n \n-\t! test_cmp head.old head.new &&\n+\ttest_cmp ! head.old head.new &&\n \ttest_cmp head.old first-parent &&\n \ttest_cmp second-parent.expected second-parent &&\n \ttest_cmp tree.expected tree &&\n@@ -81,7 +81,7 @@ test_expect_success 'trivial merge with custom strategy' '\n \tgit diff --exit-code c3 HEAD &&\n \tgit diff --exit-code c3 &&\n \n-\t! test_cmp head.old head.new &&\n+\ttest_cmp ! head.old head.new &&\n \ttest_cmp head.old first-parent &&\n \ttest_cmp second-parent.expected second-parent &&\n \ttest_cmp tree.expected tree &&\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 5be483bf887..a1d62e8566c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -365,7 +365,7 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n \t\ttest-tool bitmap list-commits | sort >bitmaps &&\n \t\tcomm -13 bitmaps commits >after &&\n \n-\t\t! test_cmp before after\n+\t\ttest_cmp ! before after\n \t)\n '\n \ndiff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\nindex b18633dee1b..d8f26d1197b 100755\n--- a/t/t8013-blame-ignore-revs.sh\n+++ b/t/t8013-blame-ignore-revs.sh\n@@ -184,10 +184,10 @@ test_expect_success mark_ignored_lines '\n \ttest_cmp expect actual &&\n \n \tsed -n \"3p\" blame_raw | cut -c1 >actual &&\n-\t! test_cmp expect actual &&\n+\ttest_cmp ! expect actual &&\n \n \tsed -n \"4p\" blame_raw | cut -c1 >actual &&\n-\t! test_cmp expect actual\n+\ttest_cmp ! expect actual\n '\n \n # For ignored revs that added 'unblamable' lines and more recent commits changed\ndiff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\nindex 1465156072e..53f240e45cf 100755\n--- a/t/t9164-git-svn-dcommit-concurrent.sh\n+++ b/t/t9164-git-svn-dcommit-concurrent.sh\n@@ -94,7 +94,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n \t\techo 1 >> file &&\n \t\tsvn_cmd commit -m \"changing file\" &&\n \t\tsvn_cmd up &&\n-\t\t! test_cmp auto_updated_file au_file_saved\n+\t\ttest_cmp ! auto_updated_file au_file_saved\n \t)\n '\n \ndiff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh\nindex 77047e250dc..29a2d3a03a7 100755\n--- a/t/t9351-fast-export-anonymize.sh\n+++ b/t/t9351-fast-export-anonymize.sh\n@@ -133,7 +133,7 @@ test_expect_success 'idents are shared' '\n \tgit log --all --format=\"%cn <%ce>\" >committers &&\n \tsort -u committers >unique &&\n \ttest_line_count = 1 unique &&\n-\t! test_cmp authors committers\n+\ttest_cmp ! authors committers\n '\n \n test_done\ndiff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\nindex 5fe83315ecd..6fdfa7d5855 100755\n--- a/t/t9810-git-p4-rcs.sh\n+++ b/t/t9810-git-p4-rcs.sh\n@@ -68,7 +68,7 @@ scrub_k_check () {\n \tfile=\"$1\" &&\n \tscrub=\"$TRASH_DIRECTORY/$file\" &&\n \t\"$PYTHON_PATH\" \"$TRASH_DIRECTORY/scrub_k.py\" <\"$git/$file\" >\"$scrub\" &&\n-\t! test_cmp \"$cli/$file\" \"$scrub\" &&\n+\ttest_cmp ! \"$cli/$file\" \"$scrub\" &&\n \ttest_cmp \"$git/$file\" \"$scrub\" &&\n \trm \"$scrub\"\n }\n@@ -76,7 +76,7 @@ scrub_ko_check () {\n \tfile=\"$1\" &&\n \tscrub=\"$TRASH_DIRECTORY/$file\" &&\n \t\"$PYTHON_PATH\" \"$TRASH_DIRECTORY/scrub_ko.py\" <\"$git/$file\" >\"$scrub\" &&\n-\t! test_cmp \"$cli/$file\" \"$scrub\" &&\n+\ttest_cmp ! \"$cli/$file\" \"$scrub\" &&\n \ttest_cmp \"$git/$file\" \"$scrub\" &&\n \trm \"$scrub\"\n }\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b32..5d853225a93 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1148,8 +1148,33 @@ test_expect_code () {\n # - not all diff versions understand \"-u\"\n \n test_cmp () {\n-\ttest \"$#\" -ne 2 && BUG \"2 param\"\n-\teval \"$GIT_TEST_CMP\" '\"$@\"'\n+\ttest \"$#\" -lt 2 && BUG \"2..3 param\"\n+\ttest \"$#\" -gt 3 && BUG \"2..3 param\"\n+\n+\tlocal negate=\n+\tif test \"$#\" = 3 && test \"$1\" = \"!\"\n+\tthen\n+\t\tnegate=t\n+\t\tshift\n+\tfi\n+\n+\tif test \"$GIT_TEST_CMP\" = \"builtin:git-no-index\"\n+\tthen\n+\t\tif test -n \"$negate\"\n+\t\tthen\n+\t\t\ttest_expect_code 1 env GIT_DIR=/dev/null \\\n+\t\t\t\t\t git diff --no-index --ignore-cr-at-eol -- \"$@\"\n+\t\telse\n+\t\t\tGIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- \"$@\"\n+\t\tfi\n+\telse\n+\t\tif test -n \"$negate\"\n+\t\tthen\n+\t\t\t! \"$GIT_TEST_CMP\" \"$@\"\n+\t\telse\n+\t\t\t\"$GIT_TEST_CMP\" \"$@\"\n+\t\tfi\n+\tfi\n }\n \n # Check that the given config key has the expected value.\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 6db377f68b8..3028283f307 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1721,7 +1721,7 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n+\tGIT_TEST_CMP=builtin:git-no-index\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n"},{"id":"467278","messageId":"Y3LD1CtKBoDhPdSZ@nand.local","threadId":"58243","inReplyTo":"128b1f348d8fad730cac1c36d3082fab49904b2c.1668434812.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-14T22:40:20Z","receivedAt":"2022-11-14T22:40:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 14, 2022 at 02:06:52PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> It is more performant to run `git diff --no-index` than running the\n> `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> Windows uses. And a lot more readable.\n>\n> The original reason why Git's test suite needs the `mingw_test_cmp`\n> function at all (and why `cmp` is not good enough) is that Git's test\n> suite is not actually trying to compare binary files when it calls\n> `test_cmp`, but it compares text files. And those text files can contain\n> CR/LF line endings depending on the circumstances.\n>\n> Note: The original fix in the Git for Windows project implemented a test\n> helper that avoids the overhead of the diff machinery, in favor of\n> implementing a behavior that is more in line with what `mingw_test_cmp`\n> does now. This was done to minimize the risk in using something as\n> complex as the diff machinery to perform something as simple as\n> determining whether text output is identical to the expected output or\n> not. This approach has served Git for Windows well for years, but the\n> attempt to upstream this saw a lot of backlash and distractions during\n> the review, was disliked by the Git maintainer and was therefore\n> abandoned. For full details, see the thread at\n> https://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t\n\nIn the previous round, you wrote that this paragraph:\n\n It explains why we replace a relatively simple logic with a relatively\n complex one.\n\nBut I'm not sure the rewritten version does what you claim, at least in\nmy own personal opinion.\n\nIt is not helpful to say the original approach \"saw a lot of backlash\".\nIt is helpful, on the other hand, to say what about the original\napproach gave reviewers pause, and why that alternative approach isn't\npursued here.\n\nMaybe I'm splitting hairs here, but I really do stand by my original\nsuggestion from back in [1].\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/Y3B36HjDJhIY5jNz@nand.local/\n"},{"id":"467520","messageId":"qp0556o2-6q18-8556-0n8s-p28831qr6qs6@tzk.qr","threadId":"58243","inReplyTo":"Y3LD1CtKBoDhPdSZ@nand.local","subject":"Re: [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-18T13:32:28Z","receivedAt":"2022-11-18T13:33:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Mon, 14 Nov 2022, Taylor Blau wrote:\n\n> On Mon, Nov 14, 2022 at 02:06:52PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > It is more performant to run `git diff --no-index` than running the\n> > `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n> > Windows uses. And a lot more readable.\n> >\n> > The original reason why Git's test suite needs the `mingw_test_cmp`\n> > function at all (and why `cmp` is not good enough) is that Git's test\n> > suite is not actually trying to compare binary files when it calls\n> > `test_cmp`, but it compares text files. And those text files can contain\n> > CR/LF line endings depending on the circumstances.\n> >\n> > Note: The original fix in the Git for Windows project implemented a test\n> > helper that avoids the overhead of the diff machinery, in favor of\n> > implementing a behavior that is more in line with what `mingw_test_cmp`\n> > does now. This was done to minimize the risk in using something as\n> > complex as the diff machinery to perform something as simple as\n> > determining whether text output is identical to the expected output or\n> > not. This approach has served Git for Windows well for years, but the\n> > attempt to upstream this saw a lot of backlash and distractions during\n> > the review, was disliked by the Git maintainer and was therefore\n> > abandoned. For full details, see the thread at\n> > https://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t\n>\n> In the previous round, you wrote that this paragraph:\n>\n> It explains why we replace a relatively simple logic with a relatively\n> complex one.\n\nYes. And I obviously got myself misunderstood.\n\nThe simple logic I refer to is the `t/helper/test-cmp.c` that I abandoned.\n\nThe complex logic is the diff machinery we now call, which is a lot more\ninvolved and goes through a lot more code paths.\n\n> But I'm not sure the rewritten version does what you claim, at least in\n> my own personal opinion.\n>\n> It is not helpful to say the original approach \"saw a lot of backlash\".\n\nIt is the nicest thing I can say about it.\n\nAnd I want people who are curious what happened to Git for Windows' custom\ncode to have an explanation in the commit messages.\n\n> It is helpful, on the other hand, to say what about the original\n> approach gave reviewers pause, and why that alternative approach isn't\n> pursued here.\n>\n> Maybe I'm splitting hairs here, but I really do stand by my original\n> suggestion from back in [1].\n\nWe can also keep hitting a dead horse, but I don't think that will make\nanything any better.\n\nThanks,\nJohannes\n\n>\n> Thanks,\n> Taylor\n>\n> [1]: https://lore.kernel.org/git/Y3B36HjDJhIY5jNz@nand.local/\n>\n"},{"id":"467526","messageId":"Y3fLdJ7CIIU8fj0g@nand.local","threadId":"58243","inReplyTo":"qp0556o2-6q18-8556-0n8s-p28831qr6qs6@tzk.qr","subject":"Re: [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-18T18:14:12Z","receivedAt":"2022-11-18T18:14:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 18, 2022 at 02:32:28PM +0100, Johannes Schindelin wrote:\n> > But I'm not sure the rewritten version does what you claim, at least in\n> > my own personal opinion.\n> >\n> > It is not helpful to say the original approach \"saw a lot of backlash\".\n>\n> It is the nicest thing I can say about it.\n\nI don't think you have to or should refer to the earlier round of review\nat all.\n\n> > It is helpful, on the other hand, to say what about the original\n> > approach gave reviewers pause, and why that alternative approach isn't\n> > pursued here.\n> >\n> > Maybe I'm splitting hairs here, but I really do stand by my original\n> > suggestion from back in [1].\n>\n> We can also keep hitting a dead horse, but I don't think that will make\n> anything any better.\n\nInstead, it would be much more helpful to explain what you tried before,\nand why you are taking a different approach now.\n\nI am simply not comfortable with taking the patch with the way the body\nis currently written.\n\nThanks,\nTaylor\n"},{"id":"467540","messageId":"xmqqv8nbkg77.fsf@gitster.g","threadId":"58243","inReplyTo":"Y3B36HjDJhIY5jNz@nand.local","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-18T23:15:08Z","receivedAt":"2022-11-18T23:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> One thing that the commit message doesn't allude to (that is covered in\n> the earlier discussion) is why it is important to pass\n> `--ignore-cr-at-eol`. I think that is worth mentioning here.\n\nIsn't it because Git on the platform is expected to use CRLF in\ncertain places, unlike on other platforms where LF is used, but the\nplatform port hasn't adjusted tests to match that expectation? And\nvice versa, where Git is expected to produce LF terminated text\neverywhere but the expected output is not \"ported\" to force LF\ntermination and instead produces CRLF terminated text on platforms\nwhose native line ending is CRLF?\n\nUse of \"ignore-cr-at-eol\" may allow such tests that are not ported\ncorrectly to prepare expected output with a \"wrong\" line ending and\nstill pass, and I do think it may be an expedite way to make tests\nappear to pass.\n\nBut I worry that it may not be a good thing for the health of the\nWindows port in the longer term.\n\nWhen Git is expected to produce platform-native line endings, if a\ntest is not adjusted to expect platform-native output and instead\nuses the ignore-cr-at-eol workaround, a future breakage that makes\nGit to produce LF terminated lines everywhere would not be caught,\nwould it?\n"},{"id":"467541","messageId":"xmqqr0xzkfz8.fsf@gitster.g","threadId":"58243","inReplyTo":"221114.86pmdplbs5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-18T23:19:55Z","receivedAt":"2022-11-18T23:33:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> IOW this bit in t/README:\n>\n> - Don't use '! git cmd' when you want to make sure the git command\n> exits with failure in a controlled way by calling \"die()\".[...]\n>\n> So, before this series a test like this on Windows, with\n> GIT_TEST_CMP=mingw_test_cmp:\n>\n> \t! test_cmp foo bar\n>\n> Would only succeed if \"foo\" and \"bar\" weren't identical, now it'll also\n> succeed if 'git diff --no-index' happens to segfault on those inputs.\n\nWell \"! test_cmp\" is wrong anyway, because it _expects_ two files\nare the same and gives more detailed diagnosis when they differ by\ngiving \"diff\" output.\n\nIf you expect them to be different, \"! test_cmp\" would give\n\"detailed diagnosis\" in the wrong case, i.e. the outcome is what we\nexpect.\n\nSo the caller must do \"test_cmp !\" whether the underlying\nimplementation of test_cmp uses \"diff -u\" or \"diff --no-index\".\n"},{"id":"467553","messageId":"Y3hFITt+8VUubC8v@nand.local","threadId":"58243","inReplyTo":"xmqqv8nbkg77.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-19T02:53:21Z","receivedAt":"2022-11-19T02:53:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 18, 2022 at 03:15:08PM -0800, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > One thing that the commit message doesn't allude to (that is covered in\n> > the earlier discussion) is why it is important to pass\n> > `--ignore-cr-at-eol`. I think that is worth mentioning here.\n>\n> Isn't it because Git on the platform is expected to use CRLF in\n> certain places, unlike on other platforms where LF is used, but the\n> platform port hasn't adjusted tests to match that expectation? And\n> vice versa, where Git is expected to produce LF terminated text\n> everywhere but the expected output is not \"ported\" to force LF\n> termination and instead produces CRLF terminated text on platforms\n> whose native line ending is CRLF?\n\nYes, I think that's right. My suggestion to Johannes was to (a) make\nsure that your and my understanding is correct, and (b) to memorialize\nthat understanding in the commit message itself.\n\n> Use of \"ignore-cr-at-eol\" may allow such tests that are not ported\n> correctly to prepare expected output with a \"wrong\" line ending and\n> still pass, and I do think it may be an expedite way to make tests\n> appear to pass.\n>\n> But I worry that it may not be a good thing for the health of the\n> Windows port in the longer term.\n\nI share your concerns, too.\n\nThanks,\nTaylor\n"},{"id":"467554","messageId":"Y3hF1vOQ8waqpvXo@nand.local","threadId":"58243","inReplyTo":"xmqqr0xzkfz8.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-19T02:56:22Z","receivedAt":"2022-11-19T02:56:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 18, 2022 at 03:19:55PM -0800, Junio C Hamano wrote:\n> Well \"! test_cmp\" is wrong anyway, because it _expects_ two files\n> are the same and gives more detailed diagnosis when they differ by\n> giving \"diff\" output.\n>\n> If you expect them to be different, \"! test_cmp\" would give\n> \"detailed diagnosis\" in the wrong case, i.e. the outcome is what we\n> expect.\n\nI agree that \"! test_cmp foo bar\" will give output about how \"foo\" is\ndifferent from \"bar\" (and halt the test only when the two have the same\ncontents).\n\n> So the caller must do \"test_cmp !\" whether the underlying\n> implementation of test_cmp uses \"diff -u\" or \"diff --no-index\".\n\nBut this confuses me. \"git grep 'test_cmp !'\" turns up no results, and\nfurthermore, test_cmp() itself begins with:\n\n test \"$#\" -ne 2 && BUG \"2 param\"\n\nSo I am not sure what you are referring to.\n\nThanks,\nTaylor\n"},{"id":"467560","messageId":"65ff24be-4392-f236-5500-ce0c0d4d42ca@kdbg.org","threadId":"58243","inReplyTo":"xmqqv8nbkg77.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-11-19T08:18:09Z","receivedAt":"2022-11-19T08:18:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.11.22 um 00:15 schrieb Junio C Hamano:\n> Taylor Blau writes:\n> \n>> One thing that the commit message doesn't allude to (that is covered in\n>> the earlier discussion) is why it is important to pass\n>> `--ignore-cr-at-eol`. I think that is worth mentioning here.\n> \n> Isn't it because Git on the platform is expected to use CRLF in\n> certain places, unlike on other platforms where LF is used, but the\n> platform port hasn't adjusted tests to match that expectation? And\n> vice versa, where Git is expected to produce LF terminated text\n> everywhere but the expected output is not \"ported\" to force LF\n> termination and instead produces CRLF terminated text on platforms\n> whose native line ending is CRLF?\n> \n> Use of \"ignore-cr-at-eol\" may allow such tests that are not ported\n> correctly to prepare expected output with a \"wrong\" line ending and\n> still pass, and I do think it may be an expedite way to make tests\n> appear to pass.\n\nThe reason that mingw_test_cmp exists is not that Git isn't ported\ncorrectly, or that tests aren't ported correctly. The reason is that\ntests assume Unix LF line endings everywhere, but there are some tools\nthat are outside our control that randomly -- to the layman's eye --\nproduce CRLF line endings even when their input has LF style.\n\nFor example, when we post-process Git output with `sed`, the result\nsuddenly has CRLF line endings instead of LF that the input had.\n\n> When Git is expected to produce platform-native line endings,...\nThere is no such requirement or expectation on Windows. LF style is\nacceptable.\n\n-- Hannes\n\n"},{"id":"467564","messageId":"221119.86wn7rdugi.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"Y3hF1vOQ8waqpvXo@nand.local","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-19T11:54:31Z","receivedAt":"2022-11-19T12:01:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 18 2022, Taylor Blau wrote:\n\n> On Fri, Nov 18, 2022 at 03:19:55PM -0800, Junio C Hamano wrote:\n>> Well \"! test_cmp\" is wrong anyway, because it _expects_ two files\n>> are the same and gives more detailed diagnosis when they differ by\n>> giving \"diff\" output.\n>>\n>> If you expect them to be different, \"! test_cmp\" would give\n>> \"detailed diagnosis\" in the wrong case, i.e. the outcome is what we\n>> expect.\n>\n> I agree that \"! test_cmp foo bar\" will give output about how \"foo\" is\n> different from \"bar\" (and halt the test only when the two have the same\n> contents).\n>\n>> So the caller must do \"test_cmp !\" whether the underlying\n>> implementation of test_cmp uses \"diff -u\" or \"diff --no-index\".\n>\n> But this confuses me. \"git grep 'test_cmp !'\" turns up no results, and\n> furthermore, test_cmp() itself begins with:\n>\n> test \"$#\" -ne 2 && BUG \"2 param\"\n>\n> So I am not sure what you are referring to.\n\nI think we've got got a bit sidetracked here.\n\nJunio's pointing out that a test that would do:\n\n\t! diff a b\n\nIs pretty stupid, why would you want to show a diff of differences, if\nyou're expecting it to be different? Just use:\n\n\t! cmp a b\n\nOr something. That's correct. So I think probably those \"! test_cmp\"\nuses would be good candidates for some clean-up to make the test suite\npretty.\n\nBut what I'm pointing out is that by having \"test_cmp\" be something that\ninvokes \"git\" (or a helper) we *must not* negate it using the shell's\nnegation, we have to use \"test_must_fail\" for anything we build\nourselves.\n\nSo, the proposed patch would make the existing cases where we do:\n\n\t! test_cmp\n\nCases where we'd hide a segfault, whereas before it was maybe a bit odd,\nbut harmless, and working as intended in the sense of reliably passing\nif we had differences.\n\nSo no, nothing currently does \"test_cmp !\", I was suggesting that\nupthread as the least invasive way of having \"test_cmp\" safe'd under\nnegation, now that we were having it call out to \"git diff\" (or a\nhelper...). It's the pattern we use for our own test helpers that invoke\nour own binaries (and if we don't, that's a bug).\n\n\n\n"},{"id":"467565","messageId":"221119.86sfifdu4n.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"Y3hFITt+8VUubC8v@nand.local","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-19T12:03:03Z","receivedAt":"2022-11-19T12:08:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 18 2022, Taylor Blau wrote:\n\n> On Fri, Nov 18, 2022 at 03:15:08PM -0800, Junio C Hamano wrote:\n>> Taylor Blau writes:\n>>\n>> > One thing that the commit message doesn't allude to (that is covered in\n>> > the earlier discussion) is why it is important to pass\n>> > `--ignore-cr-at-eol`. I think that is worth mentioning here.\n>>\n>> Isn't it because Git on the platform is expected to use CRLF in\n>> certain places, unlike on other platforms where LF is used, but the\n>> platform port hasn't adjusted tests to match that expectation? And\n>> vice versa, where Git is expected to produce LF terminated text\n>> everywhere but the expected output is not \"ported\" to force LF\n>> termination and instead produces CRLF terminated text on platforms\n>> whose native line ending is CRLF?\n>\n> Yes, I think that's right. My suggestion to Johannes was to (a) make\n> sure that your and my understanding is correct, and (b) to memorialize\n> that understanding in the commit message itself.\n\nIt's not right, but mostly right. I.e. the \"--ignore-cr-at-eol\" is\nsurely needed to avoid a deluge of errors on Windows, but as I noted in\n[1] you'll also find that the tests don't pass on a *nix box without it.\n\nWhich is apparently because \"--ignore-cr-at-eol\" is also conflating \"is\nbinary?\" with \"should I replace \\r\\n\", or something.\n\n>> Use of \"ignore-cr-at-eol\" may allow such tests that are not ported\n>> correctly to prepare expected output with a \"wrong\" line ending and\n>> still pass, and I do think it may be an expedite way to make tests\n>> appear to pass.\n>>\n>> But I worry that it may not be a good thing for the health of the\n>> Windows port in the longer term.\n>\n> I share your concerns, too.\n\nI don't really care about that, and would tend to defer to one or more\nJohannes in this thread on that question.\n\nBut as noted in [1] the upthread patch seems to be replacing some\nexisting newline munging with not-quite-the-same \"diff\" behavior.\n\nWhich I think is something that needs addressing, either by the commit\nexplaining why that's desired & OK.\n\nWhich is also a good reason to pick one of our *nix CI jobs and run it\nwith \"git diff --no-index\", but without \"--ignore-cr-at-eol\", as I\nsuggested. It would tease out exactly that difference, both for current\nand future tests.\n\n1. https://lore.kernel.org/git/221114.86pmdplbs5.gmgdl@evledraar.gmail.com/\n"},{"id":"467593","messageId":"f5d6c063-2fc4-4355-57e1-056eac1fb4a2@web.de","threadId":"58243","inReplyTo":"65ff24be-4392-f236-5500-ce0c0d4d42ca@kdbg.org","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-19T17:50:28Z","receivedAt":"2022-11-19T17:50:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.11.2022 um 09:18 schrieb Johannes Sixt:\n>\n> The reason that mingw_test_cmp exists is not that Git isn't ported\n> correctly, or that tests aren't ported correctly. The reason is that\n> tests assume Unix LF line endings everywhere, but there are some tools\n> that are outside our control that randomly -- to the layman's eye --\n> produce CRLF line endings even when their input has LF style.\n>\n> For example, when we post-process Git output with `sed`, the result\n> suddenly has CRLF line endings instead of LF that the input had.\n\nActually I see the opposite behavior -- sed eats CRs on an up-to-date\nGit for Windows SDK:\n\n $ uname -s\n MINGW64_NT-10.0-22621\n\n $ printf 'a\\r\\n' | hexdump.exe -C\n 00000000 61 0d 0a |a..|\n 00000003\n\n $ printf 'a\\r\\n' | sed '' | hexdump.exe -C\n 00000000 61 0a |a.|\n 00000002\n\nAnd with the following patch on top of eea7033409 (The twelfth batch,\n2022-11-14) the test suite passes for me -- just one case of grep\nstealing CRs seems to need adjustment to make mingw_test_cmp\nunnecessary:\n\n t/t3920-crlf-messages.sh | 2 +-\n t/test-lib.sh | 1 -\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 4c661d4d54..353b1a550e 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -12,7 +12,7 @@ create_crlf_ref () {\n \tcat >.crlf-orig-$branch.txt &&\n \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n-\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n+\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt || true &&\n \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n \ttest_tick &&\n \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 6db377f68b..af5ec357e5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1721,7 +1721,6 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n"},{"id":"467611","messageId":"20221120092925.z5jv2e2rtfpota6t@tb-raspi4","threadId":"58243","inReplyTo":"f5d6c063-2fc4-4355-57e1-056eac1fb4a2@web.de","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-11-20T09:29:25Z","receivedAt":"2022-11-20T09:29:38Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Nov 19, 2022 at 06:50:28PM +0100, René Scharfe wrote:\n> Am 19.11.2022 um 09:18 schrieb Johannes Sixt:\n> >\n> > The reason that mingw_test_cmp exists is not that Git isn't ported\n> > correctly, or that tests aren't ported correctly. The reason is that\n> > tests assume Unix LF line endings everywhere, but there are some tools\n> > that are outside our control that randomly -- to the layman's eye --\n> > produce CRLF line endings even when their input has LF style.\n> >\n> > For example, when we post-process Git output with `sed`, the result\n> > suddenly has CRLF line endings instead of LF that the input had.\n>\n> Actually I see the opposite behavior -- sed eats CRs on an up-to-date\n> Git for Windows SDK:\n>\n> $ uname -s\n> MINGW64_NT-10.0-22621\n>\n> $ printf 'a\\r\\n' | hexdump.exe -C\n> 00000000 61 0d 0a |a..|\n> 00000003\n>\n> $ printf 'a\\r\\n' | sed '' | hexdump.exe -C\n> 00000000 61 0a |a.|\n> 00000002\n\n\nThere is a \"-b\" option for sed under MINGW;\n -b, --binary\n open files in binary mode (CR+LFs are not processed specially)\n\nThe CRLF handling for sed (and probably grep and awk) had beed changed in cygwin\nhttps://cygwin.com/pipermail/cygwin/2017-June/233133.html\n\n(And I suspect that this rippled into MINGW some day)\n\n\n>\n> And with the following patch on top of eea7033409 (The twelfth batch,\n> 2022-11-14) the test suite passes for me -- just one case of grep\n> stealing CRs seems to need adjustment to make mingw_test_cmp\n> unnecessary:\n>\n> t/t3920-crlf-messages.sh | 2 +-\n> t/test-lib.sh | 1 -\n> 2 files changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index 4c661d4d54..353b1a550e 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -12,7 +12,7 @@ create_crlf_ref () {\n> \tcat >.crlf-orig-$branch.txt &&\n> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n> +\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt || true &&\n\n\nTalking about grep:\nBoth grep under Linux, MacOs and MINGW (git bash) seem to have the -U option:\n -U, --binary do not strip CR characters at EOL (MSDOS/Windows)\n\n"},{"id":"467631","messageId":"q39q9rr7-or29-5510-5177-2s92n8qs2540@tzk.qr","threadId":"58243","inReplyTo":"Y3fLdJ7CIIU8fj0g@nand.local","subject":"Re: [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-20T23:36:25Z","receivedAt":"2022-11-20T23:36:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Fri, 18 Nov 2022, Taylor Blau wrote:\n\n> On Fri, Nov 18, 2022 at 02:32:28PM +0100, Johannes Schindelin wrote:\n> > > But I'm not sure the rewritten version does what you claim, at least in\n> > > my own personal opinion.\n> > >\n> > > It is not helpful to say the original approach \"saw a lot of backlash\".\n> >\n> > It is the nicest thing I can say about it.\n>\n> I don't think you have to or should refer to the earlier round of review\n> at all.\n\nYou misunderstand.\n\nI am referring to the fact that Git for Windows has run with a very\ndifferent solution for this problem, for years, yet it was rejected upon\nupstreaming, and had to be replaced by a completely different workaround.\n\nIt's not just a simple \"earlier round of review\" at all that is the issue\nI am describing.\n\nIt is a very real concern of future readers who know what patches are\ncurrently in Git for Windows and who all of a sudden do not find the `git\ntest-tool cmp` code anymore in Git for Windows and then see that `git diff\n--no-index` is used and naturally want to know what the heck happened.\n\nThis is context relevant to understand why the particular approach\nimplemented in the patch was chosen and another one was discarded (when\nthat other approach has served Git for Windows very well for several\nyears), for which the commit message is precisely the appropriate place. I\nam quite lost trying to understand why I am asked to remove said context,\nleaving future readers puzzled e.g. in the case that it should turn out to\nhave been a terrible idea to use the quite complex diff machinery for as\nsimple a task as `test_cmp`. It sounds to me like I am asked to make my\ncontribution worse (\"worsimprove\" is the term I recently learned to\ndescribe this) instead of helping me to improve it.\n\nThe advice you provided directly contradicts what is written in\nhttps://git-scm.com/docs/SubmittingPatches#describe-changes, after all\n(ignore the funny grammar for now unless you want to add a tangent to this\nalready long thread):\n\n\tThe body should provide a meaningful commit message, which:\n\n\t [...]\n\n\t 3. alternate solutions considered but discarded, if any.\n\nThanks,\nJohannes\n"},{"id":"467635","messageId":"Y3rBWUidFe6SVmWI@nand.local","threadId":"58243","inReplyTo":"q39q9rr7-or29-5510-5177-2s92n8qs2540@tzk.qr","subject":"Re: [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-21T00:07:53Z","receivedAt":"2022-11-21T00:07:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 21, 2022 at 12:36:25AM +0100, Johannes Schindelin wrote:\n> I am referring to the fact that Git for Windows has run with a very\n> different solution for this problem, for years, yet it was rejected upon\n> upstreaming, and had to be replaced by a completely different workaround.\n>\n> It's not just a simple \"earlier round of review\" at all that is the issue\n> I am describing.\n\nRight. Having read the earlier thread myself, I am familiar with the\ncontext. So I'm not trying to dismiss it as just another round of\nreview, but instead try to steer the commit message in a more\nconstructive direction.\n\n> It is a very real concern of future readers who know what patches are\n> currently in Git for Windows and who all of a sudden do not find the `git\n> test-tool cmp` code anymore in Git for Windows and then see that `git diff\n> --no-index` is used and naturally want to know what the heck happened.\n>\n> This is context relevant to understand why the particular approach\n> implemented in the patch was chosen and another one was discarded (when\n> that other approach has served Git for Windows very well for several\n> years), for which the commit message is precisely the appropriate place. I\n> am quite lost trying to understand why I am asked to remove said context,\n> leaving future readers puzzled e.g. in the case that it should turn out to\n> have been a terrible idea to use the quite complex diff machinery for as\n> simple a task as `test_cmp`. It sounds to me like I am asked to make my\n> contribution worse (\"worsimprove\" is the term I recently learned to\n> describe this) instead of helping me to improve it.\n\nNo. I am not suggesting you remove context at all. But what I am saying\nis that describing the last attempt at upstreaming by saying it \"saw a\nlot of backlash and distractions during review and was therefore\nabandoned\" is not helpful.\n\nIf it saw backlash and distraction: why? What about the approach caused\nbacklash? Describing that thing as an alternative approach and\nexplaining concretely why it was disliked is the sort of context that I\nthink we should aim for in our commit messages.\n\nBut characterizing the review outright as full of backlash and\ndistractions is not helpful to future readers, and it is not a kind way\nto treat others on the list who may have participated in that review.\n\n> The advice you provided directly contradicts what is written in\n> https://git-scm.com/docs/SubmittingPatches#describe-changes, after all\n> (ignore the funny grammar for now unless you want to add a tangent to this\n> already long thread):\n>\n> \tThe body should provide a meaningful commit message, which:\n>\n> \t [...]\n>\n> \t 3. alternate solutions considered but discarded, if any.\n\nI am saying that, as written, the commit message does not explain what\nthe alternative approaches were in great detail.\n\nThanks,\nTaylor\n"},{"id":"467650","messageId":"xmqq1qpxyp6z.fsf@gitster.g","threadId":"58243","inReplyTo":"65ff24be-4392-f236-5500-ce0c0d4d42ca@kdbg.org","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T03:13:56Z","receivedAt":"2022-11-21T03:14:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> Use of \"ignore-cr-at-eol\" may allow such tests that are not ported\n>> correctly to prepare expected output with a \"wrong\" line ending and\n>> still pass, and I do think it may be an expedite way to make tests\n>> appear to pass.\n>\n> The reason that mingw_test_cmp exists is not that Git isn't ported\n> correctly, or that tests aren't ported correctly. The reason is that\n> tests assume Unix LF line endings everywhere, but there are some tools\n> that are outside our control that randomly -- to the layman's eye --\n> produce CRLF line endings even when their input has LF style.\n>\n> For example, when we post-process Git output with `sed`, the result\n> suddenly has CRLF line endings instead of LF that the input had.\n\nThat's exactly the kind of thing I meant by \"porting the tests\".\n\nIf we _know_ that the tests want to munge something (either the\nhardcoded expectation or command output) with LF line ending, and\nproduce output with LF line ending, then platform tools like 'sed'\nused for that task should be told not to turn LF into CRLF, with\nsomething like:\n\n case $uname_s in\n *MINGW*)\n sed () {\n command sed --binary \"$@\"\n }\n\t... other workarounds come here ...\n\t;;\n easc\n\nbefore we can call that we ported our tests to the platform.\n\n"},{"id":"467651","messageId":"xmqqtu2txahd.fsf@gitster.g","threadId":"58243","inReplyTo":"221119.86wn7rdugi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T03:17:02Z","receivedAt":"2022-11-21T03:17:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Junio's pointing out that a test that would do:\n>\n> \t! diff a b\n>\n> Is pretty stupid, why would you want to show a diff of differences, if\n> you're expecting it to be different? Just use:\n>\n> \t! cmp a b\n>\n> Or something. That's correct. So I think probably those \"! test_cmp\"\n> uses would be good candidates for some clean-up to make the test suite\n> pretty.\n\nYes.\n\nBesides, \"the outcome must be different from this other file\" is a\nvery sloppy thing to say in a test. It rarely is OK for our output\nto be merely different from one \"wrong\" output. Tests should\nspecify in what way the output must be different from the \"wrong\"\nexample, at least.\n"},{"id":"467698","messageId":"f0411dd9-bccb-b5d3-ed51-59be6b627a30@kdbg.org","threadId":"58243","inReplyTo":"f5d6c063-2fc4-4355-57e1-056eac1fb4a2@web.de","subject":"Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-11-21T17:49:06Z","receivedAt":"2022-11-21T17:49:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.11.22 um 18:50 schrieb René Scharfe:\n> And with the following patch on top of eea7033409 (The twelfth batch,\n> 2022-11-14) the test suite passes for me -- just one case of grep\n> stealing CRs seems to need adjustment to make mingw_test_cmp\n> unnecessary:\n\nWow, nice catch! Just let enough time pass, and a liability such as\nmingw_test_cmp becomes obsolete by mere magic. ;)\n\n> \n> t/t3920-crlf-messages.sh | 2 +-\n> t/test-lib.sh | 1 -\n> 2 files changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index 4c661d4d54..353b1a550e 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -12,7 +12,7 @@ create_crlf_ref () {\n> \tcat >.crlf-orig-$branch.txt &&\n> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n> +\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt || true &&\n\nThis || true looks suspicious. I'll submit a patch in a new thread.\n\n> \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n> \ttest_tick &&\n> \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 6db377f68b..af5ec357e5 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1721,7 +1721,6 @@ case $uname_s in\n> \ttest_set_prereq SED_STRIPS_CR\n> \ttest_set_prereq GREP_STRIPS_CR\n> \ttest_set_prereq WINDOWS\n> -\tGIT_TEST_CMP=mingw_test_cmp\n> \t;;\n> *CYGWIN*)\n> \ttest_set_prereq POSIXPERM\n\n"},{"id":"468624","messageId":"pull.1309.v5.git.1670339267.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v4.git.1668434812.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] tests(mingw): avoid super-slow mingw_test_cmp","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-06T15:07:45Z","receivedAt":"2022-12-06T15:11:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Half a year ago, directly after sending a patch to fix a performance\nregression due to a mis-use of test_cmp\n[https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],\nI got curious to see whether Git for Windows had the same issue. And it did\nnot: it passed t5351 in 22 seconds, even while using test_cmp to compare\npack files\n[https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].\nThis motivated me to upstream Git for Windows' changes.\n\nChanges since v4:\n\n * The commit message was rewritten almost completely.\n\nChanges since v3:\n\n * Fixed the subject of the cover letter (which should have been adjusted in\n v3)\n * Elaborated the paragraph about the historical context of this patch\n\nChanges since v2:\n\n * Dropped the test helper, using diff --no-index instead.\n\nChanges since v1:\n\n * Fixed double \"with\" in the commit message.\n * Renamed the test helper to text-cmp.\n * Made the diff --no-index call more robust by using a double-dash\n separator.\n\nJohannes Schindelin (2):\n t0021: use Windows-friendly `pwd`\n tests(mingw): avoid very slow `mingw_test_cmp`\n\n t/t0021-conversion.sh | 4 +--\n t/test-lib-functions.sh | 66 -----------------------------------------\n t/test-lib.sh | 2 +-\n 3 files changed, 3 insertions(+), 69 deletions(-)\n\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1309\n\nRange-diff vs v4:\n\n 1: b38b8fb5a85 = 1: b38b8fb5a85 t0021: use Windows-friendly `pwd`\n 2: 128b1f348d8 ! 2: 6a80fab7e39 tests(mingw): avoid very slow `mingw_test_cmp`\n @@ Metadata\n ## Commit message ##\n tests(mingw): avoid very slow `mingw_test_cmp`\n \n - It is more performant to run `git diff --no-index` than running the\n - `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for\n - Windows uses. And a lot more readable.\n + When Git's test suite uses `test_cmp`, it is not actually trying to\n + compare binary files as the name `cmp` would suggest to users familiar\n + with Unix' tools, but the tests instead verify that actual output\n + matches the expected text.\n \n - The original reason why Git's test suite needs the `mingw_test_cmp`\n - function at all (and why `cmp` is not good enough) is that Git's test\n - suite is not actually trying to compare binary files when it calls\n - `test_cmp`, but it compares text files. And those text files can contain\n - CR/LF line endings depending on the circumstances.\n + On Unix, `cmp` works well enough for Git's purposes because only Line\n + Feed characters are used as line endings. However, on Windows, while\n + most tools accept Line Feeds as line endings, many tools produce\n + Carriage Return + Line Feed line endings, including some of the tools\n + used by the test suite (which are therefore provided via Git for Windows\n + SDK). Therefore, `cmp` would frequently fail merely due to different\n + line endings.\n \n - Note: The original fix in the Git for Windows project implemented a test\n - helper that avoids the overhead of the diff machinery, in favor of\n - implementing a behavior that is more in line with what `mingw_test_cmp`\n - does now. This was done to minimize the risk in using something as\n - complex as the diff machinery to perform something as simple as\n - determining whether text output is identical to the expected output or\n - not. This approach has served Git for Windows well for years, but the\n - attempt to upstream this saw a lot of backlash and distractions during\n - the review, was disliked by the Git maintainer and was therefore\n - abandoned. For full details, see the thread at\n - https://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t\n + To accommodate for that, the `mingw_test_cmp` function was introduced\n + into Git's test suite to perform a line-by-line comparison that ignores\n + line endings. This function is a Bash function that is only used on\n + Windows, everywhere else `cmp` is used.\n +\n + This is a double whammy because `cmp` is fast, and `mingw_test_cmp` is\n + slow, even more so on Windows because it is a Bash script function, and\n + Bash scripts are known to run particularly slowly on Windows due to\n + Bash's need for the POSIX emulation layer provided by the MSYS2 runtime.\n +\n + The commit message of 32ed3314c104 (t5351: avoid using `test_cmp` for\n + binary data, 2022-07-29) provides an illuminating account of the\n + consequences: On Windows, the platform on which Git could really use all\n + the help it can get to improve its performance, the time spent on one\n + entire test script was reduced from half an hour to less than half a\n + minute merely by avoiding a single call to `mingw_test_cmp` in but a\n + single test case.\n +\n + Learning the lesson to avoid shell scripting wherever possible, the Git\n + for Windows project implemented a minimal replacement for\n + `mingw_test_cmp` in the form of a `test-tool` subcommand that parses the\n + input files line by line, ignoring line endings, and compares them.\n + Essentially the same thing as `mingw_test_cmp`, but implemented in\n + C instead of Bash. This solution served the Git for Windows project\n + well, over years.\n +\n + However, when this solution was finally upstreamed, the conclusion was\n + reached that a change to use `git diff --no-index` instead of\n + `mingw_test_cmp` was more easily reviewed and hence should be used\n + instead.\n +\n + The reason why this approach was not even considered in Git for Windows\n + is that in 2007, there was already a motion on the table to use Git's\n + own diff machinery to perform comparisons in Git's test suite, but it\n + was dismissed in https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/\n + as undesirable because tests might potentially succeed due to bugs in\n + the diff machinery when they should not succeed, and those bugs could\n + therefore hide regressions that the tests try to prevent.\n +\n + By the time Git for Windows' `mingw-test-cmp` in C was finally\n + contributed to the Git mailing list, reviewers agreed that the diff\n + machinery had matured enough and should be used instead.\n +\n + When the concern was raised that the diff machinery, due to its\n + complexity, would perform substantially worse than the test helper\n + originally implemented in the Git for Windows project, a test\n + demonstrated that these performance differences are well lost within the\n + 100+ minutes it takes to run Git's test suite on Windows.\n \n Signed-off-by: Johannes Schindelin \n \n\n-- \ngitgitgadget\n"},{"id":"468625","messageId":"6a80fab7e3936ec56e1583d6136d47487327e907.1670339267.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v5.git.1670339267.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-06T15:07:47Z","receivedAt":"2022-12-06T15:11:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen Git's test suite uses `test_cmp`, it is not actually trying to\ncompare binary files as the name `cmp` would suggest to users familiar\nwith Unix' tools, but the tests instead verify that actual output\nmatches the expected text.\n\nOn Unix, `cmp` works well enough for Git's purposes because only Line\nFeed characters are used as line endings. However, on Windows, while\nmost tools accept Line Feeds as line endings, many tools produce\nCarriage Return + Line Feed line endings, including some of the tools\nused by the test suite (which are therefore provided via Git for Windows\nSDK). Therefore, `cmp` would frequently fail merely due to different\nline endings.\n\nTo accommodate for that, the `mingw_test_cmp` function was introduced\ninto Git's test suite to perform a line-by-line comparison that ignores\nline endings. This function is a Bash function that is only used on\nWindows, everywhere else `cmp` is used.\n\nThis is a double whammy because `cmp` is fast, and `mingw_test_cmp` is\nslow, even more so on Windows because it is a Bash script function, and\nBash scripts are known to run particularly slowly on Windows due to\nBash's need for the POSIX emulation layer provided by the MSYS2 runtime.\n\nThe commit message of 32ed3314c104 (t5351: avoid using `test_cmp` for\nbinary data, 2022-07-29) provides an illuminating account of the\nconsequences: On Windows, the platform on which Git could really use all\nthe help it can get to improve its performance, the time spent on one\nentire test script was reduced from half an hour to less than half a\nminute merely by avoiding a single call to `mingw_test_cmp` in but a\nsingle test case.\n\nLearning the lesson to avoid shell scripting wherever possible, the Git\nfor Windows project implemented a minimal replacement for\n`mingw_test_cmp` in the form of a `test-tool` subcommand that parses the\ninput files line by line, ignoring line endings, and compares them.\nEssentially the same thing as `mingw_test_cmp`, but implemented in\nC instead of Bash. This solution served the Git for Windows project\nwell, over years.\n\nHowever, when this solution was finally upstreamed, the conclusion was\nreached that a change to use `git diff --no-index` instead of\n`mingw_test_cmp` was more easily reviewed and hence should be used\ninstead.\n\nThe reason why this approach was not even considered in Git for Windows\nis that in 2007, there was already a motion on the table to use Git's\nown diff machinery to perform comparisons in Git's test suite, but it\nwas dismissed in https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/\nas undesirable because tests might potentially succeed due to bugs in\nthe diff machinery when they should not succeed, and those bugs could\ntherefore hide regressions that the tests try to prevent.\n\nBy the time Git for Windows' `mingw-test-cmp` in C was finally\ncontributed to the Git mailing list, reviewers agreed that the diff\nmachinery had matured enough and should be used instead.\n\nWhen the concern was raised that the diff machinery, due to its\ncomplexity, would perform substantially worse than the test helper\noriginally implemented in the Git for Windows project, a test\ndemonstrated that these performance differences are well lost within the\n100+ minutes it takes to run Git's test suite on Windows.\n\nSigned-off-by: Johannes Schindelin \n---\n t/test-lib-functions.sh | 66 -----------------------------------------\n t/test-lib.sh | 2 +-\n 2 files changed, 1 insertion(+), 67 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8c44856eaec..452fe9bc8aa 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1541,72 +1541,6 @@ test_skip_or_die () {\n \terror \"$2\"\n }\n \n-# The following mingw_* functions obey POSIX shell syntax, but are actually\n-# bash scripts, and are meant to be used only with bash on Windows.\n-\n-# A test_cmp function that treats LF and CRLF equal and avoids to fork\n-# diff when possible.\n-mingw_test_cmp () {\n-\t# Read text into shell variables and compare them. If the results\n-\t# are different, use regular diff to report the difference.\n-\tlocal test_cmp_a= test_cmp_b=\n-\n-\t# When text came from stdin (one argument is '-') we must feed it\n-\t# to diff.\n-\tlocal stdin_for_diff=\n-\n-\t# Since it is difficult to detect the difference between an\n-\t# empty input file and a failure to read the files, we go straight\n-\t# to diff if one of the inputs is empty.\n-\tif test -s \"$1\" && test -s \"$2\"\n-\tthen\n-\t\t# regular case: both files non-empty\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\telif test -s \"$1\" && test \"$2\" = -\n-\tthen\n-\t\t# read 2nd file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b\n-\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n-\telif test \"$1\" = - && test -s \"$2\"\n-\tthen\n-\t\t# read 1st file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n-\tfi\n-\ttest -n \"$test_cmp_a\" &&\n-\ttest -n \"$test_cmp_b\" &&\n-\ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n-\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n-}\n-\n-# $1 is the name of the shell variable to fill in\n-mingw_read_file_strip_cr_ () {\n-\t# Read line-wise using LF as the line separator\n-\t# and use IFS to strip CR.\n-\tlocal line\n-\twhile :\n-\tdo\n-\t\tif IFS=$'\\r' read -r -d $'\\n' line\n-\t\tthen\n-\t\t\t# good\n-\t\t\tline=$line$'\\n'\n-\t\telse\n-\t\t\t# we get here at EOF, but also if the last line\n-\t\t\t# was not terminated by LF; in the latter case,\n-\t\t\t# some text was read\n-\t\t\tif test -z \"$line\"\n-\t\t\tthen\n-\t\t\t\t# EOF, really\n-\t\t\t\tbreak\n-\t\t\tfi\n-\t\tfi\n-\t\teval \"$1=\\$$1\\$line\"\n-\tdone\n-}\n-\n # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n # it also works for shell functions (though those functions cannot impact\n # the environment outside of the test_env invocation).\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7726d1da88a..f8c6205e08f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1546,7 +1546,7 @@ case $uname_s in\n \ttest_set_prereq SED_STRIPS_CR\n \ttest_set_prereq GREP_STRIPS_CR\n \ttest_set_prereq WINDOWS\n-\tGIT_TEST_CMP=mingw_test_cmp\n+\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n-- \ngitgitgadget\n"},{"id":"468626","messageId":"b38b8fb5a85e10d6189d68b6633adf23fca7b65d.1670339267.git.gitgitgadget@gmail.com","threadId":"58243","inReplyTo":"pull.1309.v5.git.1670339267.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] t0021: use Windows-friendly `pwd`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-06T15:07:46Z","receivedAt":"2022-12-06T15:11:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows, when passing paths from shell scripts to regular\nWin32 executables, thanks to the MSYS2 runtime a somewhat magic path\nconversion happens that lets the shell script think that there is a file\nat `/git/Makefile` and the Win32 process it spawned thinks that the\nshell script said `C:/git-sdk-64/git/Makefile` instead.\n\nThis conversion is documented in detail over here:\nhttps://www.msys2.org/docs/filesystem-paths/#automatic-unix-windows-path-conversion\n\nAs all automatic conversions, there are gaps. For example, to avoid\nmistaking command-line options like `/LOG=log.txt` (which are quite\ncommon in the Windows world) from being mistaken for a Unix-style\nabsolute path, the MSYS2 runtime specifically exempts arguments\ncontaining a `=` character from that conversion.\n\nWe are about to change `test_cmp` to use `git diff --no-index`, which\ninvolves spawning precisely such a Win32 process.\n\nIn combination, this would cause a failure in `t0021-conversion.sh`\nwhere we pass an absolute path containing an equal character to the\n`test_cmp` function.\n\nSeeing as the Unix tools like `cp` and `diff` that are used by Git's\ntest suite in the Git for Windows SDK (thanks to the MSYS2 project)\nunderstand both Unix-style as well as Windows-style paths, we can stave\noff this problem by simply switching to Windows-style paths and\nside-stepping the need for any automatic path conversion.\n\nNote: The `PATH` variable is obviously special, as it is colon-separated\nin the MSYS2 Bash used by Git for Windows, and therefore _cannot_\ncontain absolute Windows-style paths, lest the colon after the drive\nletter is mistaken for a path separator. Therefore, we need to be\ncareful to keep the Unix-style when modifying the `PATH` variable.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0021-conversion.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bad37abad2c..15482fa78e3 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -8,8 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-TEST_ROOT=\"$PWD\"\n-PATH=$TEST_ROOT:$PATH\n+PATH=$PWD:$PATH\n+TEST_ROOT=\"$(pwd)\"\n \n write_script <<\\EOF \"$TEST_ROOT/rot13.sh\"\n tr \\\n-- \ngitgitgadget\n\n"},{"id":"468632","messageId":"221206.86sfhsbau4.gmgdl@evledraar.gmail.com","threadId":"58243","inReplyTo":"6a80fab7e3936ec56e1583d6136d47487327e907.1670339267.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-06T18:55:43Z","receivedAt":"2022-12-06T19:18:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 06 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> [...]\n> The reason why this approach was not even considered in Git for Windows\n> is that in 2007, there was already a motion on the table to use Git's\n> own diff machinery to perform comparisons in Git's test suite, but it\n> was dismissed in https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/\n\nI think you mixed up your links there, that's the \"no, that should be\nfine\" from 2022, but it contains the link to the 2007 message you seem\nto be talking about.\n\n> as undesirable because tests might potentially succeed due to bugs in\n> the diff machinery when they should not succeed, and those bugs could\n> therefore hide regressions that the tests try to prevent.\n>\n> By the time Git for Windows' `mingw-test-cmp` in C was finally\n> contributed to the Git mailing list, reviewers agreed that the diff\n> machinery had matured enough and should be used instead.\n\nI think it's fine to dogfood it like this, but I've been pointing out to\nyou[1] that this will hide segfaults etc. in \"git\" as we negate the exit\ncode of \"test_cmp\" in some places.\n\nE.g. let's produce this stack overflow:\n\n\tdiff --git a/diff-no-index.c b/diff-no-index.c\n\tindex 9a8b09346bd..1c4a9e5c351 100644\n\t--- a/diff-no-index.c\n\t+++ b/diff-no-index.c\n\t@@ -279,6 +279,7 @@ int diff_no_index(struct rev_info *revs,\n\t \n\t \tfixup_paths(paths, &replacement);\n\t \n\t+\trevs++;\n\t \trevs->diffopt.skip_stat_unmatch = 1;\n\t \tif (!revs->diffopt.output_format)\n\t \t\trevs->diffopt.output_format = DIFF_FORMAT_PATCH;\n\nThen compiling with SANITIZE=address and running:\n\n\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\" ./t9351-fast-export-anonymize.sh --run=1,17 -vixd\n\nWill produce (less relevant bits elided):\n\t\n\texpecting success of 9351.17 'idents are shared': \n\t\t[...]\n\t\ttest_line_count = 1 unique &&\n\t\t! test_cmp authors committers\n\t\n\t[...]\n\t+ test 1 = 1\n\t+ test_cmp authors committers\n\t+ test 2 -ne 2\n\t+ eval GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- \"$@\"\n\t+ GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- authors committers\n\t=================================================================\n\t==2865782==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc6d64d0f4 at pc 0x00000077fa4d bp 0x7ffc6d64bcc0 sp 0x7ffc6d64bcb8\n\tWRITE of size 4 at 0x7ffc6d64d0f4 thread T0\n\t #0 0x77fa4c in diff_no_index diff-no-index.c:292\n\t[...]\n\tok 17 - idents are shared\n\t\n\t# passed all 17 test(s)\n\t1..17\n\nI.e. we'll proclaim \"all tests passed\", even though we segfaulted. I\nthink this direction will fix it for you:\n\n\tdiff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh\n\tindex 77047e250dc..29a2d3a03a7 100755\n\t--- a/t/t9351-fast-export-anonymize.sh\n\t+++ b/t/t9351-fast-export-anonymize.sh\n\t@@ -133,7 +133,7 @@ test_expect_success 'idents are shared' '\n\t \tgit log --all --format=\"%cn <%ce>\" >committers &&\n\t \tsort -u committers >unique &&\n\t \ttest_line_count = 1 unique &&\n\t-\t! test_cmp authors committers\n\t+\ttest_cmp ! authors committers\n\t '\n\t \n\t test_done\n\tdiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n\tindex 452fe9bc8aa..d640b4b7881 100644\n\t--- a/t/test-lib-functions.sh\n\t+++ b/t/test-lib-functions.sh\n\t@@ -1239,8 +1239,25 @@ test_expect_code () {\n\t # - not all diff versions understand \"-u\"\n\t \n\t test_cmp () {\n\t+\tlocal negate= &&\n\t+\tlocal exit_code= &&\n\t+\tif test \"$1\" = \"!\"\n\t+\tthen\n\t+\t\tnegate=t &&\n\t+\t\tshift\n\t+\tfi &&\n\t \ttest \"$#\" -ne 2 && BUG \"2 param\"\n\t-\teval \"$GIT_TEST_CMP\" '\"$@\"'\n\t+\tif test -n \"$GIT_TEST_CMP_BUILTIN\"\n\t+\tthen\n\t+\t\tif test -n \"$negate\"\n\t+\t\tthen\n\t+\t\t\ttest_must_fail env GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- \"$@\"\n\t+\t\telse\n\t+\t\t\tGIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol -- \"$@\"\n\t+\t\tfi\n\t+\telse\n\t+\t\teval \"$GIT_TEST_CMP\" '\"$@\"'\n\t+\tfi\n\t }\n\t \n\t # Check that the given config key has the expected value.\n\tdiff --git a/t/test-lib.sh b/t/test-lib.sh\n\tindex f8c6205e08f..0a5f8e7c7fc 100644\n\t--- a/t/test-lib.sh\n\t+++ b/t/test-lib.sh\n\t@@ -1546,7 +1546,7 @@ case $uname_s in\n\t \ttest_set_prereq SED_STRIPS_CR\n\t \ttest_set_prereq GREP_STRIPS_CR\n\t \ttest_set_prereq WINDOWS\n\t-\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n\t+\tGIT_TEST_CMP_BUILTIN=t\n\t \t;;\n\t *CYGWIN*)\n\t \ttest_set_prereq POSIXPERM\n\nI.e. it's fine to start invoking \"git\" in one of the\ntest-lib-functions.sh, but it's not OK to do so without also ensuring\nthat we're properly checking its exit code.\n\nThe above diff fixes it only for t9351-fast-export-anonymize.sh, we'd\nalso need to change \"! test_cmp \" to \"test_cmp ! \" elsewhere, but that's\na rather easy mechanical replacement.\n\n1. https://lore.kernel.org/git/221119.86wn7rdugi.gmgdl@evledraar.gmail.com/\n"},{"id":"468643","messageId":"a11c51bc-f419-f128-05c9-d9706aabc665@kdbg.org","threadId":"58243","inReplyTo":"6a80fab7e3936ec56e1583d6136d47487327e907.1670339267.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-12-06T21:52:07Z","receivedAt":"2022-12-06T21:52:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.12.22 um 16:07 schrieb Johannes Schindelin via GitGitGadget:\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..f8c6205e08f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1546,7 +1546,7 @@ case $uname_s in\n> \ttest_set_prereq SED_STRIPS_CR\n> \ttest_set_prereq GREP_STRIPS_CR\n> \ttest_set_prereq WINDOWS\n> -\tGIT_TEST_CMP=mingw_test_cmp\n> +\tGIT_TEST_CMP=\"GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --\"\n\nAfter René's work, it should be possible to drop this setting on Windows\naltogether.\n\nI started doing that, and when I proof-read the patch, I noticed the\nSED_STRIPS_CR and GREP_STRIPS_CR prerequisites that are visible in the\ncontext lines of this hunk. I wanted to explore whether they can be\nremoved as well, but I ran out of time.\n\n-- Hannes\n\n"},{"id":"468644","messageId":"31d3bf6c-c0a2-d2d5-c6e2-b185fde99170@web.de","threadId":"58243","inReplyTo":"6a80fab7e3936ec56e1583d6136d47487327e907.1670339267.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-06T21:54:36Z","receivedAt":"2022-12-06T21:54:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.2022 um 16:07 schrieb Johannes Schindelin via GitGitGadget:\n> From: Johannes Schindelin \n>\n> When Git's test suite uses `test_cmp`, it is not actually trying to\n> compare binary files as the name `cmp` would suggest to users familiar\n> with Unix' tools, but the tests instead verify that actual output\n> matches the expected text.\n>\n> On Unix, `cmp` works well enough for Git's purposes because only Line\n> Feed characters are used as line endings. However, on Windows, while\n> most tools accept Line Feeds as line endings, many tools produce\n> Carriage Return + Line Feed line endings, including some of the tools\n> used by the test suite (which are therefore provided via Git for Windows\n> SDK). Therefore, `cmp` would frequently fail merely due to different\n> line endings.\n>\n> To accommodate for that, the `mingw_test_cmp` function was introduced\n> into Git's test suite to perform a line-by-line comparison that ignores\n> line endings. This function is a Bash function that is only used on\n> Windows, everywhere else `cmp` is used.\n>\n> This is a double whammy because `cmp` is fast, and `mingw_test_cmp` is\n> slow, even more so on Windows because it is a Bash script function, and\n> Bash scripts are known to run particularly slowly on Windows due to\n> Bash's need for the POSIX emulation layer provided by the MSYS2 runtime.\n>\n> The commit message of 32ed3314c104 (t5351: avoid using `test_cmp` for\n> binary data, 2022-07-29) provides an illuminating account of the\n> consequences: On Windows, the platform on which Git could really use all\n> the help it can get to improve its performance, the time spent on one\n> entire test script was reduced from half an hour to less than half a\n> minute merely by avoiding a single call to `mingw_test_cmp` in but a\n> single test case.\n>\n> Learning the lesson to avoid shell scripting wherever possible, the Git\n> for Windows project implemented a minimal replacement for\n> `mingw_test_cmp` in the form of a `test-tool` subcommand that parses the\n> input files line by line, ignoring line endings, and compares them.\n> Essentially the same thing as `mingw_test_cmp`, but implemented in\n> C instead of Bash. This solution served the Git for Windows project\n> well, over years.\n>\n> However, when this solution was finally upstreamed, the conclusion was\n> reached that a change to use `git diff --no-index` instead of\n> `mingw_test_cmp` was more easily reviewed and hence should be used\n> instead.\n>\n> The reason why this approach was not even considered in Git for Windows\n> is that in 2007, there was already a motion on the table to use Git's\n> own diff machinery to perform comparisons in Git's test suite, but it\n> was dismissed in https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/\n> as undesirable because tests might potentially succeed due to bugs in\n> the diff machinery when they should not succeed, and those bugs could\n> therefore hide regressions that the tests try to prevent.\n>\n> By the time Git for Windows' `mingw-test-cmp` in C was finally\n> contributed to the Git mailing list, reviewers agreed that the diff\n> machinery had matured enough and should be used instead.\n>\n> When the concern was raised that the diff machinery, due to its\n> complexity, would perform substantially worse than the test helper\n> originally implemented in the Git for Windows project, a test\n> demonstrated that these performance differences are well lost within the\n> 100+ minutes it takes to run Git's test suite on Windows.\nOnly t3920 needs mingw_test_cmp on my system. [2] on top of [1] removes\nthat dependency. Does that work for you as well?\n\n\ndiff -u and git diff are slower than mingw_test_cmp when running\n\"make test\" on my system (\"uname -rs\" says \"MINGW64_NT-10.0-22621\n3.3.6-341.x86_64\"). Timings (sorry, only a single run each):\n\n2.39.0-rc2:\nAll tests successful.\nFiles=983, Tests=26154, 2082 wallclock secs ( 7.86 usr 15.97 sys + 776.73 cusr 2352.96 csys = 3153.52 CPU)\nResult: PASS\n\n2.39.0-rc2 + [1] + [2] + removal of \"GIT_TEST_CMP=mingw_test_cmp\"\nfrom t/test-lib.sh:\nAll tests successful.\nFiles=983, Tests=26154, 2174 wallclock secs ( 7.75 usr 17.27 sys + 813.53 cusr 2699.12 csys = 3537.66 CPU)\nResult: PASS\n\n2.39.0-rc2 + your v5:\nAll tests successful.\nFiles=983, Tests=26154, 2160 wallclock secs ( 4.16 usr 10.03 sys + 647.75 cusr 2038.99 csys = 2700.92 CPU)\nResult: PASS\n\n2.39.0-rc2 + [1] + [2] + the patch below:\nAll tests successful.\nFiles=983, Tests=26154, 2026 wallclock secs ( 4.91 usr 9.91 sys + 608.91 cusr 1881.98 csys = 2505.70 CPU)\nResult: PASS\n\n\nThis is noisy, running \"make test\" multiple times takes too long.\nRunning a single test using hyperfine is feasible. Here are my results\nfor the last test that needed a CR-agnostic test_cmp:\n\n2.39.0-rc2:\nBenchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 5.666 s ± 0.085 s [User: 0.000 s, System: 0.007 s]\n Range (min … max): 5.583 s … 5.858 s 10 runs\n\n2.39.0-rc2 + [1] + [2] + removal of \"GIT_TEST_CMP=mingw_test_cmp\"\nfrom t/test-lib.sh:\nBenchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 6.540 s ± 0.065 s [User: 0.000 s, System: 0.004 s]\n Range (min … max): 6.454 s … 6.681 s 10 runs\n\n2.39.0-rc2 + your v5:\nBenchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 6.632 s ± 0.090 s [User: 0.000 s, System: 0.004 s]\n Range (min … max): 6.550 s … 6.791 s 10 runs\n\n2.39.0-rc2 + [1] + [2] + the patch below:\nBenchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 5.743 s ± 0.065 s [User: 0.002 s, System: 0.001 s]\n Range (min … max): 5.684 s … 5.870 s 10 runs\n\nStill noisy, but avoiding to fork out to a comparison program seems\nworth it, i.e. the shell function wins for the typically short inputs in\ntest scripts. Do you get different numbers?\n\n\nI'm a bit disappointed by the performance of the patch below, but we'd\nneed something like this to compare precisely (no longer ignoring CRs),\nI suppose.\n\n\n[1] https://lore.kernel.org/git/febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org/\n[2] https://lore.kernel.org/git/cbe88abc-c1fb-cb50-6057-47ff27f7a12d@web.de/\n\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b3..bf10746a08 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1450,70 +1450,21 @@ test_skip_or_die () {\n \terror \"$2\"\n }\n\n-# The following mingw_* functions obey POSIX shell syntax, but are actually\n-# bash scripts, and are meant to be used only with bash on Windows.\n-\n-# A test_cmp function that treats LF and CRLF equal and avoids to fork\n-# diff when possible.\n+# A test_cmp function that avoids to fork diff when possible. It's only\n+# meant to be used with bash on Windows.\n mingw_test_cmp () {\n \t# Read text into shell variables and compare them. If the results\n \t# are different, use regular diff to report the difference.\n-\tlocal test_cmp_a= test_cmp_b=\n-\n-\t# When text came from stdin (one argument is '-') we must feed it\n-\t# to diff.\n-\tlocal stdin_for_diff=\n+\tlocal test_cmp_a=a test_cmp_b=b\n\n-\t# Since it is difficult to detect the difference between an\n-\t# empty input file and a failure to read the files, we go straight\n-\t# to diff if one of the inputs is empty.\n-\tif test -s \"$1\" && test -s \"$2\"\n-\tthen\n-\t\t# regular case: both files non-empty\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\telif test -s \"$1\" && test \"$2\" = -\n-\tthen\n-\t\t# read 2nd file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a <\"$1\"\n-\t\tmingw_read_file_strip_cr_ test_cmp_b\n-\t\tstdin_for_diff='<<<\"$test_cmp_b\"'\n-\telif test \"$1\" = - && test -s \"$2\"\n+\t# Leave the uncommon case of reading from stdin to diff.\n+\tif test \"$1\" != \"-\" && test \"$2\" != \"-\"\n \tthen\n-\t\t# read 1st file from stdin\n-\t\tmingw_read_file_strip_cr_ test_cmp_a\n-\t\tmingw_read_file_strip_cr_ test_cmp_b <\"$2\"\n-\t\tstdin_for_diff='<<<\"$test_cmp_a\"'\n+\t\tIFS= read -r -d '' test_cmp_a <\"$1\"\n+\t\tIFS= read -r -d '' test_cmp_b <\"$2\"\n \tfi\n-\ttest -n \"$test_cmp_a\" &&\n-\ttest -n \"$test_cmp_b\" &&\n \ttest \"$test_cmp_a\" = \"$test_cmp_b\" ||\n-\teval \"diff -u \\\"\\$@\\\" $stdin_for_diff\"\n-}\n-\n-# $1 is the name of the shell variable to fill in\n-mingw_read_file_strip_cr_ () {\n-\t# Read line-wise using LF as the line separator\n-\t# and use IFS to strip CR.\n-\tlocal line\n-\twhile :\n-\tdo\n-\t\tif IFS=$'\\r' read -r -d $'\\n' line\n-\t\tthen\n-\t\t\t# good\n-\t\t\tline=$line$'\\n'\n-\t\telse\n-\t\t\t# we get here at EOF, but also if the last line\n-\t\t\t# was not terminated by LF; in the latter case,\n-\t\t\t# some text was read\n-\t\t\tif test -z \"$line\"\n-\t\t\tthen\n-\t\t\t\t# EOF, really\n-\t\t\t\tbreak\n-\t\t\tfi\n-\t\tfi\n-\t\teval \"$1=\\$$1\\$line\"\n-\tdone\n+\tdiff -u \"$@\"\n }\n\n # Like \"env FOO=BAR some-program\", but run inside a subshell, which means\n"},{"id":"468670","messageId":"Y4/s1QcVFjjm31n7@nand.local","threadId":"58243","inReplyTo":"6a80fab7e3936ec56e1583d6136d47487327e907.1670339267.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-07T01:31:01Z","receivedAt":"2022-12-07T01:31:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Johannes,\n\nOn Tue, Dec 06, 2022 at 03:07:47PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> Learning the lesson to avoid shell scripting wherever possible, the Git\n> for Windows project implemented a minimal replacement for\n> `mingw_test_cmp` in the form of a `test-tool` subcommand that parses the\n> input files line by line, ignoring line endings, and compares them.\n> Essentially the same thing as `mingw_test_cmp`, but implemented in\n> C instead of Bash. This solution served the Git for Windows project\n> well, over years.\n>\n> However, when this solution was finally upstreamed, the conclusion was\n> reached that a change to use `git diff --no-index` instead of\n> `mingw_test_cmp` was more easily reviewed and hence should be used\n> instead.\n>\n> The reason why this approach was not even considered in Git for Windows\n> is that in 2007, there was already a motion on the table to use Git's\n> own diff machinery to perform comparisons in Git's test suite, but it\n> was dismissed in https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/\n> as undesirable because tests might potentially succeed due to bugs in\n> the diff machinery when they should not succeed, and those bugs could\n> therefore hide regressions that the tests try to prevent.\n>\n> By the time Git for Windows' `mingw-test-cmp` in C was finally\n> contributed to the Git mailing list, reviewers agreed that the diff\n> machinery had matured enough and should be used instead.\n>\n> When the concern was raised that the diff machinery, due to its\n> complexity, would perform substantially worse than the test helper\n> originally implemented in the Git for Windows project, a test\n> demonstrated that these performance differences are well lost within the\n> 100+ minutes it takes to run Git's test suite on Windows.\n\nThanks for this update. I think these five paragraphs are an accurate\nand helpful record of the history behind this patch and approach. It is\nvery much appreciated.\n\nThanks,\nTaylor\n"},{"id":"468684","messageId":"xmqqbkof7ryn.fsf@gitster.g","threadId":"58243","inReplyTo":"31d3bf6c-c0a2-d2d5-c6e2-b185fde99170@web.de","subject":"Re: [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T04:33:52Z","receivedAt":"2022-12-07T04:34:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Only t3920 needs mingw_test_cmp on my system. [2] on top of [1] removes\n> that dependency. Does that work for you as well?\n> ...\n> [1] https://lore.kernel.org/git/febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org/\n\nThis is on js/t3920-shell-and-or-fix topic and in 'next'.\n\n> [2] https://lore.kernel.org/git/cbe88abc-c1fb-cb50-6057-47ff27f7a12d@web.de/\n\nI forgot to pick this up, which I just did.\n\n> I'm a bit disappointed by the performance of the patch below, but we'd\n> need something like this to compare precisely (no longer ignoring CRs),\n> I suppose.\n>\n\n"}]}