[PATCH 0/5] Some fixes and an improvement for using CTest on Windows

[PATCH 0/5] Some fixes and an improvement for using CTest on Windows

[Johannes Schindelin via GitGitGadget]

As part of the CMake definition, Git enjoy support for running the test
suite via CTest.

In https://github.com/git-for-windows/git/issues/3966, it has been reported
that this does not work out of the box, though, but causes a couple of test
failures instead. These problems are not caught by Git's CI runs because the
vs-tests jobs actually use prove to run the test suite, not CTest.

In addition to fixing these problems, this patch series also addresses a
long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified
(but these modification should never be committed, they refer to a
local-only, configuration-dependent directory).

Johannes Schindelin (5):
  cmake: align CTest definition with Git's CI runs
  cmake: copy the merge tools for testing
  tests: explicitly skip `chmod` calls on Windows
  add -p: avoid ambiguous signed/unsigned comparison
  cmake: avoid editing t/test-lib.sh

 .gitignore                          |  1 +
 Makefile                            |  1 +
 add-patch.c                         |  2 +-
 contrib/buildsystems/CMakeLists.txt | 12 ++++--------
 t/test-lib-functions.sh             | 10 ++++++++--
 t/test-lib.sh                       | 11 ++++++++++-
 6 files changed, 25 insertions(+), 12 deletions(-)


base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1320
-- 
gitgitgadget

[PATCH 1/5] cmake: align CTest definition with Git's CI runs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and
`--no-chain-lint`, mainly to win back some time caused by the serious
performance penalty paid for the tests relying so heavily on POSIX shell
scripting, which only works by using a POSIX emulation layer.

Let's do the same when running the tests, say, in Visual Studio.

