{"thread":{"id":"65505","subject":"[PATCH v4 01/12] t: prepare `test_match_signal ()` calls for `set -e`","startedAt":"2026-04-17T10:51:01Z","lastAt":"2026-04-20T06:11:09Z","messageCount":23,"participants":["Patrick Steinhardt","Jeff King","Ben Knoble","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":12},"messages":[{"id":"541813","messageId":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260413-b4-pks-tests-with-set-e-v1-0-5b83763a0e84@pks.im","subject":"[PATCH v4 00/12] t: detect errors outside of test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:46Z","receivedAt":"2026-04-17T10:51:00Z","isPatch":true,"body":"Hi,\n\nthis is a follow-up to the recent discussion we had around `set -e` to\nmake our tests more robust and basically supersedes Junio's [1].\n\nI've tested the patches with both Bash and Dash, and all tests are\npassing on my machine with both of them. CI seems to be happy, as\nwell. But I would expect that this change probably has some fallout,\neven though I hope that it's generally going to be small and contained.\n\nThis series is based on 8c9303b1ff (Merge branch\n'jc/no-writev-does-not-work', 2026-04-10).\n\nI've created an MR with GitLab [2] and a PR with GitHub [3] to verify\nthat these changes work on both platforms.\n\nChanges in v4:\n  - Simplify how we read a multi-line variable value.\n  - Link to v3: https://patch.msgid.link/20260416-b4-pks-tests-with-set-e-v3-0-7a90e5dccadd@pks.im\n\nChanges in v3:\n  - Adapt `linux-TEST-vars` job to use Bash instead of Dash. Ubuntu\n    packet mirrors seem to be having problems, so I wasn't able to get\n    past installing dependencies in any jobs. All to say that I couldn't\n    verify that this works as expected :/\n  - Link to v2: https://patch.msgid.link/20260415-b4-pks-tests-with-set-e-v2-0-4e4904a96f15@pks.im\n\nChanges in v2:\n  - Use `ret=0; $command || ret=$?` pattern.\n  - Restore `echo 0` in SIGPIPE tests.\n  - Fix \"lib-git-svn.sh\" to gracefully handle the case where SVN Perl\n    modules aren't installed.\n  - Use `|| :` consistently instead of `|| true`.\n  - Fix up a couple of tests that fail on FreeBSD 15. The test suite is\n    now passing on this system, too.\n  - Only enable `set -e` on Bash 5 and newer.\n  - Link to v1: https://patch.msgid.link/20260413-b4-pks-tests-with-set-e-v1-0-5b83763a0e84@pks.im\n\nThanks!\n\nPatrick\n\n[1]: <20260325062114.2067946-1-gitster@pobox.com>\n[2]: https://gitlab.com/gitlab-org/git/-/merge_requests/541\n[3]: https://github.com/git/git/pull/2270\n\n---\nPatrick Steinhardt (12):\n      t: prepare `test_match_signal ()` calls for `set -e`\n      t: prepare `test_must_fail ()` for `set -e`\n      t: prepare `stop_git_daemon ()` for `set -e`\n      t: prepare `git config --unset` calls for `set -e`\n      t: prepare conditional test execution for `set -e`\n      t: prepare execution of potentially failing commands for `set -e`\n      t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`\n      t0008: silence error in subshell when using `grep -v`\n      t1301: don't fail in case setfacl(1) doesn't exist or fails\n      t6002: fix use of `expr` with `set -e`\n      t9902: fix use of `read` with `set -e`\n      t: detect errors outside of test cases\n\n ci/run-build-and-tests.sh          |  5 +++++\n t/lib-git-daemon.sh                |  8 +++++---\n t/lib-git-svn.sh                   |  7 +++----\n t/lib-httpd.sh                     |  3 +--\n t/t0005-signals.sh                 |  4 ++--\n t/t0008-ignores.sh                 |  4 ++--\n t/t1301-shared-repo.sh             |  2 +-\n t/t3600-rm.sh                      |  2 +-\n t/t3901-i18n-patch.sh              |  3 ++-\n t/t4032-diff-inter-hunk-context.sh | 14 ++++++++------\n t/t5000-tar-tree.sh                |  4 ++--\n t/t6002-rev-list-bisect.sh         | 17 ++++++++++-------\n t/t7422-submodule-output.sh        |  2 +-\n t/t7450-bad-git-dotfiles.sh        | 24 +++++++++++++-----------\n t/t7508-status.sh                  |  4 ++--\n t/t9138-git-svn-authors-prog.sh    |  4 ++--\n t/t9200-git-cvsexportcommit.sh     |  3 +--\n t/t9400-git-cvsserver-server.sh    |  5 +++--\n t/t9401-git-cvsserver-crlf.sh      |  4 ++--\n t/t9402-git-cvsserver-refs.sh      |  4 ++--\n t/t9902-completion.sh              |  6 ++----\n t/test-lib-functions.sh            | 12 ++++++------\n t/test-lib.sh                      | 19 +++++++++++++++----\n 23 files changed, 91 insertions(+), 69 deletions(-)\n\nRange-diff versus v3:\n\n 1:  276cd1c541 =  1:  7e57f3ba57 t: prepare `test_match_signal ()` calls for `set -e`\n 2:  3cbcf0298c =  2:  3b8f710de8 t: prepare `test_must_fail ()` for `set -e`\n 3:  e97211a468 =  3:  9cf3f458b3 t: prepare `stop_git_daemon ()` for `set -e`\n 4:  c974d59252 =  4:  8763cedd60 t: prepare `git config --unset` calls for `set -e`\n 5:  e41064dd1b =  5:  8dc43cca62 t: prepare conditional test execution for `set -e`\n 6:  890c11aa7a =  6:  ca0c250d39 t: prepare execution of potentially failing commands for `set -e`\n 7:  a7b2bb9cd5 =  7:  4631ebe1d9 t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`\n 8:  17656428f9 =  8:  64df2f3975 t0008: silence error in subshell when using `grep -v`\n 9:  7a6e730ba3 =  9:  f79e55dd96 t1301: don't fail in case setfacl(1) doesn't exist or fails\n10:  b762f10ac9 = 10:  fcf5ed7ced t6002: fix use of `expr` with `set -e`\n11:  bb588ffe22 <  -:  ---------- t9902: fix use of `read` with `set -e`\n -:  ---------- > 11:  39a5e2ffcb t9902: fix use of `read` with `set -e`\n12:  9ffcb73e64 = 12:  7dfee331e9 t: detect errors outside of test cases\n\n---\nbase-commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0\nchange-id: 20260410-b4-pks-tests-with-set-e-3ae479b24b51\n\n"},{"id":"541812","messageId":"20260417-b4-pks-tests-with-set-e-v4-1-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 01/12] t: prepare `test_match_signal ()` calls for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:47Z","receivedAt":"2026-04-17T10:51:01Z","isPatch":true,"body":"We have a couple of calls to `test_match_signal ()` where we execute a\nGit command and expect it to die with a specific signal. These calls\nwill essentially execute the process in a subshell via `foo; echo $?`,\nbut as we expect `foo` to fail this will cause the overall subshell to\nfail once we `set -e`.\n\nFix this issue by using `foo && echo 0 || echo $?` instead.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t0005-signals.sh | 4 ++--\n t/t3600-rm.sh      | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\nindex afba0fc3fc..84319cf169 100755\n--- a/t/t0005-signals.sh\n+++ b/t/t0005-signals.sh\n@@ -42,12 +42,12 @@ test_expect_success 'create blob' '\n '\n \n test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '\n-\tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n+\tOUT=$( ((large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&\n \ttest_match_signal 13 \"$OUT\"\n '\n \n test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '\n-\tOUT=$( ((trap \"\" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) &&\n+\tOUT=$( ((trap \"\" PIPE && large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&\n \ttest_match_signal 13 \"$OUT\"\n '\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 1f16e6b522..a371ea690e 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -260,7 +260,7 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft (induce S\n \n test_expect_success !MINGW 'choking \"git rm\" should not let it die with cruft (induce and check SIGPIPE)' '\n \tchoke_git_rm_setup &&\n-\tOUT=$( ((trap \"\" PIPE && git rm -n \"some-file-*\"; echo $? 1>&3) | :) 3>&1 ) &&\n+\tOUT=$( ((trap \"\" PIPE && git rm -n \"some-file-*\" && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&\n \ttest_match_signal 13 \"$OUT\" &&\n \ttest_path_is_missing .git/index.lock\n '\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541814","messageId":"20260417-b4-pks-tests-with-set-e-v4-2-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 02/12] t: prepare `test_must_fail ()` for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:48Z","receivedAt":"2026-04-17T10:51:04Z","isPatch":true,"body":"The helper function `test_must_fail ()` executes a specific Git command\nthat may or may not fail in a specific way. This is done by executing\nthe command in question and then comparing its exit code against a set\nof conditions.\n\nThis works, but once we run our test suite with `set -e` we may bail out\nof `test_must_fail ()` early in case the command actually fails, even\nthough we expect it to fail. Prepare for this change by handling the\nfailed case with `||`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/test-lib-functions.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex f3af10fb7e..5fd5494ef1 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1195,8 +1195,9 @@ test_must_fail () {\n \t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n \t\treturn 1\n \tfi\n-\t\"$@\" 2>&7\n-\texit_code=$?\n+\n+\texit_code=0; \"$@\" 2>&7 || exit_code=$?\n+\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n \t\techo >&4 \"test_must_fail: command succeeded: $*\"\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541815","messageId":"20260417-b4-pks-tests-with-set-e-v4-3-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 03/12] t: prepare `stop_git_daemon ()` for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:49Z","receivedAt":"2026-04-17T10:51:06Z","isPatch":true,"body":"We have a couple of calls to `stop_git_daemon ()` outside of specific\ntest cases that will kill a backgrounded git-daemon(1) process and\nexpect the process with a specific error code. While these function\ncalls do end up killing git-daemon(1), the error handling we have in\nthose contexts is basically ineffective. So while we expect the process\nto exit with a specific error code, we will just continue with any error\nin case it doesn't.\n\nThis will change once we enable `set -e` in a subsequent commit. There's\ntwo issues though that will make this _always_ fail:\n\n  - Our call to `wait` is expected to fail, but because it's not part of\n    a condition it will cause us to bail out immediately with `set -e`.\n\n  - We try to kill git-daemon(1) a second time via the pidfile. We can\n    generally expect that this is the same PID though as we had in the\n    \"GIT_DAEMON_PID\" environment variable, and thus it's more likely\n    than not that we have already killed it, and the call to kill will\n    fail.\n\nPrepare for this change by handling the failure of `wait` with `||` and\nby silencing failures of the second call to `kill`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/lib-git-daemon.sh | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex e62569222b..d172aa51f0 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -85,14 +85,16 @@ stop_git_daemon() {\n \n \t# kill git-daemon child of git\n \tsay >&3 \"Stopping git daemon ...\"\n+\n \tkill \"$GIT_DAEMON_PID\"\n-\twait \"$GIT_DAEMON_PID\" >&3 2>&4\n-\tret=$?\n+\tret=0; wait \"$GIT_DAEMON_PID\" >&3 2>&4 || ret=$?\n+\n \tif ! test_match_signal 15 $ret\n \tthen\n \t\terror \"git daemon exited with status: $ret\"\n \tfi\n-\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null\n+\n+\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null || :\n \tGIT_DAEMON_PID=\n \trm -f git_daemon_output \"$GIT_DAEMON_PIDFILE\"\n }\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541816","messageId":"20260417-b4-pks-tests-with-set-e-v4-4-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 04/12] t: prepare `git config --unset` calls for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:50Z","receivedAt":"2026-04-17T10:51:09Z","isPatch":true,"body":"We have a couple of calls to `git config --unset` that ultimately end up\nas no-ops as the configuration variables aren't set (anymore) in the\nfirst place. These calls are mostly intended to recover unconditionally\nfrom tests that may have executed only partially, but they'll ultimately\nfail during a normal test run.\n\nThis hasn't been a problem until now as we aren't running tests with\n`set -e`. This is about to change though, so let's silence the case\nwhere we cannot unset the config keys.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t4032-diff-inter-hunk-context.sh | 2 +-\n t/t7508-status.sh                  | 4 ++--\n t/t9138-git-svn-authors-prog.sh    | 4 ++--\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh\nindex bada0cbd32..c98eb6abb2 100755\n--- a/t/t4032-diff-inter-hunk-context.sh\n+++ b/t/t4032-diff-inter-hunk-context.sh\n@@ -17,7 +17,7 @@ f() {\n \n t() {\n \tuse_config=\n-\tgit config --unset diff.interHunkContext\n+\tgit config --unset diff.interHunkContext || :\n \n \tcase $# in\n \t4) hunks=$4; cmd=\"diff -U$3\";;\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex a5e21bf8bf..1167b835a4 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -773,8 +773,8 @@ test_expect_success TTY 'status --porcelain ignores color.status' '\n '\n \n # recover unconditionally from color tests\n-git config --unset color.status\n-git config --unset color.ui\n+git config --unset color.status || :\n+git config --unset color.ui || :\n \n test_expect_success 'status --porcelain respects -b' '\n \ndiff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh\nindex 784ec7fc2d..5bb38cb23a 100755\n--- a/t/t9138-git-svn-authors-prog.sh\n+++ b/t/t9138-git-svn-authors-prog.sh\n@@ -68,8 +68,8 @@ test_expect_success 'authors-file overrode authors-prog' '\n \t)\n '\n \n-git --git-dir=x/.git config --unset svn.authorsfile\n-git --git-dir=x/.git config --unset svn.authorsprog\n+git --git-dir=x/.git config --unset svn.authorsfile || :\n+git --git-dir=x/.git config --unset svn.authorsprog || :\n \n test_expect_success 'authors-prog imported user without email' '\n \tsvn mkdir -m gg --username gg-hermit \"$svnrepo\"/gg &&\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541817","messageId":"20260417-b4-pks-tests-with-set-e-v4-5-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 05/12] t: prepare conditional test execution for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:51Z","receivedAt":"2026-04-17T10:51:12Z","isPatch":true,"body":"We have some test in our test suite where we use the pattern of\n`test ... && test_expect_succeess` to conditionally execute a test. The\nproblem is that when we decide to not execute the test, we'll indeed\nskip the test, but the overall statement will also be unsuccessful. This\nwill become a problem once we enable `set -e`.\n\nPrepare for this future by turning this into a proper conditional, which\nis also a bit easier to read overall.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t4032-diff-inter-hunk-context.sh | 12 +++++++-----\n t/t7450-bad-git-dotfiles.sh        | 24 +++++++++++++-----------\n 2 files changed, 20 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh\nindex c98eb6abb2..2d216fb70f 100755\n--- a/t/t4032-diff-inter-hunk-context.sh\n+++ b/t/t4032-diff-inter-hunk-context.sh\n@@ -40,11 +40,13 @@ t() {\n \t\ttest $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks\n \t\"\n \n-\ttest -f $expected &&\n-\ttest_expect_success \"$label: check output\" \"\n-\t\tgit $cmd $file | grep -v '^index ' >actual &&\n-\t\ttest_cmp $expected actual\n-\t\"\n+\tif test -f $expected\n+\tthen\n+\t\ttest_expect_success \"$label: check output\" \"\n+\t\t\tgit $cmd $file | grep -v '^index ' >actual &&\n+\t\t\ttest_cmp $expected actual\n+\t\t\"\n+\tfi\n }\n \n cat <<EOF >expected.f1.0.1 || exit 1\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex f512eed278..8cc86522b2 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -220,17 +220,19 @@ check_dotx_symlink () {\n \t\t)\n \t'\n \n-\ttest -n \"$refuse_index\" &&\n-\ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n-\t\ttest_must_fail \\\n-\t\t\tgit -C $dir \\\n-\t\t\t    -c core.protectntfs \\\n-\t\t\t    -c core.protecthfs \\\n-\t\t\t    read-tree $tree 2>err &&\n-\t\tgrep \"invalid path.*$name\" err &&\n-\t\tgit -C $dir ls-files -s >out &&\n-\t\ttest_must_be_empty out\n-\t'\n+\tif test -n \"$refuse_index\"\n+\tthen\n+\t\ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n+\t\t\ttest_must_fail \\\n+\t\t\t\tgit -C $dir \\\n+\t\t\t\t    -c core.protectntfs \\\n+\t\t\t\t    -c core.protecthfs \\\n+\t\t\t\t    read-tree $tree 2>err &&\n+\t\t\tgrep \"invalid path.*$name\" err &&\n+\t\t\tgit -C $dir ls-files -s >out &&\n+\t\t\ttest_must_be_empty out\n+\t\t'\n+\tfi\n }\n \n check_dotx_symlink gitmodules vanilla .gitmodules\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541818","messageId":"20260417-b4-pks-tests-with-set-e-v4-6-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 06/12] t: prepare execution of potentially failing commands for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:52Z","receivedAt":"2026-04-17T10:51:14Z","isPatch":true,"body":"Several of our tests verify whether a certain binary can be executed,\npotentially skipping tests in case we cannot, for example because the\nbinary doesn't exist. In those cases we often run the binary outside of\nany conditionally.\n\nThis will start to fail once we enable `set -e`, as that will cause us\nto bail out the test immediately. Improve these tests by executing them\ninside of a conditional instead.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/lib-git-svn.sh                |  7 +++----\n t/lib-httpd.sh                  |  3 +--\n t/t3901-i18n-patch.sh           |  3 ++-\n t/t5000-tar-tree.sh             |  4 ++--\n t/t7422-submodule-output.sh     |  2 +-\n t/t9200-git-cvsexportcommit.sh  |  3 +--\n t/t9400-git-cvsserver-server.sh |  5 +++--\n t/t9401-git-cvsserver-crlf.sh   |  4 ++--\n t/t9402-git-cvsserver-refs.sh   |  4 ++--\n t/test-lib-functions.sh         |  3 +--\n t/test-lib.sh                   | 10 ++++++----\n 11 files changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex 2fde2353fd..52843f667d 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -15,8 +15,7 @@ GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn\n SVN_TREE=$GIT_SVN_DIR/svn-tree\n test_set_port SVNSERVE_PORT\n \n-svn >/dev/null 2>&1\n-if test $? -ne 1\n+if ! svn help >/dev/null 2>&1\n then\n \tskip_all='skipping git svn tests, svn not found'\n \ttest_done\n@@ -27,13 +26,13 @@ export svnrepo\n svnconf=$PWD/svnconf\n export svnconf\n \n+x=0\n perl -w -e \"\n use SVN::Core;\n use SVN::Repos;\n \\$SVN::Core::VERSION gt '1.1.0' or exit(42);\n system(qw/svnadmin create --fs-type fsfs/, \\$ENV{svnrepo}) == 0 or exit(41);\n-\" >&3 2>&4\n-x=$?\n+\" >&3 2>&4 || x=$?\n if test $x -ne 0\n then\n \tif test $x -eq 42; then\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 4c76e813e3..fc646447d5 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -235,11 +235,10 @@ start_httpd() {\n \n \ttest_atexit stop_httpd\n \n-\t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n+\tif ! \"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA \\\n \t\t-c \"Listen 127.0.0.1:$LIB_HTTPD_PORT\" -k start \\\n \t\t>&3 2>&4\n-\tif test $? -ne 0\n \tthen\n \t\tcat \"$HTTPD_ROOT_PATH\"/error.log >&4 2>/dev/null\n \t\ttest_skip_or_die GIT_TEST_HTTPD \"web server setup failed\"\ndiff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh\nindex f03601b49a..ef7d7e1edc 100755\n--- a/t/t3901-i18n-patch.sh\n+++ b/t/t3901-i18n-patch.sh\n@@ -28,7 +28,8 @@ check_encoding () {\n \t\t8859)\n \t\t\tgrep \"^encoding ISO8859-1\" ;;\n \t\t*)\n-\t\t\tgrep \"^encoding ISO8859-1\"; test \"$?\" != 0 ;;\n+\t\t\tret=0; grep \"^encoding ISO8859-1\" || ret=$?\n+\t\t\ttest \"$ret\" != 0 ;;\n \t\tesac || return 1\n \t\tj=$i\n \t\ti=$(($i+1))\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 5465054f17..a8c28533dc 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -503,8 +503,8 @@ test_expect_success LONG_IS_64BIT 'set up repository with huge blob' '\n # would generate the whole 64GB).\n test_expect_success LONG_IS_64BIT 'generate tar with huge size' '\n \t{\n-\t\tgit archive HEAD\n-\t\techo $? >exit-code\n+\t\t{ ret=0 && git archive HEAD || ret=$?; } &&\n+\t\techo \"$ret\" >exit-code\n \t} | test_copy_bytes 4096 >huge.tar &&\n \techo 141 >expect &&\n \ttest_cmp expect exit-code\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex aea1ddf117..852136fdfd 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -198,7 +198,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n \t(\n \t\tcd repo &&\n \t\tGIT_ALLOW_PROTOCOL=file git submodule add \"$(pwd)\"/../submodule &&\n-\t\t{ git submodule status --recursive 2>err; echo $?>status; } |\n+\t\t{ { ret=0 && git submodule status --recursive 2>err || ret=$?; } && echo $ret >status; } |\n \t\t\tgrep -q recursive-submodule-path-1 &&\n \t\ttest_must_be_empty err &&\n \t\ttest_match_signal 13 \"$(cat status)\"\ndiff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\nindex 14cbe96527..581cf3d28f 100755\n--- a/t/t9200-git-cvsexportcommit.sh\n+++ b/t/t9200-git-cvsexportcommit.sh\n@@ -11,8 +11,7 @@ if ! test_have_prereq PERL; then\n \ttest_done\n fi\n \n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+if ! cvs version >/dev/null 2>&1\n then\n     skip_all='skipping git cvsexportcommit tests, cvs not found'\n     test_done\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex e499c7f955..4b45398bab 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -17,12 +17,13 @@ if ! test_have_prereq PERL; then\n \tskip_all='skipping git cvsserver tests, perl not available'\n \ttest_done\n fi\n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+\n+if ! cvs version >/dev/null 2>&1\n then\n     skip_all='skipping git-cvsserver tests, cvs not found'\n     test_done\n fi\n+\n perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {\n     skip_all='skipping git-cvsserver tests, Perl SQLite interface unavailable'\n     test_done\ndiff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh\nindex a34805acdc..6b4cbb1651 100755\n--- a/t/t9401-git-cvsserver-crlf.sh\n+++ b/t/t9401-git-cvsserver-crlf.sh\n@@ -60,12 +60,12 @@ check_status_options() {\n     return $stat\n }\n \n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+if ! cvs version >/dev/null 2>&1\n then\n     skip_all='skipping git-cvsserver tests, cvs not found'\n     test_done\n fi\n+\n if ! test_have_prereq PERL\n then\n     skip_all='skipping git-cvsserver tests, perl not available'\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex 2ee41f9443..65f2ceedec 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -68,12 +68,12 @@ check_diff() {\n \n #########\n \n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+if ! cvs version >/dev/null 2>&1\n then\n \tskip_all='skipping git-cvsserver tests, cvs not found'\n \ttest_done\n fi\n+\n if ! test_have_prereq PERL\n then\n \tskip_all='skipping git-cvsserver tests, perl not available'\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 5fd5494ef1..879ee1ee59 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1248,8 +1248,7 @@ test_might_fail () {\n test_expect_code () {\n \twant_code=$1\n \tshift\n-\t\"$@\" 2>&7\n-\texit_code=$?\n+\texit_code=0; \"$@\" 2>&7 || exit_code=$?\n \tif test $exit_code = $want_code\n \tthen\n \t\treturn 0\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 70fd3e9baf..de7d9e7b92 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -143,8 +143,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n ################################################################\n # It appears that people try to run tests without building...\n GIT_BINARY=\"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\"\n-\"$GIT_BINARY\" >/dev/null\n-if test $? != 1\n+\n+if ! \"$GIT_BINARY\" version >/dev/null\n then\n \tif test -n \"$GIT_TEST_INSTALLED\"\n \tthen\n@@ -454,8 +454,10 @@ then\n \t# from any previous runs.\n \t>\"$GIT_TEST_TEE_OUTPUT_FILE\"\n \n-\t(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} \"$0\" \"$@\" 2>&1;\n-\t echo $? >\"$TEST_RESULTS_BASE.exit\") | tee -a \"$GIT_TEST_TEE_OUTPUT_FILE\"\n+\t(\n+\t\tret=0 && GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} \"$0\" \"$@\" 2>&1 || ret=$?\n+\t\techo \"$ret\" >\"$TEST_RESULTS_BASE.exit\"\n+\t) | tee -a \"$GIT_TEST_TEE_OUTPUT_FILE\"\n \ttest \"$(cat \"$TEST_RESULTS_BASE.exit\")\" = 0\n \texit\n fi\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541819","messageId":"20260417-b4-pks-tests-with-set-e-v4-7-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 07/12] t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:53Z","receivedAt":"2026-04-17T10:51:17Z","isPatch":true,"body":"Both `test_when_finished ()` and `test_atexit ()` build up a chain of\ncleanup commands by prepending each new command to the existing cleanup\nstring. To preserve the exit code of the test body across cleanup\nexecution, we append the following logic:\n\n    } && (exit \"$eval_ret\"); eval_ret=$?; ...\n\nThe intent of this is to run the cleanup block and then unconditionally\nrestore `eval_ret`. The original behaviour of this is is:\n\n   +------------------+---------+------------------------------------+\n   |test body         │ cleanup │ old behaviour                      │\n   +------------------+---------+------------------------------------+\n   │pass (eval_ret=0) | pass    │ && taken -> (exit 0) -> eval_ret=0 |\n   +------------------+---------+------------------------------------+\n   │pass (eval_ret=0) | fail    │ && not taken -> eval_ret=$?        |\n   +------------------+---------+------------------------------------+\n   │fail (eval_ret=1) | pass    │ && taken -> (exit 1) -> eval_ret=1 |\n   +------------------+---------+------------------------------------+\n   │fail (eval_ret=1) | fail    | && not taken -> eval_ret=$?        |\n   +------------------+---------+------------------------------------+\n\nThis logic will start to fail once we enable `set -e`. When `$eval_ret`\nis non-zero, the subshell we create will fail, and with `set -e` we'll\nthus bail out without evaluating the logic after the semicolon.\n\nFix this issue by instead using `|| eval_ret=\\$?; ...`. Besides being\na bit simpler, it also retains the original behaviour:\n\n   +------------------+---------+------------------------------------+\n   |test body         │ cleanup │ old behaviour                      │\n   +------------------+---------+------------------------------------+\n   │pass (eval_ret=0) | pass    │ || not taken -> eval_ret unchanged |\n   +------------------+---------+------------------------------------+\n   │pass (eval_ret=0) | fail    │ || taken -> eval_ret=$?            |\n   +------------------+---------+------------------------------------+\n   │fail (eval_ret=1) | pass    │ || not taken -> eval_ret unchanged |\n   +------------------+---------+------------------------------------+\n   │fail (eval_ret=1) | fail    | || taken -> eval_ret=$?            |\n   +------------------+---------+------------------------------------+\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/test-lib-functions.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 879ee1ee59..502bb0ddcb 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1512,7 +1512,7 @@ test_when_finished () {\n \ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n \tBUG \"test_when_finished does nothing in a subshell\"\n \ttest_cleanup=\"{ $*\n-\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n+\t\t} || eval_ret=\\$?; $test_cleanup\"\n }\n \n # This function can be used to schedule some commands to be run\n@@ -1540,7 +1540,7 @@ test_atexit () {\n \ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n \tBUG \"test_atexit does nothing in a subshell\"\n \ttest_atexit_cleanup=\"{ $*\n-\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_atexit_cleanup\"\n+\t\t} || eval_ret=\\$?; $test_atexit_cleanup\"\n }\n \n # Deprecated wrapper for \"git init\", use \"git init\" directly instead\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541820","messageId":"20260417-b4-pks-tests-with-set-e-v4-8-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 08/12] t0008: silence error in subshell when using `grep -v`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:54Z","receivedAt":"2026-04-17T10:51:19Z","isPatch":true,"body":"In t0008 we use `grep -v` in a subshell, but expect that this command\nwill sometimes not match anything. This would cause grep(1) to return an\nerror code, but given that we don't run with `set -e` we swallow this\nerror.\n\nWe're about to enable `set -e`. Prepare for this by ignoring any errors.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t0008-ignores.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex e716b5cdfa..d77a179bdd 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -122,8 +122,8 @@ test_expect_success_multiple () {\n \tfi\n \ttestname=\"$1\" expect_all=\"$2\" code=\"$3\"\n \n-\texpect_verbose=$( echo \"$expect_all\" | grep -v '^::\t' )\n-\texpect=$( echo \"$expect_verbose\" | sed -e 's/.*\t//' )\n+\texpect_verbose=$(echo \"$expect_all\" | grep -v '^::\t' || :)\n+\texpect=$(echo \"$expect_verbose\" | sed -e 's/.*\t//')\n \n \ttest_expect_success $prereq \"$testname${no_index_opt:+ with $no_index_opt}\" '\n \t\texpect \"$expect\" &&\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541821","messageId":"20260417-b4-pks-tests-with-set-e-v4-9-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 09/12] t1301: don't fail in case setfacl(1) doesn't exist or fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:55Z","receivedAt":"2026-04-17T10:51:22Z","isPatch":true,"body":"In t1301 we're trying to remove any potentially-existing default ACLs\nthat might exist on the transh directory by executing setfacl(1).\nAccording to 8ed0a740dd (t1301-shared-repo.sh: don't let a default ACL\ninterfere with the test, 2008-10-16), this is done because we play\naround with permissions and umasks in this test suite.\n\nThe setfacl(1) binary may not exist on some systems though, even though\ntests ultimately still pass. This doesn't matter currently, but will\ncause the test to fail once we start running with `set -e`. Silence such\nfailures by ignoring failures here.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t1301-shared-repo.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 630a47af21..0e0d07a1a1 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -12,7 +12,7 @@ TEST_CREATE_REPO_NO_TEMPLATE=1\n . ./test-lib.sh\n \n # Remove a default ACL from the test dir if possible.\n-setfacl -k . 2>/dev/null\n+setfacl -k . 2>/dev/null || :\n \n # User must have read permissions to the repo -> failure on --shared=0400\n test_expect_success 'shared = 0400 (faulty permission u-w)' '\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541822","messageId":"20260417-b4-pks-tests-with-set-e-v4-10-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 10/12] t6002: fix use of `expr` with `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:56Z","receivedAt":"2026-04-17T10:51:24Z","isPatch":true,"body":"In `test_bisection_diff ()` we use `expr` to perform some math. This\ncommand has some gotchas though in that it will only return success when\nthe result is neither null nor zero. In some of our cases though it\nactually _is_ zero, and that will cause the expressions to fail once we\nenable `set -e`.\n\nPrepare for this change by instead using `$(( ))`, which doesn't have\nthe same issue. While at it, modernize the function a tiny bit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t6002-rev-list-bisect.sh | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex daa009c9a1..f2de40b5ed 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -27,13 +27,16 @@ test_bisection_diff()\n \t# Test if bisection size is close to half of list size within\n \t# tolerance.\n \t#\n-\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2)\n-\ttest \"$_bisect_err\" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)\n-\t_bisect_err=$(expr $_bisect_err / 2) ; # floor\n-\n-\ttest_expect_success \\\n-\t\"bisection diff $_bisect_option $_head $* <= $_max_diff\" \\\n-\t'test $_bisect_err -le $_max_diff'\n+\t_bisect_err=$(($_list_size - $_bisection_size * 2))\n+\tif test \"$_bisect_err\" -lt 0\n+\tthen\n+\t\t_bisect_err=$((0 - $_bisect_err))\n+\tfi\n+\t_bisect_err=$(($_bisect_err / 2)) ; # floor\n+\n+\ttest_expect_success \"bisection diff $_bisect_option $_head $* <= $_max_diff\" '\n+\t\ttest $_bisect_err -le $_max_diff\n+\t'\n }\n \n date >path0\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541823","messageId":"20260417-b4-pks-tests-with-set-e-v4-11-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 11/12] t9902: fix use of `read` with `set -e`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:57Z","receivedAt":"2026-04-17T10:51:27Z","isPatch":true,"body":"In t9902 we're using the `read` builtin to read some values into a\nvariable. This is done by using `-d \"\"`, which cause us to read until\nthe end of the heredoc. As the read is terminated by EOF, the command\nwill end up returning a non-zero error code. This hasn't been an issue\nuntil now as we didn't run with `set -e`, but that'll change in a\nsubsequent commit.\n\nPrepare for this change by not using read at all, as we can simply store\nthe multi-line value directly.\n\nSuggested-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t9902-completion.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 2f9a597ec7..28f61f08fb 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -590,12 +590,10 @@ test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '\n \t__gitcomp \"$invalid_variable_name\"\n '\n \n-read -r -d \"\" refs <<-\\EOF\n-main\n+refs='main\n maint\n next\n-seen\n-EOF\n+seen'\n \n test_expect_success '__gitcomp_nl - trailing space' '\n \ttest_gitcomp_nl \"m\" \"$refs\" <<-EOF\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541824","messageId":"20260417-b4-pks-tests-with-set-e-v4-12-44d43efdafb1@pks.im","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im","subject":"[PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-17T10:50:58Z","receivedAt":"2026-04-17T10:51:29Z","isPatch":true,"body":"We have recently merged a patch series that had a simple misspelling of\n`test_expect_success`. Instead of making our tests fail though, this\ntypo went completely undetected and all of our tests passed, which is of\ncourse unfortunate. This is a more general issue with our test suite:\nall commands that run outside of a specific test case can fail, and if\nwe don't explicitly check for such failure then this failure will be\nsilently ignored.\n\nImprove the status quo by enabling the errexit option so that any such\nunchecked failures will cause us to abort immediately.\n\nNote that for now, we only enable this option for Bash 5 and newer. This\nis because other shells have wildly different behaviour, and older\nversions of Bash (especially on macOS) are buggy. The list of enabled\nshells may be extended going forward.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n ci/run-build-and-tests.sh | 5 +++++\n t/test-lib.sh             | 9 +++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 28cfe730ee..f0a3597184 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -15,6 +15,11 @@ fedora-breaking-changes-musl|linux-breaking-changes)\n \tMESONFLAGS=\"$MESONFLAGS -Drust=enabled\"\n \t;;\n linux-TEST-vars)\n+\t# Ubuntu uses Dash by default, but we only enable use of `set -e`\n+\t# when using Bash 5+. Ensure that we have at least one CI job that uses\n+\t# it.\n+\texport TEST_SHELL_PATH=/usr/bin/bash\n+\n \texport OPENSSL_SHA1_UNSAFE=YesPlease\n \texport GIT_TEST_SPLIT_INDEX=yes\n \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex de7d9e7b92..1f7868c537 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,6 +15,15 @@\n # You should have received a copy of the GNU General Public License\n # along with this program.  If not, see https://www.gnu.org/licenses/ .\n \n+# Enable the use of errexit so that any unexpected failures will cause us to\n+# abort tests, even when outside of a specific test case. Note that we only\n+# enable this on Bash 5 and newer, as `set -e` has wildly different behaviour\n+# across shells. The list of allowed shells may be extended going forward.\n+if test \"${BASH_VERSINFO:=0}\" -ge 5\n+then\n+\tset -e\n+fi\n+\n # Test the binaries we have just built.  The tests are kept in\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n\n-- \n2.54.0.rc2.529.gd9106f7525.dirty\n\n"},{"id":"541853","messageId":"20260418065009.GA2619713@coredump.intra.peff.net","threadId":"65505","inReplyTo":"20260417-b4-pks-tests-with-set-e-v4-12-44d43efdafb1@pks.im","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-18T06:50:09Z","receivedAt":"2026-04-18T06:50:17Z","isPatch":true,"body":"On Fri, Apr 17, 2026 at 12:50:58PM +0200, Patrick Steinhardt wrote:\n\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -15,6 +15,11 @@ fedora-breaking-changes-musl|linux-breaking-changes)\n>  \tMESONFLAGS=\"$MESONFLAGS -Drust=enabled\"\n>  \t;;\n>  linux-TEST-vars)\n> +\t# Ubuntu uses Dash by default, but we only enable use of `set -e`\n> +\t# when using Bash 5+. Ensure that we have at least one CI job that uses\n> +\t# it.\n> +\texport TEST_SHELL_PATH=/usr/bin/bash\n\nThinking on this a little more, it is a shame we cannot easily enable\nthis for dash. That would hit most CI jobs, but also the local builds of\nmost developers. And finding problems early and locally often saves a\nlot of time versus finding them in CI.\n\nUnfortunately I could not find a way to detect whether we are running\ndash at all, let alone a recent version. But what if we let the user\ntell us? Something like:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1f7868c537..a0d07f75fb 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -17,9 +17,10 @@\n \n # Enable the use of errexit so that any unexpected failures will cause us to\n # abort tests, even when outside of a specific test case. Note that we only\n-# enable this on Bash 5 and newer, as `set -e` has wildly different behaviour\n-# across shells. The list of allowed shells may be extended going forward.\n-if test \"${BASH_VERSINFO:=0}\" -ge 5\n+# enable this by default on Bash 5 and newer, as `set -e` has wildly different\n+# behaviour across shells. If you trust your shell's `set -e` implementation,\n+# you can set GIT_TEST_USE_SET_E manually.\n+if test \"$GIT_TEST_USE_SET_E\" = 1 && test \"${BASH_VERSINFO:=0}\" -ge 5\n then\n \tset -e\n fi\n\nAnd then those of us who want to stick:\n\n  export GIT_TEST_USE_SET_E = 1\n\nin our config.mak can do so, and we could even set it in the ci/ scripts\nfor all of the ubuntu builds.\n\n-Peff\n"},{"id":"541855","messageId":"AA6F33AD-25C2-4AB0-A624-35C7B0BE0F66@gmail.com","threadId":"65505","inReplyTo":"20260418065009.GA2619713@coredump.intra.peff.net","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-04-18T12:17:10Z","receivedAt":"2026-04-18T12:17:22Z","isPatch":true,"body":"\n> Le 18 avr. 2026 à 02:50, Jeff King <peff@peff.net> a écrit :\n> \n> On Fri, Apr 17, 2026 at 12:50:58PM +0200, Patrick Steinhardt wrote:\n> \n>> --- a/ci/run-build-and-tests.sh\n>> +++ b/ci/run-build-and-tests.sh\n>> @@ -15,6 +15,11 @@ fedora-breaking-changes-musl|linux-breaking-changes)\n>>    MESONFLAGS=\"$MESONFLAGS -Drust=enabled\"\n>>    ;;\n>> linux-TEST-vars)\n>> +    # Ubuntu uses Dash by default, but we only enable use of `set -e`\n>> +    # when using Bash 5+. Ensure that we have at least one CI job that uses\n>> +    # it.\n>> +    export TEST_SHELL_PATH=/usr/bin/bash\n> \n> Thinking on this a little more, it is a shame we cannot easily enable\n> this for dash. That would hit most CI jobs, but also the local builds of\n> most developers. And finding problems early and locally often saves a\n> lot of time versus finding them in CI.\n> \n> Unfortunately I could not find a way to detect whether we are running\n> dash at all, let alone a recent version. But what if we let the user\n> tell us? Something like:\n\nI was just wishing for similar! I imagine it would be useful for folks who occasionally test Zsh’s POSIX mode and want to see how it handles -e\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 1f7868c537..a0d07f75fb 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -17,9 +17,10 @@\n> \n> # Enable the use of errexit so that any unexpected failures will cause us to\n> # abort tests, even when outside of a specific test case. Note that we only\n> -# enable this on Bash 5 and newer, as `set -e` has wildly different behaviour\n> -# across shells. The list of allowed shells may be extended going forward.\n> -if test \"${BASH_VERSINFO:=0}\" -ge 5\n> +# enable this by default on Bash 5 and newer, as `set -e` has wildly different\n> +# behaviour across shells. If you trust your shell's `set -e` implementation,\n> +# you can set GIT_TEST_USE_SET_E manually.\n> +if test \"$GIT_TEST_USE_SET_E\" = 1 && test \"${BASH_VERSINFO:=0}\" -ge 5\n> then\n>    set -e\n> fi\n\nI guess that should be || instead of &&?\n\n> \n> And then those of us who want to stick:\n> \n>  export GIT_TEST_USE_SET_E = 1\n> \n> in our config.mak can do so, and we could even set it in the ci/ scripts\n> for all of the ubuntu builds.\n> \n> -Peff\n\nThanks"},{"id":"541865","messageId":"20260418174446.GA1695@coredump.intra.peff.net","threadId":"65505","inReplyTo":"AA6F33AD-25C2-4AB0-A624-35C7B0BE0F66@gmail.com","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-18T17:44:46Z","receivedAt":"2026-04-18T17:44:48Z","isPatch":true,"body":"On Sat, Apr 18, 2026 at 08:17:10AM -0400, Ben Knoble wrote:\n\n> > +if test \"$GIT_TEST_USE_SET_E\" = 1 && test \"${BASH_VERSINFO:=0}\" -ge 5\n> > then\n> >    set -e\n> > fi\n> \n> I guess that should be || instead of &&?\n\nOops, yeah. I wrote it correctly and tested it once, and then started to\nrewrite it to support setting it to 0, like:\n\n  if test -z \"$GIT_TEST_USE_SET_E\" && test \"${BASH_VERSINFO:=0}\" -ge 5\n  then\n\tGIT_TEST_USE_SET_E=1\n  fi\n  case \"$GIT_TEST_USE_SET_E\" in\n  1|on|true)\n\tset -e\n\t;;\n  esac\n\nBut I didn't want to get too much into details of the patch, so I went\nback to the original, but obviously screwed that up. ;)\n\n-Peff\n"},{"id":"541870","messageId":"aePY1x9uO39p6WDI@fruit.crustytoothpaste.net","threadId":"65505","inReplyTo":"AA6F33AD-25C2-4AB0-A624-35C7B0BE0F66@gmail.com","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-04-18T19:17:43Z","receivedAt":"2026-04-18T19:17:46Z","isPatch":true,"body":"On 2026-04-18 at 12:17:10, Ben Knoble wrote:\n> \n> > Le 18 avr. 2026 à 02:50, Jeff King <peff@peff.net> a écrit :\n> > Thinking on this a little more, it is a shame we cannot easily enable\n> > this for dash. That would hit most CI jobs, but also the local builds of\n> > most developers. And finding problems early and locally often saves a\n> > lot of time versus finding them in CI.\n> > \n> > Unfortunately I could not find a way to detect whether we are running\n> > dash at all, let alone a recent version. But what if we let the user\n> > tell us? Something like:\n> \n> I was just wishing for similar! I imagine it would be useful for folks\n> who occasionally test Zsh’s POSIX mode and want to see how it handles\n> -e\n\nI hard-coded this on with a bunch of shells in Debian unstable using the\nbelow script.  zsh, busybox, and dash passed, while mksh, lksh, and posh\nfailed.  (The latter are all pdksh variants, I believe, so they are an\nimportant set of shells to consider.)\n\nNote that the script symlinks the shell to `sh` so that everyone will be\non their best POSIX behaviour.\n\n----\n#!/bin/sh\n\ndir=$(mktemp -d)\ntrap 'rm -fr \"$dir\"' EXIT\n\nsh=\"$1\"\nln -sf \"$sh\" \"$dir/sh\"\n\nmake -j12 all && (cd t && GIT_PROVE_OPTS=-j12 GIT_TEST_DEFAULT_HASH=sha256 PATH=\"$dir:$PATH\" SHELL_PATH=\"$dir/sh\" make prove)\n----\n\nHaving said that, I actually think that mksh may be right in at least\none case.  For instance, this diff seems required for mksh to pass t1410\nand I believe this is actually the right thing to do:\n\n----\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex ce71f9a30a..f289fc11e9 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -23,7 +23,7 @@ check_have () {\n }\n \n check_fsck () {\n-\tgit fsck --full >fsck.output\n+\tgit fsck --full >fsck.output || true\n \tcase \"$1\" in\n \t'')\n \t\ttest_must_be_empty fsck.output ;;\n----\n\nI haven't checked the other cases under mksh, but I think it may be a\nfruitful source of things to look at.  And if you find a bug, I'm sure\nthe maintainer would happily accept a bug report in the Debian BTS.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"541871","messageId":"xmqqtst8ul4q.fsf@gitster.g","threadId":"65505","inReplyTo":"20260418174446.GA1695@coredump.intra.peff.net","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-18T19:24:53Z","receivedAt":"2026-04-18T19:24:56Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Apr 18, 2026 at 08:17:10AM -0400, Ben Knoble wrote:\n>\n>> > +if test \"$GIT_TEST_USE_SET_E\" = 1 && test \"${BASH_VERSINFO:=0}\" -ge 5\n>> > then\n>> >    set -e\n>> > fi\n>> \n>> I guess that should be || instead of &&?\n>\n> Oops, yeah. I wrote it correctly and tested it once, and then started to\n> rewrite it to support setting it to 0, like:\n>\n>   if test -z \"$GIT_TEST_USE_SET_E\" && test \"${BASH_VERSINFO:=0}\" -ge 5\n>   then\n> \tGIT_TEST_USE_SET_E=1\n>   fi\n>   case \"$GIT_TEST_USE_SET_E\" in\n>   1|on|true)\n> \tset -e\n> \t;;\n>   esac\n>\n> But I didn't want to get too much into details of the patch, so I went\n> back to the original, but obviously screwed that up. ;)\n\nWe could forget about \"we know this is a good shell by its name and\nversion\" and test the feature we depend on ourselves, perhaps?\n"},{"id":"541872","messageId":"20260418210518.GA9632@coredump.intra.peff.net","threadId":"65505","inReplyTo":"xmqqtst8ul4q.fsf@gitster.g","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-18T21:05:18Z","receivedAt":"2026-04-18T21:05:20Z","isPatch":true,"body":"On Sat, Apr 18, 2026 at 12:24:53PM -0700, Junio C Hamano wrote:\n\n> We could forget about \"we know this is a good shell by its name and\n> version\" and test the feature we depend on ourselves, perhaps?\n\nI looked into that but didn't get anywhere useful. You can try to test\nall of the \"set -e\" scenarios we care about, but there are a lot of\nthem. For example, I would never have thought to check how \"command\"\nbehaves inside a &&-chain while \"set -e\" is in effect.\n\nSo you basically end up adding a test case for the bugs you find, at\nwhich point it is not much better than blocking known-bad versions.\n\n-Peff\n"},{"id":"541873","messageId":"20260418213043.GB9632@coredump.intra.peff.net","threadId":"65505","inReplyTo":"aePY1x9uO39p6WDI@fruit.crustytoothpaste.net","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-18T21:30:43Z","receivedAt":"2026-04-18T21:30:45Z","isPatch":true,"body":"On Sat, Apr 18, 2026 at 07:17:43PM +0000, brian m. carlson wrote:\n\n> Having said that, I actually think that mksh may be right in at least\n> one case.  For instance, this diff seems required for mksh to pass t1410\n> and I believe this is actually the right thing to do:\n\nI think mksh is wrong here, if it is flagging this fsck call.\n\n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> index ce71f9a30a..f289fc11e9 100755\n> --- a/t/t1410-reflog.sh\n> +++ b/t/t1410-reflog.sh\n> @@ -23,7 +23,7 @@ check_have () {\n>  }\n>  \n>  check_fsck () {\n> -\tgit fsck --full >fsck.output\n> +\tgit fsck --full >fsck.output || true\n>  \tcase \"$1\" in\n>  \t'')\n>  \t\ttest_must_be_empty fsck.output ;;\n\nIf check_fsck() were run by itself then yes, this would be a problem.\nBut it is always run inside a test snippet, and there \"set -e\" should\nalways be suppressed because test_expect_success does:\n\n  if test_run_ \"$test_body\"\n\nSo we are inside a conditional, and the usual global \"set -e\"\nsuppression should happen. It sounds like it is not happening in your\nversion of mksh, but I was unable to get t1410 to fail at all using mksh\n59c-43 (from Debian unstable) or 59c-41 (from stable).\n\nAnd it is a good thing that this \"if\" suppression is here, or else tests\nwho fail the final component of the &&-chain would cause the shell to\nexit. The simplest case is just:\n\n  test_expect_success 'bad' 'false'\n\nIf \"set -e\" were in effect, then the whole script would bail upon seeing\nthat \"false\".\n\n-Peff\n"},{"id":"541874","messageId":"aeP9stvssuTv0FD7@fruit.crustytoothpaste.net","threadId":"65505","inReplyTo":"20260418213043.GB9632@coredump.intra.peff.net","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-04-18T21:54:58Z","receivedAt":"2026-04-18T21:55:06Z","isPatch":true,"body":"On 2026-04-18 at 21:30:43, Jeff King wrote:\n> If check_fsck() were run by itself then yes, this would be a problem.\n> But it is always run inside a test snippet, and there \"set -e\" should\n> always be suppressed because test_expect_success does:\n> \n>   if test_run_ \"$test_body\"\n> \n> So we are inside a conditional, and the usual global \"set -e\"\n> suppression should happen. It sounds like it is not happening in your\n> version of mksh, but I was unable to get t1410 to fail at all using mksh\n> 59c-43 (from Debian unstable) or 59c-41 (from stable).\n\nIt does fail with 59c-43 under `make prove` or if you do `sh ./t1410*.sh\n--verbose`, assuming that `sh` points to `mksh`, but since the script\nhas a `/bin/sh` shebang, you need to invoke it explicitly with the shell\nin question, or it will use the system `sh` (dash).  (I made this\nmistake when reproducing the problem.)\n\nNote that the test in question does not exit, but returns this (with\n`--verbose`):\n\n----\nChecking ref database: 100% (1/1), done.\nChecking object directories: 100% (256/256), done.\nnot ok 7 - corrupt and check\n----\n\nand this:\n\n----\nChecking ref database: 100% (1/1), done.\nChecking object directories: 100% (256/256), done.\nnot ok 8 - reflog expire --dry-run should not touch reflog\n----\n\nIt does seem like this _is_ a bug in mksh, though, which I've reproduced\nwith a test script, so I'll report it there.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"541879","messageId":"20260419021027.GA1079904@coredump.intra.peff.net","threadId":"65505","inReplyTo":"aeP9stvssuTv0FD7@fruit.crustytoothpaste.net","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-19T02:10:27Z","receivedAt":"2026-04-19T02:10:28Z","isPatch":true,"body":"On Sat, Apr 18, 2026 at 09:54:58PM +0000, brian m. carlson wrote:\n\n> > So we are inside a conditional, and the usual global \"set -e\"\n> > suppression should happen. It sounds like it is not happening in your\n> > version of mksh, but I was unable to get t1410 to fail at all using mksh\n> > 59c-43 (from Debian unstable) or 59c-41 (from stable).\n> \n> It does fail with 59c-43 under `make prove` or if you do `sh ./t1410*.sh\n> --verbose`, assuming that `sh` points to `mksh`, but since the script\n> has a `/bin/sh` shebang, you need to invoke it explicitly with the shell\n> in question, or it will use the system `sh` (dash).  (I made this\n> mistake when reproducing the problem.)\n\nDoh. The problem was none of that, but that I was using Patrick's\nversion of the patch that only turns on \"set -e\" for bash.\n\nSo yeah, after actually enabling \"set -e\" I do see the failure.\n\n> It does seem like this _is_ a bug in mksh, though, which I've reproduced\n> with a test script, so I'll report it there.\n\nI looked up your report in Debian's system. I think you're right that\nthe eval is the problem. The smallest reproduction I came up with is:\n\n  $ dash -ec 'eval \"false; true\" && echo ok'\n  ok\n\n  $ mksh -ec 'eval \"false; true\" && echo ok'\n  [no output]\n\nSo it respects \"-e\" within the eval, which is wrong, and then doubly\nweird that \"-e\" bails from the eval but not the whole script.\n\n-Peff\n"},{"id":"541902","messageId":"aeXDdvt3YGrJFcSX@pks.im","threadId":"65505","inReplyTo":"20260418210518.GA9632@coredump.intra.peff.net","subject":"Re: [PATCH v4 12/12] t: detect errors outside of test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-20T06:11:02Z","receivedAt":"2026-04-20T06:11:09Z","isPatch":true,"body":"On Sat, Apr 18, 2026 at 05:05:18PM -0400, Jeff King wrote:\n> On Sat, Apr 18, 2026 at 12:24:53PM -0700, Junio C Hamano wrote:\n> \n> > We could forget about \"we know this is a good shell by its name and\n> > version\" and test the feature we depend on ourselves, perhaps?\n> \n> I looked into that but didn't get anywhere useful. You can try to test\n> all of the \"set -e\" scenarios we care about, but there are a lot of\n> them. For example, I would never have thought to check how \"command\"\n> behaves inside a &&-chain while \"set -e\" is in effect.\n> \n> So you basically end up adding a test case for the bugs you find, at\n> which point it is not much better than blocking known-bad versions.\n\nYeah, agreed. If it was only one or two cases I'd definitely agree with\nJunio. But I have a feeling that every shell will behave slightly\ndifferent here, and there's even differences between versions of the\nsame shell.\n\nI'll go with Peff's proposal to have an explicit opt-in, thanks!\n\nPatrick\n"}]}