[PATCH 00/11] detect misspelt test_expect_success and friends

[PATCH 00/11] detect misspelt test_expect_success and friends

[Junio C Hamano]

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

-- 
2.53.0-886-g529cbd14ff

[PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'

[Junio C Hamano]

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 70fd3e9baf..a2aa97fba3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -17,6 +17,9 @@
 
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
+
+set -e
+
 if test -z "$TEST_DIRECTORY"
 then
 	# ensure that TEST_DIRECTORY is an absolute path so that it
@@ -143,8 +146,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
+status=0 && "$GIT_BINARY" >/dev/null || status=$?
+if test $status != 1
 then
 	if test -n "$GIT_TEST_INSTALLED"
 	then
-- 
2.53.0-886-g529cbd14ff

[PATCH 02/11] t0008: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index db8bde280e..8edb08d9c2 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -122,7 +122,7 @@ test_expect_success_multiple () {
 	fi
 	testname="$1" expect_all="$2" code="$3"
 
-	expect_verbose=$( echo "$expect_all" | grep -v '^::	' )
+	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}" '
-- 
2.53.0-886-g529cbd14ff

[PATCH 03/11] t6002: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index daa009c9a1..1a6ffd8fbd 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -27,9 +27,9 @@ test_bisection_diff()
 	# Test if bisection size is close to half of list size within
 	# tolerance.
 	#
-	_bisect_err=$(expr $_list_size - $_bisection_size \* 2)
+	_bisect_err=$(expr $_list_size - $_bisection_size \* 2) && test $? -le 1
 	test "$_bisect_err" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)
-	_bisect_err=$(expr $_bisect_err / 2) ; # floor
+	_bisect_err=$(expr $_bisect_err / 2) && test $? -le 1; # floor
 
 	test_expect_success \
 	"bisection diff $_bisect_option $_head $* <= $_max_diff" \
-- 
2.53.0-886-g529cbd14ff

