# [PATCH v4 01/12] t: prepare `test_match_signal ()` calls for `set -e`

23 messages from 2026-04-17 to 2026-04-20. Participants: Patrick Steinhardt, Jeff King, Ben Knoble, brian m. carlson, Junio C Hamano.
Thread: https://gitlist.dev/t/65505

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 00/12] t: detect errors outside of test cases
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>
In-Reply-To: <20260413-b4-pks-tests-with-set-e-v1-0-5b83763a0e84@pks.im>

```
Hi,

this is a follow-up to the recent discussion we had around `set -e` to
make our tests more robust and basically supersedes Junio's [1].

I've tested the patches with both Bash and Dash, and all tests are
passing on my machine with both of them. CI seems to be happy, as
well. But I would expect that this change probably has some fallout,
even though I hope that it's generally going to be small and contained.

This series is based on 8c9303b1ff (Merge branch
'jc/no-writev-does-not-work', 2026-04-10).

I've created an MR with GitLab [2] and a PR with GitHub [3] to verify
that these changes work on both platforms.

Changes in v4:
  - Simplify how we read a multi-line variable value.
  - Link to v3: https://patch.msgid.link/20260416-b4-pks-tests-with-set-e-v3-0-7a90e5dccadd@pks.im

Changes in v3:
  - Adapt `linux-TEST-vars` job to use Bash instead of Dash. Ubuntu
    packet mirrors seem to be having problems, so I wasn't able to get
    past installing dependencies in any jobs. All to say that I couldn't
    verify that this works as expected :/
  - Link to v2: https://patch.msgid.link/20260415-b4-pks-tests-with-set-e-v2-0-4e4904a96f15@pks.im

Changes in v2:
  - Use `ret=0; $command || ret=$?` pattern.
  - Restore `echo 0` in SIGPIPE tests.
  - Fix "lib-git-svn.sh" to gracefully handle the case where SVN Perl
    modules aren't installed.
  - Use `|| :` consistently instead of `|| true`.
  - Fix up a couple of tests that fail on FreeBSD 15. The test suite is
    now passing on this system, too.
  - Only enable `set -e` on Bash 5 and newer.
  - Link to v1: https://patch.msgid.link/20260413-b4-pks-tests-with-set-e-v1-0-5b83763a0e84@pks.im

Thanks!

Patrick

[1]: <20260325062114.2067946-1-gitster@pobox.com>
[2]: https://gitlab.com/gitlab-org/git/-/merge_requests/541
[3]: https://github.com/git/git/pull/2270

---
Patrick Steinhardt (12):
      t: prepare `test_match_signal ()` calls for `set -e`
      t: prepare `test_must_fail ()` for `set -e`
      t: prepare `stop_git_daemon ()` for `set -e`
      t: prepare `git config --unset` calls for `set -e`
      t: prepare conditional test execution for `set -e`
      t: prepare execution of potentially failing commands for `set -e`
      t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`
      t0008: silence error in subshell when using `grep -v`
      t1301: don't fail in case setfacl(1) doesn't exist or fails
      t6002: fix use of `expr` with `set -e`
      t9902: fix use of `read` with `set -e`
      t: detect errors outside of test cases

 ci/run-build-and-tests.sh          |  5 +++++
 t/lib-git-daemon.sh                |  8 +++++---
 t/lib-git-svn.sh                   |  7 +++----
 t/lib-httpd.sh                     |  3 +--
 t/t0005-signals.sh                 |  4 ++--
 t/t0008-ignores.sh                 |  4 ++--
 t/t1301-shared-repo.sh             |  2 +-
 t/t3600-rm.sh                      |  2 +-
 t/t3901-i18n-patch.sh              |  3 ++-
 t/t4032-diff-inter-hunk-context.sh | 14 ++++++++------
 t/t5000-tar-tree.sh                |  4 ++--
 t/t6002-rev-list-bisect.sh         | 17 ++++++++++-------
 t/t7422-submodule-output.sh        |  2 +-
 t/t7450-bad-git-dotfiles.sh        | 24 +++++++++++++-----------
 t/t7508-status.sh                  |  4 ++--
 t/t9138-git-svn-authors-prog.sh    |  4 ++--
 t/t9200-git-cvsexportcommit.sh     |  3 +--
 t/t9400-git-cvsserver-server.sh    |  5 +++--
 t/t9401-git-cvsserver-crlf.sh      |  4 ++--
 t/t9402-git-cvsserver-refs.sh      |  4 ++--
 t/t9902-completion.sh              |  6 ++----
 t/test-lib-functions.sh            | 12 ++++++------
 t/test-lib.sh                      | 19 +++++++++++++++----
 23 files changed, 91 insertions(+), 69 deletions(-)

