[v2,0/2] tests: replace mingw_test_cmp with a helper in C

Message

Johannes Schindelin via GitGitGadget Sept. 6, 2022, 1:10 p.m. UTC

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

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

Changes since v1:

 * Fixed double "with" in the commit message.

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

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


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

Range-diff vs v1:

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

Comments

Johannes Schindelin Sept. 7, 2022, 9:04 a.m. UTC | #1

Hi,

On Tue, 6 Sep 2022, Johannes Schindelin via GitGitGadget wrote:

> Changes since v1:
>
>  * Fixed double "with" in the commit message.

Whoops, I totally forgot to mention these two things:


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

Ciao,
Dscho