While at it, enable the command trace via `-x` and verbose output via
`-v`, otherwise it would be near impossible to diagnose any problems.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 1b23f2440d8..4aee1e24342 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 #test
 foreach(tsh ${test_scipts})
 	add_test(NAME ${tsh}
-		COMMAND ${SH_EXE} ${tsh}
+		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
-- 
gitgitgadget

[PATCH 2/5] cmake: copy the merge tools for testing

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Even when running the tests via CTest, t7609 and t7610 rely on more than
only a few mergetools to be copied to the build directory. Let's make it
so.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 4aee1e24342..fe606c179f7 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
 	#misc copies
 	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
 	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
-	file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
+	file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
+	file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 endif()
-- 
gitgitgadget

[PATCH 3/5] tests: explicitly skip `chmod` calls on Windows

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, we use a POSIX emulation layer, the MSYS2 runtime, to allow
for running shell scripts (albeit at a hefty performance cost, Git's
test suite takes ~700 seconds to complete on Linux, according to Git's
CI runs, while it takes more than 6,000 seconds on Windows).

This emulation layer has a funny quirk when it comes to `chmod`
invocations: it pretends that it succeeded, when in reality it did not
do a thing (because the Access Control Lists used in Windows' permission
model are so different to Unix' default permission model that Git's test
suite assumes to be in effect).

Git's test suite relies on this quirk by assuming that the `chmod` calls
in `test_chmod` and `test_write_script` simply succeed on Windows
(without actually doing anything).

However, this quirk is only in effect as long as `chmod` is run inside
the pseudo Unix root directory structure or within the home directory.
When run outside, such invocations fail like this:

	chmod: changing permissions of '<file>': Invalid argument

Now, when running Git's tests in, say, Visual Studio, we frequently are
in a worktree where the `chmod` invocations would fail.

Let's accommodate for that by explicitly skipping those `chmod`
invocations on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib-functions.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6da7273f1d5..7c63b22acab 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -492,7 +492,10 @@ test_commit_bulk () {
 # of a file in the working directory and add it to the index.
 
 test_chmod () {
-	chmod "$@" &&
+	if test_have_prereq !MINGW
+	then
+		chmod "$@"
+	fi &&
 	git update-index --add "--chmod=$@"
 }
 
@@ -548,7 +551,10 @@ write_script () {
 		echo "#!${2-"$SHELL_PATH"}" &&
 		cat
 	} >"$1" &&
-	chmod +x "$1"
+	if test_have_prereq !MINGW
+	then
+		chmod +x "$1"
+	fi
 }
 
 # Usage: test_hook [options] <hook-name> <<-\EOF
-- 
gitgitgadget

[PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the interactive `add` operation, users can choose to jump to specific
hunks, and Git will present the hunk list in that case. To avoid showing
too many lines at once, only a maximum of 21 hunks are shown, skipping
the "mode change" pseudo hunk.

The comparison performed to skip the "mode change" pseudo hunk (if any)
compares a signed integer `i` to the unsigned value `mode_change` (which
can be 0 or 1 because it is a 1-bit type).

According to section 6.3.1.8 of the C99 standard (see e.g.
https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
happen is an automatic conversion of the "lesser" type to the "greater"
type, but since the types differ in signedness, it is ill-defined what
is the correct "usual arithmetic conversion".

Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter, `add -p`'s `goto` command shows no
hunks by default because it casts a negative start offset to a pretty
large unsigned value, breaking the "goto hunk" test case in
`t3701-add-interactive.sh`.

Let's avoid that by converting the unsigned bit explicitly to a signed
integer.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..3524555e2b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1547,7 +1547,7 @@ soft_increment:
 			strbuf_remove(&s->answer, 0, 1);
 			strbuf_trim(&s->answer);
 			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
-			if (i < file_diff->mode_change)
+			if (i < (int)file_diff->mode_change)
 				i = file_diff->mode_change;
 			while (s->answer.len == 0) {
 				i = display_hunks(s, file_diff, i);
-- 
gitgitgadget

[PATCH 5/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 7f5397a07c6c (cmake: support for testing git when building out of the
source tree, 2020-06-26), we implemented support for running Git's test
scripts even after building Git in a different directory than the source
directory.

The way we did this was to edit the file `t/test-lib.sh` to override
`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
directory.

This is unideal because it always leaves a tracked file marked as
modified, and it is all too easy to commit that change by mistake.

Let's change the strategy by teaching `t/test-lib.sh` to detect the
presence of a file called `GIT-BUILD-DIR` in the source directory. If it
exists, the contents are interpreted as the location to the _actual_
build directory. We then write this file as part of the CTest
definition.

To support building Git via a regular `make` invocation after building
it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
convenience, this is done as part of the Makefile rule that is already
run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
up to date).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore                          |  1 +
 Makefile                            |  1 +
 contrib/buildsystems/CMakeLists.txt |  7 +------
 t/test-lib.sh                       | 11 ++++++++++-
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/.gitignore b/.gitignore
index a4522157641..b72ddf09346 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /fuzz_corpora
 /fuzz-pack-headers
 /fuzz-pack-idx
+/GIT-BUILD-DIR
 /GIT-BUILD-OPTIONS
 /GIT-CFLAGS
 /GIT-LDFLAGS
diff --git a/Makefile b/Makefile
index 04d0fd1fe60..9347ed90da7 100644
--- a/Makefile
+++ b/Makefile
@@ -3028,6 +3028,7 @@ else
 	@echo RUNTIME_PREFIX=\'false\' >>$@+
 endif
 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
 
 ### Detect Python interpreter path changes
 ifndef NO_PYTHON
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index fe606c179f7..29d7e236ae1 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1067,14 +1067,9 @@ endif()
 #Make the tests work when building out of the source tree
 get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
 if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
-	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
-	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
 	#Setting the build directory in test-lib.sh before running tests
 	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
-		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
-		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
-		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
-		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
+		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
 	#misc copies
 	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
 	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55857af601b..4468ac51f25 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -42,7 +42,16 @@ then
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
 GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
-if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
+	# On Windows, we must convert Windows paths lest they contain a colon
+	case "$(uname -s)" in
+	*MINGW*)
+		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+		;;
+	esac
+elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
 then
 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
 	exit 1
-- 
gitgitgadget

Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> While at it, enable the command trace via `-x` and verbose output via
> `-v`, otherwise it would be near impossible to diagnose any problems.

This sounds like a completely unrelated change.  Do the tests behave
so differently here, compared to how they are run in t/Makefile,
where -vx is not the default?

The only plausible reason I can think of that this change "while at
it" is justifiable as part of the other change is if this is only
ever used in the CI environment and no interactive human would be
sitting in front of the session that runs tests here.  If that is
the case, I do agree it would make sense, since it would be much
less convenient to go back to a failing test and rerun it with -v
or -x or whatever.  But that needs to be explained in the proposed
log message.

Thanks.


> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>  #test
>  foreach(tsh ${test_scipts})
>  	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the interactive `add` operation, users can choose to jump to specific
> hunks, and Git will present the hunk list in that case. To avoid showing
> too many lines at once, only a maximum of 21 hunks are shown, skipping
> the "mode change" pseudo hunk.
>
> The comparison performed to skip the "mode change" pseudo hunk (if any)
> compares a signed integer `i` to the unsigned value `mode_change` (which
> can be 0 or 1 because it is a 1-bit type).
>
> According to section 6.3.1.8 of the C99 standard (see e.g.
> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
> happen is an automatic conversion of the "lesser" type to the "greater"
> type, but since the types differ in signedness, it is ill-defined what
> is the correct "usual arithmetic conversion".
>
> Which means that Visual C's behavior can (and does) differ from GCC's:
> When compiling Git using the latter, `add -p`'s `goto` command shows no
> hunks by default because it casts a negative start offset to a pretty
> large unsigned value, breaking the "goto hunk" test case in
> `t3701-add-interactive.sh`.
>
> Let's avoid that by converting the unsigned bit explicitly to a signed
> integer.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

This looks more like a fix to a general problem, not limited to
windows or cmake, that we had since 9254bdfb (built-in add -p:
implement the 'g' ("goto") command, 2019-12-13).

Please pull this out of the series and let's have it reviewed
separately.

Thanks.

>  add-patch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..3524555e2b0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1547,7 +1547,7 @@ soft_increment:
>  			strbuf_remove(&s->answer, 0, 1);
>  			strbuf_trim(&s->answer);
>  			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
> -			if (i < file_diff->mode_change)
> +			if (i < (int)file_diff->mode_change)
>  				i = file_diff->mode_change;
>  			while (s->answer.len == 0) {
>  				i = display_hunks(s, file_diff, i);

Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and
> `--no-chain-lint`, mainly to win back some time caused by the serious
> performance penalty paid for the tests relying so heavily on POSIX shell
> scripting, which only works by using a POSIX emulation layer.
>
> Let's do the same when running the tests, say, in Visual Studio.
>
> While at it, enable the command trace via `-x` and verbose output via
> `-v`, otherwise it would be near impossible to diagnose any problems.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>  #test
>  foreach(tsh ${test_scipts})
>  	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()

Is this authored before a561962479c (cmake: fix CMakeLists.txt on Linux,
2022-05-24) was merged?

The "say, in Visual Studio" seems to elide that we'll now run these
tests differently when you run with cmake everywhere.

It seems much better to pass some "test arguments" to cmake itself,
which we'd then call from the ci specifically. Then e.g. a Linux user of
cmake wouldn't wonder why the "make test" spots e.g. a chain-lint issue
that the cmake testing would hide.

Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
> However, this quirk is only in effect as long as `chmod` is run inside
> the pseudo Unix root directory structure or within the home directory.
> When run outside, such invocations fail like this:
>
> 	chmod: changing permissions of '<file>': Invalid argument

..ok, but...

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 6da7273f1d5..7c63b22acab 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -492,7 +492,10 @@ test_commit_bulk () {
>  # of a file in the working directory and add it to the index.
>  
>  test_chmod () {
> -	chmod "$@" &&
> +	if test_have_prereq !MINGW
> +	then
> +		chmod "$@"
> +	fi &&
>  	git update-index --add "--chmod=$@"
>  }
>  
> @@ -548,7 +551,10 @@ write_script () {
>  		echo "#!${2-"$SHELL_PATH"}" &&
>  		cat
>  	} >"$1" &&
> -	chmod +x "$1"
> +	if test_have_prereq !MINGW
> +	then
> +		chmod +x "$1"
> +	fi

... you get +x semantics by default, so we didn't need that "chmod +x"
in the first place?

The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
don't care about that, regardless of the "pseudo Unix root directory"?

What if we get a "test_chmod -o <file>", won't this silently do the
wrong thing?

If so isn't something in this direction (untested) a more targeted &
obvious fix?:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 10258def7be..1c3b6692388 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1690,6 +1690,16 @@ case $uname_s in
 	find () {
 		/usr/bin/find "$@"
 	}
+	chmod () {
+		case "$1" in
+		+x|-x)
+			return;
+			;;
+		*)
+			;;
+		esac &&
+		/usr/bin/chmod "$@"
+	}
 	# git sees Windows-style pwd
 	pwd () {
 		builtin pwd -W

Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
>
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
>
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
>
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
>
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .gitignore                          |  1 +
>  Makefile                            |  1 +
>  contrib/buildsystems/CMakeLists.txt |  7 +------
>  t/test-lib.sh                       | 11 ++++++++++-
>  4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
>  /fuzz_corpora
>  /fuzz-pack-headers
>  /fuzz-pack-idx
> +/GIT-BUILD-DIR
>  /GIT-BUILD-OPTIONS
>  /GIT-CFLAGS
>  /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
>  	@echo RUNTIME_PREFIX=\'false\' >>$@+
>  endif
>  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>  
>  ### Detect Python interpreter path changes
>  ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
>  #Make the tests work when building out of the source tree
>  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>  	#Setting the build directory in test-lib.sh before running tests
>  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>  	#misc copies
>  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 55857af601b..4468ac51f25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,7 +42,16 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> +	# On Windows, we must convert Windows paths lest they contain a colon
> +	case "$(uname -s)" in
> +	*MINGW*)
> +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> +		;;
> +	esac
> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>  then
>  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>  	exit 1

So we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct
and assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests
run inside a "t" directory generated in contrib/buildsystems/ ?

This really seems like a hack-upon-hack, and will break some workflows
when you switch back & forth, e.g.:

	make
	# run cmake
	make -C t

Will now pick up this new file, and result in broken tests, as we'll
rely on the now-stale file.

IOW you're making the assumption that by piggy-backing on the
GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run
it*, but that's not the case.

I can think of ways to get around it, but it would be nasty as we'd need
to complete the dependency graph between the two, and figure out the
various interactions.

But why do we need this at all? Right now I tried:

 * (Manually) removing the existing hack
 * Copying t/*lib*.sh over to contrib/buildsystems/t
 * cd contrib/buildsystems/t
 * ../../../t/t0001-init.sh 

Which (aside from a small fixable nit about oid-info) Just Works,
because the cmake build is already creating a GIT-BUILD-OPTIONS.

Presumably that "copying" step should be simlynking, or we'd be smarter
about doing includes from test-lib.sh.

But isn't that a much better approach? Rather than working around the
directory discovery just teach it to run from the generated t/ directory
directly?

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Phillip Wood]

Hi Dscho

On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In the interactive `add` operation, users can choose to jump to specific
> hunks, and Git will present the hunk list in that case. To avoid showing
> too many lines at once, only a maximum of 21 hunks are shown, skipping
> the "mode change" pseudo hunk.
> 
> The comparison performed to skip the "mode change" pseudo hunk (if any)
> compares a signed integer `i` to the unsigned value `mode_change` (which
> can be 0 or 1 because it is a 1-bit type).
> 
> According to section 6.3.1.8 of the C99 standard (see e.g.
> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
> happen is an automatic conversion of the "lesser" type to the "greater"
> type, but since the types differ in signedness, it is ill-defined what
> is the correct "usual arithmetic conversion".
> 
> Which means that Visual C's behavior can (and does) differ from GCC's:
> When compiling Git using the latter, `add -p`'s `goto` command shows no
> hunks by default because it casts a negative start offset to a pretty
> large unsigned value, breaking the "goto hunk" test case in
> `t3701-add-interactive.sh`.
> 
> Let's avoid that by converting the unsigned bit explicitly to a signed
> integer.

Oh the joys of bit-fields!

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..3524555e2b0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1547,7 +1547,7 @@ soft_increment:
>   			strbuf_remove(&s->answer, 0, 1);
>   			strbuf_trim(&s->answer);

Unrelated to this change but why don't we just call strbuf_reset() here?

>   			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
> -			if (i < file_diff->mode_change)
> +			if (i < (int)file_diff->mode_change)

The change itself looks good

Best Wishes

Phillip

>   				i = file_diff->mode_change;
>   			while (s->answer.len == 0) {
>   				i = display_hunks(s, file_diff, i);

Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh

[Phillip Wood]

Hi Dscho

On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
> 
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
> 
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
> 
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.

I think it is really good to get away from editing the test files, but 
one of the nice things about CMake's out of tree builds is that you can 
have several build directories with different build configurations and 
this change does not support that. Could we pass the build directory to 
the test scripts as a commandline option or environment variable 
instead? e.g.

   foreach(tsh ${test_scipts})
    	add_test(NAME ${tsh}
-		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
+		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint 
--build-dir=${CMAKE_BINARY_DIR} -vx

   		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
   endforeach()

Doing that would avoid changing the main Makefile to remove a file which 
almost certainly does not exist every time make is invoked as well.

Best Wishes

Phillip

> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   .gitignore                          |  1 +
>   Makefile                            |  1 +
>   contrib/buildsystems/CMakeLists.txt |  7 +------
>   t/test-lib.sh                       | 11 ++++++++++-
>   4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
>   /fuzz_corpora
>   /fuzz-pack-headers
>   /fuzz-pack-idx
> +/GIT-BUILD-DIR
>   /GIT-BUILD-OPTIONS
>   /GIT-CFLAGS
>   /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
>   	@echo RUNTIME_PREFIX=\'false\' >>$@+
>   endif
>   	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>   
>   ### Detect Python interpreter path changes
>   ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
>   #Make the tests work when building out of the source tree
>   get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>   if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>   	#Setting the build directory in test-lib.sh before running tests
>   	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>   	#misc copies
>   	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>   	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 55857af601b..4468ac51f25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,7 +42,16 @@ then
>   	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>   fi
>   GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> +	# On Windows, we must convert Windows paths lest they contain a colon
> +	case "$(uname -s)" in
> +	*MINGW*)
> +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> +		;;
> +	esac
> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>   then
>   	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>   	exit 1

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Aug 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In the interactive `add` operation, users can choose to jump to specific
> > hunks, and Git will present the hunk list in that case. To avoid showing
> > too many lines at once, only a maximum of 21 hunks are shown, skipping
> > the "mode change" pseudo hunk.
> >
> > The comparison performed to skip the "mode change" pseudo hunk (if any)
> > compares a signed integer `i` to the unsigned value `mode_change` (which
> > can be 0 or 1 because it is a 1-bit type).
> >
> > According to section 6.3.1.8 of the C99 standard (see e.g.
> > https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
> > happen is an automatic conversion of the "lesser" type to the "greater"
> > type, but since the types differ in signedness, it is ill-defined what
> > is the correct "usual arithmetic conversion".
> >
> > Which means that Visual C's behavior can (and does) differ from GCC's:
> > When compiling Git using the latter, `add -p`'s `goto` command shows no
> > hunks by default because it casts a negative start offset to a pretty
> > large unsigned value, breaking the "goto hunk" test case in
> > `t3701-add-interactive.sh`.
> >
> > Let's avoid that by converting the unsigned bit explicitly to a signed
> > integer.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> This looks more like a fix to a general problem, not limited to
> windows or cmake, that we had since 9254bdfb (built-in add -p:
> implement the 'g' ("goto") command, 2019-12-13).
>
> Please pull this out of the series and let's have it reviewed
> separately.

The scope of this patch series is to fix running the tests in Visual
Studio when building using CMake.

Pulling out this patch would break that patch series because it would
leave that breakage in place.

Except if you are asking to put this patch series on the back burner and
prioritize the patch that fixes an ambiguous implicit cast between signed
and unsigned data types?

However, that would mean that I'd now have to address all of those
implicit casts, which is unfortunately a larger amount of work than I can
justify to take on.

Therefore I move that in this instance, the perfect is the enemy of the
good, and that the patch should remain within this patch series, even if
the larger-scoped project to avoid any implicit signed/unsigned casts
remains unaddressed.

BTW I would have expected your review to ask the (in hindsight, obvious)
question why the test suite still passes even with `vs-test` exercising
the code that is compiled using Visual C?

The answer to that would have been that the `vs-test` job of our CI runs
defines `NO_PERL`, and t3701 is skipped completely if that is the case.

Ciao,
Dscho

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin]

Hi Phillip,

On Thu, 11 Aug 2022, Phillip Wood wrote:

> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
>
> > diff --git a/add-patch.c b/add-patch.c
> > index 509ca04456b..3524555e2b0 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -1547,7 +1547,7 @@ soft_increment:
> >      strbuf_remove(&s->answer, 0, 1);
> >      strbuf_trim(&s->answer);
>
> Unrelated to this change but why don't we just call strbuf_reset() here?

This part of the code is used when the `g` command (to "go to hunk") was
issued by the user. And that `g` command allows for a number to be
specified, e.g. `g1` to go to the first hunk.

The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.

The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and
the number, e.g. `g 1` should also go to the first hunk.

If we called `strbuf_reset()` here, we would remove the number completely.

Ciao,
Dscho

Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin]

Hi Phillip,

On Thu, 11 Aug 2022, Phillip Wood wrote:

> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 7f5397a07c6c (cmake: support for testing git when building out of the
> > source tree, 2020-06-26), we implemented support for running Git's test
> > scripts even after building Git in a different directory than the source
> > directory.
> >
> > The way we did this was to edit the file `t/test-lib.sh` to override
> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> > directory.
> >
> > This is unideal because it always leaves a tracked file marked as
> > modified, and it is all too easy to commit that change by mistake.
> >
> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> > exists, the contents are interpreted as the location to the _actual_
> > build directory. We then write this file as part of the CTest
> > definition.
>
> I think it is really good to get away from editing the test files, but one of
> the nice things about CMake's out of tree builds is that you can have several
> build directories with different build configurations and this change does not
> support that. Could we pass the build directory to the test scripts as a
> commandline option or environment variable instead? e.g.
>
>   foreach(tsh ${test_scipts})
>    	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint
> --build-dir=${CMAKE_BINARY_DIR} -vx
>
>   		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>   endforeach()
>
> Doing that would avoid changing the main Makefile to remove a file which
> almost certainly does not exist every time make is invoked as well.

This is indeed tempting, but one of the things I recommend to Visual
Studio users for ages already is to run the tests in a Git Bash, i.e.
outside of Visual Studio, to allow for passing `--run=...` options and the
likes. This recommendation predates support for CTest, naturally, but it
is still valid.

And that recommended way of running tests would be broken by the suggested
change because the tests would no longer run except when using CTest.

Besides, while the semantics look tempting, the implementation details do
not. The reason is that we use `GIT_BUILD_DIR` to source the
`GIT-BUILD-OPTIONS` and to validate that Git was built, long before we
parse the command-line in `t/test-lib.sh`. I want to refrain from the
prerequisite extensive refactoring required to support this, at least in
the context of this here patch series.

Can we maybe agree that the proposed patch is a net improvement over the
status quo, and think about a better solution independently (without
blocking this here patch)?

Ciao,
Dscho

Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Aug 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > While at it, enable the command trace via `-x` and verbose output via
> > `-v`, otherwise it would be near impossible to diagnose any problems.
>
> This sounds like a completely unrelated change.

It may sound like it, but it is not. In order to make sense of the broken
tests, I needed access to more verbose output than our test scripts
provide by default.

When running the test suite on the command-line, it is easy to tell the
user "oh, if you need more information, just call the test script with
these here options: ...".

This is not an option when running the tests within Visual Studio.

Does this clarify the intention and validity of the proposed patch? If so,
I will gladly try my best to improve the commit message to explain that
intention better.

Ciao,
Dscho

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Phillip Wood]

Hi Dscho

On 16/08/2022 11:00, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 11 Aug 2022, Phillip Wood wrote:
> 
>> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
>>
>>> diff --git a/add-patch.c b/add-patch.c
>>> index 509ca04456b..3524555e2b0 100644
>>> --- a/add-patch.c
>>> +++ b/add-patch.c
>>> @@ -1547,7 +1547,7 @@ soft_increment:
>>>       strbuf_remove(&s->answer, 0, 1);
>>>       strbuf_trim(&s->answer);
>>
>> Unrelated to this change but why don't we just call strbuf_reset() here?
> 
> This part of the code is used when the `g` command (to "go to hunk") was
> issued by the user. And that `g` command allows for a number to be
> specified, e.g. `g1` to go to the first hunk.
> 
> The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.
> 
> The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and
> the number, e.g. `g 1` should also go to the first hunk.
> 
> If we called `strbuf_reset()` here, we would remove the number completely.

Oh so if the user is not using interactive.singleKey then they can skip 
the prompt which displays the hunks and asks which one they want to jump 
to by guessing the number of the hunk they want and typing "g <n>".

Thanks

Phillip

> Ciao,
> Dscho

Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh

[Phillip Wood]

Hi Dscho

On 16/08/2022 11:09, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 11 Aug 2022, Phillip Wood wrote:
> 
>> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> In 7f5397a07c6c (cmake: support for testing git when building out of the
>>> source tree, 2020-06-26), we implemented support for running Git's test
>>> scripts even after building Git in a different directory than the source
>>> directory.
>>>
>>> The way we did this was to edit the file `t/test-lib.sh` to override
>>> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
>>> directory.
>>>
>>> This is unideal because it always leaves a tracked file marked as
>>> modified, and it is all too easy to commit that change by mistake.
>>>
>>> Let's change the strategy by teaching `t/test-lib.sh` to detect the
>>> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
>>> exists, the contents are interpreted as the location to the _actual_
>>> build directory. We then write this file as part of the CTest
>>> definition.
>>
>> I think it is really good to get away from editing the test files, but one of
>> the nice things about CMake's out of tree builds is that you can have several
>> build directories with different build configurations and this change does not
>> support that. Could we pass the build directory to the test scripts as a
>> commandline option or environment variable instead? e.g.
>>
>>    foreach(tsh ${test_scipts})
>>     	add_test(NAME ${tsh}
>> -		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint
>> --build-dir=${CMAKE_BINARY_DIR} -vx
>>
>>    		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>>    endforeach()
>>
>> Doing that would avoid changing the main Makefile to remove a file which
>> almost certainly does not exist every time make is invoked as well.
> 
> This is indeed tempting, but one of the things I recommend to Visual
> Studio users for ages already is to run the tests in a Git Bash, i.e.
> outside of Visual Studio, to allow for passing `--run=...` options and the
> likes. This recommendation predates support for CTest, naturally, but it
> is still valid.
> 
> And that recommended way of running tests would be broken by the suggested
> change because the tests would no longer run except when using CTest.

Yes the recommendation would have to include adding --build-dir which 
would be a pain

> Besides, while the semantics look tempting, the implementation details do
> not. The reason is that we use `GIT_BUILD_DIR` to source the
> `GIT-BUILD-OPTIONS` and to validate that Git was built, long before we
> parse the command-line in `t/test-lib.sh`. I want to refrain from the
> prerequisite extensive refactoring required to support this, at least in
> the context of this here patch series.

That sounds awkward.

> Can we maybe agree that the proposed patch is a net improvement over the
> status quo, and think about a better solution independently (without
> blocking this here patch)?

It is definitely an improvement for the CMake side, I'm not sure it is 
an improvement for the Makefile but it sounds like the best option overall.

Best Wishes

Phillip

> Ciao,
> Dscho

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The scope of this patch series is to fix running the tests in Visual
> Studio when building using CMake.

That's only the perspective of who cares about VS+CMake.  From my
point of view who wants a healthy Git overall, the priority is
different.  "add -p" fix is wider than the context of VS, and I do
not discount the need that we need to fix it before we can call
VS+CMake issue fixed (and I do not mean to say it is unnecessary to
fix VS+CMake).  It's just this one can proceed with help by those
who do not care about or have no environment to help with VS+CMake
because it is more generic, and I do not think you mind to make the
rest of the narrower VS+CMake topic _depend_ on it.

> Pulling out this patch would break that patch series because it would
> leave that breakage in place.

We deal with topic that depends on another topic all the time, and I
do not think there is anything that makes VS+CMake topic so special
that it cannot be dependent on another topic.  It's just the matter
of splitting this out and make it [1/1], and make the remainder to
[1-4/4] and mark them rely on add-p fix when you send the latter
out, isn't it?  Puzzled...

Thanks.

Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Wed, 10 Aug 2022, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>
>> > While at it, enable the command trace via `-x` and verbose output via
>> > `-v`, otherwise it would be near impossible to diagnose any problems.
>>
>> This sounds like a completely unrelated change.
>
> It may sound like it, but it is not. In order to make sense of the broken
> tests, I needed access to more verbose output than our test scripts
> provide by default.
>
> When running the test suite on the command-line, it is easy to tell the
> user "oh, if you need more information, just call the test script with
> these here options: ...".
>
> This is not an option when running the tests within Visual Studio.

I gave an example of "CI environment it is cumbersome to go back and
run only a single one" in the review you are responding to that may
explain why such a change is needed, and you gave us exactly the
context that was lacking here.  The environment does not let users
run the tests as anything but a single monolithic ball of wax.

> Does this clarify the intention and validity of the proposed patch? If so,
> I will gladly try my best to improve the commit message to explain that
> intention better.

It explains why -x -v is needed and needs to be a part of VS+CMake
topic, and it will help readers to have it in the explanation.  It
still does not justify why it has to be a part of a step to omit
bin-warppers and skip chain-lint that has nothing to do it, though.

Thanks.

Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs

[Johannes Schindelin]

Hi Junio,

On Tue, 16 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Wed, 10 Aug 2022, Junio C Hamano wrote:
> >
> >> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> >> writes:
> >>
> >> > While at it, enable the command trace via `-x` and verbose output via
> >> > `-v`, otherwise it would be near impossible to diagnose any problems.
> >>
> >> This sounds like a completely unrelated change.
> >
> > It may sound like it, but it is not. In order to make sense of the broken
> > tests, I needed access to more verbose output than our test scripts
> > provide by default.
> >
> > When running the test suite on the command-line, it is easy to tell the
> > user "oh, if you need more information, just call the test script with
> > these here options: ...".
> >
> > This is not an option when running the tests within Visual Studio.
>
> I gave an example of "CI environment it is cumbersome to go back and
> run only a single one" in the review you are responding to that may
> explain why such a change is needed, and you gave us exactly the
> context that was lacking here.  The environment does not let users
> run the tests as anything but a single monolithic ball of wax.

Good, I will amend the commit message.

> > Does this clarify the intention and validity of the proposed patch? If so,
> > I will gladly try my best to improve the commit message to explain that
> > intention better.
>
> It explains why -x -v is needed and needs to be a part of VS+CMake
> topic, and it will help readers to have it in the explanation.  It
> still does not justify why it has to be a part of a step to omit
> bin-warppers and skip chain-lint that has nothing to do it, though.

Oh, that ;-)

To be honest, I reflexively do that on Windows because it saves _so much
time_.

I will amend the commit message to clarify that, too.

Ciao,
Dscho

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin]

Hi Phillip,

On Tue, 16 Aug 2022, Phillip Wood wrote:

> On 16/08/2022 11:00, Johannes Schindelin wrote:
>
> > On Thu, 11 Aug 2022, Phillip Wood wrote:
> >
> > > On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > diff --git a/add-patch.c b/add-patch.c
> > > > index 509ca04456b..3524555e2b0 100644
> > > > --- a/add-patch.c
> > > > +++ b/add-patch.c
> > > > @@ -1547,7 +1547,7 @@ soft_increment:
> > > >       strbuf_remove(&s->answer, 0, 1);
> > > >       strbuf_trim(&s->answer);
> > >
> > > Unrelated to this change but why don't we just call strbuf_reset() here?
> >
> > This part of the code is used when the `g` command (to "go to hunk") was
> > issued by the user. And that `g` command allows for a number to be
> > specified, e.g. `g1` to go to the first hunk.
> >
> > The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.
> >
> > The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and
> > the number, e.g. `g 1` should also go to the first hunk.
> >
> > If we called `strbuf_reset()` here, we would remove the number completely.
>
> Oh so if the user is not using interactive.singleKey then they can skip the
> prompt which displays the hunks and asks which one they want to jump to by
> guessing the number of the hunk they want and typing "g <n>".

Yes, precisely.

I forgot that you are using `interactive.singleKey` (I planned on opting
into that mode, but for some reason I never get around to flip the
switch). The default mode is not the single key mode, though.

Ciao,
Dscho

Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5547 bytes --]

Hi Junio,

On Tue, 16 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The scope of this patch series is to fix running the tests in Visual
> > Studio when building using CMake.
>
> That's only the perspective of who cares about VS+CMake.  From my
> point of view who wants a healthy Git overall, the priority is
> different.  "add -p" fix is wider than the context of VS, and I do
> not discount the need that we need to fix it before we can call
> VS+CMake issue fixed (and I do not mean to say it is unnecessary to
> fix VS+CMake).  It's just this one can proceed with help by those
> who do not care about or have no environment to help with VS+CMake
> because it is more generic, and I do not think you mind to make the
> rest of the narrower VS+CMake topic _depend_ on it.

Sure.

But if I pull out that patch into its code contribution, all of a sudden
the scope of _that_ contribution is to address an implicit signed/unsigned
cast. What other purpose could it have? And if we're addressing that
signed/unsigned cast, we cannot just fix this single one. We need to fix
them all, no?

So let's have a look at that project, since you are implicitly
volunteering me for it. We do have this in our `config.mak.dev`:

	# These are disabled because we have these all over the place.
	DEVELOPER_CFLAGS += -Wno-empty-body
	DEVELOPER_CFLAGS += -Wno-missing-field-initializers
	DEVELOPER_CFLAGS += -Wno-sign-compare
	DEVELOPER_CFLAGS += -Wno-unused-parameter
	endif

Note the `-Wno-sign-compare` part. If I comment that out, I get these
reports:

-- snip --
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
add-interactive.c:207:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:210:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:238:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-patch.c:423:16: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
add-interactive.c:379:25: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:389:11: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
[... 1260 more issues ...]
-- snap --

You see how that increases the amount of work you are asking me to do?

The worst part is that gcc (at least of version 9.4.0 which I have
available in my Ubuntu) does not even catch the line that is fixed by the
patch we are trying to discuss here. It catches 10 issues in
`add-patch.c`, but not the `i < file_diff->mode_change` one.

Neither does Visual C 2022, nor the gcc I have in Git for Windows' SDK,
which is v12.1.0.

So I would first need to find a tool that identifies all code locations
that compare signed with unsigned values.

And that's even before analyzing carefully how to address them (not all
instances will be as easy as upcasting from an unsigned bit to `int`, some
of those instances are about `size_t` vs `ssize_t`).

> > Pulling out this patch would break that patch series because it would
> > leave that breakage in place.
>
> We deal with topic that depends on another topic all the time, and I
> do not think there is anything that makes VS+CMake topic so special
> that it cannot be dependent on another topic.  It's just the matter
> of splitting this out and make it [1/1], and make the remainder to
> [1-4/4] and mark them rely on add-p fix when you send the latter
> out, isn't it?  Puzzled...

Sure, if you think that the signed/unsigned comparison project is more
important than fixing running Git's test suite inside Visual Studio, or
worse: if you are asking me to do a less than thorough job on those
signed/unsigned fixes by fixing only a single instance and leaving all
others unaddressed in a patch series that has nothing to do with Visual
Studio, then I understand how my stance could confuse you.

But my intention with this patch series is to fix running Git's test suite
in Visual Studio. And my intention is to provide a complete solution in my
patch series, no half measures.

As such, I would be loathe to have my authorship on a single patch that
solves neither the Visual Studio/CTest problem nor the vast majority of
the signed/unsigned problems. It would be incomplete in any way you look
at it. I would consider such a contribution lackluster, sloppy and
definitely not up to my standards.

Ciao,
Dscho

Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]

Hi Ævar,

On Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > [...]
> > However, this quirk is only in effect as long as `chmod` is run inside
> > the pseudo Unix root directory structure or within the home directory.
> > When run outside, such invocations fail like this:
> >
> > 	chmod: changing permissions of '<file>': Invalid argument
>
> ..ok, but...
>
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 6da7273f1d5..7c63b22acab 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -492,7 +492,10 @@ test_commit_bulk () {
> >  # of a file in the working directory and add it to the index.
> >
> >  test_chmod () {
> > -	chmod "$@" &&
> > +	if test_have_prereq !MINGW
> > +	then
> > +		chmod "$@"
> > +	fi &&
> >  	git update-index --add "--chmod=$@"
> >  }
> >
> > @@ -548,7 +551,10 @@ write_script () {
> >  		echo "#!${2-"$SHELL_PATH"}" &&
> >  		cat
> >  	} >"$1" &&
> > -	chmod +x "$1"
> > +	if test_have_prereq !MINGW
> > +	then
> > +		chmod +x "$1"
> > +	fi
>
> ... you get +x semantics by default, so we didn't need that "chmod +x"
> in the first place?

No. We do not get that `chmod +x` semantics by default. Those `chmod +x`
statements are treated as (expensive) no-ops by default. This is what I
meant when I said this in the commit message (that is missing from the
quoted text above):

	[...] it pretends that it succeeded, when in reality it did not
	do a thing [...]

I do not know how to say this more clearly.

> The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
> don't care about that, regardless of the "pseudo Unix root directory"?

The rest of "test_chmod" is even quoted above, so we do not need to leave
anybody guessing as to what it does:

	git update-index --add "--chmod=$@"

This asks Git to update the index with the executable bit explicitly
turned on or off, regardless of the information that is available on disk.

And yes, Git does what we expect it to do here.

> What if we get a "test_chmod -o <file>", won't this silently do the
> wrong thing?

But we don't?

The code under discussion is Git's test suite, after all, not something
that every Git user is expected to use in more ways than the core Git
developers can imagine.

So even a cursory `git grep test_chmod upstream/seen` could have shown
that there is no such user, and if this was supposed to be a "But what if
we do that at some stage in the future?" feedback, said feedback could be
construed as to intentionally use up valuable contributor time.

> If so isn't something in this direction (untested) a more targeted &
> obvious fix?:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 10258def7be..1c3b6692388 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1690,6 +1690,16 @@ case $uname_s in
>  	find () {
>  		/usr/bin/find "$@"
>  	}
> +	chmod () {
> +		case "$1" in
> +		+x|-x)
> +			return;
> +			;;
> +		*)
> +			;;
> +		esac &&
> +		/usr/bin/chmod "$@"
> +	}
>  	# git sees Windows-style pwd
>  	pwd () {
>  		builtin pwd -W
>
>

In that form, I will reject this suggestion as over-engineered and
convoluted. Why is the `*)` clause empty? Why is there an early return
guarding a single statement? Why is the `/usr/bin/chmod` call not in the
`*)` clause to begin with? Why does this code take pains to handle cases
other than `-x` and `+x` when we do not have any callers, and even in the
experimental patches, there are no such users in sight? I would like to
encourage you to address such issues during review before even sending the
mail in the future.

Having said that, there is a nugget in this feedback that I find valuable.
Instead of wasting the run time (even on non-Windows platforms!) to
determine whether the `MINGW` prereq is set in order to skip the `chmod`
call or not, we can make `chmod` a no-op explicitly in that `case
$uname_s` block.

I will make it so.

Ciao,
Johannes

Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]

Hi Ævar,

On Mon, 22 Aug 2022, Johannes Schindelin wrote:

> Instead of wasting the run time (even on non-Windows platforms!) to
> determine whether the `MINGW` prereq is set in order to skip the `chmod`
> call or not, we can make `chmod` a no-op explicitly in that `case
> $uname_s` block.
>
> I will make it so.

tl;dr this patch needs to be dropped, without the suggested replacement.

Gaaah!

After struggling with this for much longer than I care to admit, and even
debugging inside the MSYS2 runtime (which is a level boss if there ever
was one), I found out that all about this patch was wrong.

The suggested patch (making `chmod` a no-op) was wrong: the test suite
started failing left and right. Why? Because `chmod` is not a _complete_
no-op. The test suite does not use the `-o` flag you suggested (which
would not make sense in Git's test suite, whether or not you remove file
permissions for "others" than the current user or group, Git behaves
identically), but it _does_ use the `-w`/`+w` flags, and those _are_
respected, even on Windows.

The way the write permission bits are translated to ACLs is admittedly
somewhat magical or all kinds of wrong, too, depending on your point of
view.

But my original patch was also wrong. Why? Because it claimed that `chmod
+x` does not work outside of MSYS2's pseudo Unix root directory tree. And
that's simply not true.

Whether you call `chmod +x C:/Users/avar/my-file.txt` or `chmod +x
/tmp/avar.sh`, it "succeeds" (by silently ignoring the flag that is
inapplicable on Windows, whether or not a file is executable is determined
by its file extension, and yes, that means that shell scripts are never
executable and we have to live with a nasty hack in Git to pretend that
they are, based on their contents starting with a hash-bang line).

So why did I claim that the `chmod +x` invocation does not work outside of
that pseudo Unix root directory tree? Because it actually did not work, at
least for me! But it worked on the build agents. Why? Because I have a
command in my `~/.profile` that mounts several of my Git for Windows SDKs
as "short-cuts" like `/sdk64`, `/sdk32` and the likes (think of these as
bind-mounts). And those `mount` commands did not specify the `noacl` flag.

So what's that `noacl` flag? It is magic, I can tell you. It basically
makes all this pretense work where MSYS2 pretends that we are in a Unix
environment with Unix permissions when we're not actually. The details are
too gnarly and involved to explain, I won't write them up here in order to
avoid putting even more readers to sleep than I must have done already.

So my analysis was based on observation instead of inspection, cutting
corners, and I dearly regret that now. By default, MSYS2 "mounts" the
system drives as `/c`, `/d`, etc with that `noacl` flag, the same as it
mounts the pseudo Unix root as `/`. And that is why the Git test suite
does not throw up every time `chmod +x` is called. And it was a simple
pilot error on my part that caused it to fail on my system, and I did not
realize that the problem was confined to my system, and the bug was in my
`~/.profile` and not in Git's code base.

Working on Git for Windows is never boring.

Ciao,
Johannes

[PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows

[Johannes Schindelin via GitGitGadget]

Visual Studio users enjoy support for running the test suite via CTest,
thanks to Git's CMake definition.

In https://github.com/git-for-windows/git/issues/3966, it has been reported
that this does not work out of the box, though, but causes a couple of test
failures instead. These problems are not caught by Git's CI runs because the
vs-tests jobs actually use prove to run the test suite, not CTest.

In addition to fixing these problems, this patch series also addresses a
long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified
(but these modification should never be committed, they refer to a
local-only, configuration-dependent directory).

Note: The signed/unsigned comparison bug in git add -p that is fixed in this
here patch series is a relatively big one, and it merits further
investigation whether there are similar bugs lurking in Git's code base.
However, this is a much bigger project than can be addressed as part of this
patch series, in particular because the analysis would require tools other
than GCC's -Wsign-compare option (which totally misses the instance that is
fixed in this here patch series).

Changes since v1:

 * Clarified why it is a good idea to pass --no-bin-wrappers and
   --no-chain-lint when running on Windows.
 * Clarified why the add -p bug has not been caught earlier.
 * Clarified the scope of this patch series to fix running Git's tests
   within Visual Studio.
 * Increased the time-out for the very slow t7112 test script.
 * The test_chmod was determined to be not only faulty, but unneeded, and
   was dropped.

Johannes Schindelin (5):
  cmake: make it easier to diagnose regressions in CTest runs
  cmake: copy the merge tools for testing
  add -p: avoid ambiguous signed/unsigned comparison
  cmake: avoid editing t/test-lib.sh
  cmake: increase time-out for a long-running test

 .gitignore                          |  1 +
 Makefile                            |  1 +
 add-patch.c                         |  2 +-
 contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------
 t/test-lib.sh                       | 11 ++++++++++-
 5 files changed, 21 insertions(+), 10 deletions(-)


base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1320

Range-diff vs v1:

 1:  9cf14984c0a ! 1:  e00cb37b98a cmake: align CTest definition with Git's CI runs
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    cmake: align CTest definition with Git's CI runs
     +    cmake: make it easier to diagnose regressions in CTest runs
      
     -    In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and
     -    `--no-chain-lint`, mainly to win back some time caused by the serious
     -    performance penalty paid for the tests relying so heavily on POSIX shell
     -    scripting, which only works by using a POSIX emulation layer.
     +    When a test script fails in Git's test suite, the usual course of action
     +    is to re-run it using options to increase the verbosity of the output,
     +    e.g. `-v` and `-x`.
      
     -    Let's do the same when running the tests, say, in Visual Studio.
     +    Like in Git's CI runs, when running the tests in Visual Studio via the
     +    CTest route, it is cumbersome or at least requires a very unintuitive
     +    approach to pass options to the test scripts.
      
     -    While at it, enable the command trace via `-x` and verbose output via
     -    `-v`, otherwise it would be near impossible to diagnose any problems.
     +    So let's just pass those options by default: This will not clutter any
     +    output window but the log that is written to a log file will have
     +    information necessary to figure out test failures.
     +
     +    While at it, also imitate what the Windows jobs in Git's CI runs do to
     +    accelerate running the test scripts: pass the `--no-bin-wrappers` and
     +    `--no-chain-lint` options. This makes the test runs noticeably faster
     +    because the `bin-wrappers/` scripts as well as the `chain-lint` code
     +    make heavy use of POSIX shell scripting, which is really, really slow on
     +    Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
 2:  86ab58b6508 = 2:  de7b47a9aa7 cmake: copy the merge tools for testing
 3:  79abfa82c32 < -:  ----------- tests: explicitly skip `chmod` calls on Windows
 4:  4d24a4345ba ! 3:  f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison
     @@ Commit message
          Let's avoid that by converting the unsigned bit explicitly to a signed
          integer.
      
     +    Note: This is a long-standing bug in the Visual C build of Git, but it
     +    has never been caught because t3701 is skipped when `NO_PERL` is set,
     +    which is the case in the `vs-test` jobs of Git's CI runs.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## add-patch.c ##
 5:  c7fc5a4ee4c = 4:  22473d6b8f3 cmake: avoid editing t/test-lib.sh
 -:  ----------- > 5:  6aaa675301c cmake: increase time-out for a long-running test

-- 
gitgitgadget

[PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When a test script fails in Git's test suite, the usual course of action
is to re-run it using options to increase the verbosity of the output,
e.g. `-v` and `-x`.

Like in Git's CI runs, when running the tests in Visual Studio via the
CTest route, it is cumbersome or at least requires a very unintuitive
approach to pass options to the test scripts.

So let's just pass those options by default: This will not clutter any
output window but the log that is written to a log file will have
information necessary to figure out test failures.

While at it, also imitate what the Windows jobs in Git's CI runs do to
accelerate running the test scripts: pass the `--no-bin-wrappers` and
`--no-chain-lint` options. This makes the test runs noticeably faster
because the `bin-wrappers/` scripts as well as the `chain-lint` code
make heavy use of POSIX shell scripting, which is really, really slow on
Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 1b23f2440d8..4aee1e24342 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 #test
 foreach(tsh ${test_scipts})
 	add_test(NAME ${tsh}
-		COMMAND ${SH_EXE} ${tsh}
+		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
-- 
gitgitgadget

[PATCH v2 2/5] cmake: copy the merge tools for testing

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Even when running the tests via CTest, t7609 and t7610 rely on more than
only a few mergetools to be copied to the build directory. Let's make it
so.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 4aee1e24342..fe606c179f7 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
 	#misc copies
 	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
 	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
-	file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
+	file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
+	file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 endif()
-- 
gitgitgadget

[PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the interactive `add` operation, users can choose to jump to specific
hunks, and Git will present the hunk list in that case. To avoid showing
too many lines at once, only a maximum of 21 hunks are shown, skipping
the "mode change" pseudo hunk.

The comparison performed to skip the "mode change" pseudo hunk (if any)
compares a signed integer `i` to the unsigned value `mode_change` (which
can be 0 or 1 because it is a 1-bit type).

According to section 6.3.1.8 of the C99 standard (see e.g.
https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
happen is an automatic conversion of the "lesser" type to the "greater"
type, but since the types differ in signedness, it is ill-defined what
is the correct "usual arithmetic conversion".

Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter, `add -p`'s `goto` command shows no
hunks by default because it casts a negative start offset to a pretty
large unsigned value, breaking the "goto hunk" test case in
`t3701-add-interactive.sh`.

Let's avoid that by converting the unsigned bit explicitly to a signed
integer.

Note: This is a long-standing bug in the Visual C build of Git, but it
has never been caught because t3701 is skipped when `NO_PERL` is set,
which is the case in the `vs-test` jobs of Git's CI runs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..3524555e2b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1547,7 +1547,7 @@ soft_increment:
 			strbuf_remove(&s->answer, 0, 1);
 			strbuf_trim(&s->answer);
 			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
-			if (i < file_diff->mode_change)
+			if (i < (int)file_diff->mode_change)
 				i = file_diff->mode_change;
 			while (s->answer.len == 0) {
 				i = display_hunks(s, file_diff, i);
-- 
gitgitgadget

[PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 7f5397a07c6c (cmake: support for testing git when building out of the
source tree, 2020-06-26), we implemented support for running Git's test
scripts even after building Git in a different directory than the source
directory.

The way we did this was to edit the file `t/test-lib.sh` to override
`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
directory.

This is unideal because it always leaves a tracked file marked as
modified, and it is all too easy to commit that change by mistake.

Let's change the strategy by teaching `t/test-lib.sh` to detect the
presence of a file called `GIT-BUILD-DIR` in the source directory. If it
exists, the contents are interpreted as the location to the _actual_
build directory. We then write this file as part of the CTest
definition.

To support building Git via a regular `make` invocation after building
it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
convenience, this is done as part of the Makefile rule that is already
run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
up to date).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore                          |  1 +
 Makefile                            |  1 +
 contrib/buildsystems/CMakeLists.txt |  7 +------
 t/test-lib.sh                       | 11 ++++++++++-
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/.gitignore b/.gitignore
index a4522157641..b72ddf09346 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /fuzz_corpora
 /fuzz-pack-headers
 /fuzz-pack-idx
+/GIT-BUILD-DIR
 /GIT-BUILD-OPTIONS
 /GIT-CFLAGS
 /GIT-LDFLAGS
diff --git a/Makefile b/Makefile
index 04d0fd1fe60..9347ed90da7 100644
--- a/Makefile
+++ b/Makefile
@@ -3028,6 +3028,7 @@ else
 	@echo RUNTIME_PREFIX=\'false\' >>$@+
 endif
 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
 
 ### Detect Python interpreter path changes
 ifndef NO_PYTHON
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index fe606c179f7..29d7e236ae1 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1067,14 +1067,9 @@ endif()
 #Make the tests work when building out of the source tree
 get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
 if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
-	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
-	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
 	#Setting the build directory in test-lib.sh before running tests
 	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
-		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
-		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
-		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
-		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
+		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
 	#misc copies
 	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
 	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55857af601b..4468ac51f25 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -42,7 +42,16 @@ then
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
 GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
-if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
+	# On Windows, we must convert Windows paths lest they contain a colon
+	case "$(uname -s)" in
+	*MINGW*)
+		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+		;;
+	esac
+elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
 then
 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
 	exit 1
-- 
gitgitgadget

[PATCH v2 5/5] cmake: increase time-out for a long-running test

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As suggested in
https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
t7112 can run for well over one hour, which seems to be the default
maximum run time at least when running CTest-based tests in Visual
Studio.

Let's increase the time-out as a stop gap to unblock developers wishing
to run Git's test suite in Visual Studio.

Note: The actual run time is highly dependent on the circumstances. For
example, in Git's CI runs, the Windows-based tests typically take a bit
over 5 minutes to run. CI runs have the added benefit that Windows
Defender (the common anti-malware scanner on Windows) is turned off,
something many developers are not at liberty to do on their work
stations. When Defender is turned on, even on this developer's high-end
Ryzen system, t7112 takes over 15 minutes to run.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 29d7e236ae1..b1306f95256 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
+# This test script takes an extremely long time and is known to time out even
+# on fast machines because it requires in excess of one hour to run
+set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+
 endif()#BUILD_TESTING
-- 
gitgitgadget

Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Victoria Dye]

Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
> 
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts.

