Recently we saw an unusual typo in a test that misspelt "test_expect_success", but this was not noticed for a while primarily because the test script itself did not fail due to this typo. The shell and the test framework did say

    tXXXX-xxx.sh: line 22: test_expect_successo: command not found

but otherwise kept going.

One way to help us detect such an error is to run our test under "set -e", which will abort execution after any command exits with non-zero status.

However, there are a handful of places in our existing tests and the test framework itself that depends on the current behaviour of silently ignoring a failing command. Here is an attempt to fix them.

The first step turns "set -e" on very early in the test framework, and fixes one place in the framework that assumed that a failing command is OK.

The remainder of the series fix one test script per one patch, and at the end of the series, the whole test suite pass for me, even when merged to the tip of 'seen'.

Note that I let cvs, svn, and p4 tests run only up to the point that they decide to punt due to lack of external tools and language bindings they require, so for those of you who do have the necessary bindings, the scripts may still fail due to construct that are not "set -e" clean after they call "test_done" for me.

 01/11: test-lib: catch misspelt 'test_expect_successo'
 02/11: t0008: make test "set -e" clean
 03/11: t6002: make test "set -e" clean
 04/11: t4032: make test "set -e" clean
 05/11: t7450: make test "set -e" clean
 06/11: tests: make svn test "set -e" clean
 07/11: t7508: make test "set -e" clean
 08/11: t9200: make test "set -e" clean
 09/11: t940?: make test "set -e" clean
 10/11: t5570: make test "set -e" clean
 11/11: t9902: make test "set -e" clean

 t/lib-git-daemon.sh                | 6 +++---
 t/lib-git-svn.sh                   | 7 +++----
 t/t0008-ignores.sh                 | 2 +-
 t/t4032-diff-inter-hunk-context.sh | 4 ++--
 t/t6002-rev-list-bisect.sh         | 4 ++--
 t/t7450-bad-git-dotfiles.sh        | 2 +-
 t/t7508-status.sh                  | 4 ++--
 t/t9200-git-cvsexportcommit.sh     | 4 ++--
 t/t9400-git-cvsserver-server.sh    | 4 ++--
 t/t9401-git-cvsserver-crlf.sh      | 4 ++--
 t/t9402-git-cvsserver-refs.sh      | 4 ++--
 t/t9902-completion.sh              | 2 +-
 t/test-lib.sh                      | 7 +++++--
 13 files changed, 28 insertions(+), 26 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

All tests dot-source "test-lib.sh" as the first thing to do. Starting the script with "set -e" immediately reveals one place in the test framework itself that is not clean.

The test framework runs "$GIT_BINARY" without any argument. We expect it to exit with status 1 (i.e., "git<RETURN>" that spits out the list of common commands) as a sign that we have an instance of Git that we want to test. We cannot quite say

    git
    if test $? != 1; then you have not built git; fi

as the first invocation that exits non-zero is caught with "set -e".

Work this around by rewriting the construct like so:

    status=0; git || status=$?
    if test $status != 1; then you have not built git; fi

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

A piece of script used "grep" to filter out its input purely for its output, but of course, "grep" reports with its exit value when it did not see any hits, which didn't mesh quite well with "set -e".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0008-ignores.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

We often use

      val=$(expr expression)

only for the computation, and it is good that "expr" exits non-zero with syntactically invalid expression (it exits with 2) and other errors (with 3), as we do want to notice such errors.

"expr" however also exits with "1" if it yields 0 or null X-<.

Make sure we do not fail unnecessarily under "set -e".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6002-rev-list-bisect.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

A few shell construct used in this test were not ready. Make them so.

 * "git config --unset VAR" can fail when VAR is not defined.

 * The author of "test -f X && run test that uses X" written here
   really wanted to say "if file X is there, then run the test", not
   "file X must exist and the test using it must succeed".  The
   proper way to express it is to say "test ! -f X || use X".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4032-diff-inter-hunk-context.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

