git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Nov 14, 2022, 09:53 UTC
Message-ID
<8ec8f3f5-0827-25a3-464d-638378b65ddb@dunelm.org.uk>
In-Reply-To
<a7f4265ceb26c6dd9d347ef4cbef2aac7d60bf13.1668290855.git.gitgitgadget@gmail.com>
Hi Dscho
On 12/11/2022 22:07, Johannes Schindelin via GitGitGadget wrote:
Show 5 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> It is more performant to run `git diff --no-index` than running the
> `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for
> Windows uses. And a lot more readable.

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

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

Show 5 quoted lines
> Note: Earlier attempts at fixing this involved a test helper that avoids
> the overhead of the diff machinery, in favor of implementing a behavior
> that is more in line with what `mingw_test_cmp` does now, but that
> attempt saw a lot of backlash and distractions during review and was
> therefore abandoned.

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

Best Wishes
Phillip
Show 96 quoted lines
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   t/test-lib-functions.sh | 66 -----------------------------------------
>   t/test-lib.sh           |  2 +-
>   2 files changed, 1 insertion(+), 67 deletions(-)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 8c44856eaec..452fe9bc8aa 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -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..f8c6205e08f 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="GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --"
>   	;;
>   *CYGWIN*)
>   	test_set_prereq POSIXPERM
Previous: Junio C HamanoNext: Johannes Schindelin
Message 34 of 58 in “tests: replace mingw_test_cmp with a helper in C”
  1. tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Jul 29, 2022
  2. Johannes SchindelinJul 29, 2022
  3. Junio C HamanoJul 29, 2022
  4. Johannes SchindelinSep 6, 2022
  5. René ScharfeSep 7, 2022
  6. Junio C HamanoSep 7, 2022
  7. Re* [PATCH] tests: replace mingw_test_cmp with a helper in CJunio C Hamano, Sep 7, 2022
  8. René ScharfeSep 7, 2022
  9. Junio C HamanoSep 8, 2022
  10. René ScharfeSep 8, 2022
  11. Ævar Arnfjörð BjarmasonSep 8, 2022
  12. Johannes SchindelinSep 8, 2022
  13. Junio C HamanoSep 8, 2022
  14. 0/2 tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Sep 6, 2022
  15. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Sep 6, 2022
  16. 2/2 tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Sep 6, 2022
  17. Ævar Arnfjörð BjarmasonSep 7, 2022
  18. Ævar Arnfjörð BjarmasonSep 7, 2022
  19. Junio C HamanoSep 7, 2022
  20. Johannes SchindelinSep 7, 2022
  21. 0/2 tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Nov 12, 2022
  22. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Nov 12, 2022
  23. 2/2 tests(mingw): avoid very slow `mingw_test_cmp`Johannes Schindelin via GitGitGadget, Nov 12, 2022
  24. Taylor BlauNov 13, 2022
  25. Johannes SchindelinNov 14, 2022
  26. Junio C HamanoNov 18, 2022
  27. Taylor BlauNov 19, 2022
  28. Ævar Arnfjörð BjarmasonNov 19, 2022
  29. Johannes SixtNov 19, 2022
  30. René ScharfeNov 19, 2022
  31. Torsten BögershausenNov 20, 2022
  32. Johannes SixtNov 21, 2022
  33. Junio C HamanoNov 21, 2022
  34. Phillip WoodNov 14, 2022
  35. Johannes SchindelinNov 14, 2022
  36. Ævar Arnfjörð BjarmasonNov 14, 2022
  37. Johannes SchindelinNov 14, 2022
  38. Ævar Arnfjörð BjarmasonNov 14, 2022
  39. Junio C HamanoNov 18, 2022
  40. Taylor BlauNov 19, 2022
  41. Ævar Arnfjörð BjarmasonNov 19, 2022
  42. Junio C HamanoNov 21, 2022
  43. 0/2 tests(mingw): avoid super-slow mingw_test_cmpJohannes Schindelin via GitGitGadget, Nov 14, 2022
  44. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Nov 14, 2022
  45. 2/2 tests(mingw): avoid very slow `mingw_test_cmp`Johannes Schindelin via GitGitGadget, Nov 14, 2022
  46. Taylor BlauNov 14, 2022
  47. Johannes SchindelinNov 18, 2022
  48. Taylor BlauNov 18, 2022
  49. Johannes SchindelinNov 20, 2022
  50. Taylor BlauNov 21, 2022
  51. 0/2 tests(mingw): avoid super-slow mingw_test_cmpJohannes Schindelin via GitGitGadget, Dec 6, 2022
  52. 2/2 tests(mingw): avoid very slow `mingw_test_cmp`Johannes Schindelin via GitGitGadget, Dec 6, 2022
  53. Ævar Arnfjörð BjarmasonDec 6, 2022
  54. Johannes SixtDec 6, 2022
  55. René ScharfeDec 6, 2022
  56. Junio C HamanoDec 7, 2022
  57. Taylor BlauDec 7, 2022
  58. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Dec 6, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.