{"thread":{"id":"58284","subject":"[PATCH 0/5] Some fixes and an improvement for using CTest on Windows","startedAt":"2022-08-10T15:02:37Z","lastAt":"2022-10-18T15:13:48Z","messageCount":58,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Ævar Arnfjörð Bjarmason","Phillip Wood","Johannes Schindelin","Victoria Dye","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"460986","messageId":"pull.1320.git.1660143750.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":null,"subject":"[PATCH 0/5] Some fixes and an improvement for using CTest on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T15:02:25Z","receivedAt":"2022-08-10T15:02:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As part of the CMake definition, Git enjoy support for running the test\nsuite via CTest.\n\nIn https://github.com/git-for-windows/git/issues/3966, it has been reported\nthat this does not work out of the box, though, but causes a couple of test\nfailures instead. These problems are not caught by Git's CI runs because the\nvs-tests jobs actually use prove to run the test suite, not CTest.\n\nIn addition to fixing these problems, this patch series also addresses a\nlong-standing gripe I have with the way Git's CMake definition supports\nCTest: It edits t/test-lib.sh, which leaves this file eternally modified\n(but these modification should never be committed, they refer to a\nlocal-only, configuration-dependent directory).\n\nJohannes Schindelin (5):\n cmake: align CTest definition with Git's CI runs\n cmake: copy the merge tools for testing\n tests: explicitly skip `chmod` calls on Windows\n add -p: avoid ambiguous signed/unsigned comparison\n cmake: avoid editing t/test-lib.sh\n\n .gitignore | 1 +\n Makefile | 1 +\n add-patch.c | 2 +-\n contrib/buildsystems/CMakeLists.txt | 12 ++++--------\n t/test-lib-functions.sh | 10 ++++++++--\n t/test-lib.sh | 11 ++++++++++-\n 6 files changed, 25 insertions(+), 12 deletions(-)\n\n\nbase-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1320\n-- \ngitgitgadget\n"},{"id":"460987","messageId":"9cf14984c0a71b1ccdff7db0699571bf5af1209b.1660143750.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.git.1660143750.gitgitgadget@gmail.com","subject":"[PATCH 1/5] cmake: align CTest definition with Git's CI runs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T15:02:26Z","receivedAt":"2022-08-10T15:02:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and\n`--no-chain-lint`, mainly to win back some time caused by the serious\nperformance penalty paid for the tests relying so heavily on POSIX shell\nscripting, which only works by using a POSIX emulation layer.\n\nLet's do the same when running the tests, say, in Visual Studio.\n\nWhile at it, enable the command trace via `-x` and verbose output via\n`-v`, otherwise it would be near impossible to diagnose any problems.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 1b23f2440d8..4aee1e24342 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n #test\n foreach(tsh ${test_scipts})\n \tadd_test(NAME ${tsh}\n-\t\tCOMMAND ${SH_EXE} ${tsh}\n+\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n endforeach()\n \n-- \ngitgitgadget\n\n"},{"id":"460988","messageId":"86ab58b65081601678a24f99eb4afa34cccaefba.1660143750.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.git.1660143750.gitgitgadget@gmail.com","subject":"[PATCH 2/5] cmake: copy the merge tools for testing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T15:02:27Z","receivedAt":"2022-08-10T15:02:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEven when running the tests via CTest, t7609 and t7610 rely on more than\nonly a few mergetools to be copied to the build directory. Let's make it\nso.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 4aee1e24342..fe606c179f7 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n \t#misc copies\n \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n-\tfile(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n+\tfile(GLOB mergetools \"${CMAKE_SOURCE_DIR}/mergetools/*\")\n+\tfile(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n endif()\n-- \ngitgitgadget\n\n"},{"id":"460989","messageId":"79abfa82c32ea686469cfe2417bc491c04179623.1660143750.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.git.1660143750.gitgitgadget@gmail.com","subject":"[PATCH 3/5] tests: explicitly skip `chmod` calls on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T15:02:28Z","receivedAt":"2022-08-10T15:02:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, we use a POSIX emulation layer, the MSYS2 runtime, to allow\nfor running shell scripts (albeit at a hefty performance cost, Git's\ntest suite takes ~700 seconds to complete on Linux, according to Git's\nCI runs, while it takes more than 6,000 seconds on Windows).\n\nThis emulation layer has a funny quirk when it comes to `chmod`\ninvocations: it pretends that it succeeded, when in reality it did not\ndo a thing (because the Access Control Lists used in Windows' permission\nmodel are so different to Unix' default permission model that Git's test\nsuite assumes to be in effect).\n\nGit's test suite relies on this quirk by assuming that the `chmod` calls\nin `test_chmod` and `test_write_script` simply succeed on Windows\n(without actually doing anything).\n\nHowever, this quirk is only in effect as long as `chmod` is run inside\nthe pseudo Unix root directory structure or within the home directory.\nWhen run outside, such invocations fail like this:\n\n\tchmod: changing permissions of '': Invalid argument\n\nNow, when running Git's tests in, say, Visual Studio, we frequently are\nin a worktree where the `chmod` invocations would fail.\n\nLet's accommodate for that by explicitly skipping those `chmod`\ninvocations on Windows.\n\nSigned-off-by: Johannes Schindelin \n---\n t/test-lib-functions.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 6da7273f1d5..7c63b22acab 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -492,7 +492,10 @@ test_commit_bulk () {\n # of a file in the working directory and add it to the index.\n \n test_chmod () {\n-\tchmod \"$@\" &&\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tchmod \"$@\"\n+\tfi &&\n \tgit update-index --add \"--chmod=$@\"\n }\n \n@@ -548,7 +551,10 @@ write_script () {\n \t\techo \"#!${2-\"$SHELL_PATH\"}\" &&\n \t\tcat\n \t} >\"$1\" &&\n-\tchmod +x \"$1\"\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tchmod +x \"$1\"\n+\tfi\n }\n \n # Usage: test_hook [options] <<-\\EOF\n-- \ngitgitgadget\n\n"},{"id":"460990","messageId":"4d24a4345ba66031d2ccf7ce472ed93ace82e9d6.1660143750.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.git.1660143750.gitgitgadget@gmail.com","subject":"[PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T15:02:29Z","receivedAt":"2022-08-10T15:02:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the interactive `add` operation, users can choose to jump to specific\nhunks, and Git will present the hunk list in that case. To avoid showing\ntoo many lines at once, only a maximum of 21 hunks are shown, skipping\nthe \"mode change\" pseudo hunk.\n\nThe comparison performed to skip the \"mode change\" pseudo hunk (if any)\ncompares a signed integer `i` to the unsigned value `mode_change` (which\ncan be 0 or 1 because it is a 1-bit type).\n\nAccording to section 6.3.1.8 of the C99 standard (see e.g.\nhttps://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\nhappen is an automatic conversion of the \"lesser\" type to the \"greater\"\ntype, but since the types differ in signedness, it is ill-defined what\nis the correct \"usual arithmetic conversion\".\n\nWhich means that Visual C's behavior can (and does) differ from GCC's:\nWhen compiling Git using the latter, `add -p`'s `goto` command shows no\nhunks by default because it casts a negative start offset to a pretty\nlarge unsigned value, breaking the \"goto hunk\" test case in\n`t3701-add-interactive.sh`.\n\nLet's avoid that by converting the unsigned bit explicitly to a signed\ninteger.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..3524555e2b0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1547,7 +1547,7 @@ soft_increment:\n \t\t\tstrbuf_remove(&s->answer, 0, 1);\n \t\t\tstrbuf_trim(&s->answer);\n \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n-\t\t\tif (i < file_diff->mode_change)\n+\t\t\tif (i < (int)file_diff->mode_change)\n \t\t\t\ti = file_diff->mode_change;\n \t\t\twhile (s->answer.len == 0) {\n \t\t\t\ti = display_hunks(s, file_diff, i);\n-- \ngitgitgadget\n\n"},{"id":"460991","messageId":"c7fc5a4ee4cbc49b8afb2f9579763482552fffb0.1660143750.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.git.1660143750.gitgitgadget@gmail.com","subject":"[PATCH 5/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T15:02:30Z","receivedAt":"2022-08-10T15:02:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 7f5397a07c6c (cmake: support for testing git when building out of the\nsource tree, 2020-06-26), we implemented support for running Git's test\nscripts even after building Git in a different directory than the source\ndirectory.\n\nThe way we did this was to edit the file `t/test-lib.sh` to override\n`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\ndirectory.\n\nThis is unideal because it always leaves a tracked file marked as\nmodified, and it is all too easy to commit that change by mistake.\n\nLet's change the strategy by teaching `t/test-lib.sh` to detect the\npresence of a file called `GIT-BUILD-DIR` in the source directory. If it\nexists, the contents are interpreted as the location to the _actual_\nbuild directory. We then write this file as part of the CTest\ndefinition.\n\nTo support building Git via a regular `make` invocation after building\nit using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\nconvenience, this is done as part of the Makefile rule that is already\nrun with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\nup to date).\n\nSigned-off-by: Johannes Schindelin \n---\n .gitignore | 1 +\n Makefile | 1 +\n contrib/buildsystems/CMakeLists.txt | 7 +------\n t/test-lib.sh | 11 ++++++++++-\n 4 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex a4522157641..b72ddf09346 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -2,6 +2,7 @@\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\n+/GIT-BUILD-DIR\n /GIT-BUILD-OPTIONS\n /GIT-CFLAGS\n /GIT-LDFLAGS\ndiff --git a/Makefile b/Makefile\nindex 04d0fd1fe60..9347ed90da7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3028,6 +3028,7 @@ else\n \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n endif\n \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n+\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n \n ### Detect Python interpreter path changes\n ifndef NO_PYTHON\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex fe606c179f7..29d7e236ae1 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1067,14 +1067,9 @@ endif()\n #Make the tests work when building out of the source tree\n get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n-\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n-\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n \t#Setting the build directory in test-lib.sh before running tests\n \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n-\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n-\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n-\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n-\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n+\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n \t#misc copies\n \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 55857af601b..4468ac51f25 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -42,7 +42,16 @@ then\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n-if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n+if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n+then\n+\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n+\t# On Windows, we must convert Windows paths lest they contain a colon\n+\tcase \"$(uname -s)\" in\n+\t*MINGW*)\n+\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n+\t\t;;\n+\tesac\n+elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n then\n \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n \texit 1\n-- \ngitgitgadget\n"},{"id":"461011","messageId":"xmqqczd8m1lr.fsf@gitster.g","threadId":"58284","inReplyTo":"9cf14984c0a71b1ccdff7db0699571bf5af1209b.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T17:48:16Z","receivedAt":"2022-08-10T17:49:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> While at it, enable the command trace via `-x` and verbose output via\n> `-v`, otherwise it would be near impossible to diagnose any problems.\n\nThis sounds like a completely unrelated change. Do the tests behave\nso differently here, compared to how they are run in t/Makefile,\nwhere -vx is not the default?\n\nThe only plausible reason I can think of that this change \"while at\nit\" is justifiable as part of the other change is if this is only\never used in the CI environment and no interactive human would be\nsitting in front of the session that runs tests here. If that is\nthe case, I do agree it would make sense, since it would be much\nless convenient to go back to a failing test and rerun it with -v\nor -x or whatever. But that needs to be explained in the proposed\nlog message.\n\nThanks.\n\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 1b23f2440d8..4aee1e24342 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n> #test\n> foreach(tsh ${test_scipts})\n> \tadd_test(NAME ${tsh}\n> -\t\tCOMMAND ${SH_EXE} ${tsh}\n> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n"},{"id":"461013","messageId":"xmqq7d3gm1bl.fsf@gitster.g","threadId":"58284","inReplyTo":"4d24a4345ba66031d2ccf7ce472ed93ace82e9d6.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T17:54:22Z","receivedAt":"2022-08-10T17:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In the interactive `add` operation, users can choose to jump to specific\n> hunks, and Git will present the hunk list in that case. To avoid showing\n> too many lines at once, only a maximum of 21 hunks are shown, skipping\n> the \"mode change\" pseudo hunk.\n>\n> The comparison performed to skip the \"mode change\" pseudo hunk (if any)\n> compares a signed integer `i` to the unsigned value `mode_change` (which\n> can be 0 or 1 because it is a 1-bit type).\n>\n> According to section 6.3.1.8 of the C99 standard (see e.g.\n> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\n> happen is an automatic conversion of the \"lesser\" type to the \"greater\"\n> type, but since the types differ in signedness, it is ill-defined what\n> is the correct \"usual arithmetic conversion\".\n>\n> Which means that Visual C's behavior can (and does) differ from GCC's:\n> When compiling Git using the latter, `add -p`'s `goto` command shows no\n> hunks by default because it casts a negative start offset to a pretty\n> large unsigned value, breaking the \"goto hunk\" test case in\n> `t3701-add-interactive.sh`.\n>\n> Let's avoid that by converting the unsigned bit explicitly to a signed\n> integer.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nThis looks more like a fix to a general problem, not limited to\nwindows or cmake, that we had since 9254bdfb (built-in add -p:\nimplement the 'g' (\"goto\") command, 2019-12-13).\n\nPlease pull this out of the series and let's have it reviewed\nseparately.\n\nThanks.\n\n> add-patch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..3524555e2b0 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1547,7 +1547,7 @@ soft_increment:\n> \t\t\tstrbuf_remove(&s->answer, 0, 1);\n> \t\t\tstrbuf_trim(&s->answer);\n> \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n> -\t\t\tif (i < file_diff->mode_change)\n> +\t\t\tif (i < (int)file_diff->mode_change)\n> \t\t\t\ti = file_diff->mode_change;\n> \t\t\twhile (s->answer.len == 0) {\n> \t\t\t\ti = display_hunks(s, file_diff, i);\n"},{"id":"461071","messageId":"220811.861qtnqb5p.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"9cf14984c0a71b1ccdff7db0699571bf5af1209b.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-11T11:18:58Z","receivedAt":"2022-08-11T11:20:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and\n> `--no-chain-lint`, mainly to win back some time caused by the serious\n> performance penalty paid for the tests relying so heavily on POSIX shell\n> scripting, which only works by using a POSIX emulation layer.\n>\n> Let's do the same when running the tests, say, in Visual Studio.\n>\n> While at it, enable the command trace via `-x` and verbose output via\n> `-v`, otherwise it would be near impossible to diagnose any problems.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 1b23f2440d8..4aee1e24342 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n> #test\n> foreach(tsh ${test_scipts})\n> \tadd_test(NAME ${tsh}\n> -\t\tCOMMAND ${SH_EXE} ${tsh}\n> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n\nIs this authored before a561962479c (cmake: fix CMakeLists.txt on Linux,\n2022-05-24) was merged?\n\nThe \"say, in Visual Studio\" seems to elide that we'll now run these\ntests differently when you run with cmake everywhere.\n\nIt seems much better to pass some \"test arguments\" to cmake itself,\nwhich we'd then call from the ci specifically. Then e.g. a Linux user of\ncmake wouldn't wonder why the \"make test\" spots e.g. a chain-lint issue\nthat the cmake testing would hide.\n"},{"id":"461072","messageId":"220811.86wnbfowbg.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"79abfa82c32ea686469cfe2417bc491c04179623.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-11T11:22:17Z","receivedAt":"2022-08-11T11:26:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> [...]\n> However, this quirk is only in effect as long as `chmod` is run inside\n> the pseudo Unix root directory structure or within the home directory.\n> When run outside, such invocations fail like this:\n>\n> \tchmod: changing permissions of '': Invalid argument\n\n..ok, but...\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 6da7273f1d5..7c63b22acab 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -492,7 +492,10 @@ test_commit_bulk () {\n> # of a file in the working directory and add it to the index.\n> \n> test_chmod () {\n> -\tchmod \"$@\" &&\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tchmod \"$@\"\n> +\tfi &&\n> \tgit update-index --add \"--chmod=$@\"\n> }\n> \n> @@ -548,7 +551,10 @@ write_script () {\n> \t\techo \"#!${2-\"$SHELL_PATH\"}\" &&\n> \t\tcat\n> \t} >\"$1\" &&\n> -\tchmod +x \"$1\"\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tchmod +x \"$1\"\n> +\tfi\n\n... you get +x semantics by default, so we didn't need that \"chmod +x\"\nin the first place?\n\nThe rest of \"test_chmod\" seems to *happen to* pass +x or -x, but we\ndon't care about that, regardless of the \"pseudo Unix root directory\"?\n\nWhat if we get a \"test_chmod -o \", won't this silently do the\nwrong thing?\n\nIf so isn't something in this direction (untested) a more targeted &\nobvious fix?:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 10258def7be..1c3b6692388 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1690,6 +1690,16 @@ case $uname_s in\n \tfind () {\n \t\t/usr/bin/find \"$@\"\n \t}\n+\tchmod () {\n+\t\tcase \"$1\" in\n+\t\t+x|-x)\n+\t\t\treturn;\n+\t\t\t;;\n+\t\t*)\n+\t\t\t;;\n+\t\tesac &&\n+\t\t/usr/bin/chmod \"$@\"\n+\t}\n \t# git sees Windows-style pwd\n \tpwd () {\n \t\tbuiltin pwd -W\n"},{"id":"461073","messageId":"220811.86sfm3ov5z.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"c7fc5a4ee4cbc49b8afb2f9579763482552fffb0.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-11T11:35:43Z","receivedAt":"2022-08-11T11:51:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In 7f5397a07c6c (cmake: support for testing git when building out of the\n> source tree, 2020-06-26), we implemented support for running Git's test\n> scripts even after building Git in a different directory than the source\n> directory.\n>\n> The way we did this was to edit the file `t/test-lib.sh` to override\n> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> directory.\n>\n> This is unideal because it always leaves a tracked file marked as\n> modified, and it is all too easy to commit that change by mistake.\n>\n> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> exists, the contents are interpreted as the location to the _actual_\n> build directory. We then write this file as part of the CTest\n> definition.\n>\n> To support building Git via a regular `make` invocation after building\n> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> convenience, this is done as part of the Makefile rule that is already\n> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> up to date).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> contrib/buildsystems/CMakeLists.txt | 7 +------\n> t/test-lib.sh | 11 ++++++++++-\n> 4 files changed, 13 insertions(+), 7 deletions(-)\n>\n> diff --git a/.gitignore b/.gitignore\n> index a4522157641..b72ddf09346 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -2,6 +2,7 @@\n> /fuzz_corpora\n> /fuzz-pack-headers\n> /fuzz-pack-idx\n> +/GIT-BUILD-DIR\n> /GIT-BUILD-OPTIONS\n> /GIT-CFLAGS\n> /GIT-LDFLAGS\n> diff --git a/Makefile b/Makefile\n> index 04d0fd1fe60..9347ed90da7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3028,6 +3028,7 @@ else\n> \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> endif\n> \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n> \n> ### Detect Python interpreter path changes\n> ifndef NO_PYTHON\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index fe606c179f7..29d7e236ae1 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1067,14 +1067,9 @@ endif()\n> #Make the tests work when building out of the source tree\n> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> \t#Setting the build directory in test-lib.sh before running tests\n> \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> \t#misc copies\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 55857af601b..4468ac51f25 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -42,7 +42,16 @@ then\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> +then\n> +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> +\t# On Windows, we must convert Windows paths lest they contain a colon\n> +\tcase \"$(uname -s)\" in\n> +\t*MINGW*)\n> +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> +\t\t;;\n> +\tesac\n> +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> then\n> \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> \texit 1\n\nSo we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct\nand assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests\nrun inside a \"t\" directory generated in contrib/buildsystems/ ?\n\nThis really seems like a hack-upon-hack, and will break some workflows\nwhen you switch back & forth, e.g.:\n\n\tmake\n\t# run cmake\n\tmake -C t\n\nWill now pick up this new file, and result in broken tests, as we'll\nrely on the now-stale file.\n\nIOW you're making the assumption that by piggy-backing on the\nGIT-BUILD-OPTIONS rule that anything that needs it will also *re-run\nit*, but that's not the case.\n\nI can think of ways to get around it, but it would be nasty as we'd need\nto complete the dependency graph between the two, and figure out the\nvarious interactions.\n\nBut why do we need this at all? Right now I tried:\n\n * (Manually) removing the existing hack\n * Copying t/*lib*.sh over to contrib/buildsystems/t\n * cd contrib/buildsystems/t\n * ../../../t/t0001-init.sh \n\nWhich (aside from a small fixable nit about oid-info) Just Works,\nbecause the cmake build is already creating a GIT-BUILD-OPTIONS.\n\nPresumably that \"copying\" step should be simlynking, or we'd be smarter\nabout doing includes from test-lib.sh.\n\nBut isn't that a much better approach? Rather than working around the\ndirectory discovery just teach it to run from the generated t/ directory\ndirectly?\n"},{"id":"461075","messageId":"ec8204d5-9d0d-9850-f930-6fc1a8efacc5@gmail.com","threadId":"58284","inReplyTo":"4d24a4345ba66031d2ccf7ce472ed93ace82e9d6.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-11T12:49:19Z","receivedAt":"2022-08-11T12:49:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In the interactive `add` operation, users can choose to jump to specific\n> hunks, and Git will present the hunk list in that case. To avoid showing\n> too many lines at once, only a maximum of 21 hunks are shown, skipping\n> the \"mode change\" pseudo hunk.\n> \n> The comparison performed to skip the \"mode change\" pseudo hunk (if any)\n> compares a signed integer `i` to the unsigned value `mode_change` (which\n> can be 0 or 1 because it is a 1-bit type).\n> \n> According to section 6.3.1.8 of the C99 standard (see e.g.\n> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\n> happen is an automatic conversion of the \"lesser\" type to the \"greater\"\n> type, but since the types differ in signedness, it is ill-defined what\n> is the correct \"usual arithmetic conversion\".\n> \n> Which means that Visual C's behavior can (and does) differ from GCC's:\n> When compiling Git using the latter, `add -p`'s `goto` command shows no\n> hunks by default because it casts a negative start offset to a pretty\n> large unsigned value, breaking the \"goto hunk\" test case in\n> `t3701-add-interactive.sh`.\n> \n> Let's avoid that by converting the unsigned bit explicitly to a signed\n> integer.\n\nOh the joys of bit-fields!\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..3524555e2b0 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1547,7 +1547,7 @@ soft_increment:\n> \t\t\tstrbuf_remove(&s->answer, 0, 1);\n> \t\t\tstrbuf_trim(&s->answer);\n\nUnrelated to this change but why don't we just call strbuf_reset() here?\n\n> \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n> -\t\t\tif (i < file_diff->mode_change)\n> +\t\t\tif (i < (int)file_diff->mode_change)\n\nThe change itself looks good\n\nBest Wishes\n\nPhillip\n\n> \t\t\t\ti = file_diff->mode_change;\n> \t\t\twhile (s->answer.len == 0) {\n> \t\t\t\ti = display_hunks(s, file_diff, i);\n"},{"id":"461076","messageId":"23d86b94-3e2e-bdeb-3385-a996c07b699d@gmail.com","threadId":"58284","inReplyTo":"c7fc5a4ee4cbc49b8afb2f9579763482552fffb0.1660143750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-11T12:58:59Z","receivedAt":"2022-08-11T12:59:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In 7f5397a07c6c (cmake: support for testing git when building out of the\n> source tree, 2020-06-26), we implemented support for running Git's test\n> scripts even after building Git in a different directory than the source\n> directory.\n> \n> The way we did this was to edit the file `t/test-lib.sh` to override\n> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> directory.\n> \n> This is unideal because it always leaves a tracked file marked as\n> modified, and it is all too easy to commit that change by mistake.\n> \n> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> exists, the contents are interpreted as the location to the _actual_\n> build directory. We then write this file as part of the CTest\n> definition.\n\nI think it is really good to get away from editing the test files, but \none of the nice things about CMake's out of tree builds is that you can \nhave several build directories with different build configurations and \nthis change does not support that. Could we pass the build directory to \nthe test scripts as a commandline option or environment variable \ninstead? e.g.\n\n foreach(tsh ${test_scipts})\n \tadd_test(NAME ${tsh}\n-\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n+\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint \n--build-dir=${CMAKE_BINARY_DIR} -vx\n\n \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n endforeach()\n\nDoing that would avoid changing the main Makefile to remove a file which \nalmost certainly does not exist every time make is invoked as well.\n\nBest Wishes\n\nPhillip\n\n> To support building Git via a regular `make` invocation after building\n> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> convenience, this is done as part of the Makefile rule that is already\n> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> up to date).\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> contrib/buildsystems/CMakeLists.txt | 7 +------\n> t/test-lib.sh | 11 ++++++++++-\n> 4 files changed, 13 insertions(+), 7 deletions(-)\n> \n> diff --git a/.gitignore b/.gitignore\n> index a4522157641..b72ddf09346 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -2,6 +2,7 @@\n> /fuzz_corpora\n> /fuzz-pack-headers\n> /fuzz-pack-idx\n> +/GIT-BUILD-DIR\n> /GIT-BUILD-OPTIONS\n> /GIT-CFLAGS\n> /GIT-LDFLAGS\n> diff --git a/Makefile b/Makefile\n> index 04d0fd1fe60..9347ed90da7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3028,6 +3028,7 @@ else\n> \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> endif\n> \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n> \n> ### Detect Python interpreter path changes\n> ifndef NO_PYTHON\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index fe606c179f7..29d7e236ae1 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1067,14 +1067,9 @@ endif()\n> #Make the tests work when building out of the source tree\n> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> \t#Setting the build directory in test-lib.sh before running tests\n> \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> \t#misc copies\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 55857af601b..4468ac51f25 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -42,7 +42,16 @@ then\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> +then\n> +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> +\t# On Windows, we must convert Windows paths lest they contain a colon\n> +\tcase \"$(uname -s)\" in\n> +\t*MINGW*)\n> +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> +\t\t;;\n> +\tesac\n> +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> then\n> \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> \texit 1\n"},{"id":"461270","messageId":"r1461365-9np6-n988-0288-9ps4o944qqp4@tzk.qr","threadId":"58284","inReplyTo":"xmqq7d3gm1bl.fsf@gitster.g","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-16T09:56:02Z","receivedAt":"2022-08-16T10:40:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 Aug 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > In the interactive `add` operation, users can choose to jump to specific\n> > hunks, and Git will present the hunk list in that case. To avoid showing\n> > too many lines at once, only a maximum of 21 hunks are shown, skipping\n> > the \"mode change\" pseudo hunk.\n> >\n> > The comparison performed to skip the \"mode change\" pseudo hunk (if any)\n> > compares a signed integer `i` to the unsigned value `mode_change` (which\n> > can be 0 or 1 because it is a 1-bit type).\n> >\n> > According to section 6.3.1.8 of the C99 standard (see e.g.\n> > https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\n> > happen is an automatic conversion of the \"lesser\" type to the \"greater\"\n> > type, but since the types differ in signedness, it is ill-defined what\n> > is the correct \"usual arithmetic conversion\".\n> >\n> > Which means that Visual C's behavior can (and does) differ from GCC's:\n> > When compiling Git using the latter, `add -p`'s `goto` command shows no\n> > hunks by default because it casts a negative start offset to a pretty\n> > large unsigned value, breaking the \"goto hunk\" test case in\n> > `t3701-add-interactive.sh`.\n> >\n> > Let's avoid that by converting the unsigned bit explicitly to a signed\n> > integer.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n>\n> This looks more like a fix to a general problem, not limited to\n> windows or cmake, that we had since 9254bdfb (built-in add -p:\n> implement the 'g' (\"goto\") command, 2019-12-13).\n>\n> Please pull this out of the series and let's have it reviewed\n> separately.\n\nThe scope of this patch series is to fix running the tests in Visual\nStudio when building using CMake.\n\nPulling out this patch would break that patch series because it would\nleave that breakage in place.\n\nExcept if you are asking to put this patch series on the back burner and\nprioritize the patch that fixes an ambiguous implicit cast between signed\nand unsigned data types?\n\nHowever, that would mean that I'd now have to address all of those\nimplicit casts, which is unfortunately a larger amount of work than I can\njustify to take on.\n\nTherefore I move that in this instance, the perfect is the enemy of the\ngood, and that the patch should remain within this patch series, even if\nthe larger-scoped project to avoid any implicit signed/unsigned casts\nremains unaddressed.\n\nBTW I would have expected your review to ask the (in hindsight, obvious)\nquestion why the test suite still passes even with `vs-test` exercising\nthe code that is compiled using Visual C?\n\nThe answer to that would have been that the `vs-test` job of our CI runs\ndefines `NO_PERL`, and t3701 is skipped completely if that is the case.\n\nCiao,\nDscho\n"},{"id":"461271","messageId":"p1nn7o5q-9r34-n27o-s8r6-50277p3rp604@tzk.qr","threadId":"58284","inReplyTo":"ec8204d5-9d0d-9850-f930-6fc1a8efacc5@gmail.com","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-16T10:00:11Z","receivedAt":"2022-08-16T10:43:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 11 Aug 2022, Phillip Wood wrote:\n\n> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n>\n> > diff --git a/add-patch.c b/add-patch.c\n> > index 509ca04456b..3524555e2b0 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -1547,7 +1547,7 @@ soft_increment:\n> > strbuf_remove(&s->answer, 0, 1);\n> > strbuf_trim(&s->answer);\n>\n> Unrelated to this change but why don't we just call strbuf_reset() here?\n\nThis part of the code is used when the `g` command (to \"go to hunk\") was\nissued by the user. And that `g` command allows for a number to be\nspecified, e.g. `g1` to go to the first hunk.\n\nThe `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.\n\nThe `strbuf_trim(&s->answer)` allows for whitespace between the `g` and\nthe number, e.g. `g 1` should also go to the first hunk.\n\nIf we called `strbuf_reset()` here, we would remove the number completely.\n\nCiao,\nDscho\n"},{"id":"461272","messageId":"7ss9r585-14rs-so68-o2n3-9qn9qn530742@tzk.qr","threadId":"58284","inReplyTo":"xmqqczd8m1lr.fsf@gitster.g","subject":"Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-16T10:11:40Z","receivedAt":"2022-08-16T10:48:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 Aug 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > While at it, enable the command trace via `-x` and verbose output via\n> > `-v`, otherwise it would be near impossible to diagnose any problems.\n>\n> This sounds like a completely unrelated change.\n\nIt may sound like it, but it is not. In order to make sense of the broken\ntests, I needed access to more verbose output than our test scripts\nprovide by default.\n\nWhen running the test suite on the command-line, it is easy to tell the\nuser \"oh, if you need more information, just call the test script with\nthese here options: ...\".\n\nThis is not an option when running the tests within Visual Studio.\n\nDoes this clarify the intention and validity of the proposed patch? If so,\nI will gladly try my best to improve the commit message to explain that\nintention better.\n\nCiao,\nDscho\n"},{"id":"461273","messageId":"8o4q98s3-sr2r-34qq-p7pr-8o44061o0n76@tzk.qr","threadId":"58284","inReplyTo":"23d86b94-3e2e-bdeb-3385-a996c07b699d@gmail.com","subject":"Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-16T10:09:28Z","receivedAt":"2022-08-16T10:56:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 11 Aug 2022, Phillip Wood wrote:\n\n> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > In 7f5397a07c6c (cmake: support for testing git when building out of the\n> > source tree, 2020-06-26), we implemented support for running Git's test\n> > scripts even after building Git in a different directory than the source\n> > directory.\n> >\n> > The way we did this was to edit the file `t/test-lib.sh` to override\n> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> > directory.\n> >\n> > This is unideal because it always leaves a tracked file marked as\n> > modified, and it is all too easy to commit that change by mistake.\n> >\n> > Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> > exists, the contents are interpreted as the location to the _actual_\n> > build directory. We then write this file as part of the CTest\n> > definition.\n>\n> I think it is really good to get away from editing the test files, but one of\n> the nice things about CMake's out of tree builds is that you can have several\n> build directories with different build configurations and this change does not\n> support that. Could we pass the build directory to the test scripts as a\n> commandline option or environment variable instead? e.g.\n>\n> foreach(tsh ${test_scipts})\n> \tadd_test(NAME ${tsh}\n> -\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint\n> --build-dir=${CMAKE_BINARY_DIR} -vx\n>\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n>\n> Doing that would avoid changing the main Makefile to remove a file which\n> almost certainly does not exist every time make is invoked as well.\n\nThis is indeed tempting, but one of the things I recommend to Visual\nStudio users for ages already is to run the tests in a Git Bash, i.e.\noutside of Visual Studio, to allow for passing `--run=...` options and the\nlikes. This recommendation predates support for CTest, naturally, but it\nis still valid.\n\nAnd that recommended way of running tests would be broken by the suggested\nchange because the tests would no longer run except when using CTest.\n\nBesides, while the semantics look tempting, the implementation details do\nnot. The reason is that we use `GIT_BUILD_DIR` to source the\n`GIT-BUILD-OPTIONS` and to validate that Git was built, long before we\nparse the command-line in `t/test-lib.sh`. I want to refrain from the\nprerequisite extensive refactoring required to support this, at least in\nthe context of this here patch series.\n\nCan we maybe agree that the proposed patch is a net improvement over the\nstatus quo, and think about a better solution independently (without\nblocking this here patch)?\n\nCiao,\nDscho\n"},{"id":"461284","messageId":"9f8d808a-880c-fdd2-e915-d01c3f116cdc@gmail.com","threadId":"58284","inReplyTo":"p1nn7o5q-9r34-n27o-s8r6-50277p3rp604@tzk.qr","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-16T14:23:06Z","receivedAt":"2022-08-16T14:23:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 16/08/2022 11:00, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Thu, 11 Aug 2022, Phillip Wood wrote:\n> \n>> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n>>\n>>> diff --git a/add-patch.c b/add-patch.c\n>>> index 509ca04456b..3524555e2b0 100644\n>>> --- a/add-patch.c\n>>> +++ b/add-patch.c\n>>> @@ -1547,7 +1547,7 @@ soft_increment:\n>>> strbuf_remove(&s->answer, 0, 1);\n>>> strbuf_trim(&s->answer);\n>>\n>> Unrelated to this change but why don't we just call strbuf_reset() here?\n> \n> This part of the code is used when the `g` command (to \"go to hunk\") was\n> issued by the user. And that `g` command allows for a number to be\n> specified, e.g. `g1` to go to the first hunk.\n> \n> The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.\n> \n> The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and\n> the number, e.g. `g 1` should also go to the first hunk.\n> \n> If we called `strbuf_reset()` here, we would remove the number completely.\n\nOh so if the user is not using interactive.singleKey then they can skip \nthe prompt which displays the hunks and asks which one they want to jump \nto by guessing the number of the hunk they want and typing \"g \".\n\nThanks\n\nPhillip\n\n> Ciao,\n> Dscho\n"},{"id":"461285","messageId":"24bf3261-5012-d899-60c9-2c070d44c7fb@gmail.com","threadId":"58284","inReplyTo":"8o4q98s3-sr2r-34qq-p7pr-8o44061o0n76@tzk.qr","subject":"Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-16T14:27:32Z","receivedAt":"2022-08-16T14:27:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 16/08/2022 11:09, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Thu, 11 Aug 2022, Phillip Wood wrote:\n> \n>> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> In 7f5397a07c6c (cmake: support for testing git when building out of the\n>>> source tree, 2020-06-26), we implemented support for running Git's test\n>>> scripts even after building Git in a different directory than the source\n>>> directory.\n>>>\n>>> The way we did this was to edit the file `t/test-lib.sh` to override\n>>> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n>>> directory.\n>>>\n>>> This is unideal because it always leaves a tracked file marked as\n>>> modified, and it is all too easy to commit that change by mistake.\n>>>\n>>> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n>>> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n>>> exists, the contents are interpreted as the location to the _actual_\n>>> build directory. We then write this file as part of the CTest\n>>> definition.\n>>\n>> I think it is really good to get away from editing the test files, but one of\n>> the nice things about CMake's out of tree builds is that you can have several\n>> build directories with different build configurations and this change does not\n>> support that. Could we pass the build directory to the test scripts as a\n>> commandline option or environment variable instead? e.g.\n>>\n>> foreach(tsh ${test_scipts})\n>> \tadd_test(NAME ${tsh}\n>> -\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n>> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint\n>> --build-dir=${CMAKE_BINARY_DIR} -vx\n>>\n>> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n>> endforeach()\n>>\n>> Doing that would avoid changing the main Makefile to remove a file which\n>> almost certainly does not exist every time make is invoked as well.\n> \n> This is indeed tempting, but one of the things I recommend to Visual\n> Studio users for ages already is to run the tests in a Git Bash, i.e.\n> outside of Visual Studio, to allow for passing `--run=...` options and the\n> likes. This recommendation predates support for CTest, naturally, but it\n> is still valid.\n> \n> And that recommended way of running tests would be broken by the suggested\n> change because the tests would no longer run except when using CTest.\n\nYes the recommendation would have to include adding --build-dir which \nwould be a pain\n\n> Besides, while the semantics look tempting, the implementation details do\n> not. The reason is that we use `GIT_BUILD_DIR` to source the\n> `GIT-BUILD-OPTIONS` and to validate that Git was built, long before we\n> parse the command-line in `t/test-lib.sh`. I want to refrain from the\n> prerequisite extensive refactoring required to support this, at least in\n> the context of this here patch series.\n\nThat sounds awkward.\n\n> Can we maybe agree that the proposed patch is a net improvement over the\n> status quo, and think about a better solution independently (without\n> blocking this here patch)?\n\nIt is definitely an improvement for the CMake side, I'm not sure it is \nan improvement for the Makefile but it sounds like the best option overall.\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n"},{"id":"461288","messageId":"xmqqr11gxm04.fsf@gitster.g","threadId":"58284","inReplyTo":"r1461365-9np6-n988-0288-9ps4o944qqp4@tzk.qr","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-16T15:10:19Z","receivedAt":"2022-08-16T15:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The scope of this patch series is to fix running the tests in Visual\n> Studio when building using CMake.\n\nThat's only the perspective of who cares about VS+CMake. From my\npoint of view who wants a healthy Git overall, the priority is\ndifferent. \"add -p\" fix is wider than the context of VS, and I do\nnot discount the need that we need to fix it before we can call\nVS+CMake issue fixed (and I do not mean to say it is unnecessary to\nfix VS+CMake). It's just this one can proceed with help by those\nwho do not care about or have no environment to help with VS+CMake\nbecause it is more generic, and I do not think you mind to make the\nrest of the narrower VS+CMake topic _depend_ on it.\n\n> Pulling out this patch would break that patch series because it would\n> leave that breakage in place.\n\nWe deal with topic that depends on another topic all the time, and I\ndo not think there is anything that makes VS+CMake topic so special\nthat it cannot be dependent on another topic. It's just the matter\nof splitting this out and make it [1/1], and make the remainder to\n[1-4/4] and mark them rely on add-p fix when you send the latter\nout, isn't it? Puzzled...\n\nThanks.\n\n"},{"id":"461289","messageId":"xmqqmtc4xlr5.fsf@gitster.g","threadId":"58284","inReplyTo":"7ss9r585-14rs-so68-o2n3-9qn9qn530742@tzk.qr","subject":"Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-16T15:15:42Z","receivedAt":"2022-08-16T15:15:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Wed, 10 Aug 2022, Junio C Hamano wrote:\n>\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>\n>> > While at it, enable the command trace via `-x` and verbose output via\n>> > `-v`, otherwise it would be near impossible to diagnose any problems.\n>>\n>> This sounds like a completely unrelated change.\n>\n> It may sound like it, but it is not. In order to make sense of the broken\n> tests, I needed access to more verbose output than our test scripts\n> provide by default.\n>\n> When running the test suite on the command-line, it is easy to tell the\n> user \"oh, if you need more information, just call the test script with\n> these here options: ...\".\n>\n> This is not an option when running the tests within Visual Studio.\n\nI gave an example of \"CI environment it is cumbersome to go back and\nrun only a single one\" in the review you are responding to that may\nexplain why such a change is needed, and you gave us exactly the\ncontext that was lacking here. The environment does not let users\nrun the tests as anything but a single monolithic ball of wax.\n\n> Does this clarify the intention and validity of the proposed patch? If so,\n> I will gladly try my best to improve the commit message to explain that\n> intention better.\n\nIt explains why -x -v is needed and needs to be a part of VS+CMake\ntopic, and it will help readers to have it in the explanation. It\nstill does not justify why it has to be a part of a step to omit\nbin-warppers and skip chain-lint that has nothing to do it, though.\n\nThanks.\n"},{"id":"461550","messageId":"870o76sq-1056-n0q2-002p-3s2po0808335@tzk.qr","threadId":"58284","inReplyTo":"xmqqmtc4xlr5.fsf@gitster.g","subject":"Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-19T13:57:56Z","receivedAt":"2022-08-19T13:58:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 16 Aug 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > On Wed, 10 Aug 2022, Junio C Hamano wrote:\n> >\n> >> \"Johannes Schindelin via GitGitGadget\" \n> >> writes:\n> >>\n> >> > While at it, enable the command trace via `-x` and verbose output via\n> >> > `-v`, otherwise it would be near impossible to diagnose any problems.\n> >>\n> >> This sounds like a completely unrelated change.\n> >\n> > It may sound like it, but it is not. In order to make sense of the broken\n> > tests, I needed access to more verbose output than our test scripts\n> > provide by default.\n> >\n> > When running the test suite on the command-line, it is easy to tell the\n> > user \"oh, if you need more information, just call the test script with\n> > these here options: ...\".\n> >\n> > This is not an option when running the tests within Visual Studio.\n>\n> I gave an example of \"CI environment it is cumbersome to go back and\n> run only a single one\" in the review you are responding to that may\n> explain why such a change is needed, and you gave us exactly the\n> context that was lacking here. The environment does not let users\n> run the tests as anything but a single monolithic ball of wax.\n\nGood, I will amend the commit message.\n\n> > Does this clarify the intention and validity of the proposed patch? If so,\n> > I will gladly try my best to improve the commit message to explain that\n> > intention better.\n>\n> It explains why -x -v is needed and needs to be a part of VS+CMake\n> topic, and it will help readers to have it in the explanation. It\n> still does not justify why it has to be a part of a step to omit\n> bin-warppers and skip chain-lint that has nothing to do it, though.\n\nOh, that ;-)\n\nTo be honest, I reflexively do that on Windows because it saves _so much\ntime_.\n\nI will amend the commit message to clarify that, too.\n\nCiao,\nDscho\n"},{"id":"461551","messageId":"407ns96p-2n7n-rs60-40oo-81s02531q5q1@tzk.qr","threadId":"58284","inReplyTo":"9f8d808a-880c-fdd2-e915-d01c3f116cdc@gmail.com","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-19T14:07:23Z","receivedAt":"2022-08-19T14:07:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 16 Aug 2022, Phillip Wood wrote:\n\n> On 16/08/2022 11:00, Johannes Schindelin wrote:\n>\n> > On Thu, 11 Aug 2022, Phillip Wood wrote:\n> >\n> > > On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:\n> > >\n> > > > diff --git a/add-patch.c b/add-patch.c\n> > > > index 509ca04456b..3524555e2b0 100644\n> > > > --- a/add-patch.c\n> > > > +++ b/add-patch.c\n> > > > @@ -1547,7 +1547,7 @@ soft_increment:\n> > > > strbuf_remove(&s->answer, 0, 1);\n> > > > strbuf_trim(&s->answer);\n> > >\n> > > Unrelated to this change but why don't we just call strbuf_reset() here?\n> >\n> > This part of the code is used when the `g` command (to \"go to hunk\") was\n> > issued by the user. And that `g` command allows for a number to be\n> > specified, e.g. `g1` to go to the first hunk.\n> >\n> > The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.\n> >\n> > The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and\n> > the number, e.g. `g 1` should also go to the first hunk.\n> >\n> > If we called `strbuf_reset()` here, we would remove the number completely.\n>\n> Oh so if the user is not using interactive.singleKey then they can skip the\n> prompt which displays the hunks and asks which one they want to jump to by\n> guessing the number of the hunk they want and typing \"g \".\n\nYes, precisely.\n\nI forgot that you are using `interactive.singleKey` (I planned on opting\ninto that mode, but for some reason I never get around to flip the\nswitch). The default mode is not the single key mode, though.\n\nCiao,\nDscho\n"},{"id":"461554","messageId":"nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr","threadId":"58284","inReplyTo":"xmqqr11gxm04.fsf@gitster.g","subject":"Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-19T14:52:30Z","receivedAt":"2022-08-19T14:52:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 16 Aug 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > The scope of this patch series is to fix running the tests in Visual\n> > Studio when building using CMake.\n>\n> That's only the perspective of who cares about VS+CMake. From my\n> point of view who wants a healthy Git overall, the priority is\n> different. \"add -p\" fix is wider than the context of VS, and I do\n> not discount the need that we need to fix it before we can call\n> VS+CMake issue fixed (and I do not mean to say it is unnecessary to\n> fix VS+CMake). It's just this one can proceed with help by those\n> who do not care about or have no environment to help with VS+CMake\n> because it is more generic, and I do not think you mind to make the\n> rest of the narrower VS+CMake topic _depend_ on it.\n\nSure.\n\nBut if I pull out that patch into its code contribution, all of a sudden\nthe scope of _that_ contribution is to address an implicit signed/unsigned\ncast. What other purpose could it have? And if we're addressing that\nsigned/unsigned cast, we cannot just fix this single one. We need to fix\nthem all, no?\n\nSo let's have a look at that project, since you are implicitly\nvolunteering me for it. We do have this in our `config.mak.dev`:\n\n\t# These are disabled because we have these all over the place.\n\tDEVELOPER_CFLAGS += -Wno-empty-body\n\tDEVELOPER_CFLAGS += -Wno-missing-field-initializers\n\tDEVELOPER_CFLAGS += -Wno-sign-compare\n\tDEVELOPER_CFLAGS += -Wno-unused-parameter\n\tendif\n\nNote the `-Wno-sign-compare` part. If I comment that out, I get these\nreports:\n\n-- snip --\ndiff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]\ndiff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]\ndiff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]\ndiff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]\nadd-interactive.c:207:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]\nadd-interactive.c:210:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]\nadd-interactive.c:238:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]\nadd-patch.c:423:16: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]\nadd-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]\nadd-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]\n[... 1260 more issues ...]\n-- snap --\n\nYou see how that increases the amount of work you are asking me to do?\n\nThe worst part is that gcc (at least of version 9.4.0 which I have\navailable in my Ubuntu) does not even catch the line that is fixed by the\npatch we are trying to discuss here. It catches 10 issues in\n`add-patch.c`, but not the `i < file_diff->mode_change` one.\n\nNeither does Visual C 2022, nor the gcc I have in Git for Windows' SDK,\nwhich is v12.1.0.\n\nSo I would first need to find a tool that identifies all code locations\nthat compare signed with unsigned values.\n\nAnd that's even before analyzing carefully how to address them (not all\ninstances will be as easy as upcasting from an unsigned bit to `int`, some\nof those instances are about `size_t` vs `ssize_t`).\n\n> > Pulling out this patch would break that patch series because it would\n> > leave that breakage in place.\n>\n> We deal with topic that depends on another topic all the time, and I\n> do not think there is anything that makes VS+CMake topic so special\n> that it cannot be dependent on another topic. It's just the matter\n> of splitting this out and make it [1/1], and make the remainder to\n> [1-4/4] and mark them rely on add-p fix when you send the latter\n> out, isn't it? Puzzled...\n\nSure, if you think that the signed/unsigned comparison project is more\nimportant than fixing running Git's test suite inside Visual Studio, or\nworse: if you are asking me to do a less than thorough job on those\nsigned/unsigned fixes by fixing only a single instance and leaving all\nothers unaddressed in a patch series that has nothing to do with Visual\nStudio, then I understand how my stance could confuse you.\n\nBut my intention with this patch series is to fix running Git's test suite\nin Visual Studio. And my intention is to provide a complete solution in my\npatch series, no half measures.\n\nAs such, I would be loathe to have my authorship on a single patch that\nsolves neither the Visual Studio/CTest problem nor the vast majority of\nthe signed/unsigned problems. It would be incomplete in any way you look\nat it. I would consider such a contribution lackluster, sloppy and\ndefinitely not up to my standards.\n\nCiao,\nDscho\n"},{"id":"461749","messageId":"p79ppo86-022r-7sss-41q1-39n499n3266s@tzk.qr","threadId":"58284","inReplyTo":"220811.86wnbfowbg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-22T10:19:24Z","receivedAt":"2022-08-22T10:19:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> > [...]\n> > However, this quirk is only in effect as long as `chmod` is run inside\n> > the pseudo Unix root directory structure or within the home directory.\n> > When run outside, such invocations fail like this:\n> >\n> > \tchmod: changing permissions of '': Invalid argument\n>\n> ..ok, but...\n>\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > index 6da7273f1d5..7c63b22acab 100644\n> > --- a/t/test-lib-functions.sh\n> > +++ b/t/test-lib-functions.sh\n> > @@ -492,7 +492,10 @@ test_commit_bulk () {\n> > # of a file in the working directory and add it to the index.\n> >\n> > test_chmod () {\n> > -\tchmod \"$@\" &&\n> > +\tif test_have_prereq !MINGW\n> > +\tthen\n> > +\t\tchmod \"$@\"\n> > +\tfi &&\n> > \tgit update-index --add \"--chmod=$@\"\n> > }\n> >\n> > @@ -548,7 +551,10 @@ write_script () {\n> > \t\techo \"#!${2-\"$SHELL_PATH\"}\" &&\n> > \t\tcat\n> > \t} >\"$1\" &&\n> > -\tchmod +x \"$1\"\n> > +\tif test_have_prereq !MINGW\n> > +\tthen\n> > +\t\tchmod +x \"$1\"\n> > +\tfi\n>\n> ... you get +x semantics by default, so we didn't need that \"chmod +x\"\n> in the first place?\n\nNo. We do not get that `chmod +x` semantics by default. Those `chmod +x`\nstatements are treated as (expensive) no-ops by default. This is what I\nmeant when I said this in the commit message (that is missing from the\nquoted text above):\n\n\t[...] it pretends that it succeeded, when in reality it did not\n\tdo a thing [...]\n\nI do not know how to say this more clearly.\n\n> The rest of \"test_chmod\" seems to *happen to* pass +x or -x, but we\n> don't care about that, regardless of the \"pseudo Unix root directory\"?\n\nThe rest of \"test_chmod\" is even quoted above, so we do not need to leave\nanybody guessing as to what it does:\n\n\tgit update-index --add \"--chmod=$@\"\n\nThis asks Git to update the index with the executable bit explicitly\nturned on or off, regardless of the information that is available on disk.\n\nAnd yes, Git does what we expect it to do here.\n\n> What if we get a \"test_chmod -o \", won't this silently do the\n> wrong thing?\n\nBut we don't?\n\nThe code under discussion is Git's test suite, after all, not something\nthat every Git user is expected to use in more ways than the core Git\ndevelopers can imagine.\n\nSo even a cursory `git grep test_chmod upstream/seen` could have shown\nthat there is no such user, and if this was supposed to be a \"But what if\nwe do that at some stage in the future?\" feedback, said feedback could be\nconstrued as to intentionally use up valuable contributor time.\n\n> If so isn't something in this direction (untested) a more targeted &\n> obvious fix?:\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 10258def7be..1c3b6692388 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1690,6 +1690,16 @@ case $uname_s in\n> \tfind () {\n> \t\t/usr/bin/find \"$@\"\n> \t}\n> +\tchmod () {\n> +\t\tcase \"$1\" in\n> +\t\t+x|-x)\n> +\t\t\treturn;\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\t;;\n> +\t\tesac &&\n> +\t\t/usr/bin/chmod \"$@\"\n> +\t}\n> \t# git sees Windows-style pwd\n> \tpwd () {\n> \t\tbuiltin pwd -W\n>\n>\n\nIn that form, I will reject this suggestion as over-engineered and\nconvoluted. Why is the `*)` clause empty? Why is there an early return\nguarding a single statement? Why is the `/usr/bin/chmod` call not in the\n`*)` clause to begin with? Why does this code take pains to handle cases\nother than `-x` and `+x` when we do not have any callers, and even in the\nexperimental patches, there are no such users in sight? I would like to\nencourage you to address such issues during review before even sending the\nmail in the future.\n\nHaving said that, there is a nugget in this feedback that I find valuable.\nInstead of wasting the run time (even on non-Windows platforms!) to\ndetermine whether the `MINGW` prereq is set in order to skip the `chmod`\ncall or not, we can make `chmod` a no-op explicitly in that `case\n$uname_s` block.\n\nI will make it so.\n\nCiao,\nJohannes\n"},{"id":"461836","messageId":"2553p8p9-q04r-75r6-5spn-8n9npno6q8p3@tzk.qr","threadId":"58284","inReplyTo":"p79ppo86-022r-7sss-41q1-39n499n3266s@tzk.qr","subject":"Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-23T07:34:42Z","receivedAt":"2022-08-23T07:34:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 22 Aug 2022, Johannes Schindelin wrote:\n\n> Instead of wasting the run time (even on non-Windows platforms!) to\n> determine whether the `MINGW` prereq is set in order to skip the `chmod`\n> call or not, we can make `chmod` a no-op explicitly in that `case\n> $uname_s` block.\n>\n> I will make it so.\n\ntl;dr this patch needs to be dropped, without the suggested replacement.\n\nGaaah!\n\nAfter struggling with this for much longer than I care to admit, and even\ndebugging inside the MSYS2 runtime (which is a level boss if there ever\nwas one), I found out that all about this patch was wrong.\n\nThe suggested patch (making `chmod` a no-op) was wrong: the test suite\nstarted failing left and right. Why? Because `chmod` is not a _complete_\nno-op. The test suite does not use the `-o` flag you suggested (which\nwould not make sense in Git's test suite, whether or not you remove file\npermissions for \"others\" than the current user or group, Git behaves\nidentically), but it _does_ use the `-w`/`+w` flags, and those _are_\nrespected, even on Windows.\n\nThe way the write permission bits are translated to ACLs is admittedly\nsomewhat magical or all kinds of wrong, too, depending on your point of\nview.\n\nBut my original patch was also wrong. Why? Because it claimed that `chmod\n+x` does not work outside of MSYS2's pseudo Unix root directory tree. And\nthat's simply not true.\n\nWhether you call `chmod +x C:/Users/avar/my-file.txt` or `chmod +x\n/tmp/avar.sh`, it \"succeeds\" (by silently ignoring the flag that is\ninapplicable on Windows, whether or not a file is executable is determined\nby its file extension, and yes, that means that shell scripts are never\nexecutable and we have to live with a nasty hack in Git to pretend that\nthey are, based on their contents starting with a hash-bang line).\n\nSo why did I claim that the `chmod +x` invocation does not work outside of\nthat pseudo Unix root directory tree? Because it actually did not work, at\nleast for me! But it worked on the build agents. Why? Because I have a\ncommand in my `~/.profile` that mounts several of my Git for Windows SDKs\nas \"short-cuts\" like `/sdk64`, `/sdk32` and the likes (think of these as\nbind-mounts). And those `mount` commands did not specify the `noacl` flag.\n\nSo what's that `noacl` flag? It is magic, I can tell you. It basically\nmakes all this pretense work where MSYS2 pretends that we are in a Unix\nenvironment with Unix permissions when we're not actually. The details are\ntoo gnarly and involved to explain, I won't write them up here in order to\navoid putting even more readers to sleep than I must have done already.\n\nSo my analysis was based on observation instead of inspection, cutting\ncorners, and I dearly regret that now. By default, MSYS2 \"mounts\" the\nsystem drives as `/c`, `/d`, etc with that `noacl` flag, the same as it\nmounts the pseudo Unix root as `/`. And that is why the Git test suite\ndoes not throw up every time `chmod +x` is called. And it was a simple\npilot error on my part that caused it to fail on my system, and I did not\nrealize that the problem was confined to my system, and the bug was in my\n`~/.profile` and not in Git's code base.\n\nWorking on Git for Windows is never boring.\n\nCiao,\nJohannes\n"},{"id":"461837","messageId":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.git.1660143750.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T08:30:58Z","receivedAt":"2022-08-23T09:12:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Visual Studio users enjoy support for running the test suite via CTest,\nthanks to Git's CMake definition.\n\nIn https://github.com/git-for-windows/git/issues/3966, it has been reported\nthat this does not work out of the box, though, but causes a couple of test\nfailures instead. These problems are not caught by Git's CI runs because the\nvs-tests jobs actually use prove to run the test suite, not CTest.\n\nIn addition to fixing these problems, this patch series also addresses a\nlong-standing gripe I have with the way Git's CMake definition supports\nCTest: It edits t/test-lib.sh, which leaves this file eternally modified\n(but these modification should never be committed, they refer to a\nlocal-only, configuration-dependent directory).\n\nNote: The signed/unsigned comparison bug in git add -p that is fixed in this\nhere patch series is a relatively big one, and it merits further\ninvestigation whether there are similar bugs lurking in Git's code base.\nHowever, this is a much bigger project than can be addressed as part of this\npatch series, in particular because the analysis would require tools other\nthan GCC's -Wsign-compare option (which totally misses the instance that is\nfixed in this here patch series).\n\nChanges since v1:\n\n * Clarified why it is a good idea to pass --no-bin-wrappers and\n --no-chain-lint when running on Windows.\n * Clarified why the add -p bug has not been caught earlier.\n * Clarified the scope of this patch series to fix running Git's tests\n within Visual Studio.\n * Increased the time-out for the very slow t7112 test script.\n * The test_chmod was determined to be not only faulty, but unneeded, and\n was dropped.\n\nJohannes Schindelin (5):\n cmake: make it easier to diagnose regressions in CTest runs\n cmake: copy the merge tools for testing\n add -p: avoid ambiguous signed/unsigned comparison\n cmake: avoid editing t/test-lib.sh\n cmake: increase time-out for a long-running test\n\n .gitignore | 1 +\n Makefile | 1 +\n add-patch.c | 2 +-\n contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------\n t/test-lib.sh | 11 ++++++++++-\n 5 files changed, 21 insertions(+), 10 deletions(-)\n\n\nbase-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1320\n\nRange-diff vs v1:\n\n 1: 9cf14984c0a ! 1: e00cb37b98a cmake: align CTest definition with Git's CI runs\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - cmake: align CTest definition with Git's CI runs\n + cmake: make it easier to diagnose regressions in CTest runs\n \n - In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and\n - `--no-chain-lint`, mainly to win back some time caused by the serious\n - performance penalty paid for the tests relying so heavily on POSIX shell\n - scripting, which only works by using a POSIX emulation layer.\n + When a test script fails in Git's test suite, the usual course of action\n + is to re-run it using options to increase the verbosity of the output,\n + e.g. `-v` and `-x`.\n \n - Let's do the same when running the tests, say, in Visual Studio.\n + Like in Git's CI runs, when running the tests in Visual Studio via the\n + CTest route, it is cumbersome or at least requires a very unintuitive\n + approach to pass options to the test scripts.\n \n - While at it, enable the command trace via `-x` and verbose output via\n - `-v`, otherwise it would be near impossible to diagnose any problems.\n + So let's just pass those options by default: This will not clutter any\n + output window but the log that is written to a log file will have\n + information necessary to figure out test failures.\n +\n + While at it, also imitate what the Windows jobs in Git's CI runs do to\n + accelerate running the test scripts: pass the `--no-bin-wrappers` and\n + `--no-chain-lint` options. This makes the test runs noticeably faster\n + because the `bin-wrappers/` scripts as well as the `chain-lint` code\n + make heavy use of POSIX shell scripting, which is really, really slow on\n + Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n \n Signed-off-by: Johannes Schindelin \n \n 2: 86ab58b6508 = 2: de7b47a9aa7 cmake: copy the merge tools for testing\n 3: 79abfa82c32 < -: ----------- tests: explicitly skip `chmod` calls on Windows\n 4: 4d24a4345ba ! 3: f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison\n @@ Commit message\n Let's avoid that by converting the unsigned bit explicitly to a signed\n integer.\n \n + Note: This is a long-standing bug in the Visual C build of Git, but it\n + has never been caught because t3701 is skipped when `NO_PERL` is set,\n + which is the case in the `vs-test` jobs of Git's CI runs.\n +\n Signed-off-by: Johannes Schindelin \n \n ## add-patch.c ##\n 5: c7fc5a4ee4c = 4: 22473d6b8f3 cmake: avoid editing t/test-lib.sh\n -: ----------- > 5: 6aaa675301c cmake: increase time-out for a long-running test\n\n-- \ngitgitgadget\n"},{"id":"461838","messageId":"22473d6b8f3d4e4c482c27a4fb3b58705d4c93ca.1661243463.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T08:31:02Z","receivedAt":"2022-08-23T09:13:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 7f5397a07c6c (cmake: support for testing git when building out of the\nsource tree, 2020-06-26), we implemented support for running Git's test\nscripts even after building Git in a different directory than the source\ndirectory.\n\nThe way we did this was to edit the file `t/test-lib.sh` to override\n`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\ndirectory.\n\nThis is unideal because it always leaves a tracked file marked as\nmodified, and it is all too easy to commit that change by mistake.\n\nLet's change the strategy by teaching `t/test-lib.sh` to detect the\npresence of a file called `GIT-BUILD-DIR` in the source directory. If it\nexists, the contents are interpreted as the location to the _actual_\nbuild directory. We then write this file as part of the CTest\ndefinition.\n\nTo support building Git via a regular `make` invocation after building\nit using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\nconvenience, this is done as part of the Makefile rule that is already\nrun with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\nup to date).\n\nSigned-off-by: Johannes Schindelin \n---\n .gitignore | 1 +\n Makefile | 1 +\n contrib/buildsystems/CMakeLists.txt | 7 +------\n t/test-lib.sh | 11 ++++++++++-\n 4 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex a4522157641..b72ddf09346 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -2,6 +2,7 @@\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\n+/GIT-BUILD-DIR\n /GIT-BUILD-OPTIONS\n /GIT-CFLAGS\n /GIT-LDFLAGS\ndiff --git a/Makefile b/Makefile\nindex 04d0fd1fe60..9347ed90da7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3028,6 +3028,7 @@ else\n \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n endif\n \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n+\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n \n ### Detect Python interpreter path changes\n ifndef NO_PYTHON\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex fe606c179f7..29d7e236ae1 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1067,14 +1067,9 @@ endif()\n #Make the tests work when building out of the source tree\n get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n-\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n-\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n \t#Setting the build directory in test-lib.sh before running tests\n \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n-\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n-\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n-\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n-\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n+\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n \t#misc copies\n \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 55857af601b..4468ac51f25 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -42,7 +42,16 @@ then\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n-if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n+if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n+then\n+\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n+\t# On Windows, we must convert Windows paths lest they contain a colon\n+\tcase \"$(uname -s)\" in\n+\t*MINGW*)\n+\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n+\t\t;;\n+\tesac\n+elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n then\n \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n \texit 1\n-- \ngitgitgadget\n\n"},{"id":"461839","messageId":"6aaa675301cc56081d07974a0eb8662700fe96f8.1661243464.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] cmake: increase time-out for a long-running test","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T08:31:03Z","receivedAt":"2022-08-23T09:13:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs suggested in\nhttps://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,\nt7112 can run for well over one hour, which seems to be the default\nmaximum run time at least when running CTest-based tests in Visual\nStudio.\n\nLet's increase the time-out as a stop gap to unblock developers wishing\nto run Git's test suite in Visual Studio.\n\nNote: The actual run time is highly dependent on the circumstances. For\nexample, in Git's CI runs, the Windows-based tests typically take a bit\nover 5 minutes to run. CI runs have the added benefit that Windows\nDefender (the common anti-malware scanner on Windows) is turned off,\nsomething many developers are not at liberty to do on their work\nstations. When Defender is turned on, even on this developer's high-end\nRyzen system, t7112 takes over 15 minutes to run.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 29d7e236ae1..b1306f95256 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})\n \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n endforeach()\n \n+# This test script takes an extremely long time and is known to time out even\n+# on fast machines because it requires in excess of one hour to run\n+set_tests_properties(\"${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh\" PROPERTIES TIMEOUT 4000)\n+\n endif()#BUILD_TESTING\n-- \ngitgitgadget\n"},{"id":"461840","messageId":"de7b47a9aa7eb002412caac978485577b241c6b3.1661243463.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] cmake: copy the merge tools for testing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T08:31:00Z","receivedAt":"2022-08-23T09:13:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEven when running the tests via CTest, t7609 and t7610 rely on more than\nonly a few mergetools to be copied to the build directory. Let's make it\nso.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 4aee1e24342..fe606c179f7 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n \t#misc copies\n \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n-\tfile(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n+\tfile(GLOB mergetools \"${CMAKE_SOURCE_DIR}/mergetools/*\")\n+\tfile(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n endif()\n-- \ngitgitgadget\n\n"},{"id":"461841","messageId":"e00cb37b98ac09cff010e843ef19eeec761f8985.1661243463.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T08:30:59Z","receivedAt":"2022-08-23T09:13:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen a test script fails in Git's test suite, the usual course of action\nis to re-run it using options to increase the verbosity of the output,\ne.g. `-v` and `-x`.\n\nLike in Git's CI runs, when running the tests in Visual Studio via the\nCTest route, it is cumbersome or at least requires a very unintuitive\napproach to pass options to the test scripts.\n\nSo let's just pass those options by default: This will not clutter any\noutput window but the log that is written to a log file will have\ninformation necessary to figure out test failures.\n\nWhile at it, also imitate what the Windows jobs in Git's CI runs do to\naccelerate running the test scripts: pass the `--no-bin-wrappers` and\n`--no-chain-lint` options. This makes the test runs noticeably faster\nbecause the `bin-wrappers/` scripts as well as the `chain-lint` code\nmake heavy use of POSIX shell scripting, which is really, really slow on\nWindows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 1b23f2440d8..4aee1e24342 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n #test\n foreach(tsh ${test_scipts})\n \tadd_test(NAME ${tsh}\n-\t\tCOMMAND ${SH_EXE} ${tsh}\n+\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n endforeach()\n \n-- \ngitgitgadget\n\n"},{"id":"461842","messageId":"f96d5ab484c4971fce65f832185237ede0d37659.1661243463.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T08:31:01Z","receivedAt":"2022-08-23T09:13:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the interactive `add` operation, users can choose to jump to specific\nhunks, and Git will present the hunk list in that case. To avoid showing\ntoo many lines at once, only a maximum of 21 hunks are shown, skipping\nthe \"mode change\" pseudo hunk.\n\nThe comparison performed to skip the \"mode change\" pseudo hunk (if any)\ncompares a signed integer `i` to the unsigned value `mode_change` (which\ncan be 0 or 1 because it is a 1-bit type).\n\nAccording to section 6.3.1.8 of the C99 standard (see e.g.\nhttps://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\nhappen is an automatic conversion of the \"lesser\" type to the \"greater\"\ntype, but since the types differ in signedness, it is ill-defined what\nis the correct \"usual arithmetic conversion\".\n\nWhich means that Visual C's behavior can (and does) differ from GCC's:\nWhen compiling Git using the latter, `add -p`'s `goto` command shows no\nhunks by default because it casts a negative start offset to a pretty\nlarge unsigned value, breaking the \"goto hunk\" test case in\n`t3701-add-interactive.sh`.\n\nLet's avoid that by converting the unsigned bit explicitly to a signed\ninteger.\n\nNote: This is a long-standing bug in the Visual C build of Git, but it\nhas never been caught because t3701 is skipped when `NO_PERL` is set,\nwhich is the case in the `vs-test` jobs of Git's CI runs.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..3524555e2b0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1547,7 +1547,7 @@ soft_increment:\n \t\t\tstrbuf_remove(&s->answer, 0, 1);\n \t\t\tstrbuf_trim(&s->answer);\n \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n-\t\t\tif (i < file_diff->mode_change)\n+\t\t\tif (i < (int)file_diff->mode_change)\n \t\t\t\ti = file_diff->mode_change;\n \t\t\twhile (s->answer.len == 0) {\n \t\t\t\ti = display_hunks(s, file_diff, i);\n-- \ngitgitgadget\n\n"},{"id":"462769","messageId":"3df77ffd-85a2-3a54-9005-34a24ec6e82d@github.com","threadId":"58284","inReplyTo":"e00cb37b98ac09cff010e843ef19eeec761f8985.1661243463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-07T22:10:30Z","receivedAt":"2022-09-07T22:10:37Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When a test script fails in Git's test suite, the usual course of action\n> is to re-run it using options to increase the verbosity of the output,\n> e.g. `-v` and `-x`.\n> \n> Like in Git's CI runs, when running the tests in Visual Studio via the\n> CTest route, it is cumbersome or at least requires a very unintuitive\n> approach to pass options to the test scripts.\n\nAt first I wondered whether there's a way to make arg specification more\nintuitive, rather than silently changing defaults. Unfortunately, it looks\nlike even in the latest versions of CTest don't really support passing\narguments through to tests [1] (and the workarounds are unpleasant at best).\n\nBut then, you mentioned that there *is* a cumbersome/unintuitive approach to\npassing the options; what approach were you thinking?\n\n[1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470\n\n> \n> So let's just pass those options by default: This will not clutter any\n> output window but the log that is written to a log file will have\n> information necessary to figure out test failures.\n\nMakes sense, I don't see any harm in providing more verbose output by\ndefault here.\n\n> \n> While at it, also imitate what the Windows jobs in Git's CI runs do to\n> accelerate running the test scripts: pass the `--no-bin-wrappers` and\n> `--no-chain-lint` options. This makes the test runs noticeably faster\n> because the `bin-wrappers/` scripts as well as the `chain-lint` code\n> make heavy use of POSIX shell scripting, which is really, really slow on\n> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n\nI'm a bit more hesitant on including these. I see how the performance\nbenefit (on Windows in particular) would make typical user experience nicer.\nBut, if someone develops locally with '--no-chain-lint' specified, they'll\nbe much more likely to miss a broken && chain (personally, I get caught by\nchain lint errors *all the time* when I'm adding/updating tests) and cause\nunnecessary churn in their patch submissions. So, my recommendation would be\nto drop '--no-chain-lint' here (unless it's less important to the average\ndeveloper than I think it is).\n\nIt's also possible that '--no-bin-wrappers' does something weird with their\nlocal installation of Git but I think it's safe enough to make the default\nif the performance gain is substantial.\n\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 1b23f2440d8..4aee1e24342 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n> #test\n> foreach(tsh ${test_scipts})\n> \tadd_test(NAME ${tsh}\n> -\t\tCOMMAND ${SH_EXE} ${tsh}\n> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n\nAs you have it here, I don't think there's a way for a user to override\nthese defaults (unless there's something about the manual workaround you\nmentioned earlier that allows it). Since a user could feasibly want to set\ntheir own options, could you add a build variable to CMakeLists.txt like\n'GIT_TEST_OPTS'? You could use it to set the default options you have here,\nbut a user could still specify their own value at build time to override.\n\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n> \n\n"},{"id":"462784","messageId":"2313a3ee-8baf-3589-a90b-cd7ee3bf5dfa@github.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-08T03:51:55Z","receivedAt":"2022-09-08T03:52:03Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Johannes Schindelin via GitGitGadget wrote:\n> Visual Studio users enjoy support for running the test suite via CTest,\n> thanks to Git's CMake definition.\n> \n> In https://github.com/git-for-windows/git/issues/3966, it has been reported\n> that this does not work out of the box, though, but causes a couple of test\n> failures instead. These problems are not caught by Git's CI runs because the\n> vs-tests jobs actually use prove to run the test suite, not CTest.\n> \n> In addition to fixing these problems, this patch series also addresses a\n> long-standing gripe I have with the way Git's CMake definition supports\n> CTest: It edits t/test-lib.sh, which leaves this file eternally modified\n> (but these modification should never be committed, they refer to a\n> local-only, configuration-dependent directory).\n> \n> Note: The signed/unsigned comparison bug in git add -p that is fixed in this\n> here patch series is a relatively big one, and it merits further\n> investigation whether there are similar bugs lurking in Git's code base.\n> However, this is a much bigger project than can be addressed as part of this\n> patch series, in particular because the analysis would require tools other\n> than GCC's -Wsign-compare option (which totally misses the instance that is\n> fixed in this here patch series).\n> \n> Changes since v1:\n> \n> * Clarified why it is a good idea to pass --no-bin-wrappers and\n> --no-chain-lint when running on Windows.\n> * Clarified why the add -p bug has not been caught earlier.\n> * Clarified the scope of this patch series to fix running Git's tests\n> within Visual Studio.\n> * Increased the time-out for the very slow t7112 test script.\n> * The test_chmod was determined to be not only faulty, but unneeded, and\n> was dropped.\n> \n> Johannes Schindelin (5):\n> cmake: make it easier to diagnose regressions in CTest runs\n> cmake: copy the merge tools for testing\n> add -p: avoid ambiguous signed/unsigned comparison\n> cmake: avoid editing t/test-lib.sh\n> cmake: increase time-out for a long-running test\n\nI've reviewed patches 1-3 & 5 and started looking over 4, but I'd like to\nspend a bit more time on it (mostly to understand pros/cons of the\n'GIT-BUILD-DIR' vs. any alternatives). I'm planning to finish that up\ntomorrow.\n\nThanks for your patience!\n\n> \n> .gitignore | 1 +\n> Makefile | 1 +\n> add-patch.c | 2 +-\n> contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------\n> t/test-lib.sh | 11 ++++++++++-\n> 5 files changed, 21 insertions(+), 10 deletions(-)\n> \n> \n> base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1320\n> \n> Range-diff vs v1:\n> \n> 1: 9cf14984c0a ! 1: e00cb37b98a cmake: align CTest definition with Git's CI runs\n> @@ Metadata\n> Author: Johannes Schindelin \n> \n> ## Commit message ##\n> - cmake: align CTest definition with Git's CI runs\n> + cmake: make it easier to diagnose regressions in CTest runs\n> \n> - In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and\n> - `--no-chain-lint`, mainly to win back some time caused by the serious\n> - performance penalty paid for the tests relying so heavily on POSIX shell\n> - scripting, which only works by using a POSIX emulation layer.\n> + When a test script fails in Git's test suite, the usual course of action\n> + is to re-run it using options to increase the verbosity of the output,\n> + e.g. `-v` and `-x`.\n> \n> - Let's do the same when running the tests, say, in Visual Studio.\n> + Like in Git's CI runs, when running the tests in Visual Studio via the\n> + CTest route, it is cumbersome or at least requires a very unintuitive\n> + approach to pass options to the test scripts.\n> \n> - While at it, enable the command trace via `-x` and verbose output via\n> - `-v`, otherwise it would be near impossible to diagnose any problems.\n> + So let's just pass those options by default: This will not clutter any\n> + output window but the log that is written to a log file will have\n> + information necessary to figure out test failures.\n> +\n> + While at it, also imitate what the Windows jobs in Git's CI runs do to\n> + accelerate running the test scripts: pass the `--no-bin-wrappers` and\n> + `--no-chain-lint` options. This makes the test runs noticeably faster\n> + because the `bin-wrappers/` scripts as well as the `chain-lint` code\n> + make heavy use of POSIX shell scripting, which is really, really slow on\n> + Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n> \n> Signed-off-by: Johannes Schindelin \n> \n> 2: 86ab58b6508 = 2: de7b47a9aa7 cmake: copy the merge tools for testing\n> 3: 79abfa82c32 < -: ----------- tests: explicitly skip `chmod` calls on Windows\n> 4: 4d24a4345ba ! 3: f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison\n> @@ Commit message\n> Let's avoid that by converting the unsigned bit explicitly to a signed\n> integer.\n> \n> + Note: This is a long-standing bug in the Visual C build of Git, but it\n> + has never been caught because t3701 is skipped when `NO_PERL` is set,\n> + which is the case in the `vs-test` jobs of Git's CI runs.\n> +\n> Signed-off-by: Johannes Schindelin \n> \n> ## add-patch.c ##\n> 5: c7fc5a4ee4c = 4: 22473d6b8f3 cmake: avoid editing t/test-lib.sh\n> -: ----------- > 5: 6aaa675301c cmake: increase time-out for a long-running test\n> \n\n"},{"id":"462799","messageId":"220908.86illyl3yo.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"e00cb37b98ac09cff010e843ef19eeec761f8985.1661243463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-08T07:22:10Z","receivedAt":"2022-09-08T07:29:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> When a test script fails in Git's test suite, the usual course of action\n> is to re-run it using options to increase the verbosity of the output,\n> e.g. `-v` and `-x`.\n>\n> Like in Git's CI runs, when running the tests in Visual Studio via the\n> CTest route, it is cumbersome or at least requires a very unintuitive\n> approach to pass options to the test scripts.\n>\n> So let's just pass those options by default: This will not clutter any\n> output window but the log that is written to a log file will have\n> information necessary to figure out test failures.\n>\n> While at it, also imitate what the Windows jobs in Git's CI runs do to\n> accelerate running the test scripts: pass the `--no-bin-wrappers` and\n> `--no-chain-lint` options. This makes the test runs noticeably faster\n> because the `bin-wrappers/` scripts as well as the `chain-lint` code\n> make heavy use of POSIX shell scripting, which is really, really slow on\n> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 1b23f2440d8..4aee1e24342 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n> #test\n> foreach(tsh ${test_scipts})\n> \tadd_test(NAME ${tsh}\n> -\t\tCOMMAND ${SH_EXE} ${tsh}\n> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n\nThe -vx part of this looks sensible, and matches the commit message\n$subject.\n\nI think the \"--no-bin-wrappers --no-chain-lint\" and the \"while at it\"\nhere should be stripped out, and put into its own commit.\n\nWhich, as I commented on in the v1[1] is making the implicit assumption\nthat this cmake file is only used on Windows, but since 561962479c\n(cmake: fix CMakeLists.txt on Linux, 2022-05-24) that isn't the case.\n\nSo, perhaps we should have a performance hack due to Windows's slowness,\nbut:\n\n A. It should be guarded by some \"is windows?\" check. Your commit\n message justifies why this is a good idea on Windows, but completely\n elides over the fact that this isn't Windows-specific code anymore.\n\n B. We can still build wind \"make\" or \"cmake\", I don't see a reason for\n why such a perf hack (if we decide to keep it) should depend on the\n build system.\n\n C. Since I sent [1] we've had submitted chainlint.pl in-flight\n series. It's partly trying to take special considerations to be fast\n on Windows. I don't think it's the case with that series that this\n needs to be skipped on Windows anymore (and if it is, Eric would\n like to know).\n\n1. https://lore.kernel.org/git/220811.861qtnqb5p.gmgdl@evledraar.gmail.com/\n"},{"id":"462800","messageId":"220908.86edwml3hm.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"6aaa675301cc56081d07974a0eb8662700fe96f8.1661243464.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-08T07:34:28Z","receivedAt":"2022-09-08T07:39:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> As suggested in\n> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,\n> t7112 can run for well over one hour, which seems to be the default\n> maximum run time at least when running CTest-based tests in Visual\n> Studio.\n>\n> Let's increase the time-out as a stop gap to unblock developers wishing\n> to run Git's test suite in Visual Studio.\n>\n> Note: The actual run time is highly dependent on the circumstances. For\n> example, in Git's CI runs, the Windows-based tests typically take a bit\n> over 5 minutes to run. CI runs have the added benefit that Windows\n> Defender (the common anti-malware scanner on Windows) is turned off,\n> something many developers are not at liberty to do on their work\n> stations. When Defender is turned on, even on this developer's high-end\n> Ryzen system, t7112 takes over 15 minutes to run.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 29d7e236ae1..b1306f95256 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n> \n> +# This test script takes an extremely long time and is known to time out even\n> +# on fast machines because it requires in excess of one hour to run\n> +set_tests_properties(\"${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh\" PROPERTIES TIMEOUT 4000)\n> +\n> endif()#BUILD_TESTING\n\nI don't see per [1] that it would have any negative effect to just bump\nthe timeout a lot more, and for all the tests.\n\nIf we're running into 3600 seconds, then setting this to 4000 seconds\nseems to be a smal stopgap at best. That's just over a 10% increase, so\nif one person ran into it it 3600 someone with a slightly slower system\nshould be running into the same still, or if we just add a few more\ntests to 7112 (or some other slow test).\n\nSo why not set this ta 3600*10 or whatever for *all* of the test scripts\ninstead of playing whack-a-mole?\n\n1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html\n"},{"id":"462801","messageId":"220908.86a67al3c1.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"22473d6b8f3d4e4c482c27a4fb3b58705d4c93ca.1661243463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-08T07:39:38Z","receivedAt":"2022-09-08T07:42:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In 7f5397a07c6c (cmake: support for testing git when building out of the\n> source tree, 2020-06-26), we implemented support for running Git's test\n> scripts even after building Git in a different directory than the source\n> directory.\n>\n> The way we did this was to edit the file `t/test-lib.sh` to override\n> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> directory.\n>\n> This is unideal because it always leaves a tracked file marked as\n> modified, and it is all too easy to commit that change by mistake.\n>\n> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> exists, the contents are interpreted as the location to the _actual_\n> build directory. We then write this file as part of the CTest\n> definition.\n>\n> To support building Git via a regular `make` invocation after building\n> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> convenience, this is done as part of the Makefile rule that is already\n> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> up to date).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> contrib/buildsystems/CMakeLists.txt | 7 +------\n> t/test-lib.sh | 11 ++++++++++-\n> 4 files changed, 13 insertions(+), 7 deletions(-)\n>\n> diff --git a/.gitignore b/.gitignore\n> index a4522157641..b72ddf09346 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -2,6 +2,7 @@\n> /fuzz_corpora\n> /fuzz-pack-headers\n> /fuzz-pack-idx\n> +/GIT-BUILD-DIR\n> /GIT-BUILD-OPTIONS\n> /GIT-CFLAGS\n> /GIT-LDFLAGS\n> diff --git a/Makefile b/Makefile\n> index 04d0fd1fe60..9347ed90da7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3028,6 +3028,7 @@ else\n> \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> endif\n> \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n>\n> ### Detect Python interpreter path changes\n> ifndef NO_PYTHON\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index fe606c179f7..29d7e236ae1 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1067,14 +1067,9 @@ endif()\n> #Make the tests work when building out of the source tree\n> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> \t#Setting the build directory in test-lib.sh before running tests\n> \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> \t#misc copies\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 55857af601b..4468ac51f25 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -42,7 +42,16 @@ then\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> +then\n> +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> +\t# On Windows, we must convert Windows paths lest they contain a colon\n> +\tcase \"$(uname -s)\" in\n> +\t*MINGW*)\n> +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> +\t\t;;\n> +\tesac\n> +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> then\n> \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> \texit 1\n\nAs pointed out in the v1 this breaks the cmake<->make interaction in\nsome scenarios, but from some brief testing there seemed to be an easy\nworkaround which didn't suffer from that problem:\nhttps://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/\n"},{"id":"462816","messageId":"6bc8f50b-c869-11a7-b12e-dca8a2ce2a81@github.com","threadId":"58284","inReplyTo":"220908.86edwml3hm.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-08T17:29:35Z","receivedAt":"2022-09-08T17:30:36Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n> \n>> From: Johannes Schindelin \n>>\n>> As suggested in\n>> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,\n>> t7112 can run for well over one hour, which seems to be the default\n>> maximum run time at least when running CTest-based tests in Visual\n>> Studio.\n>>\n>> Let's increase the time-out as a stop gap to unblock developers wishing\n>> to run Git's test suite in Visual Studio.\n>>\n>> Note: The actual run time is highly dependent on the circumstances. For\n>> example, in Git's CI runs, the Windows-based tests typically take a bit\n>> over 5 minutes to run. CI runs have the added benefit that Windows\n>> Defender (the common anti-malware scanner on Windows) is turned off,\n>> something many developers are not at liberty to do on their work\n>> stations. When Defender is turned on, even on this developer's high-end\n>> Ryzen system, t7112 takes over 15 minutes to run.\n>>\n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> contrib/buildsystems/CMakeLists.txt | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n>> index 29d7e236ae1..b1306f95256 100644\n>> --- a/contrib/buildsystems/CMakeLists.txt\n>> +++ b/contrib/buildsystems/CMakeLists.txt\n>> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})\n>> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n>> endforeach()\n>> \n>> +# This test script takes an extremely long time and is known to time out even\n>> +# on fast machines because it requires in excess of one hour to run\n>> +set_tests_properties(\"${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh\" PROPERTIES TIMEOUT 4000)\n>> +\n>> endif()#BUILD_TESTING\n> \n> I don't see per [1] that it would have any negative effect to just bump\n> the timeout a lot more, and for all the tests.\n\nI'm a bit confused as to how that link (which explains that the 'TIMEOUT'\nproperty is used to determine when to kill a test process - more or less\nstandard behavior for timeouts in applications) shows that excessively long\ntimeouts on every test \"[wouldn't] have any negative effect.\" \n\nIn terms of the timeout length, the stated reason for adding the timeout in\nthe first place is:\n\n>> Let's increase the time-out as a stop gap to unblock developers wishing\n>> to run Git's test suite in Visual Studio.\n\nI would consider ~1 hour a manageable (albeit frustrating) amount of time to\nwait for a test, but not 10 hours (as you suggest later).\n\nAs for setting the timeout on all tests: that might be helpful, only because\n(AFAICT from CMake's source code & documentation) the default timeout is \"no\ntimeout.\" If setting a global default, though, I'd prefer a shorter timespan\n(e.g. 1800s) so that a timeout properly indicates a failure condition (\"this\ntest runs much longer than it should, so we may have introduced a\nperformance bug\"). \n\nPractically, though, I don't feel too strongly about whether or not to set a\nglobal default. If we wanted to add timeouts to more tests in the future,\nI'd want a more extensible solution with carefully-selected timeouts. But if\nit's just this one test running too long, special case-ing a timeout for it\nwith a comment explaining the reasoning seems appropriate to me. \n\n> \n> If we're running into 3600 seconds, then setting this to 4000 seconds\n> seems to be a smal stopgap at best. That's just over a 10% increase, so\n> if one person ran into it it 3600 someone with a slightly slower system\n> should be running into the same still, or if we just add a few more\n> tests to 7112 (or some other slow test).\n\nI don't think this patch is suggesting that timeout failures are happening\nat 3600s, rather that the peak observed execution time of the test is 3600s.\nIf that's the case, + an 11% buffer is a reasonable\nchoice for the value.\n\n> \n> So why not set this ta 3600*10 or whatever for *all* of the test scripts\n> instead of playing whack-a-mole?\n> \n> 1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html\n\n"},{"id":"462839","messageId":"531620e1-de4c-74aa-c840-c12ce81f8740@github.com","threadId":"58284","inReplyTo":"22473d6b8f3d4e4c482c27a4fb3b58705d4c93ca.1661243463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-08T23:37:44Z","receivedAt":"2022-09-08T23:38:17Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In 7f5397a07c6c (cmake: support for testing git when building out of the\n> source tree, 2020-06-26), we implemented support for running Git's test\n> scripts even after building Git in a different directory than the source\n> directory.\n> \n> The way we did this was to edit the file `t/test-lib.sh` to override\n> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> directory.\n> \n> This is unideal because it always leaves a tracked file marked as\n> modified, and it is all too easy to commit that change by mistake.\n> \n> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> exists, the contents are interpreted as the location to the _actual_\n> build directory. We then write this file as part of the CTest\n> definition.\n> \n> To support building Git via a regular `make` invocation after building\n> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> convenience, this is done as part of the Makefile rule that is already\n> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> up to date).\n\nWhile I like that this removes a user error case, it sacrifices some of the\nseparation between contrib/ and the main Git tree by adding logic to\n'test-lib.sh' that only really benefits the CMake build.\n\nTo your point in [1]:\n\n> Can we maybe agree that the proposed patch is a net improvement over the\n> status quo, and think about a better solution independently (without\n> blocking this here patch)?\n\nI don't think it does more harm than good, but I wouldn't go so far as to\ncall it a definitive \"net improvement.\" I'd personally very much prefer a\nsolution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of\nCMake. Unfortunately, after spending a pretty substantial amount of time\nlooking for an alternative, I couldn't think of anything that didn't either\n1) change how users ran tests or 2) change where CMake builds wrote Git's\nbinary files. \n\nSo, I could go either way on this patch - if others feel strongly that it\nshould be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless\nsomeone can think of a better alternative.\n\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> contrib/buildsystems/CMakeLists.txt | 7 +------\n> t/test-lib.sh | 11 ++++++++++-\n> 4 files changed, 13 insertions(+), 7 deletions(-)\n> \n> diff --git a/.gitignore b/.gitignore\n> index a4522157641..b72ddf09346 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -2,6 +2,7 @@\n> /fuzz_corpora\n> /fuzz-pack-headers\n> /fuzz-pack-idx\n> +/GIT-BUILD-DIR\n> /GIT-BUILD-OPTIONS\n> /GIT-CFLAGS\n> /GIT-LDFLAGS\n> diff --git a/Makefile b/Makefile\n> index 04d0fd1fe60..9347ed90da7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3028,6 +3028,7 @@ else\n> \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> endif\n> \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n> \n> ### Detect Python interpreter path changes\n> ifndef NO_PYTHON\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index fe606c179f7..29d7e236ae1 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1067,14 +1067,9 @@ endif()\n> #Make the tests work when building out of the source tree\n> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> \t#Setting the build directory in test-lib.sh before running tests\n> \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> \t#misc copies\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 55857af601b..4468ac51f25 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -42,7 +42,16 @@ then\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> +then\n> +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> +\t# On Windows, we must convert Windows paths lest they contain a colon\n> +\tcase \"$(uname -s)\" in\n> +\t*MINGW*)\n> +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> +\t\t;;\n> +\tesac\n> +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> then\n> \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> \texit 1\nReferring to Ævar's review in [1] - while I'm not overly concerned about the\n\"switching between make & CMake\" file staleness (if I'm not mistaken, the\nsame thing can happen now with the modified 'test-lib.sh', so this patch\ndoesn't really make anything worse), I do think the changes to 'test-lib.sh'\nshould be rearranged to preserve the \"PANIC\" check:\n\n----------------->8----------------->8----------------->8-----------------\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4468ac51f2..7b57f55c37 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -42,6 +42,11 @@ then\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n+if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n+then\n+\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n+\texit 1\n+fi\n if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n then\n \tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n@@ -51,10 +56,6 @@ then\n \t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n \t\t;;\n \tesac\n-elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n-then\n-\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n-\texit 1\n fi\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n-----------------8<-----------------8<-----------------8<-----------------\n\nOtherwise, a user could run the tests from outside a 't/' directory if they\nbuilt Git with CMake, which doesn't appear to be part of the intended\nbehavior of this patch.\n"},{"id":"462840","messageId":"dfd01ce9-bde6-9217-c2c4-556ddb69d7d7@github.com","threadId":"58284","inReplyTo":"531620e1-de4c-74aa-c840-c12ce81f8740@github.com","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-08T23:42:32Z","receivedAt":"2022-09-08T23:42:38Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Victoria Dye wrote:\n> Johannes Schindelin via GitGitGadget wrote:\n>> From: Johannes Schindelin \n>>\n>> In 7f5397a07c6c (cmake: support for testing git when building out of the\n>> source tree, 2020-06-26), we implemented support for running Git's test\n>> scripts even after building Git in a different directory than the source\n>> directory.\n>>\n>> The way we did this was to edit the file `t/test-lib.sh` to override\n>> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n>> directory.\n>>\n>> This is unideal because it always leaves a tracked file marked as\n>> modified, and it is all too easy to commit that change by mistake.\n>>\n>> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n>> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n>> exists, the contents are interpreted as the location to the _actual_\n>> build directory. We then write this file as part of the CTest\n>> definition.\n>>\n>> To support building Git via a regular `make` invocation after building\n>> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n>> convenience, this is done as part of the Makefile rule that is already\n>> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n>> up to date).\n> \n> While I like that this removes a user error case, it sacrifices some of the\n> separation between contrib/ and the main Git tree by adding logic to\n> 'test-lib.sh' that only really benefits the CMake build.\n> \n> To your point in [1]:\n\nForgot this link: https://lore.kernel.org/git/8o4q98s3-sr2r-34qq-p7pr-8o44061o0n76@tzk.qr/\n\n> \n>> Can we maybe agree that the proposed patch is a net improvement over the\n>> status quo, and think about a better solution independently (without\n>> blocking this here patch)?\n> \n> I don't think it does more harm than good, but I wouldn't go so far as to\n> call it a definitive \"net improvement.\" I'd personally very much prefer a\n> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of\n> CMake. Unfortunately, after spending a pretty substantial amount of time\n> looking for an alternative, I couldn't think of anything that didn't either\n> 1) change how users ran tests or 2) change where CMake builds wrote Git's\n> binary files. \n> \n> So, I could go either way on this patch - if others feel strongly that it\n> should be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless\n> someone can think of a better alternative.\n> \n...\n\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 55857af601b..4468ac51f25 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -42,7 +42,16 @@ then\n>> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n>> fi\n>> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n>> -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n>> +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n>> +then\n>> +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n>> +\t# On Windows, we must convert Windows paths lest they contain a colon\n>> +\tcase \"$(uname -s)\" in\n>> +\t*MINGW*)\n>> +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n>> +\t\t;;\n>> +\tesac\n>> +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n>> then\n>> \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n>> \texit 1\n> Referring to Ævar's review in [1] - while I'm not overly concerned about the\n\nAND this one (which should be [2]): https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/\n\nSorry about that!\n\n> \"switching between make & CMake\" file staleness (if I'm not mistaken, the\n> same thing can happen now with the modified 'test-lib.sh', so this patch\n> doesn't really make anything worse), I do think the changes to 'test-lib.sh'\n> should be rearranged to preserve the \"PANIC\" check:\n> \n> ----------------->8----------------->8----------------->8-----------------\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 4468ac51f2..7b57f55c37 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -42,6 +42,11 @@ then\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> +if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> +then\n> +\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> +\texit 1\n> +fi\n> if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> then\n> \tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> @@ -51,10 +56,6 @@ then\n> \t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> \t\t;;\n> \tesac\n> -elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> -then\n> -\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> -\texit 1\n> fi\n> \n> # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n> -----------------8<-----------------8<-----------------8<-----------------\n> \n> Otherwise, a user could run the tests from outside a 't/' directory if they\n> built Git with CMake, which doesn't appear to be part of the intended\n> behavior of this patch.\n\n"},{"id":"462841","messageId":"xmqq35d1o1ts.fsf@gitster.g","threadId":"58284","inReplyTo":"531620e1-de4c-74aa-c840-c12ce81f8740@github.com","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-08T23:58:55Z","receivedAt":"2022-09-08T23:59:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> I don't think it does more harm than good, but I wouldn't go so far as to\n> call it a definitive \"net improvement.\" I'd personally very much prefer a\n> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of\n> CMake.\n> ...\n> ... \"switching between make & CMake\" file staleness (if I'm not mistaken, the\n> same thing can happen now with the modified 'test-lib.sh', so this patch\n> doesn't really make anything worse), I do think the changes to 'test-lib.sh'\n> should be rearranged to preserve the \"PANIC\" check:\n\nThanks for a quality review.\n"},{"id":"463795","messageId":"CAPig+cQTxVvBtKymLeWkDtRSBqejjydP6o-PfDriQHP-M+cShg@mail.gmail.com","threadId":"58284","inReplyTo":"220908.86illyl3yo.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Eric Sunshine","fromEmail":"ericsunshine@gmail.com","sentAt":"2022-09-28T06:55:24Z","receivedAt":"2022-09-28T06:56:43Z","isPatch":true,"sender":{"key":"ericsunshine@gmail.com","avatar":null},"body":"On Thu, Sep 8, 2022 at 3:28 AM Ævar Arnfjörð Bjarmason wrote:\n> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n> > When a test script fails in Git's test suite, the usual course of action\n> > is to re-run it using options to increase the verbosity of the output,\n> > e.g. `-v` and `-x`.\n> > [...]\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> > + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n>\n> I think the \"--no-bin-wrappers --no-chain-lint\" and the \"while at it\"\n> here should be stripped out, and put into its own commit.\n>\n> So, perhaps we should have a performance hack due to Windows's slowness,\n> but:\n>\n> C. Since I sent [1] we've had submitted chainlint.pl in-flight\n> series. It's partly trying to take special considerations to be fast\n> on Windows. I don't think it's the case with that series that this\n> needs to be skipped on Windows anymore (and if it is, Eric would\n> like to know).\n\nI had the opportunity to test chainlint.pl on an 8-core machine using\nboth Linux and Windows 10. The machine is more than a few years old,\nthough not nearly as old as my own development machine. Although\nchainlint.pl checked all test definitions in the entire project in\nless than one _second_ on Linux, it took two _minutes_ on Windows 10\non the same machine (using all 8 cores). Despite the fact that Perl is\nrun just once to check all test definitions -- unlike chainlint.sed\nrunning 'sed' tens of thousands of times, chainlint.pl is still\npainfully slow on Windows, so I certainly understand the desire to use\n--no-chain-lint, at least on Windows.\n"},{"id":"465180","messageId":"pull.1320.v3.git.1666090745.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v2.git.1661243463.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] Some fixes and an improvement for using CTest on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T10:59:00Z","receivedAt":"2022-10-18T10:59:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Visual Studio users enjoy support for running the test suite via CTest,\nthanks to Git's CMake definition.\n\nIn https://github.com/git-for-windows/git/issues/3966, it has been reported\nthat this does not work out of the box, though, but causes a couple of test\nfailures instead. These problems are not caught by Git's CI runs because the\nvs-tests jobs actually use prove to run the test suite, not CTest.\n\nIn addition to fixing these problems, this patch series also addresses a\nlong-standing gripe I have with the way Git's CMake definition supports\nCTest: It edits t/test-lib.sh, which leaves this file eternally modified\n(but these modification should never be committed, they refer to a\nlocal-only, configuration-dependent directory).\n\nNote: The signed/unsigned comparison bug in git add -p that is fixed in this\nhere patch series is a relatively big one, and it merits further\ninvestigation whether there are similar bugs lurking in Git's code base.\nHowever, this is a much bigger project than can be addressed as part of this\npatch series, in particular because the analysis would require tools other\nthan GCC's -Wsign-compare option (which totally misses the instance that is\nfixed in this here patch series).\n\nChanges since v2:\n\n * Enhanced a commit message to clarify how the \"cumbersome approach\" looks\n like when a developer wishes to pass options to the test scripts in\n Visual Studio.\n * Changed the base branch to ac/fuzzers, to avoid merge conflicts in\n .gitignore.\n * Moved the PANIC check in test-lib.sh so that it is not skipped when the\n file GIT-BUILD-DIR exists.\n\nChanges since v1:\n\n * Clarified why it is a good idea to pass --no-bin-wrappers and\n --no-chain-lint when running on Windows.\n * Clarified why the add -p bug has not been caught earlier.\n * Clarified the scope of this patch series to fix running Git's tests\n within Visual Studio.\n * Increased the time-out for the very slow t7112 test script.\n * The test_chmod was determined to be not only faulty, but unneeded, and\n was dropped.\n\nJohannes Schindelin (5):\n cmake: make it easier to diagnose regressions in CTest runs\n cmake: copy the merge tools for testing\n add -p: avoid ambiguous signed/unsigned comparison\n cmake: avoid editing t/test-lib.sh\n cmake: increase time-out for a long-running test\n\n .gitignore | 1 +\n Makefile | 1 +\n add-patch.c | 2 +-\n contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------\n t/test-lib.sh | 10 ++++++++++\n 5 files changed, 21 insertions(+), 9 deletions(-)\n\n\nbase-commit: 6713bfc70c4dc6da1fa4084f000b72f5d74fecfb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1320\n\nRange-diff vs v2:\n\n 1: e00cb37b98a ! 1: 356b2e9a100 cmake: make it easier to diagnose regressions in CTest runs\n @@ Commit message\n \n Like in Git's CI runs, when running the tests in Visual Studio via the\n CTest route, it is cumbersome or at least requires a very unintuitive\n - approach to pass options to the test scripts.\n + approach to pass options to the test scripts: the CMakeLists.txt file\n + would have to be modified, passing the desired options to _all_ test\n + scripts, and then the CMake Cache would have to be reconfigured before\n + running the test in question individually. Unintuitive at best, and\n + opposite to the niceties IDE users expect.\n \n So let's just pass those options by default: This will not clutter any\n output window but the log that is written to a log file will have\n @@ Commit message\n \n While at it, also imitate what the Windows jobs in Git's CI runs do to\n accelerate running the test scripts: pass the `--no-bin-wrappers` and\n - `--no-chain-lint` options. This makes the test runs noticeably faster\n - because the `bin-wrappers/` scripts as well as the `chain-lint` code\n - make heavy use of POSIX shell scripting, which is really, really slow on\n - Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n + `--no-chain-lint` options.\n +\n + This makes the test runs noticeably faster because the `bin-wrappers/`\n + scripts as well as the `chain-lint` code make heavy use of POSIX shell\n + scripting, which is really, really slow on Windows due to the need to\n + emulate POSIX behavior via the MSYS2 runtime. In a test by Eric\n + Sunshine, it added two minutes (!) just to perform the chain-lint task.\n +\n + The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was\n + considered during the development of this patch, but then dropped: such\n + a setting is global, across _all_ tests, where e.g. `--run=...` would\n + not make sense. Users wishing to override these new defaults are better\n + advised running the test script manually, in a Git Bash, with full\n + control over the command line.\n \n Signed-off-by: Johannes Schindelin \n \n 2: de7b47a9aa7 = 2: 9faca9d5cbe cmake: copy the merge tools for testing\n 3: f96d5ab484c = 3: 41a8021a4bd add -p: avoid ambiguous signed/unsigned comparison\n 4: 22473d6b8f3 ! 4: 5b0c2a150e9 cmake: avoid editing t/test-lib.sh\n @@ Commit message\n ## .gitignore ##\n @@\n /fuzz_corpora\n - /fuzz-pack-headers\n - /fuzz-pack-idx\n +/GIT-BUILD-DIR\n /GIT-BUILD-OPTIONS\n /GIT-CFLAGS\n @@ contrib/buildsystems/CMakeLists.txt: endif()\n \n ## t/test-lib.sh ##\n @@ t/test-lib.sh: then\n - \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n + \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n + \texit 1\n fi\n - GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n --if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n +then\n +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n @@ t/test-lib.sh: then\n +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n +\t\t;;\n +\tesac\n -+elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n - then\n - \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n - \texit 1\n ++fi\n + \n + # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n + # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n 5: 6aaa675301c = 5: 40cf872f483 cmake: increase time-out for a long-running test\n\n-- \ngitgitgadget\n"},{"id":"465181","messageId":"356b2e9a1007bcd1382f26f333926ff0d5b9abe2.1666090745.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v3.git.1666090745.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T10:59:01Z","receivedAt":"2022-10-18T10:59:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen a test script fails in Git's test suite, the usual course of action\nis to re-run it using options to increase the verbosity of the output,\ne.g. `-v` and `-x`.\n\nLike in Git's CI runs, when running the tests in Visual Studio via the\nCTest route, it is cumbersome or at least requires a very unintuitive\napproach to pass options to the test scripts: the CMakeLists.txt file\nwould have to be modified, passing the desired options to _all_ test\nscripts, and then the CMake Cache would have to be reconfigured before\nrunning the test in question individually. Unintuitive at best, and\nopposite to the niceties IDE users expect.\n\nSo let's just pass those options by default: This will not clutter any\noutput window but the log that is written to a log file will have\ninformation necessary to figure out test failures.\n\nWhile at it, also imitate what the Windows jobs in Git's CI runs do to\naccelerate running the test scripts: pass the `--no-bin-wrappers` and\n`--no-chain-lint` options.\n\nThis makes the test runs noticeably faster because the `bin-wrappers/`\nscripts as well as the `chain-lint` code make heavy use of POSIX shell\nscripting, which is really, really slow on Windows due to the need to\nemulate POSIX behavior via the MSYS2 runtime. In a test by Eric\nSunshine, it added two minutes (!) just to perform the chain-lint task.\n\nThe idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was\nconsidered during the development of this patch, but then dropped: such\na setting is global, across _all_ tests, where e.g. `--run=...` would\nnot make sense. Users wishing to override these new defaults are better\nadvised running the test script manually, in a Git Bash, with full\ncontrol over the command line.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 2237109b57f..6ac20bc5054 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n #test\n foreach(tsh ${test_scipts})\n \tadd_test(NAME ${tsh}\n-\t\tCOMMAND ${SH_EXE} ${tsh}\n+\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n endforeach()\n \n-- \ngitgitgadget\n\n"},{"id":"465182","messageId":"9faca9d5cbe706c00d03ca2a125f2e113294a548.1666090745.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v3.git.1666090745.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] cmake: copy the merge tools for testing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T10:59:02Z","receivedAt":"2022-10-18T10:59:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEven when running the tests via CTest, t7609 and t7610 rely on more than\nonly a few mergetools to be copied to the build directory. Let's make it\nso.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 6ac20bc5054..0c741e7d878 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n \t#misc copies\n \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n-\tfile(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n+\tfile(GLOB mergetools \"${CMAKE_SOURCE_DIR}/mergetools/*\")\n+\tfile(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n endif()\n-- \ngitgitgadget\n\n"},{"id":"465183","messageId":"41a8021a4bd09e1470d04ec11d392777d4f5824f.1666090745.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v3.git.1666090745.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T10:59:03Z","receivedAt":"2022-10-18T10:59:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the interactive `add` operation, users can choose to jump to specific\nhunks, and Git will present the hunk list in that case. To avoid showing\ntoo many lines at once, only a maximum of 21 hunks are shown, skipping\nthe \"mode change\" pseudo hunk.\n\nThe comparison performed to skip the \"mode change\" pseudo hunk (if any)\ncompares a signed integer `i` to the unsigned value `mode_change` (which\ncan be 0 or 1 because it is a 1-bit type).\n\nAccording to section 6.3.1.8 of the C99 standard (see e.g.\nhttps://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\nhappen is an automatic conversion of the \"lesser\" type to the \"greater\"\ntype, but since the types differ in signedness, it is ill-defined what\nis the correct \"usual arithmetic conversion\".\n\nWhich means that Visual C's behavior can (and does) differ from GCC's:\nWhen compiling Git using the latter, `add -p`'s `goto` command shows no\nhunks by default because it casts a negative start offset to a pretty\nlarge unsigned value, breaking the \"goto hunk\" test case in\n`t3701-add-interactive.sh`.\n\nLet's avoid that by converting the unsigned bit explicitly to a signed\ninteger.\n\nNote: This is a long-standing bug in the Visual C build of Git, but it\nhas never been caught because t3701 is skipped when `NO_PERL` is set,\nwhich is the case in the `vs-test` jobs of Git's CI runs.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..3524555e2b0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1547,7 +1547,7 @@ soft_increment:\n \t\t\tstrbuf_remove(&s->answer, 0, 1);\n \t\t\tstrbuf_trim(&s->answer);\n \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n-\t\t\tif (i < file_diff->mode_change)\n+\t\t\tif (i < (int)file_diff->mode_change)\n \t\t\t\ti = file_diff->mode_change;\n \t\t\twhile (s->answer.len == 0) {\n \t\t\t\ti = display_hunks(s, file_diff, i);\n-- \ngitgitgadget\n\n"},{"id":"465184","messageId":"5b0c2a150e9fce1ca0284d65628b42ed5a7aad9a.1666090745.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v3.git.1666090745.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T10:59:04Z","receivedAt":"2022-10-18T10:59:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 7f5397a07c6c (cmake: support for testing git when building out of the\nsource tree, 2020-06-26), we implemented support for running Git's test\nscripts even after building Git in a different directory than the source\ndirectory.\n\nThe way we did this was to edit the file `t/test-lib.sh` to override\n`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\ndirectory.\n\nThis is unideal because it always leaves a tracked file marked as\nmodified, and it is all too easy to commit that change by mistake.\n\nLet's change the strategy by teaching `t/test-lib.sh` to detect the\npresence of a file called `GIT-BUILD-DIR` in the source directory. If it\nexists, the contents are interpreted as the location to the _actual_\nbuild directory. We then write this file as part of the CTest\ndefinition.\n\nTo support building Git via a regular `make` invocation after building\nit using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\nconvenience, this is done as part of the Makefile rule that is already\nrun with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\nup to date).\n\nSigned-off-by: Johannes Schindelin \n---\n .gitignore | 1 +\n Makefile | 1 +\n contrib/buildsystems/CMakeLists.txt | 7 +------\n t/test-lib.sh | 10 ++++++++++\n 4 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex 73df7295795..89ad7e68b4b 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,4 +1,5 @@\n /fuzz_corpora\n+/GIT-BUILD-DIR\n /GIT-BUILD-OPTIONS\n /GIT-CFLAGS\n /GIT-LDFLAGS\ndiff --git a/Makefile b/Makefile\nindex 88178c5b466..886614340c7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3029,6 +3029,7 @@ else\n \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n endif\n \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n+\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n \n ### Detect Python interpreter path changes\n ifndef NO_PYTHON\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 0c741e7d878..1d8cebb4cfe 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1067,14 +1067,9 @@ endif()\n #Make the tests work when building out of the source tree\n get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n-\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n-\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n \t#Setting the build directory in test-lib.sh before running tests\n \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n-\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n-\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n-\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n-\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n+\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n \t#misc copies\n \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 120f11812c3..dfc0144ed3b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -47,6 +47,16 @@ then\n \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n \texit 1\n fi\n+if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n+then\n+\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n+\t# On Windows, we must convert Windows paths lest they contain a colon\n+\tcase \"$(uname -s)\" in\n+\t*MINGW*)\n+\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n+\t\t;;\n+\tesac\n+fi\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n-- \ngitgitgadget\n\n"},{"id":"465185","messageId":"40cf872f48386f8eca0fa814e4cdfb0ded915ed8.1666090745.git.gitgitgadget@gmail.com","threadId":"58284","inReplyTo":"pull.1320.v3.git.1666090745.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] cmake: increase time-out for a long-running test","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T10:59:05Z","receivedAt":"2022-10-18T10:59:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs suggested in\nhttps://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,\nt7112 can run for well over one hour, which seems to be the default\nmaximum run time at least when running CTest-based tests in Visual\nStudio.\n\nLet's increase the time-out as a stop gap to unblock developers wishing\nto run Git's test suite in Visual Studio.\n\nNote: The actual run time is highly dependent on the circumstances. For\nexample, in Git's CI runs, the Windows-based tests typically take a bit\nover 5 minutes to run. CI runs have the added benefit that Windows\nDefender (the common anti-malware scanner on Windows) is turned off,\nsomething many developers are not at liberty to do on their work\nstations. When Defender is turned on, even on this developer's high-end\nRyzen system, t7112 takes over 15 minutes to run.\n\nSigned-off-by: Johannes Schindelin \n---\n contrib/buildsystems/CMakeLists.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 1d8cebb4cfe..7e0d040e0f6 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})\n \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n endforeach()\n \n+# This test script takes an extremely long time and is known to time out even\n+# on fast machines because it requires in excess of one hour to run\n+set_tests_properties(\"${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh\" PROPERTIES TIMEOUT 4000)\n+\n endif()#BUILD_TESTING\n-- \ngitgitgadget\n"},{"id":"465194","messageId":"221018.865yghi6nv.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"356b2e9a1007bcd1382f26f333926ff0d5b9abe2.1666090745.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T13:41:43Z","receivedAt":"2022-10-18T13:49:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> When a test script fails in Git's test suite, the usual course of action\n> is to re-run it using options to increase the verbosity of the output,\n> e.g. `-v` and `-x`.\n>\n> Like in Git's CI runs, when running the tests in Visual Studio via the\n> CTest route, it is cumbersome or at least requires a very unintuitive\n> approach to pass options to the test scripts: the CMakeLists.txt file\n> would have to be modified, passing the desired options to _all_ test\n> scripts, and then the CMake Cache would have to be reconfigured before\n> running the test in question individually. Unintuitive at best, and\n> opposite to the niceties IDE users expect.\n>\n> So let's just pass those options by default: This will not clutter any\n> output window but the log that is written to a log file will have\n> information necessary to figure out test failures.\n>\n> While at it, also imitate what the Windows jobs in Git's CI runs do to\n> accelerate running the test scripts: pass the `--no-bin-wrappers` and\n> `--no-chain-lint` options.\n>\n> This makes the test runs noticeably faster because the `bin-wrappers/`\n> scripts as well as the `chain-lint` code make heavy use of POSIX shell\n> scripting, which is really, really slow on Windows due to the need to\n> emulate POSIX behavior via the MSYS2 runtime. In a test by Eric\n> Sunshine, it added two minutes (!) just to perform the chain-lint task.\n>\n> The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was\n> considered during the development of this patch, but then dropped: such\n> a setting is global, across _all_ tests, where e.g. `--run=...` would\n> not make sense. Users wishing to override these new defaults are better\n> advised running the test script manually, in a Git Bash, with full\n> control over the command line.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 2237109b57f..6ac20bc5054 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n> #test\n> foreach(tsh ${test_scipts})\n> \tadd_test(NAME ${tsh}\n> -\t\tCOMMAND ${SH_EXE} ${tsh}\n> +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n> \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> endforeach()\n\nThis all seems sensible, but where you lose me is why this needs to be\ncmake-specific.\n\nMaybe I'm just missing something, but everything you're describing seems\nto me to be Windows-specific, e.g. \"chain lint is slow on windows\" and\nthe like.\n\nWhy aren't we just declaring that it's not worth it on Windows, but\ndoing so in test-lib.sh, or at least doing it in both cmake's recipe &\nthe Makefile?\n\nI can see why we'd e.g. want it not to run with \"make test\", but want it\nwhen you run individual tests, but then both could just set some \"this\nis running as a batch\" env flag or whatever.\n\nAlso, even then everything you're describing is eliding that:\n\n - This ill makes sense for Windows, sure, but the cmake recipe is no\n longer Windows-specific, was this authored before that happened &\n rebased? Why do we want cmake/ctest on Linux to have --no-chain-lint?\n\n - Having cmake be some \"mostly the Makefile, but also other defaults\"\n is really changing the scope of the whole cmake build alternative,\n which until now (but I may have missed some other special-cases) has\n been \"let's build it like the Makefile\".\n\n - That \"ctest\" doesn't work on Linux *at all* currently, this fixes it,\n but is really sweeping the underlying issue under the rug. AFAICT\n it's been limping along because there's no \"chmod +x\" or whatever on\n Windows (IIRC we magically look at she shebang).\n\nFor that last one & more I hacked up\nhttps://github.com/avar/git/tree/avar/cmake-test-path a few days ago as\nan alternative to some parts of this series, but given past exchanges\nwanted to ask you first what you thought about it off-list, but didn't\nget around to it..\n\nBut anyway, I do think starting with something like\nhttps://github.com/avar/git/commit/5bd0ee2bc826d2bb358af9d93c88ba28584bbda1\nmakes more sense, i.e. get \"ctest\" to work at all with the\nbin-wrappers/* in a portable way, and then *maybe* tweak it/Makefile to\npass some custom flags on some platform(s).\n"},{"id":"465195","messageId":"221018.861qr5i6fy.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"9faca9d5cbe706c00d03ca2a125f2e113294a548.1666090745.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/5] cmake: copy the merge tools for testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T13:49:13Z","receivedAt":"2022-10-18T13:54:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> Even when running the tests via CTest, t7609 and t7610 rely on more than\n> only a few mergetools to be copied to the build directory. Let's make it\n> so.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> contrib/buildsystems/CMakeLists.txt | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 6ac20bc5054..0c741e7d878 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> \t#misc copies\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> -\tfile(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n> +\tfile(GLOB mergetools \"${CMAKE_SOURCE_DIR}/mergetools/*\")\n> +\tfile(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n> endif()\n\nI mentioned that I hacked up a \"root cause fix\" for some issues in cmake\nin 1/5. Continuing that this works, but is really just a\nhack-for-the-hack fix, where we should fix the root cause:\n\nWhich actually, is really simple:\nhttps://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245\n\nAs you know we fake up the $GIT_BUILD_DIR for cmake, but then later in\nthe recipe (i.e. here) we have to scurry around and adjust that because\nfor some stuff we mean contrib/buildsystems/out/, whereas in\nothers we mean the original .\n\nSo here (and this isn't on you, we had this before) if you run \"cmake\"\nand e.g. tweak the mergetools sources we'll need to re-copy them, blah\nblah blah.\n\nBut we can just ... not copy it at all, and have the test-lib.sh learn\nthat some stuff it needs from whatever the \"build dir\" is, but other\nstuff is *always* relative to the root of the project.\n\nSo, for most of that & the context it simply goes away, we don't need to\ncopy po/is.po, the tests can just know where to find it, ditto\nmergetools/tkdiff (which is not generated), and the bash completion.\n\n"},{"id":"465196","messageId":"221018.86wn8xgrum.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"41a8021a4bd09e1470d04ec11d392777d4f5824f.1666090745.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T13:53:45Z","receivedAt":"2022-10-18T13:54:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In the interactive `add` operation, users can choose to jump to specific\n> hunks, and Git will present the hunk list in that case. To avoid showing\n> too many lines at once, only a maximum of 21 hunks are shown, skipping\n> the \"mode change\" pseudo hunk.\n>\n> The comparison performed to skip the \"mode change\" pseudo hunk (if any)\n> compares a signed integer `i` to the unsigned value `mode_change` (which\n> can be 0 or 1 because it is a 1-bit type).\n>\n> According to section 6.3.1.8 of the C99 standard (see e.g.\n> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\n> happen is an automatic conversion of the \"lesser\" type to the \"greater\"\n> type, but since the types differ in signedness, it is ill-defined what\n> is the correct \"usual arithmetic conversion\".\n>\n> Which means that Visual C's behavior can (and does) differ from GCC's:\n> When compiling Git using the latter, `add -p`'s `goto` command shows no\n> hunks by default because it casts a negative start offset to a pretty\n> large unsigned value, breaking the \"goto hunk\" test case in\n> `t3701-add-interactive.sh`.\n>\n> Let's avoid that by converting the unsigned bit explicitly to a signed\n> integer.\n>\n> Note: This is a long-standing bug in the Visual C build of Git, but it\n> has never been caught because t3701 is skipped when `NO_PERL` is set,\n> which is the case in the `vs-test` jobs of Git's CI runs.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..3524555e2b0 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1547,7 +1547,7 @@ soft_increment:\n> \t\t\tstrbuf_remove(&s->answer, 0, 1);\n> \t\t\tstrbuf_trim(&s->answer);\n> \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n> -\t\t\tif (i < file_diff->mode_change)\n> +\t\t\tif (i < (int)file_diff->mode_change)\n> \t\t\t\ti = file_diff->mode_change;\n> \t\t\twhile (s->answer.len == 0) {\n> \t\t\t\ti = display_hunks(s, file_diff, i);\n\nJunio pointed out in the last round that this really could use a\nseparate submission, and I tend to agree. It's really nothing to do with\ncmake...\n"},{"id":"465197","messageId":"221018.86sfjlgr54.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"5b0c2a150e9fce1ca0284d65628b42ed5a7aad9a.1666090745.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T13:54:25Z","receivedAt":"2022-10-18T14:09:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In 7f5397a07c6c (cmake: support for testing git when building out of the\n> source tree, 2020-06-26), we implemented support for running Git's test\n> scripts even after building Git in a different directory than the source\n> directory.\n>\n> The way we did this was to edit the file `t/test-lib.sh` to override\n> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> directory.\n>\n> This is unideal because it always leaves a tracked file marked as\n> modified, and it is all too easy to commit that change by mistake.\n>\n> Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> exists, the contents are interpreted as the location to the _actual_\n> build directory. We then write this file as part of the CTest\n> definition.\n>\n> To support building Git via a regular `make` invocation after building\n> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> convenience, this is done as part of the Makefile rule that is already\n> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> up to date).\n>\n> Signed-off-by: Johannes Schindelin \n\nRe my earlier feedback, I came up with this as an alternative, which\nnicely allows us to have \"cmake\" and \"make\" play together, you can even\nrun them concurrently!:\n\n\thttps://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245\n\nIn case that OID changes it's on my\nhttps://github.com/avar/git/commits/avar/cmake-test-path branch,\ncurrently 30f2265fd07 (cmake & test-lib.sh: add a $GIT_SOURCE_DIR\nvariable, 2022-10-14).\n\nAnd it...\n\n> diff --git a/Makefile b/Makefile\n> index 88178c5b466..886614340c7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3029,6 +3029,7 @@ else\n> \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> endif\n> \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n\n...allows us to get rid of this, which you understandably need with your\napproach, but which I'd *really* prefer we not have. Let's not sneak\nthings into make's dependency DAG that it doesn't know about in FORCE'd\nshell command (but more on that later).\n\n> ### Detect Python interpreter path changes\n> ifndef NO_PYTHON\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 0c741e7d878..1d8cebb4cfe 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1067,14 +1067,9 @@ endif()\n> #Make the tests work when building out of the source tree\n> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> \t#Setting the build directory in test-lib.sh before running tests\n> \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> \t#misc copies\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n\n...and this whole section just goes away, we don't need any\ncmake-specifi hacking here, and actually it's not cmake-specific at\nall. It's just a \"GIT_TEST_INSTALLED for things that are built, not\ninstalled\". E.g.:\n\n (cd g/git.scratch && make)\n (cd g/git && make clean && GIT_TEST_BUILD_DIR=\"$PWD/../git.scratch\" make -C t)\n\nSupporting cmake then just becomes a special-case of test-lib.sh knowing\n\"hey, my built stuff is at instead of \"../\".\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 120f11812c3..dfc0144ed3b 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -47,6 +47,16 @@ then\n> \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> \texit 1\n> fi\n> +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> +then\n> +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> +\t# On Windows, we must convert Windows paths lest they contain a colon\n> +\tcase \"$(uname -s)\" in\n> +\t*MINGW*)\n> +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> +\t\t;;\n> +\tesac\n> +fi\n\n...but one thing that I migh thave missed (and would really appreciate\nyour testing for) is that I didn't invoke cygpath in my version. CI\npasses, but since Windows CI doesn't use \"ctest\" that doesn't tell us\nmuch, and in any case that's Cygwin, no, which we don't run anyway\nthere?\n\nAnyway, we could run that \"cypath\" easily in the cmake recipe itself, or\njust pass a \"hey, please make this canonical\" flag to test-lib.sh.\n\nBut anyway, one thing that approach explicitly leaves out is that you\nwant to be able to:\n\n 1. Build with cmake\n 2. cd t\n 3. Run a test\n\nAnd have the test itself know to locate and use the cmake binaries\ninstead of the \"main\" binaries.\n\nNow, I suspect that we don't actually have cases anyone cares about\nwhere we have *both*, but that's how this code behaves. I.e. a\ntop-level:\n\n\tmake test\n\nWill wpe that GIT-BUILD-DIR and use the \"make\" built \"git\", but e.g.:\n\n\tmake\n\t\n\tcd t\n\t# At this point I forgot I used cmake earlier\n\t./t0001-init.sh # silently uses cmake...\n\nI can see thy case for auto-discovery, per the IDE case you mentioned,\nbut isn't it much better to just make this part of the slightly later\npart (but we need to set it up here now) part where we discover the\nbuilt \"git\" and:\n\n A. Do we not have it in ../git?\n B. Do we have it it contrib/buildsystems/out/git\n\nThen (pseudocode):\n\n\tif (!A && B)\n\t\tuse_cmake();\n\telse if (A && B)\n\t\tdie(\"you have both, pick one!\");\n\nOr just say that \"make\" entry points always run with stuff it built, and\n\"ctest\" runs with contrib/buildsystems/out/git, that's explicitly what\nyou don't want though...\n\nAnyway, to wrap this up, I really wish the interaction between the two\nwouldn't have these pitfalls. I get that you want to support it on the\nspecific Windows IDE case, but can't we more narrowlry do that without:\n\n\t(cd t && ./t0001-init.sh)\n\nHaving this new \"pick either one\" behavior? Cheers.\n\t\t\n\n\n"},{"id":"465199","messageId":"29r609np-s0r1-r904-16o3-6918930o2ror@tzk.qr","threadId":"58284","inReplyTo":"3df77ffd-85a2-3a54-9005-34a24ec6e82d@github.com","subject":"Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-10-18T14:02:50Z","receivedAt":"2022-10-18T14:48:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Victoria,\n\nOn Wed, 7 Sep 2022, Victoria Dye wrote:\n\n> Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > When a test script fails in Git's test suite, the usual course of action\n> > is to re-run it using options to increase the verbosity of the output,\n> > e.g. `-v` and `-x`.\n> >\n> > Like in Git's CI runs, when running the tests in Visual Studio via the\n> > CTest route, it is cumbersome or at least requires a very unintuitive\n> > approach to pass options to the test scripts.\n>\n> At first I wondered whether there's a way to make arg specification more\n> intuitive, rather than silently changing defaults. Unfortunately, it looks\n> like even in the latest versions of CTest don't really support passing\n> arguments through to tests [1] (and the workarounds are unpleasant at best).\n>\n> But then, you mentioned that there *is* a cumbersome/unintuitive approach to\n> passing the options; what approach were you thinking?\n\nThank you for pointing out that I assumed this to be more obvious than it\nactually is.\n\nThe cumbersome way is to edit this part of the CMake definition\n(https://github.com/git/git/blob/v2.38.0/contrib/buildsystems/CMakeLists.txt#L1091-L1096):\n\n\t#test\n\tforeach(tsh ${test_scipts})\n\t\tadd_test(NAME ${tsh}\n\t\t\tCOMMAND ${SH_EXE} ${tsh}\n\t\t\tWORKING_DIRECTORY\n\t\t\t${CMAKE_SOURCE_DIR}/t)\n\tendforeach()\n\nTo pass an option like `-v --run=1,29`, the user would have to edit the\nline `COMMAND ${SH_EXE} ${tsh}`, appending the options, and then\nre-configure the CMake cache. And then not forget to _only_ run the test\nscript in question because those options do not make sense for the other\ntests. And then not forget to undo the changes and re-configure the CMake\ncache.\n\n> [1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470\n>\n> >\n> > So let's just pass those options by default: This will not clutter any\n> > output window but the log that is written to a log file will have\n> > information necessary to figure out test failures.\n>\n> Makes sense, I don't see any harm in providing more verbose output by\n> default here.\n>\n> >\n> > While at it, also imitate what the Windows jobs in Git's CI runs do to\n> > accelerate running the test scripts: pass the `--no-bin-wrappers` and\n> > `--no-chain-lint` options. This makes the test runs noticeably faster\n> > because the `bin-wrappers/` scripts as well as the `chain-lint` code\n> > make heavy use of POSIX shell scripting, which is really, really slow on\n> > Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.\n>\n> I'm a bit more hesitant on including these. I see how the performance\n> benefit (on Windows in particular) would make typical user experience nicer.\n> But, if someone develops locally with '--no-chain-lint' specified, they'll\n> be much more likely to miss a broken && chain (personally, I get caught by\n> chain lint errors *all the time* when I'm adding/updating tests) and cause\n> unnecessary churn in their patch submissions. So, my recommendation would be\n> to drop '--no-chain-lint' here (unless it's less important to the average\n> developer than I think it is).\n>\n> It's also possible that '--no-bin-wrappers' does something weird with their\n> local installation of Git but I think it's safe enough to make the default\n> if the performance gain is substantial.\n>\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > contrib/buildsystems/CMakeLists.txt | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> > index 1b23f2440d8..4aee1e24342 100644\n> > --- a/contrib/buildsystems/CMakeLists.txt\n> > +++ b/contrib/buildsystems/CMakeLists.txt\n> > @@ -1088,7 +1088,7 @@ file(GLOB test_scipts \"${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh\")\n> > #test\n> > foreach(tsh ${test_scipts})\n> > \tadd_test(NAME ${tsh}\n> > -\t\tCOMMAND ${SH_EXE} ${tsh}\n> > +\t\tCOMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx\n>\n> As you have it here, I don't think there's a way for a user to override\n> these defaults (unless there's something about the manual workaround you\n> mentioned earlier that allows it). Since a user could feasibly want to set\n> their own options, could you add a build variable to CMakeLists.txt like\n> 'GIT_TEST_OPTS'? You could use it to set the default options you have here,\n> but a user could still specify their own value at build time to override.\n\nIndeed, there is no way to override this, unless the user edits the\n(Git-tracked) `CMakeLists.txt` file and then re-configures the cache.\n\nIn my experience, the most common scenario where a developer needs to pass\noptions to test script is when they need to restrict execution to\nindividual test cases via, say, `--run=1,29`.\n\nSuch a use case cannot be covered via the CMake approach because it is not\nfine-grained enough: any options the user could set would apply to the\nentire test suite.\n\nI added a comment to the commit message advising users to run the test\nscript manually via Git Bash instead, if they wish to pass a different set\nof command-line options to the test script.\n\nThanks,\nDscho\n\n>\n> > \t\tWORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)\n> > endforeach()\n> >\n>\n>\n"},{"id":"465200","messageId":"3q98po6o-561o-0r9n-qsqp-1052oqpo889r@tzk.qr","threadId":"58284","inReplyTo":"220811.86sfm3ov5z.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-10-18T14:02:57Z","receivedAt":"2022-10-18T14:48:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > In 7f5397a07c6c (cmake: support for testing git when building out of the\n> > source tree, 2020-06-26), we implemented support for running Git's test\n> > scripts even after building Git in a different directory than the source\n> > directory.\n> >\n> > The way we did this was to edit the file `t/test-lib.sh` to override\n> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> > directory.\n> >\n> > This is unideal because it always leaves a tracked file marked as\n> > modified, and it is all too easy to commit that change by mistake.\n> >\n> > Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> > exists, the contents are interpreted as the location to the _actual_\n> > build directory. We then write this file as part of the CTest\n> > definition.\n> >\n> > To support building Git via a regular `make` invocation after building\n> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> > convenience, this is done as part of the Makefile rule that is already\n> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> > up to date).\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > .gitignore | 1 +\n> > Makefile | 1 +\n> > contrib/buildsystems/CMakeLists.txt | 7 +------\n> > t/test-lib.sh | 11 ++++++++++-\n> > 4 files changed, 13 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/.gitignore b/.gitignore\n> > index a4522157641..b72ddf09346 100644\n> > --- a/.gitignore\n> > +++ b/.gitignore\n> > @@ -2,6 +2,7 @@\n> > /fuzz_corpora\n> > /fuzz-pack-headers\n> > /fuzz-pack-idx\n> > +/GIT-BUILD-DIR\n> > /GIT-BUILD-OPTIONS\n> > /GIT-CFLAGS\n> > /GIT-LDFLAGS\n> > diff --git a/Makefile b/Makefile\n> > index 04d0fd1fe60..9347ed90da7 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -3028,6 +3028,7 @@ else\n> > \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> > endif\n> > \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> > +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n> >\n> > ### Detect Python interpreter path changes\n> > ifndef NO_PYTHON\n> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> > index fe606c179f7..29d7e236ae1 100644\n> > --- a/contrib/buildsystems/CMakeLists.txt\n> > +++ b/contrib/buildsystems/CMakeLists.txt\n> > @@ -1067,14 +1067,9 @@ endif()\n> > #Make the tests work when building out of the source tree\n> > get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> > if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> > -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> > -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> > \t#Setting the build directory in test-lib.sh before running tests\n> > \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> > -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> > -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> > -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> > -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> > +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> > \t#misc copies\n> > \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> > \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 55857af601b..4468ac51f25 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -42,7 +42,16 @@ then\n> > \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> > fi\n> > GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> > -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> > +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> > +then\n> > +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> > +\t# On Windows, we must convert Windows paths lest they contain a colon\n> > +\tcase \"$(uname -s)\" in\n> > +\t*MINGW*)\n> > +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> > +\t\t;;\n> > +\tesac\n> > +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> > then\n> > \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> > \texit 1\n>\n> So we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct\n> and assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests\n> run inside a \"t\" directory generated in contrib/buildsystems/ ?\n>\n> This really seems like a hack-upon-hack, and will break some workflows\n> when you switch back & forth, e.g.:\n>\n> \tmake\n> \t# run cmake\n> \tmake -C t\n>\n> Will now pick up this new file, and result in broken tests, as we'll\n> rely on the now-stale file.\n>\n> IOW you're making the assumption that by piggy-backing on the\n> GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run\n> it*, but that's not the case.\n>\n> I can think of ways to get around it, but it would be nasty as we'd need\n> to complete the dependency graph between the two, and figure out the\n> various interactions.\n>\n> But why do we need this at all? Right now I tried:\n>\n> * (Manually) removing the existing hack\n> * Copying t/*lib*.sh over to contrib/buildsystems/t\n> * cd contrib/buildsystems/t\n> * ../../../t/t0001-init.sh\n\nI appreciate that you want to make it easier for developers. From my\nexperience with (Windows) developers, I have to point out that what you\ndescribe is _not_ easier than `cd t && ./t0001-init.sh`, though.\n\nIt might be easier from some point of view that seems vaguely familiar,\nbut the user experience of this approach looks prohibitively bad to me.\n\n> Which (aside from a small fixable nit about oid-info) Just Works,\n> because the cmake build is already creating a GIT-BUILD-OPTIONS.\n>\n> Presumably that \"copying\" step should be simlynking, or we'd be smarter\n> about doing includes from test-lib.sh.\n>\n> But isn't that a much better approach? Rather than working around the\n> directory discovery just teach it to run from the generated t/ directory\n> directly?\n\nI am sure I could poke more holes into that approach, probably whipping\nout something something about `contrib/`, but the user experience alone\n(so, dear developer, contrary to what you read in the README and in other\ndocumentation flying all over the internet, yes, you need to switch to\n`contrib/buildsystems`, no wait, there's a typo, there's no dash between\n`build` and `systems`, there you go, why `buildsystems`? Well, it's just\nthe way it is. Yes. Now, type out, no, no, wait, *wait!*, you cannot run\n`t0006-date.sh` directly as documented, no, wait, you need to prefix it\nwith this here, and now carefully count the dots, yep, that's right, six\ndots in total, three slashes, then a `t/`, yes, I know you were already in\n`t/` but not _that_ `t/`, there are now two of 'em, yes, it is\nunintuitive, you're absolutely correct, but you know...) is enough for me\nto not want to consider this approach any further.\n\nCiao,\nJohannes\n"},{"id":"465201","messageId":"851094n0-2o53-77n8-41o6-773q479q5sqq@tzk.qr","threadId":"58284","inReplyTo":"220908.86a67al3c1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-10-18T14:03:04Z","receivedAt":"2022-10-18T14:48:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 8 Sep 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > In 7f5397a07c6c (cmake: support for testing git when building out of the\n> > source tree, 2020-06-26), we implemented support for running Git's test\n> > scripts even after building Git in a different directory than the source\n> > directory.\n> >\n> > The way we did this was to edit the file `t/test-lib.sh` to override\n> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> > directory.\n> >\n> > This is unideal because it always leaves a tracked file marked as\n> > modified, and it is all too easy to commit that change by mistake.\n> >\n> > Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> > exists, the contents are interpreted as the location to the _actual_\n> > build directory. We then write this file as part of the CTest\n> > definition.\n> >\n> > To support building Git via a regular `make` invocation after building\n> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> > convenience, this is done as part of the Makefile rule that is already\n> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> > up to date).\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > .gitignore | 1 +\n> > Makefile | 1 +\n> > contrib/buildsystems/CMakeLists.txt | 7 +------\n> > t/test-lib.sh | 11 ++++++++++-\n> > 4 files changed, 13 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/.gitignore b/.gitignore\n> > index a4522157641..b72ddf09346 100644\n> > --- a/.gitignore\n> > +++ b/.gitignore\n> > @@ -2,6 +2,7 @@\n> > /fuzz_corpora\n> > /fuzz-pack-headers\n> > /fuzz-pack-idx\n> > +/GIT-BUILD-DIR\n> > /GIT-BUILD-OPTIONS\n> > /GIT-CFLAGS\n> > /GIT-LDFLAGS\n> > diff --git a/Makefile b/Makefile\n> > index 04d0fd1fe60..9347ed90da7 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -3028,6 +3028,7 @@ else\n> > \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n> > endif\n> > \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> > +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n> >\n> > ### Detect Python interpreter path changes\n> > ifndef NO_PYTHON\n> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> > index fe606c179f7..29d7e236ae1 100644\n> > --- a/contrib/buildsystems/CMakeLists.txt\n> > +++ b/contrib/buildsystems/CMakeLists.txt\n> > @@ -1067,14 +1067,9 @@ endif()\n> > #Make the tests work when building out of the source tree\n> > get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n> > if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> > -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n> > -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n> > \t#Setting the build directory in test-lib.sh before running tests\n> > \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n> > -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n> > -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n> > -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> > -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> > +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n> > \t#misc copies\n> > \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> > \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 55857af601b..4468ac51f25 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -42,7 +42,16 @@ then\n> > \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> > fi\n> > GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> > -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> > +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> > +then\n> > +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> > +\t# On Windows, we must convert Windows paths lest they contain a colon\n> > +\tcase \"$(uname -s)\" in\n> > +\t*MINGW*)\n> > +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> > +\t\t;;\n> > +\tesac\n> > +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> > then\n> > \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> > \texit 1\n>\n> As pointed out in the v1 this breaks the cmake<->make interaction in\n> some scenarios, but from some brief testing there seemed to be an easy\n> workaround which didn't suffer from that problem:\n> https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/\n\nI do not think that the CMake <-> make interaction will come up in any\nother scenario than your and my tests in the context of this mailing list\nthread.\n\nTherefore, I am certain that we need not cater to that scenario at all.\n\nCiao,\nJohannes\n"},{"id":"465202","messageId":"s9r33841-8197-1r04-5528-37qqo6363s62@tzk.qr","threadId":"58284","inReplyTo":"531620e1-de4c-74aa-c840-c12ce81f8740@github.com","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-10-18T14:03:17Z","receivedAt":"2022-10-18T14:48:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Victoria,\n\nOn Thu, 8 Sep 2022, Victoria Dye wrote:\n\n> Johannes Schindelin via GitGitGadget wrote:\n>\n> > [...]\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 55857af601b..4468ac51f25 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -42,7 +42,16 @@ then\n> > \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> > fi\n> > GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> > -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> > +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> > +then\n> > +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> > +\t# On Windows, we must convert Windows paths lest they contain a colon\n> > +\tcase \"$(uname -s)\" in\n> > +\t*MINGW*)\n> > +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> > +\t\t;;\n> > +\tesac\n> > +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> > then\n> > \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> > \texit 1\n> Referring to Ævar's review in [1] - while I'm not overly concerned about the\n> \"switching between make & CMake\" file staleness (if I'm not mistaken, the\n> same thing can happen now with the modified 'test-lib.sh', so this patch\n> doesn't really make anything worse), I do think the changes to 'test-lib.sh'\n> should be rearranged to preserve the \"PANIC\" check:\n>\n> ----------------->8----------------->8----------------->8-----------------\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 4468ac51f2..7b57f55c37 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -42,6 +42,11 @@ then\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n> +if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> +then\n> +\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> +\texit 1\n> +fi\n> if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n> then\n> \tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n> @@ -51,10 +56,6 @@ then\n> \t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n> \t\t;;\n> \tesac\n> -elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n> -then\n> -\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> -\texit 1\n> fi\n>\n> # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n> -----------------8<-----------------8<-----------------8<-----------------\n>\n> Otherwise, a user could run the tests from outside a 't/' directory if they\n> built Git with CMake, which doesn't appear to be part of the intended\n> behavior of this patch.\n\nGood point!\n\nThank you,\nDscho\n"},{"id":"465203","messageId":"on2q3qos-sr0n-0p8p-606p-5pq39n46qq4q@tzk.qr","threadId":"58284","inReplyTo":"221018.86sfjlgr54.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-10-18T14:21:10Z","receivedAt":"2022-10-18T15:06:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nyou did not even give me a chance to send my reply to your original mail\n;-)\n\nOn Tue, 18 Oct 2022, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > In 7f5397a07c6c (cmake: support for testing git when building out of the\n> > source tree, 2020-06-26), we implemented support for running Git's test\n> > scripts even after building Git in a different directory than the source\n> > directory.\n> >\n> > The way we did this was to edit the file `t/test-lib.sh` to override\n> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n> > directory.\n> >\n> > This is unideal because it always leaves a tracked file marked as\n> > modified, and it is all too easy to commit that change by mistake.\n> >\n> > Let's change the strategy by teaching `t/test-lib.sh` to detect the\n> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n> > exists, the contents are interpreted as the location to the _actual_\n> > build directory. We then write this file as part of the CTest\n> > definition.\n> >\n> > To support building Git via a regular `make` invocation after building\n> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n> > convenience, this is done as part of the Makefile rule that is already\n> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n> > up to date).\n> >\n> > Signed-off-by: Johannes Schindelin \n>\n> Re my earlier feedback, I came up with this as an alternative, which\n> nicely allows us to have \"cmake\" and \"make\" play together, you can even\n> run them concurrently!:\n>\n> \thttps://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245\n\nThis approach _still_ modifies the `test-lib.sh`, which is the entire\nreason for the patch under review.\n\nI hope you find an elegant, user-friendly alternative that leaves\n`test-lib.sh` unmodified even when building via CMake. I would gladly take\nthat and drop my `GIT-BUILD-DIR` patch.\n\nCiao,\nJohannes\n"},{"id":"465204","messageId":"221018.86o7u9go63.gmgdl@evledraar.gmail.com","threadId":"58284","inReplyTo":"851094n0-2o53-77n8-41o6-773q479q5sqq@tzk.qr","subject":"Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T15:09:44Z","receivedAt":"2022-10-18T15:13:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Thu, 8 Sep 2022, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:\n>>\n>> > From: Johannes Schindelin \n>> >\n>> > In 7f5397a07c6c (cmake: support for testing git when building out of the\n>> > source tree, 2020-06-26), we implemented support for running Git's test\n>> > scripts even after building Git in a different directory than the source\n>> > directory.\n>> >\n>> > The way we did this was to edit the file `t/test-lib.sh` to override\n>> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`\n>> > directory.\n>> >\n>> > This is unideal because it always leaves a tracked file marked as\n>> > modified, and it is all too easy to commit that change by mistake.\n>> >\n>> > Let's change the strategy by teaching `t/test-lib.sh` to detect the\n>> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it\n>> > exists, the contents are interpreted as the location to the _actual_\n>> > build directory. We then write this file as part of the CTest\n>> > definition.\n>> >\n>> > To support building Git via a regular `make` invocation after building\n>> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for\n>> > convenience, this is done as part of the Makefile rule that is already\n>> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is\n>> > up to date).\n>> >\n>> > Signed-off-by: Johannes Schindelin \n>> > ---\n>> > .gitignore | 1 +\n>> > Makefile | 1 +\n>> > contrib/buildsystems/CMakeLists.txt | 7 +------\n>> > t/test-lib.sh | 11 ++++++++++-\n>> > 4 files changed, 13 insertions(+), 7 deletions(-)\n>> >\n>> > diff --git a/.gitignore b/.gitignore\n>> > index a4522157641..b72ddf09346 100644\n>> > --- a/.gitignore\n>> > +++ b/.gitignore\n>> > @@ -2,6 +2,7 @@\n>> > /fuzz_corpora\n>> > /fuzz-pack-headers\n>> > /fuzz-pack-idx\n>> > +/GIT-BUILD-DIR\n>> > /GIT-BUILD-OPTIONS\n>> > /GIT-CFLAGS\n>> > /GIT-LDFLAGS\n>> > diff --git a/Makefile b/Makefile\n>> > index 04d0fd1fe60..9347ed90da7 100644\n>> > --- a/Makefile\n>> > +++ b/Makefile\n>> > @@ -3028,6 +3028,7 @@ else\n>> > \t@echo RUNTIME_PREFIX=\\'false\\' >>$@+\n>> > endif\n>> > \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n>> > +\t@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi\n>> >\n>> > ### Detect Python interpreter path changes\n>> > ifndef NO_PYTHON\n>> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n>> > index fe606c179f7..29d7e236ae1 100644\n>> > --- a/contrib/buildsystems/CMakeLists.txt\n>> > +++ b/contrib/buildsystems/CMakeLists.txt\n>> > @@ -1067,14 +1067,9 @@ endif()\n>> > #Make the tests work when building out of the source tree\n>> > get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)\n>> > if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n>> > -\tfile(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)\n>> > -\tstring(REPLACE \"/CMakeCache.txt\" \"\" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})\n>> > \t#Setting the build directory in test-lib.sh before running tests\n>> > \tfile(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake\n>> > -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \\\"GIT_BUILD_DIR=(.*)\\\")\\n\"\n>> > -\t\t\"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\\n\"\n>> > -\t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n>> > -\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n>> > +\t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \\\"${CMAKE_BINARY_DIR}\\\")\")\n>> > \t#misc copies\n>> > \tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n>> > \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n>> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> > index 55857af601b..4468ac51f25 100644\n>> > --- a/t/test-lib.sh\n>> > +++ b/t/test-lib.sh\n>> > @@ -42,7 +42,16 @@ then\n>> > \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n>> > fi\n>> > GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n>> > -if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n>> > +if test -f \"$GIT_BUILD_DIR/GIT-BUILD-DIR\"\n>> > +then\n>> > +\tGIT_BUILD_DIR=\"$(cat \"$GIT_BUILD_DIR/GIT-BUILD-DIR\")\" || exit 1\n>> > +\t# On Windows, we must convert Windows paths lest they contain a colon\n>> > +\tcase \"$(uname -s)\" in\n>> > +\t*MINGW*)\n>> > +\t\tGIT_BUILD_DIR=\"$(cygpath -au \"$GIT_BUILD_DIR\")\"\n>> > +\t\t;;\n>> > +\tesac\n>> > +elif test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n>> > then\n>> > \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n>> > \texit 1\n>>\n>> As pointed out in the v1 this breaks the cmake<->make interaction in\n>> some scenarios, but from some brief testing there seemed to be an easy\n>> workaround which didn't suffer from that problem:\n>> https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/\n>\n> I do not think that the CMake <-> make interaction will come up in any\n> other scenario than your and my tests in the context of this mailing list\n> thread.\n>\n> Therefore, I am certain that we need not cater to that scenario at all.\n\nI run it like that now when I build locally, and it slots nicely in with:\n\n \t1. First run a bunch of 'build' targets\n\t2. Then run a bunch of 'test' targets\n\nI'm not opposed to having this e.g. as a special-case on Windows, or\nthat we pick up when you *only* have cmake/ctest unconditionally, and\nauto-do the right thing.\n\nBut I do think that:\n\n\tA. Getting a \"run test-lib against this build dir over there\" is\n\t *different* from the bundled behavior of the auto-flip-flopping.\n\n\tB. Your case for doing it this way seems to be the\n\t Windows-specific case noted in 1/5, can't we just make that\n\t part Windows-specific then? I don't get why it needs to be\n\t bundled up with cmake everywhere....\n"}]}