Range-diff versus v3:

 1:  276cd1c541 =  1:  7e57f3ba57 t: prepare `test_match_signal ()` calls for `set -e`
 2:  3cbcf0298c =  2:  3b8f710de8 t: prepare `test_must_fail ()` for `set -e`
 3:  e97211a468 =  3:  9cf3f458b3 t: prepare `stop_git_daemon ()` for `set -e`
 4:  c974d59252 =  4:  8763cedd60 t: prepare `git config --unset` calls for `set -e`
 5:  e41064dd1b =  5:  8dc43cca62 t: prepare conditional test execution for `set -e`
 6:  890c11aa7a =  6:  ca0c250d39 t: prepare execution of potentially failing commands for `set -e`
 7:  a7b2bb9cd5 =  7:  4631ebe1d9 t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`
 8:  17656428f9 =  8:  64df2f3975 t0008: silence error in subshell when using `grep -v`
 9:  7a6e730ba3 =  9:  f79e55dd96 t1301: don't fail in case setfacl(1) doesn't exist or fails
10:  b762f10ac9 = 10:  fcf5ed7ced t6002: fix use of `expr` with `set -e`
11:  bb588ffe22 <  -:  ---------- t9902: fix use of `read` with `set -e`
 -:  ---------- > 11:  39a5e2ffcb t9902: fix use of `read` with `set -e`
12:  9ffcb73e64 = 12:  7dfee331e9 t: detect errors outside of test cases

---
base-commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0
change-id: 20260410-b4-pks-tests-with-set-e-3ae479b24b51


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 01/12] t: prepare `test_match_signal ()` calls for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-1-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
We have a couple of calls to `test_match_signal ()` where we execute a
Git command and expect it to die with a specific signal. These calls
will essentially execute the process in a subshell via `foo; echo $?`,
but as we expect `foo` to fail this will cause the overall subshell to
fail once we `set -e`.

Fix this issue by using `foo && echo 0 || echo $?` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0005-signals.sh | 4 ++--
 t/t3600-rm.sh      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index afba0fc3fc..84319cf169 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -42,12 +42,12 @@ test_expect_success 'create blob' '
 '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
-	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
+	OUT=$( ((large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '
-	OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) &&
+	OUT=$( ((trap "" PIPE && large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 1f16e6b522..a371ea690e 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -260,7 +260,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft (induce S
 
 test_expect_success !MINGW 'choking "git rm" should not let it die with cruft (induce and check SIGPIPE)' '
 	choke_git_rm_setup &&
-	OUT=$( ((trap "" PIPE && git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) &&
+	OUT=$( ((trap "" PIPE && git rm -n "some-file-*" && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT" &&
 	test_path_is_missing .git/index.lock
 '

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 02/12] t: prepare `test_must_fail ()` for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-2-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
The helper function `test_must_fail ()` executes a specific Git command
that may or may not fail in a specific way. This is done by executing
the command in question and then comparing its exit code against a set
of conditions.

This works, but once we run our test suite with `set -e` we may bail out
of `test_must_fail ()` early in case the command actually fails, even
though we expect it to fail. Prepare for this change by handling the
failed case with `||`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/test-lib-functions.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f3af10fb7e..5fd5494ef1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1195,8 +1195,9 @@ test_must_fail () {
 		echo >&7 "test_must_fail: only 'git' is allowed: $*"
 		return 1
 	fi
-	"$@" 2>&7
-	exit_code=$?
+
+	exit_code=0; "$@" 2>&7 || exit_code=$?
+
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
 		echo >&4 "test_must_fail: command succeeded: $*"

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 03/12] t: prepare `stop_git_daemon ()` for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-3-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
We have a couple of calls to `stop_git_daemon ()` outside of specific
test cases that will kill a backgrounded git-daemon(1) process and
expect the process with a specific error code. While these function
calls do end up killing git-daemon(1), the error handling we have in
those contexts is basically ineffective. So while we expect the process
to exit with a specific error code, we will just continue with any error
in case it doesn't.

This will change once we enable `set -e` in a subsequent commit. There's
two issues though that will make this _always_ fail:

  - Our call to `wait` is expected to fail, but because it's not part of
    a condition it will cause us to bail out immediately with `set -e`.

  - We try to kill git-daemon(1) a second time via the pidfile. We can
    generally expect that this is the same PID though as we had in the
    "GIT_DAEMON_PID" environment variable, and thus it's more likely
    than not that we have already killed it, and the call to kill will
    fail.

Prepare for this change by handling the failure of `wait` with `||` and
by silencing failures of the second call to `kill`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-git-daemon.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index e62569222b..d172aa51f0 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -85,14 +85,16 @@ stop_git_daemon() {
 
 	# kill git-daemon child of git
 	say >&3 "Stopping git daemon ..."
+
 	kill "$GIT_DAEMON_PID"
-	wait "$GIT_DAEMON_PID" >&3 2>&4
-	ret=$?
+	ret=0; wait "$GIT_DAEMON_PID" >&3 2>&4 || ret=$?
+
 	if ! test_match_signal 15 $ret
 	then
 		error "git daemon exited with status: $ret"
 	fi
-	kill "$(cat "$GIT_DAEMON_PIDFILE")" 2>/dev/null
+
+	kill "$(cat "$GIT_DAEMON_PIDFILE")" 2>/dev/null || :
 	GIT_DAEMON_PID=
 	rm -f git_daemon_output "$GIT_DAEMON_PIDFILE"
 }

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 04/12] t: prepare `git config --unset` calls for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-4-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
We have a couple of calls to `git config --unset` that ultimately end up
as no-ops as the configuration variables aren't set (anymore) in the
first place. These calls are mostly intended to recover unconditionally
from tests that may have executed only partially, but they'll ultimately
fail during a normal test run.

This hasn't been a problem until now as we aren't running tests with
`set -e`. This is about to change though, so let's silence the case
where we cannot unset the config keys.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t4032-diff-inter-hunk-context.sh | 2 +-
 t/t7508-status.sh                  | 4 ++--
 t/t9138-git-svn-authors-prog.sh    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh
index bada0cbd32..c98eb6abb2 100755
--- a/t/t4032-diff-inter-hunk-context.sh
+++ b/t/t4032-diff-inter-hunk-context.sh
@@ -17,7 +17,7 @@ f() {
 
 t() {
 	use_config=
-	git config --unset diff.interHunkContext
+	git config --unset diff.interHunkContext || :
 
 	case $# in
 	4) hunks=$4; cmd="diff -U$3";;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index a5e21bf8bf..1167b835a4 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -773,8 +773,8 @@ test_expect_success TTY 'status --porcelain ignores color.status' '
 '
 
 # recover unconditionally from color tests
-git config --unset color.status
-git config --unset color.ui
+git config --unset color.status || :
+git config --unset color.ui || :
 
 test_expect_success 'status --porcelain respects -b' '
 
diff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh
index 784ec7fc2d..5bb38cb23a 100755
--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -68,8 +68,8 @@ test_expect_success 'authors-file overrode authors-prog' '
 	)
 '
 
-git --git-dir=x/.git config --unset svn.authorsfile
-git --git-dir=x/.git config --unset svn.authorsprog
+git --git-dir=x/.git config --unset svn.authorsfile || :
+git --git-dir=x/.git config --unset svn.authorsprog || :
 
 test_expect_success 'authors-prog imported user without email' '
 	svn mkdir -m gg --username gg-hermit "$svnrepo"/gg &&

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 05/12] t: prepare conditional test execution for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-5-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
We have some test in our test suite where we use the pattern of
`test ... && test_expect_succeess` to conditionally execute a test. The
problem is that when we decide to not execute the test, we'll indeed
skip the test, but the overall statement will also be unsuccessful. This
will become a problem once we enable `set -e`.

Prepare for this future by turning this into a proper conditional, which
is also a bit easier to read overall.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t4032-diff-inter-hunk-context.sh | 12 +++++++-----
 t/t7450-bad-git-dotfiles.sh        | 24 +++++++++++++-----------
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh
index c98eb6abb2..2d216fb70f 100755
--- a/t/t4032-diff-inter-hunk-context.sh
+++ b/t/t4032-diff-inter-hunk-context.sh
@@ -40,11 +40,13 @@ t() {
 		test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks
 	"
 
-	test -f $expected &&
-	test_expect_success "$label: check output" "
-		git $cmd $file | grep -v '^index ' >actual &&
-		test_cmp $expected actual
-	"
+	if test -f $expected
+	then
+		test_expect_success "$label: check output" "
+			git $cmd $file | grep -v '^index ' >actual &&
+			test_cmp $expected actual
+		"
+	fi
 }
 
 cat <<EOF >expected.f1.0.1 || exit 1
diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index f512eed278..8cc86522b2 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -220,17 +220,19 @@ check_dotx_symlink () {
 		)
 	'
 
-	test -n "$refuse_index" &&
-	test_expect_success "refuse to load symlinked $name into index ($type)" '
-		test_must_fail \
-			git -C $dir \
-			    -c core.protectntfs \
-			    -c core.protecthfs \
-			    read-tree $tree 2>err &&
-		grep "invalid path.*$name" err &&
-		git -C $dir ls-files -s >out &&
-		test_must_be_empty out
-	'
+	if test -n "$refuse_index"
+	then
+		test_expect_success "refuse to load symlinked $name into index ($type)" '
+			test_must_fail \
+				git -C $dir \
+				    -c core.protectntfs \
+				    -c core.protecthfs \
+				    read-tree $tree 2>err &&
+			grep "invalid path.*$name" err &&
+			git -C $dir ls-files -s >out &&
+			test_must_be_empty out
+		'
+	fi
 }
 
 check_dotx_symlink gitmodules vanilla .gitmodules

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 06/12] t: prepare execution of potentially failing commands for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-6-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
Several of our tests verify whether a certain binary can be executed,
potentially skipping tests in case we cannot, for example because the
binary doesn't exist. In those cases we often run the binary outside of
any conditionally.

This will start to fail once we enable `set -e`, as that will cause us
to bail out the test immediately. Improve these tests by executing them
inside of a conditional instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-git-svn.sh                |  7 +++----
 t/lib-httpd.sh                  |  3 +--
 t/t3901-i18n-patch.sh           |  3 ++-
 t/t5000-tar-tree.sh             |  4 ++--
 t/t7422-submodule-output.sh     |  2 +-
 t/t9200-git-cvsexportcommit.sh  |  3 +--
 t/t9400-git-cvsserver-server.sh |  5 +++--
 t/t9401-git-cvsserver-crlf.sh   |  4 ++--
 t/t9402-git-cvsserver-refs.sh   |  4 ++--
 t/test-lib-functions.sh         |  3 +--
 t/test-lib.sh                   | 10 ++++++----
 11 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 2fde2353fd..52843f667d 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -15,8 +15,7 @@ GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
 SVN_TREE=$GIT_SVN_DIR/svn-tree
 test_set_port SVNSERVE_PORT
 
-svn >/dev/null 2>&1
-if test $? -ne 1
+if ! svn help >/dev/null 2>&1
 then
 	skip_all='skipping git svn tests, svn not found'
 	test_done
@@ -27,13 +26,13 @@ export svnrepo
 svnconf=$PWD/svnconf
 export svnconf
 
+x=0
 perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);
 system(qw/svnadmin create --fs-type fsfs/, \$ENV{svnrepo}) == 0 or exit(41);
-" >&3 2>&4
-x=$?
+" >&3 2>&4 || x=$?
 if test $x -ne 0
 then
 	if test $x -eq 42; then
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 4c76e813e3..fc646447d5 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -235,11 +235,10 @@ start_httpd() {
 
 	test_atexit stop_httpd
 
-	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
+	if ! "$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
 		-f "$TEST_PATH/apache.conf" $HTTPD_PARA \
 		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start \
 		>&3 2>&4
-	if test $? -ne 0
 	then
 		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
 		test_skip_or_die GIT_TEST_HTTPD "web server setup failed"
diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
index f03601b49a..ef7d7e1edc 100755
--- a/t/t3901-i18n-patch.sh
+++ b/t/t3901-i18n-patch.sh
@@ -28,7 +28,8 @@ check_encoding () {
 		8859)
 			grep "^encoding ISO8859-1" ;;
 		*)
-			grep "^encoding ISO8859-1"; test "$?" != 0 ;;
+			ret=0; grep "^encoding ISO8859-1" || ret=$?
+			test "$ret" != 0 ;;
 		esac || return 1
 		j=$i
 		i=$(($i+1))
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 5465054f17..a8c28533dc 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -503,8 +503,8 @@ test_expect_success LONG_IS_64BIT 'set up repository with huge blob' '
 # would generate the whole 64GB).
 test_expect_success LONG_IS_64BIT 'generate tar with huge size' '
 	{
-		git archive HEAD
-		echo $? >exit-code
+		{ ret=0 && git archive HEAD || ret=$?; } &&
+		echo "$ret" >exit-code
 	} | test_copy_bytes 4096 >huge.tar &&
 	echo 141 >expect &&
 	test_cmp expect exit-code
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
index aea1ddf117..852136fdfd 100755
--- a/t/t7422-submodule-output.sh
+++ b/t/t7422-submodule-output.sh
@@ -198,7 +198,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'
 	(
 		cd repo &&
 		GIT_ALLOW_PROTOCOL=file git submodule add "$(pwd)"/../submodule &&
-		{ git submodule status --recursive 2>err; echo $?>status; } |
+		{ { ret=0 && git submodule status --recursive 2>err || ret=$?; } && echo $ret >status; } |
 			grep -q recursive-submodule-path-1 &&
 		test_must_be_empty err &&
 		test_match_signal 13 "$(cat status)"
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 14cbe96527..581cf3d28f 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -11,8 +11,7 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+if ! cvs version >/dev/null 2>&1
 then
     skip_all='skipping git cvsexportcommit tests, cvs not found'
     test_done
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index e499c7f955..4b45398bab 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -17,12 +17,13 @@ if ! test_have_prereq PERL; then
 	skip_all='skipping git cvsserver tests, perl not available'
 	test_done
 fi
-cvs >/dev/null 2>&1
-if test $? -ne 1
+
+if ! cvs version >/dev/null 2>&1
 then
     skip_all='skipping git-cvsserver tests, cvs not found'
     test_done
 fi
+
 perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     skip_all='skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index a34805acdc..6b4cbb1651 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -60,12 +60,12 @@ check_status_options() {
     return $stat
 }
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+if ! cvs version >/dev/null 2>&1
 then
     skip_all='skipping git-cvsserver tests, cvs not found'
     test_done
 fi
+
 if ! test_have_prereq PERL
 then
     skip_all='skipping git-cvsserver tests, perl not available'
diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh
index 2ee41f9443..65f2ceedec 100755
--- a/t/t9402-git-cvsserver-refs.sh
+++ b/t/t9402-git-cvsserver-refs.sh
@@ -68,12 +68,12 @@ check_diff() {
 
 #########
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+if ! cvs version >/dev/null 2>&1
 then
 	skip_all='skipping git-cvsserver tests, cvs not found'
 	test_done
 fi
+
 if ! test_have_prereq PERL
 then
 	skip_all='skipping git-cvsserver tests, perl not available'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5fd5494ef1..879ee1ee59 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1248,8 +1248,7 @@ test_might_fail () {
 test_expect_code () {
 	want_code=$1
 	shift
-	"$@" 2>&7
-	exit_code=$?
+	exit_code=0; "$@" 2>&7 || exit_code=$?
 	if test $exit_code = $want_code
 	then
 		return 0
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 70fd3e9baf..de7d9e7b92 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -143,8 +143,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 ################################################################
 # It appears that people try to run tests without building...
 GIT_BINARY="${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X"
-"$GIT_BINARY" >/dev/null
-if test $? != 1
+
+if ! "$GIT_BINARY" version >/dev/null
 then
 	if test -n "$GIT_TEST_INSTALLED"
 	then
@@ -454,8 +454,10 @@ then
 	# from any previous runs.
 	>"$GIT_TEST_TEE_OUTPUT_FILE"
 
-	(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
-	 echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+	(
+		ret=0 && GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1 || ret=$?
+		echo "$ret" >"$TEST_RESULTS_BASE.exit"
+	) | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
 	test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
 	exit
 fi

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 07/12] t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-7-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
Both `test_when_finished ()` and `test_atexit ()` build up a chain of
cleanup commands by prepending each new command to the existing cleanup
string. To preserve the exit code of the test body across cleanup
execution, we append the following logic:

    } && (exit "$eval_ret"); eval_ret=$?; ...

The intent of this is to run the cleanup block and then unconditionally
restore `eval_ret`. The original behaviour of this is is:

   +------------------+---------+------------------------------------+
   |test body         │ cleanup │ old behaviour                      │
   +------------------+---------+------------------------------------+
   │pass (eval_ret=0) | pass    │ && taken -> (exit 0) -> eval_ret=0 |
   +------------------+---------+------------------------------------+
   │pass (eval_ret=0) | fail    │ && not taken -> eval_ret=$?        |
   +------------------+---------+------------------------------------+
   │fail (eval_ret=1) | pass    │ && taken -> (exit 1) -> eval_ret=1 |
   +------------------+---------+------------------------------------+
   │fail (eval_ret=1) | fail    | && not taken -> eval_ret=$?        |
   +------------------+---------+------------------------------------+

This logic will start to fail once we enable `set -e`. When `$eval_ret`
is non-zero, the subshell we create will fail, and with `set -e` we'll
thus bail out without evaluating the logic after the semicolon.

Fix this issue by instead using `|| eval_ret=\$?; ...`. Besides being
a bit simpler, it also retains the original behaviour:

   +------------------+---------+------------------------------------+
   |test body         │ cleanup │ old behaviour                      │
   +------------------+---------+------------------------------------+
   │pass (eval_ret=0) | pass    │ || not taken -> eval_ret unchanged |
   +------------------+---------+------------------------------------+
   │pass (eval_ret=0) | fail    │ || taken -> eval_ret=$?            |
   +------------------+---------+------------------------------------+
   │fail (eval_ret=1) | pass    │ || not taken -> eval_ret unchanged |
   +------------------+---------+------------------------------------+
   │fail (eval_ret=1) | fail    | || taken -> eval_ret=$?            |
   +------------------+---------+------------------------------------+

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 879ee1ee59..502bb0ddcb 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1512,7 +1512,7 @@ test_when_finished () {
 	test "${BASH_SUBSHELL-0}" = 0 ||
 	BUG "test_when_finished does nothing in a subshell"
 	test_cleanup="{ $*
-		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
+		} || eval_ret=\$?; $test_cleanup"
 }
 
 # This function can be used to schedule some commands to be run
@@ -1540,7 +1540,7 @@ test_atexit () {
 	test "${BASH_SUBSHELL-0}" = 0 ||
 	BUG "test_atexit does nothing in a subshell"
 	test_atexit_cleanup="{ $*
-		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
+		} || eval_ret=\$?; $test_atexit_cleanup"
 }
 
 # Deprecated wrapper for "git init", use "git init" directly instead

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 08/12] t0008: silence error in subshell when using `grep -v`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-8-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
In t0008 we use `grep -v` in a subshell, but expect that this command
will sometimes not match anything. This would cause grep(1) to return an
error code, but given that we don't run with `set -e` we swallow this
error.

We're about to enable `set -e`. Prepare for this by ignoring any errors.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0008-ignores.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index e716b5cdfa..d77a179bdd 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -122,8 +122,8 @@ test_expect_success_multiple () {
 	fi
 	testname="$1" expect_all="$2" code="$3"
 
-	expect_verbose=$( echo "$expect_all" | grep -v '^::	' )
-	expect=$( echo "$expect_verbose" | sed -e 's/.*	//' )
+	expect_verbose=$(echo "$expect_all" | grep -v '^::	' || :)
+	expect=$(echo "$expect_verbose" | sed -e 's/.*	//')
 
 	test_expect_success $prereq "$testname${no_index_opt:+ with $no_index_opt}" '
 		expect "$expect" &&

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 09/12] t1301: don't fail in case setfacl(1) doesn't exist or fails
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-9-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
In t1301 we're trying to remove any potentially-existing default ACLs
that might exist on the transh directory by executing setfacl(1).
According to 8ed0a740dd (t1301-shared-repo.sh: don't let a default ACL
interfere with the test, 2008-10-16), this is done because we play
around with permissions and umasks in this test suite.

The setfacl(1) binary may not exist on some systems though, even though
tests ultimately still pass. This doesn't matter currently, but will
cause the test to fail once we start running with `set -e`. Silence such
failures by ignoring failures here.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1301-shared-repo.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 630a47af21..0e0d07a1a1 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -12,7 +12,7 @@ TEST_CREATE_REPO_NO_TEMPLATE=1
 . ./test-lib.sh
 
 # Remove a default ACL from the test dir if possible.
-setfacl -k . 2>/dev/null
+setfacl -k . 2>/dev/null || :
 
 # User must have read permissions to the repo -> failure on --shared=0400
 test_expect_success 'shared = 0400 (faulty permission u-w)' '

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 10/12] t6002: fix use of `expr` with `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-10-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
In `test_bisection_diff ()` we use `expr` to perform some math. This
command has some gotchas though in that it will only return success when
the result is neither null nor zero. In some of our cases though it
actually _is_ zero, and that will cause the expressions to fail once we
enable `set -e`.