At first I wondered whether there's a way to make arg specification more
intuitive, rather than silently changing defaults. Unfortunately, it looks
like even in the latest versions of CTest don't really support passing
arguments through to tests [1] (and the workarounds are unpleasant at best).

But then, you mentioned that there *is* a cumbersome/unintuitive approach to
passing the options; what approach were you thinking?

[1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470

> 
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.

Makes sense, I don't see any harm in providing more verbose output by
default here.

> 
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options. This makes the test runs noticeably faster
> because the `bin-wrappers/` scripts as well as the `chain-lint` code
> make heavy use of POSIX shell scripting, which is really, really slow on
> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.

I'm a bit more hesitant on including these. I see how the performance
benefit (on Windows in particular) would make typical user experience nicer.
But, if someone develops locally with '--no-chain-lint' specified, they'll
be much more likely to miss a broken && chain (personally, I get caught by
chain lint errors *all the time* when I'm adding/updating tests) and cause
unnecessary churn in their patch submissions. So, my recommendation would be
to drop '--no-chain-lint' here (unless it's less important to the average
developer than I think it is).

It's also possible that '--no-bin-wrappers' does something weird with their
local installation of Git but I think it's safe enough to make the default
if the performance gain is substantial.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>  #test
>  foreach(tsh ${test_scipts})
>  	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx

As you have it here, I don't think there's a way for a user to override
these defaults (unless there's something about the manual workaround you
mentioned earlier that allows it). Since a user could feasibly want to set
their own options, could you add a build variable to CMakeLists.txt like
'GIT_TEST_OPTS'? You could use it to set the default options you have here,
but a user could still specify their own value at build time to override.

>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()
>  

Re: [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows

[Victoria Dye]

Johannes Schindelin via GitGitGadget wrote:
> Visual Studio users enjoy support for running the test suite via CTest,
> thanks to Git's CMake definition.
> 
> In https://github.com/git-for-windows/git/issues/3966, it has been reported
> that this does not work out of the box, though, but causes a couple of test
> failures instead. These problems are not caught by Git's CI runs because the
> vs-tests jobs actually use prove to run the test suite, not CTest.
> 
> In addition to fixing these problems, this patch series also addresses a
> long-standing gripe I have with the way Git's CMake definition supports
> CTest: It edits t/test-lib.sh, which leaves this file eternally modified
> (but these modification should never be committed, they refer to a
> local-only, configuration-dependent directory).
> 
> Note: The signed/unsigned comparison bug in git add -p that is fixed in this
> here patch series is a relatively big one, and it merits further
> investigation whether there are similar bugs lurking in Git's code base.
> However, this is a much bigger project than can be addressed as part of this
> patch series, in particular because the analysis would require tools other
> than GCC's -Wsign-compare option (which totally misses the instance that is
> fixed in this here patch series).
> 
> Changes since v1:
> 
>  * Clarified why it is a good idea to pass --no-bin-wrappers and
>    --no-chain-lint when running on Windows.
>  * Clarified why the add -p bug has not been caught earlier.
>  * Clarified the scope of this patch series to fix running Git's tests
>    within Visual Studio.
>  * Increased the time-out for the very slow t7112 test script.
>  * The test_chmod was determined to be not only faulty, but unneeded, and
>    was dropped.
> 
> Johannes Schindelin (5):
>   cmake: make it easier to diagnose regressions in CTest runs
>   cmake: copy the merge tools for testing
>   add -p: avoid ambiguous signed/unsigned comparison
>   cmake: avoid editing t/test-lib.sh
>   cmake: increase time-out for a long-running test

I've reviewed patches 1-3 & 5 and started looking over 4, but I'd like to
spend a bit more time on it (mostly to understand pros/cons of the
'GIT-BUILD-DIR' vs. any alternatives). I'm planning to finish that up
tomorrow.

Thanks for your patience!

> 
>  .gitignore                          |  1 +
>  Makefile                            |  1 +
>  add-patch.c                         |  2 +-
>  contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------
>  t/test-lib.sh                       | 11 ++++++++++-
>  5 files changed, 21 insertions(+), 10 deletions(-)
> 
> 
> base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1320
> 
> Range-diff vs v1:
> 
>  1:  9cf14984c0a ! 1:  e00cb37b98a cmake: align CTest definition with Git's CI runs
>      @@ Metadata
>       Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>       
>        ## Commit message ##
>      -    cmake: align CTest definition with Git's CI runs
>      +    cmake: make it easier to diagnose regressions in CTest runs
>       
>      -    In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and
>      -    `--no-chain-lint`, mainly to win back some time caused by the serious
>      -    performance penalty paid for the tests relying so heavily on POSIX shell
>      -    scripting, which only works by using a POSIX emulation layer.
>      +    When a test script fails in Git's test suite, the usual course of action
>      +    is to re-run it using options to increase the verbosity of the output,
>      +    e.g. `-v` and `-x`.
>       
>      -    Let's do the same when running the tests, say, in Visual Studio.
>      +    Like in Git's CI runs, when running the tests in Visual Studio via the
>      +    CTest route, it is cumbersome or at least requires a very unintuitive
>      +    approach to pass options to the test scripts.
>       
>      -    While at it, enable the command trace via `-x` and verbose output via
>      -    `-v`, otherwise it would be near impossible to diagnose any problems.
>      +    So let's just pass those options by default: This will not clutter any
>      +    output window but the log that is written to a log file will have
>      +    information necessary to figure out test failures.
>      +
>      +    While at it, also imitate what the Windows jobs in Git's CI runs do to
>      +    accelerate running the test scripts: pass the `--no-bin-wrappers` and
>      +    `--no-chain-lint` options. This makes the test runs noticeably faster
>      +    because the `bin-wrappers/` scripts as well as the `chain-lint` code
>      +    make heavy use of POSIX shell scripting, which is really, really slow on
>      +    Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>       
>           Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>       
>  2:  86ab58b6508 = 2:  de7b47a9aa7 cmake: copy the merge tools for testing
>  3:  79abfa82c32 < -:  ----------- tests: explicitly skip `chmod` calls on Windows
>  4:  4d24a4345ba ! 3:  f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison
>      @@ Commit message
>           Let's avoid that by converting the unsigned bit explicitly to a signed
>           integer.
>       
>      +    Note: This is a long-standing bug in the Visual C build of Git, but it
>      +    has never been caught because t3701 is skipped when `NO_PERL` is set,
>      +    which is the case in the `vs-test` jobs of Git's CI runs.
>      +
>           Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>       
>        ## add-patch.c ##
>  5:  c7fc5a4ee4c = 4:  22473d6b8f3 cmake: avoid editing t/test-lib.sh
>  -:  ----------- > 5:  6aaa675301c cmake: increase time-out for a long-running test
> 

Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
>
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts.
>
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.
>
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options. This makes the test runs noticeably faster
> because the `bin-wrappers/` scripts as well as the `chain-lint` code
> make heavy use of POSIX shell scripting, which is really, really slow on
> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>  #test
>  foreach(tsh ${test_scipts})
>  	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()

The -vx part of this looks sensible, and matches the commit message
$subject.

I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
here should be stripped out, and put into its own commit.

Which, as I commented on in the v1[1] is making the implicit assumption
that this cmake file is only used on Windows, but since 561962479c
(cmake: fix CMakeLists.txt on Linux, 2022-05-24) that isn't the case.

So, perhaps we should have a performance hack due to Windows's slowness,
but:

 A. It should be guarded by some "is windows?" check. Your commit
    message justifies why this is a good idea on Windows, but completely
    elides over the fact that this isn't Windows-specific code anymore.

 B. We can still build wind "make" or "cmake", I don't see a reason for
    why such a perf hack (if we decide to keep it) should depend on the
    build system.

 C. Since I sent [1] we've had submitted chainlint.pl in-flight
    series. It's partly trying to take special considerations to be fast
    on Windows. I don't think it's the case with that series that this
    needs to be skipped on Windows anymore (and if it is, Eric would
    like to know).

1.  https://lore.kernel.org/git/220811.861qtnqb5p.gmgdl@evledraar.gmail.com/

Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> As suggested in
> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
> t7112 can run for well over one hour, which seems to be the default
> maximum run time at least when running CTest-based tests in Visual
> Studio.
>
> Let's increase the time-out as a stop gap to unblock developers wishing
> to run Git's test suite in Visual Studio.
>
> Note: The actual run time is highly dependent on the circumstances. For
> example, in Git's CI runs, the Windows-based tests typically take a bit
> over 5 minutes to run. CI runs have the added benefit that Windows
> Defender (the common anti-malware scanner on Windows) is turned off,
> something many developers are not at liberty to do on their work
> stations. When Defender is turned on, even on this developer's high-end
> Ryzen system, t7112 takes over 15 minutes to run.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 29d7e236ae1..b1306f95256 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()
>  
> +# This test script takes an extremely long time and is known to time out even
> +# on fast machines because it requires in excess of one hour to run
> +set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
> +
>  endif()#BUILD_TESTING

I don't see per [1] that it would have any negative effect to just bump
the timeout a lot more, and for all the tests.

If we're running into 3600 seconds, then setting this to 4000 seconds
seems to be a smal stopgap at best. That's just over a 10% increase, so
if one person ran into it it 3600 someone with a slightly slower system
should be running into the same still, or if we just add a few more
tests to 7112 (or some other slow test).

So why not set this ta 3600*10 or whatever for *all* of the test scripts
instead of playing whack-a-mole?

1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
>
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
>
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
>
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
>
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .gitignore                          |  1 +
>  Makefile                            |  1 +
>  contrib/buildsystems/CMakeLists.txt |  7 +------
>  t/test-lib.sh                       | 11 ++++++++++-
>  4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
>  /fuzz_corpora
>  /fuzz-pack-headers
>  /fuzz-pack-idx
> +/GIT-BUILD-DIR
>  /GIT-BUILD-OPTIONS
>  /GIT-CFLAGS
>  /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
>  	@echo RUNTIME_PREFIX=\'false\' >>$@+
>  endif
>  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>
>  ### Detect Python interpreter path changes
>  ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
>  #Make the tests work when building out of the source tree
>  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>  	#Setting the build directory in test-lib.sh before running tests
>  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>  	#misc copies
>  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 55857af601b..4468ac51f25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,7 +42,16 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> +	# On Windows, we must convert Windows paths lest they contain a colon
> +	case "$(uname -s)" in
> +	*MINGW*)
> +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> +		;;
> +	esac
> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>  then
>  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>  	exit 1

As pointed out in the v1 this breaks the cmake<->make interaction in
some scenarios, but from some brief testing there seemed to be an easy
workaround which didn't suffer from that problem:
https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/

Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test

[Victoria Dye]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> As suggested in
>> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
>> t7112 can run for well over one hour, which seems to be the default
>> maximum run time at least when running CTest-based tests in Visual
>> Studio.
>>
>> Let's increase the time-out as a stop gap to unblock developers wishing
>> to run Git's test suite in Visual Studio.
>>
>> Note: The actual run time is highly dependent on the circumstances. For
>> example, in Git's CI runs, the Windows-based tests typically take a bit
>> over 5 minutes to run. CI runs have the added benefit that Windows
>> Defender (the common anti-malware scanner on Windows) is turned off,
>> something many developers are not at liberty to do on their work
>> stations. When Defender is turned on, even on this developer's high-end
>> Ryzen system, t7112 takes over 15 minutes to run.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  contrib/buildsystems/CMakeLists.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
>> index 29d7e236ae1..b1306f95256 100644
>> --- a/contrib/buildsystems/CMakeLists.txt
>> +++ b/contrib/buildsystems/CMakeLists.txt
>> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
>>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>>  endforeach()
>>  
>> +# This test script takes an extremely long time and is known to time out even
>> +# on fast machines because it requires in excess of one hour to run
>> +set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
>> +
>>  endif()#BUILD_TESTING
> 
> I don't see per [1] that it would have any negative effect to just bump
> the timeout a lot more, and for all the tests.

I'm a bit confused as to how that link (which explains that the 'TIMEOUT'
property is used to determine when to kill a test process - more or less
standard behavior for timeouts in applications) shows that excessively long
timeouts on every test "[wouldn't] have any negative effect." 