[PATCH 04/11] t4032: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh
index bada0cbd32..efcd863126 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";;
@@ -40,7 +40,7 @@ t() {
 		test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks
 	"
 
-	test -f $expected &&
+	test ! -f $expected ||
 	test_expect_success "$label: check output" "
 		git $cmd $file | grep -v '^index ' >actual &&
 		test_cmp $expected actual
-- 
2.53.0-886-g529cbd14ff

[PATCH 05/11] t7450: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index f512eed278..047e4085d7 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -220,7 +220,7 @@ check_dotx_symlink () {
 		)
 	'
 
-	test -n "$refuse_index" &&
+	test -z "$refuse_index" ||
 	test_expect_success "refuse to load symlinked $name into index ($type)" '
 		test_must_fail \
 			git -C $dir \
-- 
2.53.0-886-g529cbd14ff

[PATCH 06/11] tests: make svn test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 2fde2353fd..a73b997f8f 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -15,8 +15,8 @@ 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
+x=0; svn >/dev/null 2>&1 || x=$?
+if test $x -ne 1
 then
 	skip_all='skipping git svn tests, svn not found'
 	test_done
@@ -32,8 +32,7 @@ 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
-- 
2.53.0-886-g529cbd14ff

[PATCH 07/11] t7508: make test "set -e" clean

[Junio C Hamano]

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

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' '
 
-- 
2.53.0-886-g529cbd14ff

[PATCH 08/11] t9200: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 14cbe96527..65ef1d7c82 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -11,8 +11,8 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+status=0; cvs >/dev/null 2>&1 || status=$?
+if test $status -ne 1
 then
     skip_all='skipping git cvsexportcommit tests, cvs not found'
     test_done
-- 
2.53.0-886-g529cbd14ff

[PATCH 09/11] t940?: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index e499c7f955..e1cc18e834 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -17,8 +17,8 @@ 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
+status=0; cvs >/dev/null 2>&1 || status=$?
+if test $status -ne 1
 then
     skip_all='skipping git-cvsserver tests, cvs not found'
     test_done
diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index a34805acdc..715723f675 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -60,8 +60,8 @@ check_status_options() {
     return $stat
 }
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+status=0; cvs >/dev/null 2>&1 || status=$?
+if test $status -ne 1
 then
     skip_all='skipping git-cvsserver tests, cvs not found'
     test_done
diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh
index 2ee41f9443..dd9ffe021b 100755
--- a/t/t9402-git-cvsserver-refs.sh
+++ b/t/t9402-git-cvsserver-refs.sh
@@ -68,8 +68,8 @@ check_diff() {
 
 #########
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+status=0; cvs >/dev/null 2>&1 || status=$?
+if test $status -ne 1
 then
 	skip_all='skipping git-cvsserver tests, cvs not found'
 	test_done
-- 
2.53.0-886-g529cbd14ff

[PATCH 10/11] t5570: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index e62569222b..6850f08c1d 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -86,13 +86,13 @@ 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.53.0-886-g529cbd14ff

[PATCH 11/11] t9902: make test "set -e" clean

[Junio C Hamano]

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

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2f9a597ec7..e3a7df7691 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -590,7 +590,7 @@ test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '
 	__gitcomp "$invalid_variable_name"
 '
 
-read -r -d "" refs <<-\EOF
+read -r -d "" refs <<-\EOF || :
 main
 maint
 next
-- 
2.53.0-886-g529cbd14ff

Re: [PATCH 00/11] detect misspelt test_expect_success and friends

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:03PM -0700, Junio C Hamano wrote:
> 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

Oh well, you beat me to it :)

Patrick

Re: [PATCH 03/11] t6002: make test "set -e" clean

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:06PM -0700, Junio C Hamano wrote:
> 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(-)
> 
> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> index daa009c9a1..1a6ffd8fbd 100755
> --- a/t/t6002-rev-list-bisect.sh
> +++ b/t/t6002-rev-list-bisect.sh
> @@ -27,9 +27,9 @@ test_bisection_diff()
>  	# Test if bisection size is close to half of list size within
>  	# tolerance.
>  	#
> -	_bisect_err=$(expr $_list_size - $_bisection_size \* 2)
> +	_bisect_err=$(expr $_list_size - $_bisection_size \* 2) && test $? -le 1
>  	test "$_bisect_err" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)
> -	_bisect_err=$(expr $_bisect_err / 2) ; # floor
> +	_bisect_err=$(expr $_bisect_err / 2) && test $? -le 1; # floor
>  
>  	test_expect_success \
>  	"bisection diff $_bisect_option $_head $* <= $_max_diff" \

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

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

Re: [PATCH 04/11] t4032: make test "set -e" clean

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:07PM -0700, Junio C Hamano wrote:
> diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh
> index bada0cbd32..efcd863126 100755
> --- a/t/t4032-diff-inter-hunk-context.sh
> +++ b/t/t4032-diff-inter-hunk-context.sh
> @@ -40,7 +40,7 @@ t() {
>  		test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks
>  	"
>  
> -	test -f $expected &&
> +	test ! -f $expected ||
>  	test_expect_success "$label: check output" "
>  		git $cmd $file | grep -v '^index ' >actual &&
>  		test_cmp $expected actual

I fixed this by applying this change:

@@ -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
 }

More churn, but the intent is easier to reason about, if you ask me.

Patrick

Re: [PATCH 05/11] t7450: make test "set -e" clean

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:08PM -0700, Junio C Hamano wrote:
> 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

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

Patrick

Re: [PATCH 06/11] tests: make svn test "set -e" clean

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:09PM -0700, Junio C Hamano wrote:
> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
> index 2fde2353fd..a73b997f8f 100644
> --- a/t/lib-git-svn.sh
> +++ b/t/lib-git-svn.sh
> @@ -15,8 +15,8 @@ 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
> +x=0; svn >/dev/null 2>&1 || x=$?
> +if test $x -ne 1
>  then
>  	skip_all='skipping git svn tests, svn not found'
>  	test_done

An alternative:

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 2fde2353fd..07d86ea244 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

Patrick