Prepare for this change by instead using `$(( ))`, which doesn't have
the same issue. While at it, modernize the function a tiny bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t6002-rev-list-bisect.sh | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index daa009c9a1..f2de40b5ed 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -27,13 +27,16 @@ test_bisection_diff()
 	# Test if bisection size is close to half of list size within
 	# tolerance.
 	#
-	_bisect_err=$(expr $_list_size - $_bisection_size \* 2)
-	test "$_bisect_err" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)
-	_bisect_err=$(expr $_bisect_err / 2) ; # floor
-
-	test_expect_success \
-	"bisection diff $_bisect_option $_head $* <= $_max_diff" \
-	'test $_bisect_err -le $_max_diff'
+	_bisect_err=$(($_list_size - $_bisection_size * 2))
+	if test "$_bisect_err" -lt 0
+	then
+		_bisect_err=$((0 - $_bisect_err))
+	fi
+	_bisect_err=$(($_bisect_err / 2)) ; # floor
+
+	test_expect_success "bisection diff $_bisect_option $_head $* <= $_max_diff" '
+		test $_bisect_err -le $_max_diff
+	'
 }
 
 date >path0

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 11/12] t9902: fix use of `read` with `set -e`
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-11-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
In t9902 we're using the `read` builtin to read some values into a
variable. This is done by using `-d ""`, which cause us to read until
the end of the heredoc. As the read is terminated by EOF, the command
will end up returning a non-zero error code. This hasn't been an issue
until now as we didn't run with `set -e`, but that'll change in a
subsequent commit.