In terms of the timeout length, the stated reason for adding the timeout in
the first place is:

>> Let's increase the time-out as a stop gap to unblock developers wishing
>> to run Git's test suite in Visual Studio.

I would consider ~1 hour a manageable (albeit frustrating) amount of time to
wait for a test, but not 10 hours (as you suggest later).

As for setting the timeout on all tests: that might be helpful, only because
(AFAICT from CMake's source code & documentation) the default timeout is "no
timeout." If setting a global default, though, I'd prefer a shorter timespan
(e.g. 1800s) so that a timeout properly indicates a failure condition ("this
test runs much longer than it should, so we may have introduced a
performance bug"). 

Practically, though, I don't feel too strongly about whether or not to set a
global default. If we wanted to add timeouts to more tests in the future,
I'd want a more extensible solution with carefully-selected timeouts. But if
it's just this one test running too long, special case-ing a timeout for it
with a comment explaining the reasoning seems appropriate to me. 

> 
> If we're running into 3600 seconds, then setting this to 4000 seconds
> seems to be a smal stopgap at best. That's just over a 10% increase, so
> if one person ran into it it 3600 someone with a slightly slower system
> should be running into the same still, or if we just add a few more
> tests to 7112 (or some other slow test).

I don't think this patch is suggesting that timeout failures are happening
at 3600s, rather that the peak observed execution time of the test is 3600s.
If that's the case, <peak execution time> + an 11% buffer is a reasonable
choice for the value.

> 
> So why not set this ta 3600*10 or whatever for *all* of the test scripts
> instead of playing whack-a-mole?
> 
> 1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Victoria Dye]

Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
> 
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
> 
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
> 
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
> 
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).