Often we write "A && test_expect_success ..." and want it to mean "If and only if A holds true, this needs to be tested", but under "set -e", this will cause failure when A does not hold true. We need to write "!A || test_expect_success ..." if we want to run the test conditionally.

Or write it properly with if/then/fi, perhaps like:

	if ! A
	then
		test_expect_success ...
	fi

Make sure we do not fail unnecessarily under "set -e".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7450-bad-git-dotfiles.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

The git-svn helper scriptlet suffers from two instances of the recurring pattern where a sequence

	cmd ...
	if test $? ...

expects cmd to be allowed to fail freely and we can act on its exit status, which is not possible under "set -e".

As the second instance uses an extra variable $x to capture the status of the failed command already, let's use that variable to rewrite the above pattern to

	x=0; cmd ... || x=$?
	if test $x ...

which means the same thing but does not fail under "set -e".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-git-svn.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

This test tries to unconditionally clear a few configuration variables, but "git config --unset VAR" fails if VAR is not set. Work it around by telling the shell that failures from them are OK.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7508-status.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

This test uses the usual pattern, where it

	cmd ...
	if test $? ...

expects cmd to be allowed to fail freely and we can act on its exit status, which is not possible under "set -e". Rewrite it using the common pattern:

	status=0; cmd ... || status=$?
	if test $status ...

which means the same thing but does not fail under "set -e".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9200-git-cvsexportcommit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

The cverserver tests have the usual pattern, where it

	cmd ...
	if test $? ...

expects cmd to be allowed to fail freely and we can act on its exit status, which is not possible under "set -e". Rewrite it using the common pattern:

	status=0; cmd ... || status=$?
	if test $status ...

which means the same thing but does not fail under "set -e".

Note that I do not run cvs tests myself, so while this change makes the scripts pass to the point where they correctly sets skip_all='message' and triggers test_done, it is very likely that there needs further work to make the rest of the scripts "set -e" clean.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9400-git-cvsserver-server.sh | 4 ++--
 t/t9401-git-cvsserver-crlf.sh   | 4 ++--
 t/t9402-git-cvsserver-refs.sh   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

Among a few scripts that use lib-git-daemon.sh, t5570 starts and stops git-daemon process multiple times. Make stop_git_daemon function "set -e" clean.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-git-daemon.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

In order to catch mistakes like misspelling "test_expect_success", we would like to eventually be able to run our test suite with the "-e" option on.

This script uses the "read" utility to populate a single variable with the contents of a here-document. As "read" signals that it saw the EOF by exiting with status 1, this triggers "set -e".

Here, we squelch it with the standard "|| :" trick. A simpler alternative may be to use a simpler assignment, e.g.,

    refs='main
    maint
    next
    seen'

Either way would work, but just honor the original author's preference.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9902-completion.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Mar 24, 2026 at 11:21:03PM -0700, Junio C Hamano wrote:

Oh well, you beat me to it :)

Patrick

On Tue, Mar 24, 2026 at 11:21:06PM -0700, Junio C Hamano wrote:

I've got this alternate fix, which I find a bit cleaner overall. With `$((...))` we don't have to worry about the return value of expr.

Patrick

On Tue, Mar 24, 2026 at 11:21:07PM -0700, Junio C Hamano wrote:

I fixed this by applying this change:

On Tue, Mar 24, 2026 at 11:21:08PM -0700, Junio C Hamano wrote:

Yeah, that's what I have, mostly because I find it easier to reason about.

Patrick

On Tue, Mar 24, 2026 at 11:21:09PM -0700, Junio C Hamano wrote:

An alternative:

On Tue, Mar 24, 2026 at 11:21:11PM -0700, Junio C Hamano wrote:

Similarly, here I've got:

On Tue, Mar 24, 2026 at 11:21:13PM -0700, Junio C Hamano wrote:

This test actually made me pause a bit. In theory, you can use the function to verify that git-daemon(1) exits successfully because we do bubble up its exit code. So instead of silencing the error code, I simply added `|| :` to all callsites that don't care about it at all.