Prepare for this change by not using read at all, as we can simply store
the multi-line value directly.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9902-completion.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2f9a597ec7..28f61f08fb 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -590,12 +590,10 @@ test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '
 	__gitcomp "$invalid_variable_name"
 '
 
-read -r -d "" refs <<-\EOF
-main
+refs='main
 maint
 next
-seen
-EOF
+seen'
 
 test_expect_success '__gitcomp_nl - trailing space' '
 	test_gitcomp_nl "m" "$refs" <<-EOF

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Patrick Steinhardt, 2026-04-17 10:50

Subject: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <20260417-b4-pks-tests-with-set-e-v4-12-44d43efdafb1@pks.im>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-0-44d43efdafb1@pks.im>

```
We have recently merged a patch series that had a simple misspelling of
`test_expect_success`. Instead of making our tests fail though, this
typo went completely undetected and all of our tests passed, which is of
course unfortunate. This is a more general issue with our test suite:
all commands that run outside of a specific test case can fail, and if
we don't explicitly check for such failure then this failure will be
silently ignored.

Improve the status quo by enabling the errexit option so that any such
unchecked failures will cause us to abort immediately.

Note that for now, we only enable this option for Bash 5 and newer. This
is because other shells have wildly different behaviour, and older
versions of Bash (especially on macOS) are buggy. The list of enabled
shells may be extended going forward.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/run-build-and-tests.sh | 5 +++++
 t/test-lib.sh             | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 28cfe730ee..f0a3597184 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -15,6 +15,11 @@ fedora-breaking-changes-musl|linux-breaking-changes)
 	MESONFLAGS="$MESONFLAGS -Drust=enabled"
 	;;
 linux-TEST-vars)
+	# Ubuntu uses Dash by default, but we only enable use of `set -e`
+	# when using Bash 5+. Ensure that we have at least one CI job that uses
+	# it.
+	export TEST_SHELL_PATH=/usr/bin/bash
+
 	export OPENSSL_SHA1_UNSAFE=YesPlease
 	export GIT_TEST_SPLIT_INDEX=yes
 	export GIT_TEST_FULL_IN_PACK_ARRAY=true
diff --git a/t/test-lib.sh b/t/test-lib.sh
index de7d9e7b92..1f7868c537 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,6 +15,15 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see https://www.gnu.org/licenses/ .
 
+# Enable the use of errexit so that any unexpected failures will cause us to
+# abort tests, even when outside of a specific test case. Note that we only
+# enable this on Bash 5 and newer, as `set -e` has wildly different behaviour
+# across shells. The list of allowed shells may be extended going forward.
+if test "${BASH_VERSINFO:=0}" -ge 5
+then
+	set -e
+fi
+
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"

-- 
2.54.0.rc2.529.gd9106f7525.dirty


```

