Re: [PATCH] tests: replace mingw_test_cmp with a helper in C

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 29 Jul 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This helper is more performant than running the `mingw_test_cmp` code
> with with MSYS2's Bash. And a lot more readable.

Ooops... s/with with/with/

Ciao,
Dscho

>
> To accommodate t1050, which wants to compare files weighing in with 3MB
> (falling outside of t1050's malloc limit of 1.5MB), we simply lift the
> allocation limit by setting the environment variable GIT_ALLOC_LIMIT to
> zero when calling the helper.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     tests: replace mingw_test_cmp with a helper in C
>
>     On the heels of sending a patch to fix a performance regression due to a
>     mis-use of test_cmp
>     [https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/],
>     I was curious to see whether Git for Windows had the same issue. And it
>     does not
>     [https://github.com/git-for-windows/git/runs/7556381815?check_suite_focus=true#step:5:127]:
>     it passes t5351 in 22 seconds, even while using test_cmp to compare pack
>     files
>     [https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].
>
>     The answer is of course that a test helper written in C is much faster
>     than writing the same in Bash, especially on Windows. This is especially
>     sad when said Bash code is only used on Windows. So I pulled out this
>     helper from the years-long effort to let Git for Windows use BusyBox'
>     ash to run the test suite. The result is this patch, which has been in
>     Git for Windows since June 2018.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1309
>
>  Makefile                |  1 +
>  t/helper/test-cmp.c     | 73 +++++++++++++++++++++++++++++++++++++++++
>  t/helper/test-tool.c    |  1 +
>  t/helper/test-tool.h    |  1 +
>  t/test-lib-functions.sh | 68 +-------------------------------------
>  t/test-lib.sh           |  2 +-
>  6 files changed, 78 insertions(+), 68 deletions(-)
>  create mode 100644 t/helper/test-cmp.c
>
> diff --git a/Makefile b/Makefile
> index 1624471badc..45f108e43a1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o
>  TEST_BUILTINS_OBJS += test-bitmap.o
>  TEST_BUILTINS_OBJS += test-bloom.o
>  TEST_BUILTINS_OBJS += test-chmtime.o
> +TEST_BUILTINS_OBJS += test-cmp.o
>  TEST_BUILTINS_OBJS += test-config.o
>  TEST_BUILTINS_OBJS += test-crontab.o
>  TEST_BUILTINS_OBJS += test-csprng.o
> diff --git a/t/helper/test-cmp.c b/t/helper/test-cmp.c
> new file mode 100644
> index 00000000000..1c646a54bf6
> --- /dev/null
> +++ b/t/helper/test-cmp.c
> @@ -0,0 +1,73 @@
> +#include "test-tool.h"
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +#include "gettext.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +
> +#ifdef WIN32
> +#define NO_SUCH_DIR "\\\\.\\GLOBALROOT\\invalid"
> +#else
> +#define NO_SUCH_DIR "/dev/null"
> +#endif
> +
> +static int run_diff(const char *path1, const char *path2)
> +{
> +	const char *argv[] = {
> +		"diff", "--no-index", NULL, NULL, NULL
> +	};
> +	const char *env[] = {
> +		"GIT_PAGER=cat",
> +		"GIT_DIR=" NO_SUCH_DIR,
> +		"HOME=" NO_SUCH_DIR,
> +		NULL
> +	};
> +
> +	argv[2] = path1;
> +	argv[3] = 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)
> +{
> +	FILE *f0, *f1;
> +	struct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;
> +
> +	if (argc != 3)
> +		die("Require exactly 2 arguments, got %d", argc);
> +
> +	if (!(f0 = !strcmp(argv[1], "-") ? stdin : fopen(argv[1], "r")))
> +		return error_errno("could not open '%s'", argv[1]);
> +	if (!(f1 = !strcmp(argv[2], "-") ? stdin : fopen(argv[2], "r"))) {
> +		fclose(f0);
> +		return error_errno("could not open '%s'", argv[2]);
> +	}
> +
> +	for (;;) {
> +		int r0 = strbuf_getline(&b0, f0);
> +		int r1 = strbuf_getline(&b1, f1);
> +
> +		if (r0 == EOF) {
> +			fclose(f0);
> +			fclose(f1);
> +			strbuf_release(&b0);
> +			strbuf_release(&b1);
> +			if (r1 == EOF)
> +				return 0;
> +cmp_failed:
> +			if (!run_diff(argv[1], argv[2]))
> +				die("Huh? 'diff --no-index %s %s' succeeded",
> +				    argv[1], argv[2]);
> +			return 1;
> +		}
> +		if (r1 == EOF || strbuf_cmp(&b0, &b1)) {
> +			fclose(f0);
> +			fclose(f1);
> +			strbuf_release(&b0);
> +			strbuf_release(&b1);
> +			goto cmp_failed;
> +		}
> +	}
> +}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 318fdbab0c3..3334de248a1 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -18,6 +18,7 @@ static struct test_cmd cmds[] = {
>  	{ "bitmap", cmd__bitmap },
>  	{ "bloom", cmd__bloom },
>  	{ "chmtime", cmd__chmtime },
> +	{ "cmp", cmd__cmp },
>  	{ "config", cmd__config },
>  	{ "crontab", cmd__crontab },
>  	{ "csprng", cmd__csprng },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index bb799271631..e1104898cc3 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);
>  int cmd__bitmap(int argc, const char **argv);
>  int cmd__bloom(int argc, const char **argv);
>  int cmd__chmtime(int argc, const char **argv);
> +int cmd__cmp(int argc, const char **argv);
>  int cmd__config(int argc, const char **argv);
>  int cmd__crontab(int argc, const char **argv);
>  int cmd__csprng(int argc, const char **argv);
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 8c44856eaec..28eddbc8e36 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1240,7 +1240,7 @@ test_expect_code () {
>
>  test_cmp () {
>  	test "$#" -ne 2 && BUG "2 param"
> -	eval "$GIT_TEST_CMP" '"$@"'
> +	GIT_ALLOC_LIMIT=0 eval "$GIT_TEST_CMP" '"$@"'
>  }
>
>  # Check that the given config key has the expected value.
> @@ -1541,72 +1541,6 @@ test_skip_or_die () {
>  	error "$2"
>  }
>
> -# The following mingw_* functions obey POSIX shell syntax, but are actually
> -# bash scripts, and are meant to be used only with bash on Windows.
> -
> -# A test_cmp function that treats LF and CRLF equal and avoids to fork
> -# diff when possible.
> -mingw_test_cmp () {
> -	# Read text into shell variables and compare them. If the results
> -	# are different, use regular diff to report the difference.
> -	local test_cmp_a= test_cmp_b=
> -
> -	# When text came from stdin (one argument is '-') we must feed it
> -	# to diff.
> -	local stdin_for_diff=
> -
> -	# Since it is difficult to detect the difference between an
> -	# empty input file and a failure to read the files, we go straight
> -	# to diff if one of the inputs is empty.
> -	if test -s "$1" && test -s "$2"
> -	then
> -		# regular case: both files non-empty
> -		mingw_read_file_strip_cr_ test_cmp_a <"$1"
> -		mingw_read_file_strip_cr_ test_cmp_b <"$2"
> -	elif test -s "$1" && test "$2" = -
> -	then
> -		# read 2nd file from stdin
> -		mingw_read_file_strip_cr_ test_cmp_a <"$1"
> -		mingw_read_file_strip_cr_ test_cmp_b
> -		stdin_for_diff='<<<"$test_cmp_b"'
> -	elif test "$1" = - && test -s "$2"
> -	then
> -		# read 1st file from stdin
> -		mingw_read_file_strip_cr_ test_cmp_a
> -		mingw_read_file_strip_cr_ test_cmp_b <"$2"
> -		stdin_for_diff='<<<"$test_cmp_a"'
> -	fi
> -	test -n "$test_cmp_a" &&
> -	test -n "$test_cmp_b" &&
> -	test "$test_cmp_a" = "$test_cmp_b" ||
> -	eval "diff -u \"\$@\" $stdin_for_diff"
> -}
> -
> -# $1 is the name of the shell variable to fill in
> -mingw_read_file_strip_cr_ () {
> -	# Read line-wise using LF as the line separator
> -	# and use IFS to strip CR.
> -	local line
> -	while :
> -	do
> -		if IFS=$'\r' read -r -d $'\n' line
> -		then
> -			# good
> -			line=$line$'\n'
> -		else
> -			# we get here at EOF, but also if the last line
> -			# was not terminated by LF; in the latter case,
> -			# some text was read
> -			if test -z "$line"
> -			then
> -				# EOF, really
> -				break
> -			fi
> -		fi
> -		eval "$1=\$$1\$line"
> -	done
> -}
> -
>  # Like "env FOO=BAR some-program", but run inside a subshell, which means
>  # it also works for shell functions (though those functions cannot impact
>  # the environment outside of the test_env invocation).
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 7726d1da88a..220c259e796 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1546,7 +1546,7 @@ case $uname_s in
>  	test_set_prereq SED_STRIPS_CR
>  	test_set_prereq GREP_STRIPS_CR
>  	test_set_prereq WINDOWS
> -	GIT_TEST_CMP=mingw_test_cmp
> +	GIT_TEST_CMP="test-tool cmp"
>  	;;
>  *CYGWIN*)
>  	test_set_prereq POSIXPERM
>
> base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
> --
> gitgitgadget
>
>