While I like that this removes a user error case, it sacrifices some of the
separation between contrib/ and the main Git tree by adding logic to
'test-lib.sh' that only really benefits the CMake build.

To your point in [1]:

> Can we maybe agree that the proposed patch is a net improvement over the
> status quo, and think about a better solution independently (without
> blocking this here patch)?

I don't think it does more harm than good, but I wouldn't go so far as to
call it a definitive "net improvement." I'd personally very much prefer a
solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
CMake. Unfortunately, after spending a pretty substantial amount of time
looking for an alternative, I couldn't think of anything that didn't either
1) change how users ran tests or 2) change where CMake builds wrote Git's
binary files. 

So, I could go either way on this patch - if others feel strongly that it
should be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless
someone can think of a better alternative.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .gitignore                          |  1 +
>  Makefile                            |  1 +
>  contrib/buildsystems/CMakeLists.txt |  7 +------
>  t/test-lib.sh                       | 11 ++++++++++-
>  4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
>  /fuzz_corpora
>  /fuzz-pack-headers
>  /fuzz-pack-idx
> +/GIT-BUILD-DIR
>  /GIT-BUILD-OPTIONS
>  /GIT-CFLAGS
>  /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
>  	@echo RUNTIME_PREFIX=\'false\' >>$@+
>  endif
>  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>  
>  ### Detect Python interpreter path changes
>  ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
>  #Make the tests work when building out of the source tree
>  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>  	#Setting the build directory in test-lib.sh before running tests
>  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>  	#misc copies
>  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 55857af601b..4468ac51f25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,7 +42,16 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> +	# On Windows, we must convert Windows paths lest they contain a colon
> +	case "$(uname -s)" in
> +	*MINGW*)
> +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> +		;;
> +	esac
> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>  then
>  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>  	exit 1
Referring to Ævar's review in [1] - while I'm not overly concerned about the
"switching between make & CMake" file staleness (if I'm not mistaken, the
same thing can happen now with the modified 'test-lib.sh', so this patch
doesn't really make anything worse), I do think the changes to 'test-lib.sh'
should be rearranged to preserve the "PANIC" check:

----------------->8----------------->8----------------->8-----------------
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4468ac51f2..7b57f55c37 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -42,6 +42,11 @@ then
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
 GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
+if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+then
+	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
+	exit 1
+fi
 if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
 then
 	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
@@ -51,10 +56,6 @@ then
 		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
 		;;
 	esac
-elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
-then
-	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
-	exit 1
 fi
 
 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
-----------------8<-----------------8<-----------------8<-----------------

Otherwise, a user could run the tests from outside a 't/' directory if they
built Git with CMake, which doesn't appear to be part of the intended
behavior of this patch.

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Victoria Dye]

Victoria Dye wrote:
> Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> In 7f5397a07c6c (cmake: support for testing git when building out of the
>> source tree, 2020-06-26), we implemented support for running Git's test
>> scripts even after building Git in a different directory than the source
>> directory.
>>
>> The way we did this was to edit the file `t/test-lib.sh` to override
>> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
>> directory.
>>
>> This is unideal because it always leaves a tracked file marked as
>> modified, and it is all too easy to commit that change by mistake.
>>
>> Let's change the strategy by teaching `t/test-lib.sh` to detect the
>> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
>> exists, the contents are interpreted as the location to the _actual_
>> build directory. We then write this file as part of the CTest
>> definition.
>>
>> To support building Git via a regular `make` invocation after building
>> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
>> convenience, this is done as part of the Makefile rule that is already
>> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
>> up to date).
> 
> While I like that this removes a user error case, it sacrifices some of the
> separation between contrib/ and the main Git tree by adding logic to
> 'test-lib.sh' that only really benefits the CMake build.
> 
> To your point in [1]:

Forgot this link: https://lore.kernel.org/git/8o4q98s3-sr2r-34qq-p7pr-8o44061o0n76@tzk.qr/