## Jeff King, 2026-04-18 06:50

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <20260418065009.GA2619713@coredump.intra.peff.net>
In-Reply-To: <20260417-b4-pks-tests-with-set-e-v4-12-44d43efdafb1@pks.im>

```
On Fri, Apr 17, 2026 at 12:50:58PM +0200, Patrick Steinhardt wrote:

> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -15,6 +15,11 @@ fedora-breaking-changes-musl|linux-breaking-changes)
>  	MESONFLAGS="$MESONFLAGS -Drust=enabled"
>  	;;
>  linux-TEST-vars)
> +	# Ubuntu uses Dash by default, but we only enable use of `set -e`
> +	# when using Bash 5+. Ensure that we have at least one CI job that uses
> +	# it.
> +	export TEST_SHELL_PATH=/usr/bin/bash

Thinking on this a little more, it is a shame we cannot easily enable
this for dash. That would hit most CI jobs, but also the local builds of
most developers. And finding problems early and locally often saves a
lot of time versus finding them in CI.

Unfortunately I could not find a way to detect whether we are running
dash at all, let alone a recent version. But what if we let the user
tell us? Something like:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1f7868c537..a0d07f75fb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -17,9 +17,10 @@
 
 # Enable the use of errexit so that any unexpected failures will cause us to
 # abort tests, even when outside of a specific test case. Note that we only
-# enable this on Bash 5 and newer, as `set -e` has wildly different behaviour
-# across shells. The list of allowed shells may be extended going forward.
-if test "${BASH_VERSINFO:=0}" -ge 5
+# enable this by default on Bash 5 and newer, as `set -e` has wildly different
+# behaviour across shells. If you trust your shell's `set -e` implementation,
+# you can set GIT_TEST_USE_SET_E manually.
+if test "$GIT_TEST_USE_SET_E" = 1 && test "${BASH_VERSINFO:=0}" -ge 5
 then
 	set -e
 fi

And then those of us who want to stick:

  export GIT_TEST_USE_SET_E = 1

in our config.mak can do so, and we could even set it in the ci/ scripts
for all of the ubuntu builds.

-Peff

```