Re: [PATCH 08/11] t9200: make test "set -e" clean

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:11PM -0700, Junio C Hamano wrote:
> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> index 14cbe96527..65ef1d7c82 100755
> --- a/t/t9200-git-cvsexportcommit.sh
> +++ b/t/t9200-git-cvsexportcommit.sh
> @@ -11,8 +11,8 @@ if ! test_have_prereq PERL; then
>  	test_done
>  fi
>  
> -cvs >/dev/null 2>&1
> -if test $? -ne 1
> +status=0; cvs >/dev/null 2>&1 || status=$?
> +if test $status -ne 1
>  then
>      skip_all='skipping git cvsexportcommit tests, cvs not found'
>      test_done

Similarly, here I've got:

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

Patrick

Re: [PATCH 10/11] t5570: make test "set -e" clean

[Patrick Steinhardt]

On Tue, Mar 24, 2026 at 11:21:13PM -0700, Junio C Hamano wrote:
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index e62569222b..6850f08c1d 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -86,13 +86,13 @@ 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"
>  }

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

Re: [PATCH 00/11] detect misspelt test_expect_success and friends

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> 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
>
> Oh well, you beat me to it :)

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.

Re: [PATCH 03/11] t6002: make test "set -e" clean

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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

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


> 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

Re: [PATCH 10/11] t5570: make test "set -e" clean

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Mar 24, 2026 at 11:21:13PM -0700, Junio C Hamano wrote:
>> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
>> index e62569222b..6850f08c1d 100644
>> --- a/t/lib-git-daemon.sh
>> +++ b/t/lib-git-daemon.sh
>> @@ -86,13 +86,13 @@ 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"
>>  }
>
> 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.

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.

Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'

[Jeff King]

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

>  # Test the binaries we have just built.  The tests are kept in
>  # t/ subdirectory and are run in 'trash directory' subdirectory.
> +
> +set -e

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:

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index afba0fc3fc..0bf1f16750 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -42,7 +42,7 @@ 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 $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 

That neglects to echo $? when large_git surprisingly succeeds, but that
would mean $OUT is empty, which would cause the test to (correctly)
fail. I kind of hate it, though.

-Peff

Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
> index afba0fc3fc..0bf1f16750 100755
> --- a/t/t0005-signals.sh
> +++ b/t/t0005-signals.sh
> @@ -42,7 +42,7 @@ 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 $? 1>&3) | :) 3>&1 ) &&
>  	test_match_signal 13 "$OUT"
>  '
>  
>
> That neglects to echo $? when large_git surprisingly succeeds, but that
> would mean $OUT is empty, which would cause the test to (correctly)
> fail. I kind of hate it, though.

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...

Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'

[Jeff King]

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

> >  test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
> > -	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
> > +	OUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&
> >  	test_match_signal 13 "$OUT"
> >  '
> >  
> >
> > That neglects to echo $? when large_git surprisingly succeeds, but that
> > would mean $OUT is empty, which would cause the test to (correctly)
> > fail. I kind of hate it, though.
> 
> Would
> 
> 	OUT=$( ((large_git && echo 0 || echo $? 1>&3) | :) 3>&1 )
> 
> do a bit better?

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

> 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.

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

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 630a47af21..7f920d7b9e 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 || true
 
 # User must have read permissions to the repo -> failure on --shared=0400
 test_expect_success 'shared = 0400 (faulty permission u-w)' '

and another seems to involve test_done barfing when no tests have been
run (e.g., if we hit a skip_all case). I didn't investigate further.

-Peff

Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'

[Patrick Steinhardt]

On Thu, Mar 26, 2026 at 01:29:20PM -0400, Jeff King wrote:
> On Thu, Mar 26, 2026 at 07:27:44AM -0700, Junio C Hamano wrote:
> 
> > >  test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
> > > -	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
> > > +	OUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&
> > >  	test_match_signal 13 "$OUT"
> > >  '
> > >  
> > >
> > > That neglects to echo $? when large_git surprisingly succeeds, but that
> > > would mean $OUT is empty, which would cause the test to (correctly)
> > > fail. I kind of hate it, though.
> > 
> > Would
> > 
> > 	OUT=$( ((large_git && echo 0 || echo $? 1>&3) | :) 3>&1 )
> > 
> > do a bit better?
> 
> Yeah, that is better (though in practice the same for our purposes in
> this particular test).
> 
> > 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.
> 
> Yeah, after getting the tests passing locally I pushed to CI and saw a
> ton of failures. I think one is just:

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

Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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.