But in practice, that turned out to be every callsite, so that exercise may not be worth it.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

I may have posted these before you did, but from what you see on your comments to these patches, it seems that you did a better job, perhaps? I focused on staying as close to the original implemenation as possible to reduce the chances of silly mistakes that subtly change the semantics, but for some obvious cases, trivial improvements like turning "! A || B" into "if A; then B; fi" may be worthwhile clean-up to be done in the same series (if not in the same patch).

Thanks.

Patrick Steinhardt <ps@pks.im> writes:

Yup.  I agree that arithmetic expansion is much easier to grok.

Patrick Steinhardt <ps@pks.im> writes:

Yeah. Worse, I think this is called even when I suspect that the daemon is in the process of exiting (i.e., racy), has already exited (i.e., kill and wait will say "huh? what are you talking about"), or simply when we do not know what status it is in, so I think it is OK to ignore errors from these.

On Tue, Mar 24, 2026 at 11:21:04PM -0700, Junio C Hamano wrote:

This causes failures in t0005 and t3600 with dash, but not bash.

It looks like the suppression of "-e" on the left-hand-side of an && is different when there is command substitution in play:

  $ dash -c 'OUT=$( ((yes; echo $? 1>&3) | :) 3>&1) && echo out=$OUT'
  out=141

  $ dash -ec 'OUT=$( ((yes; echo $? 1>&3) | :) 3>&1) && echo out=$OUT'
  out=

whereas with bash, both produce 141.

The idea is that $OUT becomes the exit status of "yes" here, and we are expecting to see SIGPIPE. With "-e" in effect, the failing "yes" will terminate before we echo $?.

To demonstrate the effect as we build it up from smaller pieces:

  # produces 141, SIGPIPE from yes
  dash -c '((yes; echo $? 1>&3) | :) 3>&1'

  # produces nothing, "-e" kills subshell after yes fails
  dash -ec '((yes; echo $? 1>&3) | :) 3>&1'

  # produces 141 (and "ok"), as the && suppresses -e
  dash -ec '((yes; echo $? 1>&3) | :) 3>&1 && echo ok'

  # produces "out="; the $() makes us forget that we're on LHS of &&
  dash -ec 'OUT=$( ((yes; echo $? 1>&3) | :) 3>&1) && echo out=$OUT'

The actual failing code in t0005 is:

  OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
  test_match_signal 13 "$OUT"

and the one in t3600 is similar. I guess you could do:

Jeff King <peff@peff.net> writes:

Would

	OUT=$( ((large_git && echo 0 || echo $? 1>&3) | :) 3>&1 )

do a bit better?

We can keep fixing things one by one as we find these little glitches and gochas, of it may be a whack-a-mole exercise that eventually will turn out to be futile. I dunno.

In any case, the "Add 'set -e' to test-lib.sh to affect everybody" step has to come at the very end of the series to keep tests pass at each step, I guess. I wonder how much better Patrick's version does...

On Thu, Mar 26, 2026 at 07:27:44AM -0700, Junio C Hamano wrote:

Yeah, that is better (though in practice the same for our purposes in this particular test).

Yeah, after getting the tests passing locally I pushed to CI and saw a ton of failures. I think one is just:

On Thu, Mar 26, 2026 at 01:29:20PM -0400, Jeff King wrote:

I think the exercise is still worth it -t most of the changes are trivial, and it does help to make our tests a bit more robust.

Let me know in case you get worn out by this though and then I'm happy to take over. I like to have a numb task every now and then where I don't have to think much, and this here very much is such a task :)

Patrick

Patrick Steinhardt <ps@pks.im> writes:

I've stopped merging mine to 'seen', as I did not mean to carry it all the way to the end anyway (I do not have time to wait for the tests to the set of tests I run regularly with cvs, svn, and p4 added), so it's yours if you want it ;-)

Thanks.

On Fri, Mar 27, 2026 at 11:11:58AM -0700, Junio C Hamano wrote:

I'll pick it up, thanks! Let's see what I'm getting myself into.

Patrick