## Ben Knoble, 2026-04-18 12:17

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <AA6F33AD-25C2-4AB0-A624-35C7B0BE0F66@gmail.com>
In-Reply-To: <20260418065009.GA2619713@coredump.intra.peff.net>

```

> Le 18 avr. 2026 à 02:50, Jeff King <peff@peff.net> a écrit :
> 
> On Fri, Apr 17, 2026 at 12:50:58PM +0200, Patrick Steinhardt wrote:
> 
>> --- a/ci/run-build-and-tests.sh
>> +++ b/ci/run-build-and-tests.sh
>> @@ -15,6 +15,11 @@ fedora-breaking-changes-musl|linux-breaking-changes)
>>    MESONFLAGS="$MESONFLAGS -Drust=enabled"
>>    ;;
>> linux-TEST-vars)
>> +    # Ubuntu uses Dash by default, but we only enable use of `set -e`
>> +    # when using Bash 5+. Ensure that we have at least one CI job that uses
>> +    # it.
>> +    export TEST_SHELL_PATH=/usr/bin/bash
> 
> Thinking on this a little more, it is a shame we cannot easily enable
> this for dash. That would hit most CI jobs, but also the local builds of
> most developers. And finding problems early and locally often saves a
> lot of time versus finding them in CI.
> 
> Unfortunately I could not find a way to detect whether we are running
> dash at all, let alone a recent version. But what if we let the user
> tell us? Something like:

I 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

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1f7868c537..a0d07f75fb 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -17,9 +17,10 @@
> 
> # Enable the use of errexit so that any unexpected failures will cause us to
> # abort tests, even when outside of a specific test case. Note that we only
> -# enable this on Bash 5 and newer, as `set -e` has wildly different behaviour
> -# across shells. The list of allowed shells may be extended going forward.
> -if test "${BASH_VERSINFO:=0}" -ge 5
> +# enable this by default on Bash 5 and newer, as `set -e` has wildly different
> +# behaviour across shells. If you trust your shell's `set -e` implementation,
> +# you can set GIT_TEST_USE_SET_E manually.
> +if test "$GIT_TEST_USE_SET_E" = 1 && test "${BASH_VERSINFO:=0}" -ge 5
> then
>    set -e
> fi

I guess that should be || instead of &&?

> 
> And then those of us who want to stick:
> 
>  export GIT_TEST_USE_SET_E = 1
> 
> in our config.mak can do so, and we could even set it in the ci/ scripts
> for all of the ubuntu builds.
> 
> -Peff

Thanks
```

