{"thread":{"id":"65349","subject":"[PATCH 00/11] detect misspelt test_expect_success and friends","startedAt":"2026-03-25T06:21:17Z","lastAt":"2026-03-31T11:44:05Z","messageCount":28,"participants":["Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"539897","messageId":"20260325062114.2067946-1-gitster@pobox.com","threadId":"65349","inReplyTo":null,"subject":"[PATCH 00/11] detect misspelt test_expect_success and friends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:03Z","receivedAt":"2026-03-25T06:21:17Z","isPatch":true,"body":"Recently we saw an unusual typo in a test that misspelt\n\"test_expect_success\", but this was not noticed for a while\nprimarily because the test script itself did not fail due to this\ntypo.  The shell and the test framework did say\n\n    tXXXX-xxx.sh: line 22: test_expect_successo: command not found\n\nbut otherwise kept going.\n\nOne way to help us detect such an error is to run our test under\n\"set -e\", which will abort execution after any command exits with\nnon-zero status.\n\nHowever, there are a handful of places in our existing tests and the\ntest framework itself that depends on the current behaviour of\nsilently ignoring a failing command.  Here is an attempt to fix them.\n\nThe first step turns \"set -e\" on very early in the test framework,\nand fixes one place in the framework that assumed that a failing\ncommand is OK.\n\nThe remainder of the series fix one test script per one patch, and\nat the end of the series, the whole test suite pass for me, even\nwhen merged to the tip of 'seen'.\n\nNote that I let cvs, svn, and p4 tests run only up to the point that\nthey decide to punt due to lack of external tools and language\nbindings they require, so for those of you who do have the necessary\nbindings, the scripts may still fail due to construct that are not\n\"set -e\" clean after they call \"test_done\" for me.\n\n 01/11: test-lib: catch misspelt 'test_expect_successo'\n 02/11: t0008: make test \"set -e\" clean\n 03/11: t6002: make test \"set -e\" clean\n 04/11: t4032: make test \"set -e\" clean\n 05/11: t7450: make test \"set -e\" clean\n 06/11: tests: make svn test \"set -e\" clean\n 07/11: t7508: make test \"set -e\" clean\n 08/11: t9200: make test \"set -e\" clean\n 09/11: t940?: make test \"set -e\" clean\n 10/11: t5570: make test \"set -e\" clean\n 11/11: t9902: make test \"set -e\" clean\n\n t/lib-git-daemon.sh                | 6 +++---\n t/lib-git-svn.sh                   | 7 +++----\n t/t0008-ignores.sh                 | 2 +-\n t/t4032-diff-inter-hunk-context.sh | 4 ++--\n t/t6002-rev-list-bisect.sh         | 4 ++--\n t/t7450-bad-git-dotfiles.sh        | 2 +-\n t/t7508-status.sh                  | 4 ++--\n t/t9200-git-cvsexportcommit.sh     | 4 ++--\n t/t9400-git-cvsserver-server.sh    | 4 ++--\n t/t9401-git-cvsserver-crlf.sh      | 4 ++--\n t/t9402-git-cvsserver-refs.sh      | 4 ++--\n t/t9902-completion.sh              | 2 +-\n t/test-lib.sh                      | 7 +++++--\n 13 files changed, 28 insertions(+), 26 deletions(-)\n\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539898","messageId":"20260325062114.2067946-2-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:04Z","receivedAt":"2026-03-25T06:21:18Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nAll tests dot-source \"test-lib.sh\" as the first thing to do.\nStarting the script with \"set -e\" immediately reveals one place in\nthe test framework itself that is not clean.\n\nThe test framework runs \"$GIT_BINARY\" without any argument. We\nexpect it to exit with status 1 (i.e., \"git<RETURN>\" that spits out\nthe list of common commands) as a sign that we have an instance of\nGit that we want to test.  We cannot quite say\n\n    git\n    if test $? != 1; then you have not built git; fi\n\nas the first invocation that exits non-zero is caught with \"set -e\".\n\nWork this around by rewriting the construct like so:\n\n    status=0; git || status=$?\n    if test $status != 1; then you have not built git; fi\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/test-lib.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 70fd3e9baf..a2aa97fba3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -17,6 +17,9 @@\n \n # Test the binaries we have just built.  The tests are kept in\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n+\n+set -e\n+\n if test -z \"$TEST_DIRECTORY\"\n then\n \t# ensure that TEST_DIRECTORY is an absolute path so that it\n@@ -143,8 +146,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n ################################################################\n # It appears that people try to run tests without building...\n GIT_BINARY=\"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\"\n-\"$GIT_BINARY\" >/dev/null\n-if test $? != 1\n+status=0 && \"$GIT_BINARY\" >/dev/null || status=$?\n+if test $status != 1\n then\n \tif test -n \"$GIT_TEST_INSTALLED\"\n \tthen\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539899","messageId":"20260325062114.2067946-3-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 02/11] t0008: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:05Z","receivedAt":"2026-03-25T06:21:20Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nA piece of script used \"grep\" to filter out its input purely for its\noutput, but of course, \"grep\" reports with its exit value when it\ndid not see any hits, which didn't mesh quite well with \"set -e\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t0008-ignores.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex db8bde280e..8edb08d9c2 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -122,7 +122,7 @@ test_expect_success_multiple () {\n \tfi\n \ttestname=\"$1\" expect_all=\"$2\" code=\"$3\"\n \n-\texpect_verbose=$( echo \"$expect_all\" | grep -v '^::\t' )\n+\texpect_verbose=$( echo \"$expect_all\" | grep -v '^::\t' ) || :\n \texpect=$( echo \"$expect_verbose\" | sed -e 's/.*\t//' )\n \n \ttest_expect_success $prereq \"$testname${no_index_opt:+ with $no_index_opt}\" '\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539900","messageId":"20260325062114.2067946-4-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 03/11] t6002: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:06Z","receivedAt":"2026-03-25T06:21:22Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nWe often use\n\n      val=$(expr expression)\n\nonly for the computation, and it is good that \"expr\" exits non-zero\nwith syntactically invalid expression (it exits with 2) and other\nerrors (with 3), as we do want to notice such errors.\n\n\"expr\" however also exits with \"1\" if it yields 0 or null X-<.\n\nMake sure we do not fail unnecessarily under \"set -e\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t6002-rev-list-bisect.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex daa009c9a1..1a6ffd8fbd 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -27,9 +27,9 @@ test_bisection_diff()\n \t# Test if bisection size is close to half of list size within\n \t# tolerance.\n \t#\n-\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2)\n+\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2) && test $? -le 1\n \ttest \"$_bisect_err\" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)\n-\t_bisect_err=$(expr $_bisect_err / 2) ; # floor\n+\t_bisect_err=$(expr $_bisect_err / 2) && test $? -le 1; # floor\n \n \ttest_expect_success \\\n \t\"bisection diff $_bisect_option $_head $* <= $_max_diff\" \\\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539901","messageId":"20260325062114.2067946-5-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 04/11] t4032: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:07Z","receivedAt":"2026-03-25T06:21:24Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nA few shell construct used in this test were not ready.  Make them\nso.\n\n * \"git config --unset VAR\" can fail when VAR is not defined.\n\n * The author of \"test -f X && run test that uses X\" written here\n   really wanted to say \"if file X is there, then run the test\", not\n   \"file X must exist and the test using it must succeed\".  The\n   proper way to express it is to say \"test ! -f X || use X\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4032-diff-inter-hunk-context.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh\nindex bada0cbd32..efcd863126 100755\n--- a/t/t4032-diff-inter-hunk-context.sh\n+++ b/t/t4032-diff-inter-hunk-context.sh\n@@ -17,7 +17,7 @@ f() {\n \n t() {\n \tuse_config=\n-\tgit config --unset diff.interHunkContext\n+\tgit config --unset diff.interHunkContext || :\n \n \tcase $# in\n \t4) hunks=$4; cmd=\"diff -U$3\";;\n@@ -40,7 +40,7 @@ t() {\n \t\ttest $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks\n \t\"\n \n-\ttest -f $expected &&\n+\ttest ! -f $expected ||\n \ttest_expect_success \"$label: check output\" \"\n \t\tgit $cmd $file | grep -v '^index ' >actual &&\n \t\ttest_cmp $expected actual\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539902","messageId":"20260325062114.2067946-6-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 05/11] t7450: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:08Z","receivedAt":"2026-03-25T06:21:25Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nOften we write \"A && test_expect_success ...\" and want it to mean\n\"If and only if A holds true, this needs to be tested\", but under\n\"set -e\", this will cause failure when A does not hold true.  We\nneed to write \"!A || test_expect_success ...\" if we want to run the\ntest conditionally.\n\nOr write it properly with if/then/fi, perhaps like:\n\n\tif ! A\n\tthen\n\t\ttest_expect_success ...\n\tfi\n\nMake sure we do not fail unnecessarily under \"set -e\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t7450-bad-git-dotfiles.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex f512eed278..047e4085d7 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -220,7 +220,7 @@ check_dotx_symlink () {\n \t\t)\n \t'\n \n-\ttest -n \"$refuse_index\" &&\n+\ttest -z \"$refuse_index\" ||\n \ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n \t\ttest_must_fail \\\n \t\t\tgit -C $dir \\\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539903","messageId":"20260325062114.2067946-7-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 06/11] tests: make svn test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:09Z","receivedAt":"2026-03-25T06:21:27Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nThe git-svn helper scriptlet suffers from two instances of the\nrecurring pattern where a sequence\n\n\tcmd ...\n\tif test $? ...\n\nexpects cmd to be allowed to fail freely and we can act on its exit\nstatus, which is not possible under \"set -e\".\n\nAs the second instance uses an extra variable $x to capture the\nstatus of the failed command already, let's use that variable to\nrewrite the above pattern to\n\n\tx=0; cmd ... || x=$?\n\tif test $x ...\n\nwhich means the same thing but does not fail under \"set -e\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/lib-git-svn.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex 2fde2353fd..a73b997f8f 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -15,8 +15,8 @@ GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn\n SVN_TREE=$GIT_SVN_DIR/svn-tree\n test_set_port SVNSERVE_PORT\n \n-svn >/dev/null 2>&1\n-if test $? -ne 1\n+x=0; svn >/dev/null 2>&1 || x=$?\n+if test $x -ne 1\n then\n \tskip_all='skipping git svn tests, svn not found'\n \ttest_done\n@@ -32,8 +32,7 @@ use SVN::Core;\n use SVN::Repos;\n \\$SVN::Core::VERSION gt '1.1.0' or exit(42);\n system(qw/svnadmin create --fs-type fsfs/, \\$ENV{svnrepo}) == 0 or exit(41);\n-\" >&3 2>&4\n-x=$?\n+\" >&3 2>&4 || x=$?\n if test $x -ne 0\n then\n \tif test $x -eq 42; then\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539904","messageId":"20260325062114.2067946-8-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 07/11] t7508: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:10Z","receivedAt":"2026-03-25T06:21:29Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nThis test tries to unconditionally clear a few configuration\nvariables, but \"git config --unset VAR\" fails if VAR is not set.\nWork it around by telling the shell that failures from them are OK.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t7508-status.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex a5e21bf8bf..1167b835a4 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -773,8 +773,8 @@ test_expect_success TTY 'status --porcelain ignores color.status' '\n '\n \n # recover unconditionally from color tests\n-git config --unset color.status\n-git config --unset color.ui\n+git config --unset color.status || :\n+git config --unset color.ui || :\n \n test_expect_success 'status --porcelain respects -b' '\n \n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539905","messageId":"20260325062114.2067946-9-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 08/11] t9200: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:11Z","receivedAt":"2026-03-25T06:21:30Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nThis test uses the usual pattern, where it\n\n\tcmd ...\n\tif test $? ...\n\nexpects cmd to be allowed to fail freely and we can act on its exit\nstatus, which is not possible under \"set -e\".  Rewrite it using the\ncommon pattern:\n\n\tstatus=0; cmd ... || status=$?\n\tif test $status ...\n\nwhich means the same thing but does not fail under \"set -e\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t9200-git-cvsexportcommit.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\nindex 14cbe96527..65ef1d7c82 100755\n--- a/t/t9200-git-cvsexportcommit.sh\n+++ b/t/t9200-git-cvsexportcommit.sh\n@@ -11,8 +11,8 @@ if ! test_have_prereq PERL; then\n \ttest_done\n fi\n \n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+status=0; cvs >/dev/null 2>&1 || status=$?\n+if test $status -ne 1\n then\n     skip_all='skipping git cvsexportcommit tests, cvs not found'\n     test_done\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539906","messageId":"20260325062114.2067946-10-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 09/11] t940?: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:12Z","receivedAt":"2026-03-25T06:21:32Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nThe cverserver tests have the usual pattern, where it\n\n\tcmd ...\n\tif test $? ...\n\nexpects cmd to be allowed to fail freely and we can act on its exit\nstatus, which is not possible under \"set -e\".  Rewrite it using the\ncommon pattern:\n\n\tstatus=0; cmd ... || status=$?\n\tif test $status ...\n\nwhich means the same thing but does not fail under \"set -e\".\n\nNote that I do not run cvs tests myself, so while this change\nmakes the scripts pass to the point where they correctly sets\nskip_all='message' and triggers test_done, it is very likely\nthat there needs further work to make the rest of the scripts\n\"set -e\" clean.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t9400-git-cvsserver-server.sh | 4 ++--\n t/t9401-git-cvsserver-crlf.sh   | 4 ++--\n t/t9402-git-cvsserver-refs.sh   | 4 ++--\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex e499c7f955..e1cc18e834 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -17,8 +17,8 @@ if ! test_have_prereq PERL; then\n \tskip_all='skipping git cvsserver tests, perl not available'\n \ttest_done\n fi\n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+status=0; cvs >/dev/null 2>&1 || status=$?\n+if test $status -ne 1\n then\n     skip_all='skipping git-cvsserver tests, cvs not found'\n     test_done\ndiff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh\nindex a34805acdc..715723f675 100755\n--- a/t/t9401-git-cvsserver-crlf.sh\n+++ b/t/t9401-git-cvsserver-crlf.sh\n@@ -60,8 +60,8 @@ check_status_options() {\n     return $stat\n }\n \n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+status=0; cvs >/dev/null 2>&1 || status=$?\n+if test $status -ne 1\n then\n     skip_all='skipping git-cvsserver tests, cvs not found'\n     test_done\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex 2ee41f9443..dd9ffe021b 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -68,8 +68,8 @@ check_diff() {\n \n #########\n \n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+status=0; cvs >/dev/null 2>&1 || status=$?\n+if test $status -ne 1\n then\n \tskip_all='skipping git-cvsserver tests, cvs not found'\n \ttest_done\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539907","messageId":"20260325062114.2067946-11-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 10/11] t5570: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:13Z","receivedAt":"2026-03-25T06:21:34Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nAmong a few scripts that use lib-git-daemon.sh, t5570 starts and\nstops git-daemon process multiple times.  Make stop_git_daemon\nfunction \"set -e\" clean.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/lib-git-daemon.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex e62569222b..6850f08c1d 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -86,13 +86,13 @@ stop_git_daemon() {\n \t# kill git-daemon child of git\n \tsay >&3 \"Stopping git daemon ...\"\n \tkill \"$GIT_DAEMON_PID\"\n-\twait \"$GIT_DAEMON_PID\" >&3 2>&4\n-\tret=$?\n+\tret=0\n+\twait \"$GIT_DAEMON_PID\" >&3 2>&4 || ret=$?\n \tif ! test_match_signal 15 $ret\n \tthen\n \t\terror \"git daemon exited with status: $ret\"\n \tfi\n-\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null\n+\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null || :\n \tGIT_DAEMON_PID=\n \trm -f git_daemon_output \"$GIT_DAEMON_PIDFILE\"\n }\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539908","messageId":"20260325062114.2067946-12-gitster@pobox.com","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"[PATCH 11/11] t9902: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T06:21:14Z","receivedAt":"2026-03-25T06:21:36Z","isPatch":true,"body":"In order to catch mistakes like misspelling \"test_expect_success\",\nwe would like to eventually be able to run our test suite with the\n\"-e\" option on.\n\nThis script uses the \"read\" utility to populate a single variable\nwith the contents of a here-document.  As \"read\" signals that it saw\nthe EOF by exiting with status 1, this triggers \"set -e\".\n\nHere, we squelch it with the standard \"|| :\" trick.  A simpler\nalternative may be to use a simpler assignment, e.g.,\n\n    refs='main\n    maint\n    next\n    seen'\n\nEither way would work, but just honor the original author's\npreference.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t9902-completion.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 2f9a597ec7..e3a7df7691 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -590,7 +590,7 @@ test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '\n \t__gitcomp \"$invalid_variable_name\"\n '\n \n-read -r -d \"\" refs <<-\\EOF\n+read -r -d \"\" refs <<-\\EOF || :\n main\n maint\n next\n-- \n2.53.0-886-g529cbd14ff\n\n"},{"id":"539912","messageId":"acOJ7EHFF11LJRKS@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-1-gitster@pobox.com","subject":"Re: [PATCH 00/11] detect misspelt test_expect_success and friends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:08:28Z","receivedAt":"2026-03-25T07:08:34Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:03PM -0700, Junio C Hamano wrote:\n> Recently we saw an unusual typo in a test that misspelt\n> \"test_expect_success\", but this was not noticed for a while\n> primarily because the test script itself did not fail due to this\n> typo.  The shell and the test framework did say\n> \n>     tXXXX-xxx.sh: line 22: test_expect_successo: command not found\n> \n> but otherwise kept going.\n> \n> One way to help us detect such an error is to run our test under\n> \"set -e\", which will abort execution after any command exits with\n> non-zero status.\n> \n> However, there are a handful of places in our existing tests and the\n> test framework itself that depends on the current behaviour of\n> silently ignoring a failing command.  Here is an attempt to fix them.\n> \n> The first step turns \"set -e\" on very early in the test framework,\n> and fixes one place in the framework that assumed that a failing\n> command is OK.\n> \n> The remainder of the series fix one test script per one patch, and\n> at the end of the series, the whole test suite pass for me, even\n> when merged to the tip of 'seen'.\n> \n> Note that I let cvs, svn, and p4 tests run only up to the point that\n> they decide to punt due to lack of external tools and language\n> bindings they require, so for those of you who do have the necessary\n> bindings, the scripts may still fail due to construct that are not\n> \"set -e\" clean after they call \"test_done\" for me.\n> \n>  01/11: test-lib: catch misspelt 'test_expect_successo'\n>  02/11: t0008: make test \"set -e\" clean\n>  03/11: t6002: make test \"set -e\" clean\n>  04/11: t4032: make test \"set -e\" clean\n>  05/11: t7450: make test \"set -e\" clean\n>  06/11: tests: make svn test \"set -e\" clean\n>  07/11: t7508: make test \"set -e\" clean\n>  08/11: t9200: make test \"set -e\" clean\n>  09/11: t940?: make test \"set -e\" clean\n>  10/11: t5570: make test \"set -e\" clean\n>  11/11: t9902: make test \"set -e\" clean\n\nOh well, you beat me to it :)\n\nPatrick\n"},{"id":"539913","messageId":"acOLlLzphGMfZeN6@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-4-gitster@pobox.com","subject":"Re: [PATCH 03/11] t6002: make test \"set -e\" clean","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:15:32Z","receivedAt":"2026-03-25T07:15:38Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:06PM -0700, Junio C Hamano wrote:\n> In order to catch mistakes like misspelling \"test_expect_success\",\n> we would like to eventually be able to run our test suite with the\n> \"-e\" option on.\n> \n> We often use\n> \n>       val=$(expr expression)\n> \n> only for the computation, and it is good that \"expr\" exits non-zero\n> with syntactically invalid expression (it exits with 2) and other\n> errors (with 3), as we do want to notice such errors.\n> \n> \"expr\" however also exits with \"1\" if it yields 0 or null X-<.\n> \n> Make sure we do not fail unnecessarily under \"set -e\".\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  t/t6002-rev-list-bisect.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n> index daa009c9a1..1a6ffd8fbd 100755\n> --- a/t/t6002-rev-list-bisect.sh\n> +++ b/t/t6002-rev-list-bisect.sh\n> @@ -27,9 +27,9 @@ test_bisection_diff()\n>  \t# Test if bisection size is close to half of list size within\n>  \t# tolerance.\n>  \t#\n> -\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2)\n> +\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2) && test $? -le 1\n>  \ttest \"$_bisect_err\" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)\n> -\t_bisect_err=$(expr $_bisect_err / 2) ; # floor\n> +\t_bisect_err=$(expr $_bisect_err / 2) && test $? -le 1; # floor\n>  \n>  \ttest_expect_success \\\n>  \t\"bisection diff $_bisect_option $_head $* <= $_max_diff\" \\\n\nI've got this alternate fix, which I find a bit cleaner overall. With\n`$((...))` we don't have to worry about the return value of expr.\n\nPatrick\n\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex daa009c9a1..f2de40b5ed 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -27,13 +27,16 @@ test_bisection_diff()\n \t# Test if bisection size is close to half of list size within\n \t# tolerance.\n \t#\n-\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2)\n-\ttest \"$_bisect_err\" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)\n-\t_bisect_err=$(expr $_bisect_err / 2) ; # floor\n-\n-\ttest_expect_success \\\n-\t\"bisection diff $_bisect_option $_head $* <= $_max_diff\" \\\n-\t'test $_bisect_err -le $_max_diff'\n+\t_bisect_err=$(($_list_size - $_bisection_size * 2))\n+\tif test \"$_bisect_err\" -lt 0\n+\tthen\n+\t\t_bisect_err=$((0 - $_bisect_err))\n+\tfi\n+\t_bisect_err=$(($_bisect_err / 2)) ; # floor\n+\n+\ttest_expect_success \"bisection diff $_bisect_option $_head $* <= $_max_diff\" '\n+\t\ttest $_bisect_err -le $_max_diff\n+\t'\n }\n \n date >path0\n\n"},{"id":"539914","messageId":"acOLmU891XXsTcre@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-5-gitster@pobox.com","subject":"Re: [PATCH 04/11] t4032: make test \"set -e\" clean","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:15:37Z","receivedAt":"2026-03-25T07:15:42Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:07PM -0700, Junio C Hamano wrote:\n> diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh\n> index bada0cbd32..efcd863126 100755\n> --- a/t/t4032-diff-inter-hunk-context.sh\n> +++ b/t/t4032-diff-inter-hunk-context.sh\n> @@ -40,7 +40,7 @@ t() {\n>  \t\ttest $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks\n>  \t\"\n>  \n> -\ttest -f $expected &&\n> +\ttest ! -f $expected ||\n>  \ttest_expect_success \"$label: check output\" \"\n>  \t\tgit $cmd $file | grep -v '^index ' >actual &&\n>  \t\ttest_cmp $expected actual\n\nI fixed this by applying this change:\n\n@@ -40,11 +40,13 @@ t() {\n                test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks\n        \"\n\n-       test -f $expected &&\n-       test_expect_success \"$label: check output\" \"\n-               git $cmd $file | grep -v '^index ' >actual &&\n-               test_cmp $expected actual\n-       \"\n+       if test -f $expected\n+       then\n+               test_expect_success \"$label: check output\" \"\n+                       git $cmd $file | grep -v '^index ' >actual &&\n+                       test_cmp $expected actual\n+               \"\n+       fi\n }\n\nMore churn, but the intent is easier to reason about, if you ask me.\n\nPatrick\n"},{"id":"539915","messageId":"acOLo9Jdw2VkwQpc@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-6-gitster@pobox.com","subject":"Re: [PATCH 05/11] t7450: make test \"set -e\" clean","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:15:47Z","receivedAt":"2026-03-25T07:15:52Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:08PM -0700, Junio C Hamano wrote:\n> In order to catch mistakes like misspelling \"test_expect_success\",\n> we would like to eventually be able to run our test suite with the\n> \"-e\" option on.\n> \n> Often we write \"A && test_expect_success ...\" and want it to mean\n> \"If and only if A holds true, this needs to be tested\", but under\n> \"set -e\", this will cause failure when A does not hold true.  We\n> need to write \"!A || test_expect_success ...\" if we want to run the\n> test conditionally.\n> \n> Or write it properly with if/then/fi, perhaps like:\n> \n> \tif ! A\n> \tthen\n> \t\ttest_expect_success ...\n> \tfi\n\nYeah, that's what I have, mostly because I find it easier to reason\nabout.\n\nPatrick\n"},{"id":"539916","messageId":"acOLqvP1De5kYjPT@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-7-gitster@pobox.com","subject":"Re: [PATCH 06/11] tests: make svn test \"set -e\" clean","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:15:54Z","receivedAt":"2026-03-25T07:15:58Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:09PM -0700, Junio C Hamano wrote:\n> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\n> index 2fde2353fd..a73b997f8f 100644\n> --- a/t/lib-git-svn.sh\n> +++ b/t/lib-git-svn.sh\n> @@ -15,8 +15,8 @@ GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn\n>  SVN_TREE=$GIT_SVN_DIR/svn-tree\n>  test_set_port SVNSERVE_PORT\n>  \n> -svn >/dev/null 2>&1\n> -if test $? -ne 1\n> +x=0; svn >/dev/null 2>&1 || x=$?\n> +if test $x -ne 1\n>  then\n>  \tskip_all='skipping git svn tests, svn not found'\n>  \ttest_done\n\nAn alternative:\n\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex 2fde2353fd..07d86ea244 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -15,8 +15,7 @@ GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn\n SVN_TREE=$GIT_SVN_DIR/svn-tree\n test_set_port SVNSERVE_PORT\n\n-svn >/dev/null 2>&1\n-if test $? -ne 1\n+if ! svn help >/dev/null 2>&1\n then\n        skip_all='skipping git svn tests, svn not found'\n        test_done\n\nPatrick\n"},{"id":"539917","messageId":"acOLsfavUHJZA1tW@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-9-gitster@pobox.com","subject":"Re: [PATCH 08/11] t9200: make test \"set -e\" clean","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:16:01Z","receivedAt":"2026-03-25T07:16:05Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:11PM -0700, Junio C Hamano wrote:\n> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\n> index 14cbe96527..65ef1d7c82 100755\n> --- a/t/t9200-git-cvsexportcommit.sh\n> +++ b/t/t9200-git-cvsexportcommit.sh\n> @@ -11,8 +11,8 @@ if ! test_have_prereq PERL; then\n>  \ttest_done\n>  fi\n>  \n> -cvs >/dev/null 2>&1\n> -if test $? -ne 1\n> +status=0; cvs >/dev/null 2>&1 || status=$?\n> +if test $status -ne 1\n>  then\n>      skip_all='skipping git cvsexportcommit tests, cvs not found'\n>      test_done\n\nSimilarly, here I've got:\n\ndiff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\nindex 14cbe96527..581cf3d28f 100755\n--- a/t/t9200-git-cvsexportcommit.sh\n+++ b/t/t9200-git-cvsexportcommit.sh\n@@ -11,8 +11,7 @@ if ! test_have_prereq PERL; then\n        test_done\n fi\n\n-cvs >/dev/null 2>&1\n-if test $? -ne 1\n+if ! cvs version >/dev/null 2>&1\n then\n     skip_all='skipping git cvsexportcommit tests, cvs not found'\n     test_done\n\nPatrick\n"},{"id":"539918","messageId":"acOLt7GuLTpg_QYM@pks.im","threadId":"65349","inReplyTo":"20260325062114.2067946-11-gitster@pobox.com","subject":"Re: [PATCH 10/11] t5570: make test \"set -e\" clean","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:16:07Z","receivedAt":"2026-03-25T07:16:12Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:13PM -0700, Junio C Hamano wrote:\n> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n> index e62569222b..6850f08c1d 100644\n> --- a/t/lib-git-daemon.sh\n> +++ b/t/lib-git-daemon.sh\n> @@ -86,13 +86,13 @@ stop_git_daemon() {\n>  \t# kill git-daemon child of git\n>  \tsay >&3 \"Stopping git daemon ...\"\n>  \tkill \"$GIT_DAEMON_PID\"\n> -\twait \"$GIT_DAEMON_PID\" >&3 2>&4\n> -\tret=$?\n> +\tret=0\n> +\twait \"$GIT_DAEMON_PID\" >&3 2>&4 || ret=$?\n>  \tif ! test_match_signal 15 $ret\n>  \tthen\n>  \t\terror \"git daemon exited with status: $ret\"\n>  \tfi\n> -\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null\n> +\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null || :\n>  \tGIT_DAEMON_PID=\n>  \trm -f git_daemon_output \"$GIT_DAEMON_PIDFILE\"\n>  }\n\nThis test actually made me pause a bit. In theory, you can use the\nfunction to verify that git-daemon(1) exits successfully because we do\nbubble up its exit code. So instead of silencing the error code, I\nsimply added `|| :` to all callsites that don't care about it at all.\n\nBut in practice, that turned out to be every callsite, so that exercise\nmay not be worth it.\n\nPatrick\n"},{"id":"539940","messageId":"87bjgcja13.fsf@gitster.g","threadId":"65349","inReplyTo":"acOJ7EHFF11LJRKS@pks.im","subject":"Re: [PATCH 00/11] detect misspelt test_expect_success and friends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T13:47:36Z","receivedAt":"2026-03-25T13:47:42Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Note that I let cvs, svn, and p4 tests run only up to the point that\n>> they decide to punt due to lack of external tools and language\n>> bindings they require, so for those of you who do have the necessary\n>> bindings, the scripts may still fail due to construct that are not\n>> \"set -e\" clean after they call \"test_done\" for me.\n>> \n>>  01/11: test-lib: catch misspelt 'test_expect_successo'\n>>  02/11: t0008: make test \"set -e\" clean\n>>  03/11: t6002: make test \"set -e\" clean\n>>  04/11: t4032: make test \"set -e\" clean\n>>  05/11: t7450: make test \"set -e\" clean\n>>  06/11: tests: make svn test \"set -e\" clean\n>>  07/11: t7508: make test \"set -e\" clean\n>>  08/11: t9200: make test \"set -e\" clean\n>>  09/11: t940?: make test \"set -e\" clean\n>>  10/11: t5570: make test \"set -e\" clean\n>>  11/11: t9902: make test \"set -e\" clean\n>\n> Oh well, you beat me to it :)\n\nI may have posted these before you did, but from what you see on\nyour comments to these patches, it seems that you did a better job,\nperhaps?  I focused on staying as close to the original implemenation\nas possible to reduce the chances of silly mistakes that subtly change\nthe semantics, but for some obvious cases, trivial improvements like\nturning \"! A || B\" into \"if A; then B; fi\" may be worthwhile clean-up\nto be done in the same series (if not in the same patch).\n\nThanks.\n"},{"id":"539948","messageId":"xmqqcy0rykst.fsf@gitster.g","threadId":"65349","inReplyTo":"acOLlLzphGMfZeN6@pks.im","subject":"Re: [PATCH 03/11] t6002: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T15:45:54Z","receivedAt":"2026-03-25T15:45:57Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I've got this alternate fix, which I find a bit cleaner overall. With\n> `$((...))` we don't have to worry about the return value of expr.\n>\n> Patrick\n\nYup.  I agree that arithmetic expansion is much easier to grok.\n\n\n> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n> index daa009c9a1..f2de40b5ed 100755\n> --- a/t/t6002-rev-list-bisect.sh\n> +++ b/t/t6002-rev-list-bisect.sh\n> @@ -27,13 +27,16 @@ test_bisection_diff()\n>  \t# Test if bisection size is close to half of list size within\n>  \t# tolerance.\n>  \t#\n> -\t_bisect_err=$(expr $_list_size - $_bisection_size \\* 2)\n> -\ttest \"$_bisect_err\" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)\n> -\t_bisect_err=$(expr $_bisect_err / 2) ; # floor\n> -\n> -\ttest_expect_success \\\n> -\t\"bisection diff $_bisect_option $_head $* <= $_max_diff\" \\\n> -\t'test $_bisect_err -le $_max_diff'\n> +\t_bisect_err=$(($_list_size - $_bisection_size * 2))\n> +\tif test \"$_bisect_err\" -lt 0\n> +\tthen\n> +\t\t_bisect_err=$((0 - $_bisect_err))\n> +\tfi\n> +\t_bisect_err=$(($_bisect_err / 2)) ; # floor\n> +\n> +\ttest_expect_success \"bisection diff $_bisect_option $_head $* <= $_max_diff\" '\n> +\t\ttest $_bisect_err -le $_max_diff\n> +\t'\n>  }\n>  \n>  date >path0\n"},{"id":"539949","messageId":"xmqq8qbfyklm.fsf@gitster.g","threadId":"65349","inReplyTo":"acOLt7GuLTpg_QYM@pks.im","subject":"Re: [PATCH 10/11] t5570: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T15:50:13Z","receivedAt":"2026-03-25T15:50:15Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Tue, Mar 24, 2026 at 11:21:13PM -0700, Junio C Hamano wrote:\n>> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n>> index e62569222b..6850f08c1d 100644\n>> --- a/t/lib-git-daemon.sh\n>> +++ b/t/lib-git-daemon.sh\n>> @@ -86,13 +86,13 @@ stop_git_daemon() {\n>>  \t# kill git-daemon child of git\n>>  \tsay >&3 \"Stopping git daemon ...\"\n>>  \tkill \"$GIT_DAEMON_PID\"\n>> -\twait \"$GIT_DAEMON_PID\" >&3 2>&4\n>> -\tret=$?\n>> +\tret=0\n>> +\twait \"$GIT_DAEMON_PID\" >&3 2>&4 || ret=$?\n>>  \tif ! test_match_signal 15 $ret\n>>  \tthen\n>>  \t\terror \"git daemon exited with status: $ret\"\n>>  \tfi\n>> -\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null\n>> +\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null || :\n>>  \tGIT_DAEMON_PID=\n>>  \trm -f git_daemon_output \"$GIT_DAEMON_PIDFILE\"\n>>  }\n>\n> This test actually made me pause a bit. In theory, you can use the\n> function to verify that git-daemon(1) exits successfully because we do\n> bubble up its exit code. So instead of silencing the error code, I\n> simply added `|| :` to all callsites that don't care about it at all.\n>\n> But in practice, that turned out to be every callsite, so that exercise\n> may not be worth it.\n\nYeah.  Worse, I think this is called even when I suspect that the\ndaemon is in the process of exiting (i.e., racy), has already exited\n(i.e., kill and wait will say \"huh? what are you talking about\"), or\nsimply when we do not know what status it is in, so I think it is OK\nto ignore errors from these.\n"},{"id":"540035","messageId":"20260326040828.GA686242@coredump.intra.peff.net","threadId":"65349","inReplyTo":"20260325062114.2067946-2-gitster@pobox.com","subject":"Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-26T04:08:28Z","receivedAt":"2026-03-26T04:08:29Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:21:04PM -0700, Junio C Hamano wrote:\n\n>  # Test the binaries we have just built.  The tests are kept in\n>  # t/ subdirectory and are run in 'trash directory' subdirectory.\n> +\n> +set -e\n\nThis causes failures in t0005 and t3600 with dash, but not bash.\n\nIt looks like the suppression of \"-e\" on the left-hand-side of an && is\ndifferent when there is command substitution in play:\n\n  $ dash -c 'OUT=$( ((yes; echo $? 1>&3) | :) 3>&1) && echo out=$OUT'\n  out=141\n\n  $ dash -ec 'OUT=$( ((yes; echo $? 1>&3) | :) 3>&1) && echo out=$OUT'\n  out=\n\nwhereas with bash, both produce 141.\n\nThe idea is that $OUT becomes the exit status of \"yes\" here, and we are\nexpecting to see SIGPIPE. With \"-e\" in effect, the failing \"yes\" will\nterminate before we echo $?.\n\nTo demonstrate the effect as we build it up from smaller pieces:\n\n  # produces 141, SIGPIPE from yes\n  dash -c '((yes; echo $? 1>&3) | :) 3>&1'\n\n  # produces nothing, \"-e\" kills subshell after yes fails\n  dash -ec '((yes; echo $? 1>&3) | :) 3>&1'\n\n  # produces 141 (and \"ok\"), as the && suppresses -e\n  dash -ec '((yes; echo $? 1>&3) | :) 3>&1 && echo ok'\n\n  # produces \"out=\"; the $() makes us forget that we're on LHS of &&\n  dash -ec 'OUT=$( ((yes; echo $? 1>&3) | :) 3>&1) && echo out=$OUT'\n\nThe actual failing code in t0005 is:\n\n  OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n  test_match_signal 13 \"$OUT\"\n\nand the one in t3600 is similar. I guess you could do:\n\ndiff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\nindex afba0fc3fc..0bf1f16750 100755\n--- a/t/t0005-signals.sh\n+++ b/t/t0005-signals.sh\n@@ -42,7 +42,7 @@ test_expect_success 'create blob' '\n '\n \n test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '\n-\tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n+\tOUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&\n \ttest_match_signal 13 \"$OUT\"\n '\n \n\nThat neglects to echo $? when large_git surprisingly succeeds, but that\nwould mean $OUT is empty, which would cause the test to (correctly)\nfail. I kind of hate it, though.\n\n-Peff\n"},{"id":"540083","messageId":"xmqq8qbesm1r.fsf@gitster.g","threadId":"65349","inReplyTo":"20260326040828.GA686242@coredump.intra.peff.net","subject":"Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-26T14:27:44Z","receivedAt":"2026-03-26T14:27:47Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\n> index afba0fc3fc..0bf1f16750 100755\n> --- a/t/t0005-signals.sh\n> +++ b/t/t0005-signals.sh\n> @@ -42,7 +42,7 @@ test_expect_success 'create blob' '\n>  '\n>  \n>  test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '\n> -\tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n> +\tOUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&\n>  \ttest_match_signal 13 \"$OUT\"\n>  '\n>  \n>\n> That neglects to echo $? when large_git surprisingly succeeds, but that\n> would mean $OUT is empty, which would cause the test to (correctly)\n> fail. I kind of hate it, though.\n\nWould\n\n\tOUT=$( ((large_git && echo 0 || echo $? 1>&3) | :) 3>&1 )\n\ndo a bit better?\n\nWe can keep fixing things one by one as we find these little\nglitches and gochas, of it may be a whack-a-mole exercise that\neventually will turn out to be futile.  I dunno.\n\nIn any case, the \"Add 'set -e' to test-lib.sh to affect everybody\"\nstep has to come at the very end of the series to keep tests pass at\neach step, I guess.  I wonder how much better Patrick's version\ndoes...\n\n"},{"id":"540110","messageId":"20260326172920.GA2447148@coredump.intra.peff.net","threadId":"65349","inReplyTo":"xmqq8qbesm1r.fsf@gitster.g","subject":"Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-26T17:29:20Z","receivedAt":"2026-03-26T17:29:28Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 07:27:44AM -0700, Junio C Hamano wrote:\n\n> >  test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '\n> > -\tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n> > +\tOUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&\n> >  \ttest_match_signal 13 \"$OUT\"\n> >  '\n> >  \n> >\n> > That neglects to echo $? when large_git surprisingly succeeds, but that\n> > would mean $OUT is empty, which would cause the test to (correctly)\n> > fail. I kind of hate it, though.\n> \n> Would\n> \n> \tOUT=$( ((large_git && echo 0 || echo $? 1>&3) | :) 3>&1 )\n> \n> do a bit better?\n\nYeah, that is better (though in practice the same for our purposes in\nthis particular test).\n\n> We can keep fixing things one by one as we find these little\n> glitches and gochas, of it may be a whack-a-mole exercise that\n> eventually will turn out to be futile.  I dunno.\n\nYeah, after getting the tests passing locally I pushed to CI and saw a\nton of failures. I think one is just:\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 630a47af21..7f920d7b9e 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -12,7 +12,7 @@ TEST_CREATE_REPO_NO_TEMPLATE=1\n . ./test-lib.sh\n \n # Remove a default ACL from the test dir if possible.\n-setfacl -k . 2>/dev/null\n+setfacl -k . 2>/dev/null || true\n \n # User must have read permissions to the repo -> failure on --shared=0400\n test_expect_success 'shared = 0400 (faulty permission u-w)' '\n\nand another seems to involve test_done barfing when no tests have been\nrun (e.g., if we hit a skip_all case). I didn't investigate further.\n\n-Peff\n"},{"id":"540169","messageId":"acY3haGPHPLSfalj@pks.im","threadId":"65349","inReplyTo":"20260326172920.GA2447148@coredump.intra.peff.net","subject":"Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-27T07:53:41Z","receivedAt":"2026-03-27T07:53:46Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 01:29:20PM -0400, Jeff King wrote:\n> On Thu, Mar 26, 2026 at 07:27:44AM -0700, Junio C Hamano wrote:\n> \n> > >  test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '\n> > > -\tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n> > > +\tOUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&\n> > >  \ttest_match_signal 13 \"$OUT\"\n> > >  '\n> > >  \n> > >\n> > > That neglects to echo $? when large_git surprisingly succeeds, but that\n> > > would mean $OUT is empty, which would cause the test to (correctly)\n> > > fail. I kind of hate it, though.\n> > \n> > Would\n> > \n> > \tOUT=$( ((large_git && echo 0 || echo $? 1>&3) | :) 3>&1 )\n> > \n> > do a bit better?\n> \n> Yeah, that is better (though in practice the same for our purposes in\n> this particular test).\n> \n> > We can keep fixing things one by one as we find these little\n> > glitches and gochas, of it may be a whack-a-mole exercise that\n> > eventually will turn out to be futile.  I dunno.\n> \n> Yeah, after getting the tests passing locally I pushed to CI and saw a\n> ton of failures. I think one is just:\n\nI think the exercise is still worth it -t most of the changes are\ntrivial, and it does help to make our tests a bit more robust.\n\nLet me know in case you get worn out by this though and then I'm happy\nto take over. I like to have a numb task every now and then where I\ndon't have to think much, and this here very much is such a task :)\n\nPatrick\n"},{"id":"540213","messageId":"xmqqldfdjg5t.fsf@gitster.g","threadId":"65349","inReplyTo":"acY3haGPHPLSfalj@pks.im","subject":"Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-27T18:11:58Z","receivedAt":"2026-03-27T18:12:01Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Let me know in case you get worn out by this though and then I'm happy\n> to take over. I like to have a numb task every now and then where I\n> don't have to think much, and this here very much is such a task :)\n\nI've stopped merging mine to 'seen', as I did not mean to carry it\nall the way to the end anyway (I do not have time to wait for the\ntests to the set of tests I run regularly with cvs, svn, and p4\nadded), so it's yours if you want it ;-)\n\nThanks.\n\n\n"},{"id":"540511","messageId":"acuzfzqIT7849jyX@pks.im","threadId":"65349","inReplyTo":"xmqqldfdjg5t.fsf@gitster.g","subject":"Re: [PATCH 01/11] test-lib: catch misspelt 'test_expect_successo'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-31T11:43:59Z","receivedAt":"2026-03-31T11:44:05Z","isPatch":true,"body":"On Fri, Mar 27, 2026 at 11:11:58AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Let me know in case you get worn out by this though and then I'm happy\n> > to take over. I like to have a numb task every now and then where I\n> > don't have to think much, and this here very much is such a task :)\n> \n> I've stopped merging mine to 'seen', as I did not mean to carry it\n> all the way to the end anyway (I do not have time to wait for the\n> tests to the set of tests I run regularly with cvs, svn, and p4\n> added), so it's yours if you want it ;-)\n\nI'll pick it up, thanks! Let's see what I'm getting myself into.\n\nPatrick\n"}]}