> 
>> Can we maybe agree that the proposed patch is a net improvement over the
>> status quo, and think about a better solution independently (without
>> blocking this here patch)?
> 
> I don't think it does more harm than good, but I wouldn't go so far as to
> call it a definitive "net improvement." I'd personally very much prefer a
> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
> CMake. Unfortunately, after spending a pretty substantial amount of time
> looking for an alternative, I couldn't think of anything that didn't either
> 1) change how users ran tests or 2) change where CMake builds wrote Git's
> binary files. 
> 
> So, I could go either way on this patch - if others feel strongly that it
> should be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless
> someone can think of a better alternative.
> 
...

>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 55857af601b..4468ac51f25 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -42,7 +42,16 @@ then
>>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>>  fi
>>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>> +then
>> +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
>> +	# On Windows, we must convert Windows paths lest they contain a colon
>> +	case "$(uname -s)" in
>> +	*MINGW*)
>> +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>> +		;;
>> +	esac
>> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>>  then
>>  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>>  	exit 1
> Referring to Ævar's review in [1] - while I'm not overly concerned about the

AND this one (which should be [2]): https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/

Sorry about that!

> "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
> 
> ----------------->8----------------->8----------------->8-----------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4468ac51f2..7b57f55c37 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,6 +42,11 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> +if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +then
> +	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> +	exit 1
> +fi
>  if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>  then
>  	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> @@ -51,10 +56,6 @@ then
>  		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>  		;;
>  	esac
> -elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> -then
> -	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> -	exit 1
>  fi
>  
>  # Prepend a string to a VAR using an arbitrary ":" delimiter, not
> -----------------8<-----------------8<-----------------8<-----------------
> 
> Otherwise, a user could run the tests from outside a 't/' directory if they
> built Git with CMake, which doesn't appear to be part of the intended
> behavior of this patch.

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> I don't think it does more harm than good, but I wouldn't go so far as to
> call it a definitive "net improvement." I'd personally very much prefer a
> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
> CMake.
> ...
> ... "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:

Thanks for a quality review.

Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Eric Sunshine]

On Thu, Sep 8, 2022 at 3:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> > When a test script fails in Git's test suite, the usual course of action
> > is to re-run it using options to increase the verbosity of the output,
> > e.g. `-v` and `-x`.
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > +             COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>
> I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
> here should be stripped out, and put into its own commit.
>
> So, perhaps we should have a performance hack due to Windows's slowness,
> but:
>
>  C. Since I sent [1] we've had submitted chainlint.pl in-flight
>     series. It's partly trying to take special considerations to be fast
>     on Windows. I don't think it's the case with that series that this
>     needs to be skipped on Windows anymore (and if it is, Eric would
>     like to know).

I had the opportunity to test chainlint.pl on an 8-core machine using
both Linux and Windows 10. The machine is more than a few years old,
though not nearly as old as my own development machine. Although
chainlint.pl checked all test definitions in the entire project in
less than one _second_ on Linux, it took two _minutes_ on Windows 10
on the same machine (using all 8 cores). Despite the fact that Perl is
run just once to check all test definitions -- unlike chainlint.sed
running 'sed' tens of thousands of times, chainlint.pl is still
painfully slow on Windows, so I certainly understand the desire to use
--no-chain-lint, at least on Windows.

[PATCH v3 0/5] Some fixes and an improvement for using CTest on Windows

[Johannes Schindelin via GitGitGadget]

Visual Studio users enjoy support for running the test suite via CTest,
thanks to Git's CMake definition.

In https://github.com/git-for-windows/git/issues/3966, it has been reported
that this does not work out of the box, though, but causes a couple of test
failures instead. These problems are not caught by Git's CI runs because the
vs-tests jobs actually use prove to run the test suite, not CTest.

In addition to fixing these problems, this patch series also addresses a
long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified
(but these modification should never be committed, they refer to a
local-only, configuration-dependent directory).

Note: The signed/unsigned comparison bug in git add -p that is fixed in this
here patch series is a relatively big one, and it merits further
investigation whether there are similar bugs lurking in Git's code base.
However, this is a much bigger project than can be addressed as part of this
patch series, in particular because the analysis would require tools other
than GCC's -Wsign-compare option (which totally misses the instance that is
fixed in this here patch series).

Changes since v2:

 * Enhanced a commit message to clarify how the "cumbersome approach" looks
   like when a developer wishes to pass options to the test scripts in
   Visual Studio.
 * Changed the base branch to ac/fuzzers, to avoid merge conflicts in
   .gitignore.
 * Moved the PANIC check in test-lib.sh so that it is not skipped when the
   file GIT-BUILD-DIR exists.

Changes since v1:

 * Clarified why it is a good idea to pass --no-bin-wrappers and
   --no-chain-lint when running on Windows.
 * Clarified why the add -p bug has not been caught earlier.
 * Clarified the scope of this patch series to fix running Git's tests
   within Visual Studio.
 * Increased the time-out for the very slow t7112 test script.
 * The test_chmod was determined to be not only faulty, but unneeded, and
   was dropped.

Johannes Schindelin (5):
  cmake: make it easier to diagnose regressions in CTest runs
  cmake: copy the merge tools for testing
  add -p: avoid ambiguous signed/unsigned comparison
  cmake: avoid editing t/test-lib.sh
  cmake: increase time-out for a long-running test

 .gitignore                          |  1 +
 Makefile                            |  1 +
 add-patch.c                         |  2 +-
 contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------
 t/test-lib.sh                       | 10 ++++++++++
 5 files changed, 21 insertions(+), 9 deletions(-)


base-commit: 6713bfc70c4dc6da1fa4084f000b72f5d74fecfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1320

Range-diff vs v2:

 1:  e00cb37b98a ! 1:  356b2e9a100 cmake: make it easier to diagnose regressions in CTest runs
     @@ Commit message
      
          Like in Git's CI runs, when running the tests in Visual Studio via the
          CTest route, it is cumbersome or at least requires a very unintuitive
     -    approach to pass options to the test scripts.
     +    approach to pass options to the test scripts: the CMakeLists.txt file
     +    would have to be modified, passing the desired options to _all_ test
     +    scripts, and then the CMake Cache would have to be reconfigured before
     +    running the test in question individually. Unintuitive at best, and
     +    opposite to the niceties IDE users expect.
      
          So let's just pass those options by default: This will not clutter any
          output window but the log that is written to a log file will have
     @@ Commit message
      
          While at it, also imitate what the Windows jobs in Git's CI runs do to
          accelerate running the test scripts: pass the `--no-bin-wrappers` and
     -    `--no-chain-lint` options. This makes the test runs noticeably faster
     -    because the `bin-wrappers/` scripts as well as the `chain-lint` code
     -    make heavy use of POSIX shell scripting, which is really, really slow on
     -    Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
     +    `--no-chain-lint` options.
     +
     +    This makes the test runs noticeably faster because the `bin-wrappers/`
     +    scripts as well as the `chain-lint` code make heavy use of POSIX shell
     +    scripting, which is really, really slow on Windows due to the need to
     +    emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
     +    Sunshine, it added two minutes (!) just to perform the chain-lint task.
     +
     +    The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was
     +    considered during the development of this patch, but then dropped: such
     +    a setting is global, across _all_ tests, where e.g. `--run=...` would
     +    not make sense. Users wishing to override these new defaults are better
     +    advised running the test script manually, in a Git Bash, with full
     +    control over the command line.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
 2:  de7b47a9aa7 = 2:  9faca9d5cbe cmake: copy the merge tools for testing
 3:  f96d5ab484c = 3:  41a8021a4bd add -p: avoid ambiguous signed/unsigned comparison
 4:  22473d6b8f3 ! 4:  5b0c2a150e9 cmake: avoid editing t/test-lib.sh
     @@ Commit message
       ## .gitignore ##
      @@
       /fuzz_corpora
     - /fuzz-pack-headers
     - /fuzz-pack-idx
      +/GIT-BUILD-DIR
       /GIT-BUILD-OPTIONS
       /GIT-CFLAGS
     @@ contrib/buildsystems/CMakeLists.txt: endif()
      
       ## t/test-lib.sh ##
      @@ t/test-lib.sh: then
     - 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
     + 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
     + 	exit 1
       fi
     - GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
     --if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
      +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
      +then
      +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
     @@ t/test-lib.sh: then
      +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
      +		;;
      +	esac
     -+elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
     - then
     - 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
     - 	exit 1
     ++fi
     + 
     + # Prepend a string to a VAR using an arbitrary ":" delimiter, not
     + # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
 5:  6aaa675301c = 5:  40cf872f483 cmake: increase time-out for a long-running test

-- 
gitgitgadget

[PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When a test script fails in Git's test suite, the usual course of action
is to re-run it using options to increase the verbosity of the output,
e.g. `-v` and `-x`.

Like in Git's CI runs, when running the tests in Visual Studio via the
CTest route, it is cumbersome or at least requires a very unintuitive
approach to pass options to the test scripts: the CMakeLists.txt file
would have to be modified, passing the desired options to _all_ test
scripts, and then the CMake Cache would have to be reconfigured before
running the test in question individually. Unintuitive at best, and
opposite to the niceties IDE users expect.

So let's just pass those options by default: This will not clutter any
output window but the log that is written to a log file will have
information necessary to figure out test failures.

While at it, also imitate what the Windows jobs in Git's CI runs do to
accelerate running the test scripts: pass the `--no-bin-wrappers` and
`--no-chain-lint` options.

This makes the test runs noticeably faster because the `bin-wrappers/`
scripts as well as the `chain-lint` code make heavy use of POSIX shell
scripting, which is really, really slow on Windows due to the need to
emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
Sunshine, it added two minutes (!) just to perform the chain-lint task.

The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was
considered during the development of this patch, but then dropped: such
a setting is global, across _all_ tests, where e.g. `--run=...` would
not make sense. Users wishing to override these new defaults are better
advised running the test script manually, in a Git Bash, with full
control over the command line.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2237109b57f..6ac20bc5054 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 #test
 foreach(tsh ${test_scipts})
 	add_test(NAME ${tsh}
-		COMMAND ${SH_EXE} ${tsh}
+		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
-- 
gitgitgadget

[PATCH v3 2/5] cmake: copy the merge tools for testing

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Even when running the tests via CTest, t7609 and t7610 rely on more than
only a few mergetools to be copied to the build directory. Let's make it
so.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 6ac20bc5054..0c741e7d878 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
 	#misc copies
 	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
 	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
-	file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
+	file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
+	file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 endif()
-- 
gitgitgadget

[PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the interactive `add` operation, users can choose to jump to specific
hunks, and Git will present the hunk list in that case. To avoid showing
too many lines at once, only a maximum of 21 hunks are shown, skipping
the "mode change" pseudo hunk.

The comparison performed to skip the "mode change" pseudo hunk (if any)
compares a signed integer `i` to the unsigned value `mode_change` (which
can be 0 or 1 because it is a 1-bit type).

According to section 6.3.1.8 of the C99 standard (see e.g.
https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
happen is an automatic conversion of the "lesser" type to the "greater"
type, but since the types differ in signedness, it is ill-defined what
is the correct "usual arithmetic conversion".

Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter, `add -p`'s `goto` command shows no
hunks by default because it casts a negative start offset to a pretty
large unsigned value, breaking the "goto hunk" test case in
`t3701-add-interactive.sh`.

Let's avoid that by converting the unsigned bit explicitly to a signed
integer.

Note: This is a long-standing bug in the Visual C build of Git, but it
has never been caught because t3701 is skipped when `NO_PERL` is set,
which is the case in the `vs-test` jobs of Git's CI runs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..3524555e2b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1547,7 +1547,7 @@ soft_increment:
 			strbuf_remove(&s->answer, 0, 1);
 			strbuf_trim(&s->answer);
 			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
-			if (i < file_diff->mode_change)
+			if (i < (int)file_diff->mode_change)
 				i = file_diff->mode_change;
 			while (s->answer.len == 0) {
 				i = display_hunks(s, file_diff, i);
-- 
gitgitgadget

[PATCH v3 4/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 7f5397a07c6c (cmake: support for testing git when building out of the
source tree, 2020-06-26), we implemented support for running Git's test
scripts even after building Git in a different directory than the source
directory.

The way we did this was to edit the file `t/test-lib.sh` to override
`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
directory.

This is unideal because it always leaves a tracked file marked as
modified, and it is all too easy to commit that change by mistake.

Let's change the strategy by teaching `t/test-lib.sh` to detect the
presence of a file called `GIT-BUILD-DIR` in the source directory. If it
exists, the contents are interpreted as the location to the _actual_
build directory. We then write this file as part of the CTest
definition.

To support building Git via a regular `make` invocation after building
it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
convenience, this is done as part of the Makefile rule that is already
run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
up to date).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore                          |  1 +
 Makefile                            |  1 +
 contrib/buildsystems/CMakeLists.txt |  7 +------
 t/test-lib.sh                       | 10 ++++++++++
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/.gitignore b/.gitignore
index 73df7295795..89ad7e68b4b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
 /fuzz_corpora
+/GIT-BUILD-DIR
 /GIT-BUILD-OPTIONS
 /GIT-CFLAGS
 /GIT-LDFLAGS
diff --git a/Makefile b/Makefile
index 88178c5b466..886614340c7 100644
--- a/Makefile
+++ b/Makefile
@@ -3029,6 +3029,7 @@ else
 	@echo RUNTIME_PREFIX=\'false\' >>$@+
 endif
 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
 
 ### Detect Python interpreter path changes
 ifndef NO_PYTHON
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 0c741e7d878..1d8cebb4cfe 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1067,14 +1067,9 @@ endif()
 #Make the tests work when building out of the source tree
 get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
 if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
-	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
-	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
 	#Setting the build directory in test-lib.sh before running tests
 	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
-		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
-		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
-		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
-		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
+		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
 	#misc copies
 	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
 	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 120f11812c3..dfc0144ed3b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -47,6 +47,16 @@ then
 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
 	exit 1
 fi
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
+	# On Windows, we must convert Windows paths lest they contain a colon
+	case "$(uname -s)" in
+	*MINGW*)
+		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+		;;
+	esac
+fi
 
 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
 # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
-- 
gitgitgadget

[PATCH v3 5/5] cmake: increase time-out for a long-running test

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As suggested in
https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
t7112 can run for well over one hour, which seems to be the default
maximum run time at least when running CTest-based tests in Visual
Studio.

Let's increase the time-out as a stop gap to unblock developers wishing
to run Git's test suite in Visual Studio.

Note: The actual run time is highly dependent on the circumstances. For
example, in Git's CI runs, the Windows-based tests typically take a bit
over 5 minutes to run. CI runs have the added benefit that Windows
Defender (the common anti-malware scanner on Windows) is turned off,
something many developers are not at liberty to do on their work
stations. When Defender is turned on, even on this developer's high-end
Ryzen system, t7112 takes over 15 minutes to run.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 1d8cebb4cfe..7e0d040e0f6 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
+# This test script takes an extremely long time and is known to time out even
+# on fast machines because it requires in excess of one hour to run
+set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+
 endif()#BUILD_TESTING
-- 
gitgitgadget

Re: [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
>
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts: the CMakeLists.txt file
> would have to be modified, passing the desired options to _all_ test
> scripts, and then the CMake Cache would have to be reconfigured before
> running the test in question individually. Unintuitive at best, and
> opposite to the niceties IDE users expect.
>
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.
>
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options.
>
> This makes the test runs noticeably faster because the `bin-wrappers/`
> scripts as well as the `chain-lint` code make heavy use of POSIX shell
> scripting, which is really, really slow on Windows due to the need to
> emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
> Sunshine, it added two minutes (!) just to perform the chain-lint task.
>
> The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was
> considered during the development of this patch, but then dropped: such
> a setting is global, across _all_ tests, where e.g. `--run=...` would
> not make sense. Users wishing to override these new defaults are better
> advised running the test script manually, in a Git Bash, with full
> control over the command line.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 2237109b57f..6ac20bc5054 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>  #test
>  foreach(tsh ${test_scipts})
>  	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()

This all seems sensible, but where you lose me is why this needs to be
cmake-specific.

Maybe I'm just missing something, but everything you're describing seems
to me to be Windows-specific, e.g. "chain lint is slow on windows" and
the like.

Why aren't we just declaring that it's not worth it on Windows, but
doing so in test-lib.sh, or at least doing it in both cmake's recipe &
the Makefile?

I can see why we'd e.g. want it not to run with "make test", but want it
when you run individual tests, but then both could just set some "this
is running as a batch" env flag or whatever.

Also, even then everything you're describing is eliding that:

 - This ill makes sense for Windows, sure, but the cmake recipe is no
   longer Windows-specific, was this authored before that happened &
   rebased? Why do we want cmake/ctest on Linux to have --no-chain-lint?

 - Having cmake be some "mostly the Makefile, but also other defaults"
   is really changing the scope of the whole cmake build alternative,
   which until now (but I may have missed some other special-cases) has
   been "let's build it like the Makefile".

 - That "ctest" doesn't work on Linux *at all* currently, this fixes it,
   but is really sweeping the underlying issue under the rug. AFAICT
   it's been limping along because there's no "chmod +x" or whatever on
   Windows (IIRC we magically look at she shebang).

For that last one & more I hacked up
https://github.com/avar/git/tree/avar/cmake-test-path a few days ago as
an alternative to some parts of this series, but given past exchanges
wanted to ask you first what you thought about it off-list, but didn't
get around to it..

But anyway, I do think starting with something like
https://github.com/avar/git/commit/5bd0ee2bc826d2bb358af9d93c88ba28584bbda1
makes more sense, i.e. get "ctest" to work at all with the
bin-wrappers/* in a portable way, and then *maybe* tweak it/Makefile to
pass some custom flags on some platform(s).

Re: [PATCH v3 2/5] cmake: copy the merge tools for testing

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Even when running the tests via CTest, t7609 and t7610 rely on more than
> only a few mergetools to be copied to the build directory. Let's make it
> so.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 6ac20bc5054..0c741e7d878 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
>  	#misc copies
>  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> -	file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
> +	file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
> +	file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
>  endif()

I mentioned that I hacked up a "root cause fix" for some issues in cmake
in 1/5. Continuing that this works, but is really just a
hack-for-the-hack fix, where we should fix the root cause:

Which actually, is really simple:
https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245

As you know we fake up the $GIT_BUILD_DIR for cmake, but then later in
the recipe (i.e. here) we have to scurry around and adjust that because
for some stuff we mean contrib/buildsystems/out/<path>, whereas in
others we mean the original <path>.

So here (and this isn't on you, we had this before) if you run "cmake"
and e.g. tweak the mergetools sources we'll need to re-copy them, blah
blah blah.

But we can just ... not copy it at all, and have the test-lib.sh learn
that some stuff it needs from whatever the "build dir" is, but other
stuff is *always* relative to the root of the project.

So, for most of that & the context it simply goes away, we don't need to
copy po/is.po, the tests can just know where to find it, ditto
mergetools/tkdiff (which is not generated), and the bash completion.

Re: [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the interactive `add` operation, users can choose to jump to specific
> hunks, and Git will present the hunk list in that case. To avoid showing
> too many lines at once, only a maximum of 21 hunks are shown, skipping
> the "mode change" pseudo hunk.
>
> The comparison performed to skip the "mode change" pseudo hunk (if any)
> compares a signed integer `i` to the unsigned value `mode_change` (which
> can be 0 or 1 because it is a 1-bit type).
>
> According to section 6.3.1.8 of the C99 standard (see e.g.
> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
> happen is an automatic conversion of the "lesser" type to the "greater"
> type, but since the types differ in signedness, it is ill-defined what
> is the correct "usual arithmetic conversion".
>
> Which means that Visual C's behavior can (and does) differ from GCC's:
> When compiling Git using the latter, `add -p`'s `goto` command shows no
> hunks by default because it casts a negative start offset to a pretty
> large unsigned value, breaking the "goto hunk" test case in
> `t3701-add-interactive.sh`.
>
> Let's avoid that by converting the unsigned bit explicitly to a signed
> integer.
>
> Note: This is a long-standing bug in the Visual C build of Git, but it
> has never been caught because t3701 is skipped when `NO_PERL` is set,
> which is the case in the `vs-test` jobs of Git's CI runs.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  add-patch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..3524555e2b0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1547,7 +1547,7 @@ soft_increment:
>  			strbuf_remove(&s->answer, 0, 1);
>  			strbuf_trim(&s->answer);
>  			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
> -			if (i < file_diff->mode_change)
> +			if (i < (int)file_diff->mode_change)
>  				i = file_diff->mode_change;
>  			while (s->answer.len == 0) {
>  				i = display_hunks(s, file_diff, i);

Junio pointed out in the last round that this really could use a
separate submission, and I tend to agree. It's really nothing to do with
cmake...

Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
>
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
>
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
>
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
>
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Re my earlier feedback, I came up with this as an alternative, which
nicely allows us to have "cmake" and "make" play together, you can even
run them concurrently!:

	https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245

In case that OID changes it's on my
https://github.com/avar/git/commits/avar/cmake-test-path branch,
currently 30f2265fd07 (cmake & test-lib.sh: add a $GIT_SOURCE_DIR
variable, 2022-10-14).

And it...

> diff --git a/Makefile b/Makefile
> index 88178c5b466..886614340c7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3029,6 +3029,7 @@ else
>  	@echo RUNTIME_PREFIX=\'false\' >>$@+
>  endif
>  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi

...allows us to get rid of this, which you understandably need with your
approach, but which I'd *really* prefer we not have. Let's not sneak
things into make's dependency DAG that it doesn't know about in FORCE'd
shell command (but more on that later).

>  ### Detect Python interpreter path changes
>  ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 0c741e7d878..1d8cebb4cfe 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
>  #Make the tests work when building out of the source tree
>  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>  	#Setting the build directory in test-lib.sh before running tests
>  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>  	#misc copies
>  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)

...and this whole section just goes away, we don't need any
cmake-specifi hacking here, and actually it's not cmake-specific at
all. It's just a "GIT_TEST_INSTALLED for things that are built, not
installed". E.g.:

            (cd g/git.scratch && make)
            (cd g/git && make clean && GIT_TEST_BUILD_DIR="$PWD/../git.scratch" make -C t)

Supporting cmake then just becomes a special-case of test-lib.sh knowing
"hey, my built stuff is at <dir> instead of "../".

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 120f11812c3..dfc0144ed3b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -47,6 +47,16 @@ then
>  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>  	exit 1
>  fi
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> +	# On Windows, we must convert Windows paths lest they contain a colon
> +	case "$(uname -s)" in
> +	*MINGW*)
> +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> +		;;
> +	esac
> +fi

...but one thing that I migh thave missed (and would really appreciate
your testing for) is that I didn't invoke cygpath in my version. CI
passes, but since Windows CI doesn't use "ctest" that doesn't tell us
much, and in any case that's Cygwin, no, which we don't run anyway
there?

Anyway, we could run that "cypath" easily in the cmake recipe itself, or
just pass a "hey, please make this canonical" flag to test-lib.sh.

But anyway, one thing that approach explicitly leaves out is that you
want to be able to:

 1. Build with cmake
 2. cd t
 3. Run a test

And have the test itself know to locate and use the cmake binaries
instead of the "main" binaries.

Now, I suspect that we don't actually have cases anyone cares about
where we have *both*, but that's how this code behaves. I.e. a
top-level:

	make test

Will wpe that GIT-BUILD-DIR and use the "make" built "git", but e.g.:

	make
	<build with cmake>
	cd t
	# At this point I forgot I used cmake earlier
	./t0001-init.sh # silently uses cmake...

I can see thy case for auto-discovery, per the IDE case you mentioned,
but isn't it much better to just make this part of the slightly later
part (but we need to set it up here now) part where we discover the
built "git" and:

 A. Do we not have it in ../git?
 B. Do we have it it contrib/buildsystems/out/git

Then (pseudocode):

	if (!A && B)
		use_cmake();
	else if (A && B)
		die("you have both, pick one!");

Or just say that "make" entry points always run with stuff it built, and
"ctest" runs with contrib/buildsystems/out/git, that's explicitly what
you don't want though...

Anyway, to wrap this up, I really wish the interaction between the two
wouldn't have these pitfalls. I get that you want to support it on the
specific Windows IDE case, but can't we more narrowlry do that without:

	(cd t && ./t0001-init.sh)

Having this new "pick either one" behavior? Cheers.
		

Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

[Johannes Schindelin]

Hi Victoria,

On Wed, 7 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When a test script fails in Git's test suite, the usual course of action
> > is to re-run it using options to increase the verbosity of the output,
> > e.g. `-v` and `-x`.
> >
> > Like in Git's CI runs, when running the tests in Visual Studio via the
> > CTest route, it is cumbersome or at least requires a very unintuitive
> > approach to pass options to the test scripts.
>
> At first I wondered whether there's a way to make arg specification more
> intuitive, rather than silently changing defaults. Unfortunately, it looks
> like even in the latest versions of CTest don't really support passing
> arguments through to tests [1] (and the workarounds are unpleasant at best).
>
> But then, you mentioned that there *is* a cumbersome/unintuitive approach to
> passing the options; what approach were you thinking?

Thank you for pointing out that I assumed this to be more obvious than it
actually is.

The cumbersome way is to edit this part of the CMake definition
(https://github.com/git/git/blob/v2.38.0/contrib/buildsystems/CMakeLists.txt#L1091-L1096):

	#test
	foreach(tsh ${test_scipts})
		add_test(NAME ${tsh}
			COMMAND ${SH_EXE} ${tsh}
			WORKING_DIRECTORY
			${CMAKE_SOURCE_DIR}/t)
	endforeach()

To pass an option like `-v --run=1,29`, the user would have to edit the
line `COMMAND ${SH_EXE} ${tsh}`, appending the options, and then
re-configure the CMake cache. And then not forget to _only_ run the test
script in question because those options do not make sense for the other
tests. And then not forget to undo the changes and re-configure the CMake
cache.

> [1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470
>
> >
> > So let's just pass those options by default: This will not clutter any
> > output window but the log that is written to a log file will have
> > information necessary to figure out test failures.
>
> Makes sense, I don't see any harm in providing more verbose output by
> default here.
>
> >
> > While at it, also imitate what the Windows jobs in Git's CI runs do to
> > accelerate running the test scripts: pass the `--no-bin-wrappers` and
> > `--no-chain-lint` options. This makes the test runs noticeably faster
> > because the `bin-wrappers/` scripts as well as the `chain-lint` code
> > make heavy use of POSIX shell scripting, which is really, really slow on
> > Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> I'm a bit more hesitant on including these. I see how the performance
> benefit (on Windows in particular) would make typical user experience nicer.
> But, if someone develops locally with '--no-chain-lint' specified, they'll
> be much more likely to miss a broken && chain (personally, I get caught by
> chain lint errors *all the time* when I'm adding/updating tests) and cause
> unnecessary churn in their patch submissions. So, my recommendation would be
> to drop '--no-chain-lint' here (unless it's less important to the average
> developer than I think it is).
>
> It's also possible that '--no-bin-wrappers' does something weird with their
> local installation of Git but I think it's safe enough to make the default
> if the performance gain is substantial.
>
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  contrib/buildsystems/CMakeLists.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index 1b23f2440d8..4aee1e24342 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
> >  #test
> >  foreach(tsh ${test_scipts})
> >  	add_test(NAME ${tsh}
> > -		COMMAND ${SH_EXE} ${tsh}
> > +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>
> As you have it here, I don't think there's a way for a user to override
> these defaults (unless there's something about the manual workaround you
> mentioned earlier that allows it). Since a user could feasibly want to set
> their own options, could you add a build variable to CMakeLists.txt like
> 'GIT_TEST_OPTS'? You could use it to set the default options you have here,
> but a user could still specify their own value at build time to override.

Indeed, there is no way to override this, unless the user edits the
(Git-tracked) `CMakeLists.txt` file and then re-configures the cache.

In my experience, the most common scenario where a developer needs to pass
options to test script is when they need to restrict execution to
individual test cases via, say, `--run=1,29`.

Such a use case cannot be covered via the CMake approach because it is not
fine-grained enough: any options the user could set would apply to the
entire test suite.

I added a comment to the commit message advising users to run the test
script manually via Git Bash instead, if they wish to pass a different set
of command-line options to the test script.

Thanks,
Dscho

>
> >  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> >  endforeach()
> >
>
>

Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 7578 bytes --]

Hi Ævar,

On Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 7f5397a07c6c (cmake: support for testing git when building out of the
> > source tree, 2020-06-26), we implemented support for running Git's test
> > scripts even after building Git in a different directory than the source
> > directory.
> >
> > The way we did this was to edit the file `t/test-lib.sh` to override
> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> > directory.
> >
> > This is unideal because it always leaves a tracked file marked as
> > modified, and it is all too easy to commit that change by mistake.
> >
> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> > exists, the contents are interpreted as the location to the _actual_
> > build directory. We then write this file as part of the CTest
> > definition.
> >
> > To support building Git via a regular `make` invocation after building
> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> > convenience, this is done as part of the Makefile rule that is already
> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> > up to date).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  .gitignore                          |  1 +
> >  Makefile                            |  1 +
> >  contrib/buildsystems/CMakeLists.txt |  7 +------
> >  t/test-lib.sh                       | 11 ++++++++++-
> >  4 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/.gitignore b/.gitignore
> > index a4522157641..b72ddf09346 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -2,6 +2,7 @@
> >  /fuzz_corpora
> >  /fuzz-pack-headers
> >  /fuzz-pack-idx
> > +/GIT-BUILD-DIR
> >  /GIT-BUILD-OPTIONS
> >  /GIT-CFLAGS
> >  /GIT-LDFLAGS
> > diff --git a/Makefile b/Makefile
> > index 04d0fd1fe60..9347ed90da7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -3028,6 +3028,7 @@ else
> >  	@echo RUNTIME_PREFIX=\'false\' >>$@+
> >  endif
> >  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> > +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
> >
> >  ### Detect Python interpreter path changes
> >  ifndef NO_PYTHON
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index fe606c179f7..29d7e236ae1 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1067,14 +1067,9 @@ endif()
> >  #Make the tests work when building out of the source tree
> >  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> >  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> > -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> > -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> >  	#Setting the build directory in test-lib.sh before running tests
> >  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> > -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> > -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> > -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> > -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> > +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
> >  	#misc copies
> >  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> >  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 55857af601b..4468ac51f25 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -42,7 +42,16 @@ then
> >  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> >  fi
> >  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > +	# On Windows, we must convert Windows paths lest they contain a colon
> > +	case "$(uname -s)" in
> > +	*MINGW*)
> > +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > +		;;
> > +	esac
> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> >  then
> >  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> >  	exit 1
>
> So we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct
> and assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests
> run inside a "t" directory generated in contrib/buildsystems/ ?
>
> This really seems like a hack-upon-hack, and will break some workflows
> when you switch back & forth, e.g.:
>
> 	make
> 	# run cmake
> 	make -C t
>
> Will now pick up this new file, and result in broken tests, as we'll
> rely on the now-stale file.
>
> IOW you're making the assumption that by piggy-backing on the
> GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run
> it*, but that's not the case.
>
> I can think of ways to get around it, but it would be nasty as we'd need
> to complete the dependency graph between the two, and figure out the
> various interactions.
>
> But why do we need this at all? Right now I tried:
>
>  * (Manually) removing the existing hack
>  * Copying t/*lib*.sh over to contrib/buildsystems/t
>  * cd contrib/buildsystems/t
>  * ../../../t/t0001-init.sh

I appreciate that you want to make it easier for developers. From my
experience with (Windows) developers, I have to point out that what you
describe is _not_ easier than `cd t && ./t0001-init.sh`, though.

It might be easier from some point of view that seems vaguely familiar,
but the user experience of this approach looks prohibitively bad to me.

> Which (aside from a small fixable nit about oid-info) Just Works,
> because the cmake build is already creating a GIT-BUILD-OPTIONS.
>
> Presumably that "copying" step should be simlynking, or we'd be smarter
> about doing includes from test-lib.sh.
>
> But isn't that a much better approach? Rather than working around the
> directory discovery just teach it to run from the generated t/ directory
> directly?

I am sure I could poke more holes into that approach, probably whipping
out something something about `contrib/`, but the user experience alone
(so, dear developer, contrary to what you read in the README and in other
documentation flying all over the internet, yes, you need to switch to
`contrib/buildsystems`, no wait, there's a typo, there's no dash between
`build` and `systems`, there you go, why `buildsystems`? Well, it's just
the way it is. Yes. Now, type out, no, no, wait, *wait!*, you cannot run
`t0006-date.sh` directly as documented, no, wait, you need to prefix it
with this here, and now carefully count the dots, yep, that's right, six
dots in total, three slashes, then a `t/`, yes, I know you were already in
`t/` but not _that_ `t/`, there are now two of 'em, yes, it is
unintuitive, you're absolutely correct, but you know...) is enough for me
to not want to consider this approach any further.

Ciao,
Johannes

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]

Hi Ævar,

On Thu, 8 Sep 2022, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 7f5397a07c6c (cmake: support for testing git when building out of the
> > source tree, 2020-06-26), we implemented support for running Git's test
> > scripts even after building Git in a different directory than the source
> > directory.
> >
> > The way we did this was to edit the file `t/test-lib.sh` to override
> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> > directory.
> >
> > This is unideal because it always leaves a tracked file marked as
> > modified, and it is all too easy to commit that change by mistake.
> >
> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> > exists, the contents are interpreted as the location to the _actual_
> > build directory. We then write this file as part of the CTest
> > definition.
> >
> > To support building Git via a regular `make` invocation after building
> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> > convenience, this is done as part of the Makefile rule that is already
> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> > up to date).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  .gitignore                          |  1 +
> >  Makefile                            |  1 +
> >  contrib/buildsystems/CMakeLists.txt |  7 +------
> >  t/test-lib.sh                       | 11 ++++++++++-
> >  4 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/.gitignore b/.gitignore
> > index a4522157641..b72ddf09346 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -2,6 +2,7 @@
> >  /fuzz_corpora
> >  /fuzz-pack-headers
> >  /fuzz-pack-idx
> > +/GIT-BUILD-DIR
> >  /GIT-BUILD-OPTIONS
> >  /GIT-CFLAGS
> >  /GIT-LDFLAGS
> > diff --git a/Makefile b/Makefile
> > index 04d0fd1fe60..9347ed90da7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -3028,6 +3028,7 @@ else
> >  	@echo RUNTIME_PREFIX=\'false\' >>$@+
> >  endif
> >  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> > +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
> >
> >  ### Detect Python interpreter path changes
> >  ifndef NO_PYTHON
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index fe606c179f7..29d7e236ae1 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1067,14 +1067,9 @@ endif()
> >  #Make the tests work when building out of the source tree
> >  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> >  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> > -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> > -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> >  	#Setting the build directory in test-lib.sh before running tests
> >  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> > -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> > -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> > -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> > -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> > +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
> >  	#misc copies
> >  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> >  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 55857af601b..4468ac51f25 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -42,7 +42,16 @@ then
> >  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> >  fi
> >  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > +	# On Windows, we must convert Windows paths lest they contain a colon
> > +	case "$(uname -s)" in
> > +	*MINGW*)
> > +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > +		;;
> > +	esac
> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> >  then
> >  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> >  	exit 1
>
> As pointed out in the v1 this breaks the cmake<->make interaction in
> some scenarios, but from some brief testing there seemed to be an easy
> workaround which didn't suffer from that problem:
> https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/

I do not think that the CMake <-> make interaction will come up in any
other scenario than your and my tests in the context of this mailing list
thread.

Therefore, I am certain that we need not cater to that scenario at all.

Ciao,
Johannes

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]

Hi Victoria,

On Thu, 8 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin via GitGitGadget wrote:
>
> > [...]
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 55857af601b..4468ac51f25 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -42,7 +42,16 @@ then
> >  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> >  fi
> >  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > +	# On Windows, we must convert Windows paths lest they contain a colon
> > +	case "$(uname -s)" in
> > +	*MINGW*)
> > +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > +		;;
> > +	esac
> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> >  then
> >  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> >  	exit 1
> Referring to Ævar's review in [1] - while I'm not overly concerned about the
> "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
>
> ----------------->8----------------->8----------------->8-----------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4468ac51f2..7b57f55c37 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,6 +42,11 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> +if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +then
> +	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> +	exit 1
> +fi
>  if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>  then
>  	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> @@ -51,10 +56,6 @@ then
>  		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>  		;;
>  	esac
> -elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> -then
> -	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> -	exit 1
>  fi
>
>  # Prepend a string to a VAR using an arbitrary ":" delimiter, not
> -----------------8<-----------------8<-----------------8<-----------------
>
> Otherwise, a user could run the tests from outside a 't/' directory if they
> built Git with CMake, which doesn't appear to be part of the intended
> behavior of this patch.

Good point!

Thank you,
Dscho

Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

Hi Ævar,

you did not even give me a chance to send my reply to your original mail
;-)

On Tue, 18 Oct 2022, Ævar Arnfjörð Bjarmason wrote:

>
> On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 7f5397a07c6c (cmake: support for testing git when building out of the
> > source tree, 2020-06-26), we implemented support for running Git's test
> > scripts even after building Git in a different directory than the source
> > directory.
> >
> > The way we did this was to edit the file `t/test-lib.sh` to override
> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> > directory.
> >
> > This is unideal because it always leaves a tracked file marked as
> > modified, and it is all too easy to commit that change by mistake.
> >
> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> > exists, the contents are interpreted as the location to the _actual_
> > build directory. We then write this file as part of the CTest
> > definition.
> >
> > To support building Git via a regular `make` invocation after building
> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> > convenience, this is done as part of the Makefile rule that is already
> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> > up to date).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Re my earlier feedback, I came up with this as an alternative, which
> nicely allows us to have "cmake" and "make" play together, you can even
> run them concurrently!:
>
> 	https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245

This approach _still_ modifies the `test-lib.sh`, which is the entire
reason for the patch under review.

I hope you find an elegant, user-friendly alternative that leaves
`test-lib.sh` unmodified even when building via CMake. I would gladly take
that and drop my `GIT-BUILD-DIR` patch.

Ciao,
Johannes

Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 18 2022, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 8 Sep 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
>>
>> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> >
>> > In 7f5397a07c6c (cmake: support for testing git when building out of the
>> > source tree, 2020-06-26), we implemented support for running Git's test
>> > scripts even after building Git in a different directory than the source
>> > directory.
>> >
>> > The way we did this was to edit the file `t/test-lib.sh` to override
>> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
>> > directory.
>> >
>> > This is unideal because it always leaves a tracked file marked as
>> > modified, and it is all too easy to commit that change by mistake.
>> >
>> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
>> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
>> > exists, the contents are interpreted as the location to the _actual_
>> > build directory. We then write this file as part of the CTest
>> > definition.
>> >
>> > To support building Git via a regular `make` invocation after building
>> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
>> > convenience, this is done as part of the Makefile rule that is already
>> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
>> > up to date).
>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> >  .gitignore                          |  1 +
>> >  Makefile                            |  1 +
>> >  contrib/buildsystems/CMakeLists.txt |  7 +------
>> >  t/test-lib.sh                       | 11 ++++++++++-
>> >  4 files changed, 13 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/.gitignore b/.gitignore
>> > index a4522157641..b72ddf09346 100644
>> > --- a/.gitignore
>> > +++ b/.gitignore
>> > @@ -2,6 +2,7 @@
>> >  /fuzz_corpora
>> >  /fuzz-pack-headers
>> >  /fuzz-pack-idx
>> > +/GIT-BUILD-DIR
>> >  /GIT-BUILD-OPTIONS
>> >  /GIT-CFLAGS
>> >  /GIT-LDFLAGS
>> > diff --git a/Makefile b/Makefile
>> > index 04d0fd1fe60..9347ed90da7 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -3028,6 +3028,7 @@ else
>> >  	@echo RUNTIME_PREFIX=\'false\' >>$@+
>> >  endif
>> >  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
>> > +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>> >
>> >  ### Detect Python interpreter path changes
>> >  ifndef NO_PYTHON
>> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
>> > index fe606c179f7..29d7e236ae1 100644
>> > --- a/contrib/buildsystems/CMakeLists.txt
>> > +++ b/contrib/buildsystems/CMakeLists.txt
>> > @@ -1067,14 +1067,9 @@ endif()
>> >  #Make the tests work when building out of the source tree
>> >  get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>> >  if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
>> > -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
>> > -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>> >  	#Setting the build directory in test-lib.sh before running tests
>> >  	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
>> > -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
>> > -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
>> > -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
>> > -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
>> > +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>> >  	#misc copies
>> >  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>> >  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
>> > diff --git a/t/test-lib.sh b/t/test-lib.sh
>> > index 55857af601b..4468ac51f25 100644
>> > --- a/t/test-lib.sh
>> > +++ b/t/test-lib.sh
>> > @@ -42,7 +42,16 @@ then
>> >  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>> >  fi
>> >  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>> > +then
>> > +	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
>> > +	# On Windows, we must convert Windows paths lest they contain a colon
>> > +	case "$(uname -s)" in
>> > +	*MINGW*)
>> > +		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>> > +		;;
>> > +	esac
>> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> >  then
>> >  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>> >  	exit 1
>>
>> As pointed out in the v1 this breaks the cmake<->make interaction in
>> some scenarios, but from some brief testing there seemed to be an easy
>> workaround which didn't suffer from that problem:
>> https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/
>
> I do not think that the CMake <-> make interaction will come up in any
> other scenario than your and my tests in the context of this mailing list
> thread.
>
> Therefore, I am certain that we need not cater to that scenario at all.

I run it like that now when I build locally, and it slots nicely in with:

 	1. First run a bunch of 'build' targets
	2. Then run a bunch of 'test' targets

I'm not opposed to having this e.g. as a special-case on Windows, or
that we pick up when you *only* have cmake/ctest unconditionally, and
auto-do the right thing.

But I do think that:

	A. Getting a "run test-lib against this build dir over there" is
	   *different* from the bundled behavior of the auto-flip-flopping.

	B. Your case for doing it this way seems to be the
	   Windows-specific case noted in 1/5, can't we just make that
	   part Windows-specific then? I don't get why it needs to be
	   bundled up with cmake everywhere....