## Jeff King, 2026-04-18 17:44

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <20260418174446.GA1695@coredump.intra.peff.net>
In-Reply-To: <AA6F33AD-25C2-4AB0-A624-35C7B0BE0F66@gmail.com>

```
On Sat, Apr 18, 2026 at 08:17:10AM -0400, Ben Knoble wrote:

> > +if test "$GIT_TEST_USE_SET_E" = 1 && test "${BASH_VERSINFO:=0}" -ge 5
> > then
> >    set -e
> > fi
> 
> I guess that should be || instead of &&?

Oops, yeah. I wrote it correctly and tested it once, and then started to
rewrite it to support setting it to 0, like:

  if test -z "$GIT_TEST_USE_SET_E" && test "${BASH_VERSINFO:=0}" -ge 5
  then
	GIT_TEST_USE_SET_E=1
  fi
  case "$GIT_TEST_USE_SET_E" in
  1|on|true)
	set -e
	;;
  esac

But I didn't want to get too much into details of the patch, so I went
back to the original, but obviously screwed that up. ;)

-Peff

```

## brian m. carlson, 2026-04-18 19:17

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <aePY1x9uO39p6WDI@fruit.crustytoothpaste.net>
In-Reply-To: <AA6F33AD-25C2-4AB0-A624-35C7B0BE0F66@gmail.com>

```
On 2026-04-18 at 12:17:10, Ben Knoble wrote:
> 
> > Le 18 avr. 2026 à 02:50, Jeff King <peff@peff.net> a écrit :
> > Thinking on this a little more, it is a shame we cannot easily enable
> > this for dash. That would hit most CI jobs, but also the local builds of
> > most developers. And finding problems early and locally often saves a
> > lot of time versus finding them in CI.
> > 
> > Unfortunately I could not find a way to detect whether we are running
> > dash at all, let alone a recent version. But what if we let the user
> > tell us? Something like:
> 
> I 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

I hard-coded this on with a bunch of shells in Debian unstable using the
below script.  zsh, busybox, and dash passed, while mksh, lksh, and posh
failed.  (The latter are all pdksh variants, I believe, so they are an
important set of shells to consider.)

Note that the script symlinks the shell to `sh` so that everyone will be
on their best POSIX behaviour.

----
#!/bin/sh

dir=$(mktemp -d)
trap 'rm -fr "$dir"' EXIT

sh="$1"
ln -sf "$sh" "$dir/sh"

make -j12 all && (cd t && GIT_PROVE_OPTS=-j12 GIT_TEST_DEFAULT_HASH=sha256 PATH="$dir:$PATH" SHELL_PATH="$dir/sh" make prove)
----

Having said that, I actually think that mksh may be right in at least
one case.  For instance, this diff seems required for mksh to pass t1410
and I believe this is actually the right thing to do:

----
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index ce71f9a30a..f289fc11e9 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -23,7 +23,7 @@ check_have () {
 }
 
 check_fsck () {
-	git fsck --full >fsck.output
+	git fsck --full >fsck.output || true
 	case "$1" in
 	'')
 		test_must_be_empty fsck.output ;;
----

I haven't checked the other cases under mksh, but I think it may be a
fruitful source of things to look at.  And if you find a bug, I'm sure
the maintainer would happily accept a bug report in the Debian BTS.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

```

## Junio C Hamano, 2026-04-18 19:24

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <xmqqtst8ul4q.fsf@gitster.g>
In-Reply-To: <20260418174446.GA1695@coredump.intra.peff.net>

```
Jeff King <peff@peff.net> writes:

> On Sat, Apr 18, 2026 at 08:17:10AM -0400, Ben Knoble wrote:
>
>> > +if test "$GIT_TEST_USE_SET_E" = 1 && test "${BASH_VERSINFO:=0}" -ge 5
>> > then
>> >    set -e
>> > fi
>> 
>> I guess that should be || instead of &&?
>
> Oops, yeah. I wrote it correctly and tested it once, and then started to
> rewrite it to support setting it to 0, like:
>
>   if test -z "$GIT_TEST_USE_SET_E" && test "${BASH_VERSINFO:=0}" -ge 5
>   then
> 	GIT_TEST_USE_SET_E=1
>   fi
>   case "$GIT_TEST_USE_SET_E" in
>   1|on|true)
> 	set -e
> 	;;
>   esac
>
> But I didn't want to get too much into details of the patch, so I went
> back to the original, but obviously screwed that up. ;)

We could forget about "we know this is a good shell by its name and
version" and test the feature we depend on ourselves, perhaps?

```

## Jeff King, 2026-04-18 21:05

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <20260418210518.GA9632@coredump.intra.peff.net>
In-Reply-To: <xmqqtst8ul4q.fsf@gitster.g>

```
On Sat, Apr 18, 2026 at 12:24:53PM -0700, Junio C Hamano wrote:

> We could forget about "we know this is a good shell by its name and
> version" and test the feature we depend on ourselves, perhaps?

I looked into that but didn't get anywhere useful. You can try to test
all of the "set -e" scenarios we care about, but there are a lot of
them. For example, I would never have thought to check how "command"
behaves inside a &&-chain while "set -e" is in effect.

So you basically end up adding a test case for the bugs you find, at
which point it is not much better than blocking known-bad versions.

-Peff

```

## Jeff King, 2026-04-18 21:30

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <20260418213043.GB9632@coredump.intra.peff.net>
In-Reply-To: <aePY1x9uO39p6WDI@fruit.crustytoothpaste.net>

