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

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(+)

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

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 guess that should be || instead of &&?

Thanks

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

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

On 2026-04-18 at 12:17:10, Ben Knoble wrote:

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:

----

Jeff King <peff@peff.net> writes:

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?

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

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

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

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

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

On 2026-04-18 at 21:30:43, Jeff King wrote:

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.

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

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.

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

On Sat, Apr 18, 2026 at 05:05:18PM -0400, Jeff King wrote:

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