```
On Sat, Apr 18, 2026 at 07:17:43PM +0000, brian m. carlson wrote:

> Having said that, I actually think that mksh may be right in at least
> one case.  For instance, this diff seems required for mksh to pass t1410
> and I believe this is actually the right thing to do:

I think mksh is wrong here, if it is flagging this fsck call.

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index ce71f9a30a..f289fc11e9 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -23,7 +23,7 @@ check_have () {
>  }
>  
>  check_fsck () {
> -	git fsck --full >fsck.output
> +	git fsck --full >fsck.output || true
>  	case "$1" in
>  	'')
>  		test_must_be_empty fsck.output ;;

If check_fsck() were run by itself then yes, this would be a problem.
But it is always run inside a test snippet, and there "set -e" should
always be suppressed because test_expect_success does:

  if test_run_ "$test_body"

So we are inside a conditional, and the usual global "set -e"
suppression should happen. It sounds like it is not happening in your
version of mksh, but I was unable to get t1410 to fail at all using mksh
59c-43 (from Debian unstable) or 59c-41 (from stable).

And it is a good thing that this "if" suppression is here, or else tests
who fail the final component of the &&-chain would cause the shell to
exit. The simplest case is just:

  test_expect_success 'bad' 'false'

If "set -e" were in effect, then the whole script would bail upon seeing
that "false".

-Peff

```

## brian m. carlson, 2026-04-18 21:54

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <aeP9stvssuTv0FD7@fruit.crustytoothpaste.net>
In-Reply-To: <20260418213043.GB9632@coredump.intra.peff.net>

```
On 2026-04-18 at 21:30:43, Jeff King wrote:
> If check_fsck() were run by itself then yes, this would be a problem.
> But it is always run inside a test snippet, and there "set -e" should
> always be suppressed because test_expect_success does:
> 
>   if test_run_ "$test_body"
> 
> So we are inside a conditional, and the usual global "set -e"
> suppression should happen. It sounds like it is not happening in your
> version of mksh, but I was unable to get t1410 to fail at all using mksh
> 59c-43 (from Debian unstable) or 59c-41 (from stable).

It does fail with 59c-43 under `make prove` or if you do `sh ./t1410*.sh
--verbose`, assuming that `sh` points to `mksh`, but since the script
has a `/bin/sh` shebang, you need to invoke it explicitly with the shell
in question, or it will use the system `sh` (dash).  (I made this
mistake when reproducing the problem.)

Note that the test in question does not exit, but returns this (with
`--verbose`):

----
Checking ref database: 100% (1/1), done.
Checking object directories: 100% (256/256), done.
not ok 7 - corrupt and check
----

and this:

----
Checking ref database: 100% (1/1), done.
Checking object directories: 100% (256/256), done.
not ok 8 - reflog expire --dry-run should not touch reflog
----

It does seem like this _is_ a bug in mksh, though, which I've reproduced
with a test script, so I'll report it there.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

```

## Jeff King, 2026-04-19 02:10

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <20260419021027.GA1079904@coredump.intra.peff.net>
In-Reply-To: <aeP9stvssuTv0FD7@fruit.crustytoothpaste.net>

```
On Sat, Apr 18, 2026 at 09:54:58PM +0000, brian m. carlson wrote:

> > So we are inside a conditional, and the usual global "set -e"
> > suppression should happen. It sounds like it is not happening in your
> > version of mksh, but I was unable to get t1410 to fail at all using mksh
> > 59c-43 (from Debian unstable) or 59c-41 (from stable).
> 
> It does fail with 59c-43 under `make prove` or if you do `sh ./t1410*.sh
> --verbose`, assuming that `sh` points to `mksh`, but since the script
> has a `/bin/sh` shebang, you need to invoke it explicitly with the shell
> in question, or it will use the system `sh` (dash).  (I made this
> mistake when reproducing the problem.)

Doh. The problem was none of that, but that I was using Patrick's
version of the patch that only turns on "set -e" for bash.

So yeah, after actually enabling "set -e" I do see the failure.

> It does seem like this _is_ a bug in mksh, though, which I've reproduced
> with a test script, so I'll report it there.

I looked up your report in Debian's system. I think you're right that
the eval is the problem. The smallest reproduction I came up with is:

  $ dash -ec 'eval "false; true" && echo ok'
  ok

  $ mksh -ec 'eval "false; true" && echo ok'
  [no output]

So it respects "-e" within the eval, which is wrong, and then doubly
weird that "-e" bails from the eval but not the whole script.

-Peff

```

## Patrick Steinhardt, 2026-04-20 06:11

Subject: Re: [PATCH v4 12/12] t: detect errors outside of test cases
Message-ID: <aeXDdvt3YGrJFcSX@pks.im>
In-Reply-To: <20260418210518.GA9632@coredump.intra.peff.net>

```
On Sat, Apr 18, 2026 at 05:05:18PM -0400, Jeff King wrote:
> On Sat, Apr 18, 2026 at 12:24:53PM -0700, Junio C Hamano wrote:
> 
> > We could forget about "we know this is a good shell by its name and
> > version" and test the feature we depend on ourselves, perhaps?
> 
> I looked into that but didn't get anywhere useful. You can try to test
> all of the "set -e" scenarios we care about, but there are a lot of
> them. For example, I would never have thought to check how "command"
> behaves inside a &&-chain while "set -e" is in effect.
> 
> So you basically end up adding a test case for the bugs you find, at
> which point it is not much better than blocking known-bad versions.

Yeah, agreed. If it was only one or two cases I'd definitely agree with
Junio. But I have a feeling that every shell will behave slightly
different here, and there's even differences between versions of the
same shell.

I'll go with Peff's proposal to have an explicit opt-in, thanks!

Patrick

```