{"thread":{"id":"65345","subject":"[PATCH] t4014: fix call to `test_expect_success ()`","startedAt":"2026-03-24T14:52:36Z","lastAt":"2026-03-25T07:07:15Z","messageCount":16,"participants":["Patrick Steinhardt","Mirko Faina","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"539846","messageId":"20260324-b4-pks-t4014-fix-test-execution-v1-1-ac83c1bcc828@pks.im","threadId":"65345","inReplyTo":null,"subject":"[PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-24T14:52:30Z","receivedAt":"2026-03-24T14:52:36Z","isPatch":true,"body":"We have added a couple of new tests to t4014 in 6005932d95\n(format-patch: add ability to use alt cover format, 2026-03-07). One of\nthe tests has typoed the call to `test_expect_success ()` and instead\ninvokes `test_expected_success ()`. Fix this.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis fixes a test bug in one of the new tests introduced via\n\"mf/format-patch-cover-letter-format\". Thanks!\n\nPatrick\n---\n t/t4014-format-patch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7c67bdf922..4f8967e283 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -393,7 +393,7 @@ test_expect_success 'cover letter with subject, author and count' '\n \ttest_line_count = 1 result\n '\n \n-test_expected_success 'cover letter with author and count' '\n+test_expect_success 'cover letter with author and count' '\n \ttest_when_finished \"git reset --hard HEAD~1\" &&\n \ttest_when_finished \"rm -rf patches result test_file\" &&\n \ttouch test_file &&\n\n---\nbase-commit: 927a571e75d06037d46dc9ef5fe26b0dc37bbff6\nchange-id: 20260324-b4-pks-t4014-fix-test-execution-b2fb17a880f9\n\n"},{"id":"539847","messageId":"acKqvI0EhaORjoD7@exploit","threadId":"65345","inReplyTo":"20260324-b4-pks-t4014-fix-test-execution-v1-1-ac83c1bcc828@pks.im","subject":"Re: [PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-24T15:18:35Z","receivedAt":"2026-03-24T15:18:45Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 03:52:30PM +0100, Patrick Steinhardt wrote:\n> We have added a couple of new tests to t4014 in 6005932d95\n> (format-patch: add ability to use alt cover format, 2026-03-07). One of\n> the tests has typoed the call to `test_expect_success ()` and instead\n> invokes `test_expected_success ()`. Fix this.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> Hi,\n> \n> this fixes a test bug in one of the new tests introduced via\n> \"mf/format-patch-cover-letter-format\". Thanks!\n> \n> Patrick\n\nThis has already been fixed in a follow-up series under\nmf/format-patch-cover-letter-format [1].\n\nThank you\n\n[1] https://lore.kernel.org/git/5d061d6398bae368a7cc95700b5df44854d1d8e8.1774284699.git.mroik@delayed.space\n"},{"id":"539848","messageId":"xmqq5x6l2q5y.fsf@gitster.g","threadId":"65345","inReplyTo":"acKqvI0EhaORjoD7@exploit","subject":"Re: [PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T15:38:49Z","receivedAt":"2026-03-24T15:38:52Z","isPatch":true,"body":"Mirko Faina <mroik@delayed.space> writes:\n\n> On Tue, Mar 24, 2026 at 03:52:30PM +0100, Patrick Steinhardt wrote:\n>> We have added a couple of new tests to t4014 in 6005932d95\n>> (format-patch: add ability to use alt cover format, 2026-03-07). One of\n>> the tests has typoed the call to `test_expect_success ()` and instead\n>> invokes `test_expected_success ()`. Fix this.\n>> \n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> ---\n>> Hi,\n>> \n>> this fixes a test bug in one of the new tests introduced via\n>> \"mf/format-patch-cover-letter-format\". Thanks!\n>> \n>> Patrick\n>\n> This has already been fixed in a follow-up series under\n> mf/format-patch-cover-letter-format [1].\n>\n> Thank you\n>\n> [1] https://lore.kernel.org/git/5d061d6398bae368a7cc95700b5df44854d1d8e8.1774284699.git.mroik@delayed.space\n\nCould either of you remind us why \"make test\" did not catch this?\n\nThanks.\n"},{"id":"539851","messageId":"acKx6yBi-BWUVJcv@exploit","threadId":"65345","inReplyTo":"xmqq5x6l2q5y.fsf@gitster.g","subject":"Re: [PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-24T15:48:35Z","receivedAt":"2026-03-24T15:48:39Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 08:38:49AM -0700, Junio C Hamano wrote:\n> Could either of you remind us why \"make test\" did not catch this?\n\nMy bad. At the time when I ran it I simply saw no failing tests and\nassumed everything worked fine. Next time I'll check that the actual\nname of the test is present in the output.\n"},{"id":"539857","messageId":"xmqqo6kd18sr.fsf@gitster.g","threadId":"65345","inReplyTo":"acKx6yBi-BWUVJcv@exploit","subject":"Re: [PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T16:39:16Z","receivedAt":"2026-03-24T16:39:18Z","isPatch":true,"body":"Mirko Faina <mroik@delayed.space> writes:\n\n> On Tue, Mar 24, 2026 at 08:38:49AM -0700, Junio C Hamano wrote:\n>> Could either of you remind us why \"make test\" did not catch this?\n>\n> My bad. At the time when I ran it I simply saw no failing tests and\n> assumed everything worked fine. Next time I'll check that the actual\n> name of the test is present in the output.\n\nNo, I wasn't complaining a human tester not running tests.\n\nI was wondering if we can make the test framework better so that a\nmisspelt test_expect_success would cause a louder failure than what\nwe have now, which is something like:\n\n\t...\n        ok 5 - check hash-object\n\n        t0002-gitfile.sh: line 46: test_expect_successo: command not found\n        expecting success of 0002.6 'check update-index':\n                test_path_is_missing \"$REAL/index\" &&\n                rm -f \"$REAL/objects/$(objpath $SHA)\" &&\n                git update-index --add bar &&\n                test_path_is_file \"$REAL/index\" &&\n                test_path_is_file \"$REAL/objects/$(objpath $SHA)\"\n\n        ok 6 - check update-index\n        ...\n        expecting success of 0002.13 'enter_repo strict mode':\n                head=$(git -C enter_repo rev-parse HEAD) &&\n                ...\n                test_cmp expected actual\n\n        ok 13 - enter_repo strict mode\n\n        # passed all 13 test(s)\n        1..13\n\nwhen I corrupt the 6th test of a random script.\n\n        diff --git i/t/t0002-gitfile.sh w/t/t0002-gitfile.sh\n        index dfbcdddbcc..d65f664914 100755\n        --- i/t/t0002-gitfile.sh\n        +++ w/t/t0002-gitfile.sh\n        @@ -43,7 +43,7 @@ test_expect_success 'check hash-object' '\n                test_path_is_file \"$REAL/objects/$(objpath $SHA)\"\n         '\n\n        -test_expect_success 'check cat-file' '\n        +test_expect_successo 'check cat-file' '\n                git cat-file blob $SHA >actual &&\n                test_cmp bar actual\n         '\n\nThere is no indication of something bad happened, other than\n\"command not found\" and 13 tests passed instead of 14 the script\nhas, which nobody knows.\n\nSo, no, it hardly is your fault.\n\nI wonder if the test framework is safe to run with \"set -e\".\n"},{"id":"539859","messageId":"xmqqcy0t178a.fsf_-_@gitster.g","threadId":"65345","inReplyTo":"xmqqo6kd18sr.fsf@gitster.g","subject":"Re* [PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T17:13:09Z","receivedAt":"2026-03-24T17:13:13Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I was wondering if we can make the test framework better so that a\n> misspelt test_expect_success would cause a louder failure than what\n> we have now, which is something like:\n>\n> \t...\n>         ok 5 - check hash-object\n>\n>         t0002-gitfile.sh: line 46: test_expect_successo: command not found\n>         expecting success of 0002.6 'check update-index':\n>                 test_path_is_missing \"$REAL/index\" &&\n>         ...\n>         ok 13 - enter_repo strict mode\n>\n>         # passed all 13 test(s)\n>         1..13\n>\n> when I corrupt the 6th test of a random script.\n>\n>         diff --git i/t/t0002-gitfile.sh w/t/t0002-gitfile.sh\n>         index dfbcdddbcc..d65f664914 100755\n>         --- i/t/t0002-gitfile.sh\n>         +++ w/t/t0002-gitfile.sh\n>         @@ -43,7 +43,7 @@ test_expect_success 'check hash-object' '\n>                 test_path_is_file \"$REAL/objects/$(objpath $SHA)\"\n>          '\n>\n>         -test_expect_success 'check cat-file' '\n>         +test_expect_successo 'check cat-file' '\n>                 git cat-file blob $SHA >actual &&\n>                 test_cmp bar actual\n>          '\n>\n> There is no indication of something bad happened, other than\n> \"command not found\" and 13 tests passed instead of 14 the script\n> has, which nobody knows.\n>\n> So, no, it hardly is your fault.\n>\n> I wonder if the test framework is safe to run with \"set -e\".\n\nIt turns out that the test framework itself is not so clean.  If I\nadd \"set -e\" near the beginning of <t/test-lib.sh>, the first\nroadblock we hit is this one:\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        then\n\t\t... complain that you haven't built and ...\n\t\texit 1\n\tfi\n\nWith \"set -e\", \"$GIT_BINARY\" we expect to exit with status 1 (i.e.,\n\"git<RETURN>\" that spits out the list of common commands) as a sign\nthat we have an instance of Git that we want to test is not even\nallowed to do so.  \n\nI did this single liner at the end of <t/test-lib.sh>\n\n         t/test-lib.sh | 2 ++\n         1 file changed, 2 insertions(+)\n\n        diff --git c/t/test-lib.sh w/t/test-lib.sh\n        index 70fd3e9baf..4a80933487 100644\n        --- c/t/test-lib.sh\n        +++ w/t/test-lib.sh\n        @@ -1971,3 +1971,5 @@ test_lazy_prereq FSMONITOR_DAEMON '\n                git version --build-options >output &&\n                grep \"feature: fsmonitor--daemon\" output\n         '\n        +\n        +set -e\n\nand started running \"make test\".  I see some failures I haven't yet\nlooked into, but it seems promising.\n\nFixing all may involve finding and fixing little things like the\nattached patch.  I am not sure if this would be a good microproject\ncanidate for the next year.  There are a handful of them that\nmultiple students can work on independently, but some of them\nrequire familiarity with the test framework and shell scripting.\n\nI'll stop at marking this #leftoverbits but it probably is not for\nmicroproject.\n\n\n---- >8 ----\nSubject: [PATCH] t4032: make test \"set -e\" clean\n\nIn 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 c/t/t4032-diff-inter-hunk-context.sh w/t/t4032-diff-inter-hunk-context.sh\nindex bada0cbd32..efcd863126 100755\n--- c/t/t4032-diff-inter-hunk-context.sh\n+++ w/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"},{"id":"539862","messageId":"xmqqwlz1yuga.fsf_-_@gitster.g","threadId":"65345","inReplyTo":"xmqqcy0t178a.fsf_-_@gitster.g","subject":"[PATCH] t6002: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T18:05:09Z","receivedAt":"2026-03-24T18:05:11Z","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).\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\n * It was fun to figure this one out.\n\n t/t6002-rev-list-bisect.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git i/t/t6002-rev-list-bisect.sh w/t/t6002-rev-list-bisect.sh\nindex daa009c9a1..1bd720d240 100755\n--- i/t/t6002-rev-list-bisect.sh\n+++ w/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"},{"id":"539863","messageId":"xmqqmrzxyu2h.fsf_-_@gitster.g","threadId":"65345","inReplyTo":"xmqqcy0t178a.fsf_-_@gitster.g","subject":"[PATCH] test-lib: catch misspelt 'test_expect_successo'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T18:13:26Z","receivedAt":"2026-03-24T18:13:28Z","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\n * As we look into other breakages, we may discover more breakages\n   in the test framework that need to be fixed, but this change\n   alone seems to get thing going for many test scripts.\n\n t/test-lib.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git i/t/test-lib.sh w/t/test-lib.sh\nindex 70fd3e9baf..a2aa97fba3 100644\n--- i/t/test-lib.sh\n+++ w/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"},{"id":"539864","messageId":"xmqqh5q5ytq0.fsf_-_@gitster.g","threadId":"65345","inReplyTo":"xmqqcy0t178a.fsf_-_@gitster.g","subject":"[PATCH] t0008: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T18:20:55Z","receivedAt":"2026-03-24T18:20:57Z","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 i/t/t0008-ignores.sh w/t/t0008-ignores.sh\nindex db8bde280e..8edb08d9c2 100755\n--- i/t/t0008-ignores.sh\n+++ w/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"},{"id":"539865","messageId":"xmqqbjgdyt6l.fsf_-_@gitster.g","threadId":"65345","inReplyTo":"xmqqcy0t178a.fsf_-_@gitster.g","subject":"[PATCH] t7450: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T18:32:34Z","receivedAt":"2026-03-24T18:32: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\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 i/t/t7450-bad-git-dotfiles.sh w/t/t7450-bad-git-dotfiles.sh\nindex f512eed278..047e4085d7 100755\n--- i/t/t7450-bad-git-dotfiles.sh\n+++ w/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"},{"id":"539866","messageId":"CAPig+cQPD3vAxbRAJsqyd5=x2xCkTHj0Z6Gt2t+GiGjXDYei0Q@mail.gmail.com","threadId":"65345","inReplyTo":"xmqqbjgdyt6l.fsf_-_@gitster.g","subject":"Re: [PATCH] t7450: make test \"set -e\" clean","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-24T18:38:35Z","receivedAt":"2026-03-24T18:38:47Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 2:32 PM Junio C Hamano <gitster@pobox.com> 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>         if ! A\n>         then\n>                 test_expect_success ...\n>         fi\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> diff --git i/t/t7450-bad-git-dotfiles.sh w/t/t7450-bad-git-dotfiles.sh\n> @@ -220,7 +220,7 @@ check_dotx_symlink () {\n> -       test -n \"$refuse_index\" &&\n> +       test -z \"$refuse_index\" ||\n>         test_expect_success \"refuse to load symlinked $name into index ($type)\" '\n>                 test_must_fail \\\n>                         git -C $dir \\\n\nI suppose this is the absolute minimum change to make this work, but\ntypically we would handle this sort of case by defining a PREREQ,\nwouldn't we? Using a PREREQ would also set a better example for those\nnew to the codebase.\n"},{"id":"539870","messageId":"xmqq7br1yrr7.fsf@gitster.g","threadId":"65345","inReplyTo":"CAPig+cQPD3vAxbRAJsqyd5=x2xCkTHj0Z6Gt2t+GiGjXDYei0Q@mail.gmail.com","subject":"Re: [PATCH] t7450: make test \"set -e\" clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T19:03:24Z","receivedAt":"2026-03-24T19:03:27Z","isPatch":true,"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, Mar 24, 2026 at 2:32 PM Junio C Hamano <gitster@pobox.com> 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>>         if ! A\n>>         then\n>>                 test_expect_success ...\n>>         fi\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>> diff --git i/t/t7450-bad-git-dotfiles.sh w/t/t7450-bad-git-dotfiles.sh\n>> @@ -220,7 +220,7 @@ check_dotx_symlink () {\n>> -       test -n \"$refuse_index\" &&\n>> +       test -z \"$refuse_index\" ||\n>>         test_expect_success \"refuse to load symlinked $name into index ($type)\" '\n>>                 test_must_fail \\\n>>                         git -C $dir \\\n>\n> I suppose this is the absolute minimum change to make this work, but\n> typically we would handle this sort of case by defining a PREREQ,\n> wouldn't we? Using a PREREQ would also set a better example for those\n> new to the codebase.\n\nIn some situations, maybe, but I do not think this one is a good fit\nfor a prerequisite, whose typical pattern is \"let's see what we have\nin the executing platform environment just once, and act accordingly\".\n\nThis is a \"the outside helper function is repeatedly called, and the\ncaller may or may not call it with an option, depending on which\nthis extra test may or may not make sense to run, so run this one\nconditionally\".\n\n\n\n"},{"id":"539876","messageId":"20260324193514.GA1870130@coredump.intra.peff.net","threadId":"65345","inReplyTo":"xmqqmrzxyu2h.fsf_-_@gitster.g","subject":"Re: [PATCH] test-lib: catch misspelt 'test_expect_successo'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-24T19:35:14Z","receivedAt":"2026-03-24T19:35:17Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 11:13:26AM -0700, Junio C Hamano wrote:\n\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\nUsing \"-e\" makes me very nervous, given all of its quirks. Granted, most\nof them are related to it _not_ kicking in when you'd want it to, but I\nworry it will create false positive/negative headaches.\n\nIn the past I've caught errors outside of the test snippet by noticing\ncruft on stderr. This is especially obvious if you use \"prove\", which\ncaptures stdout and gives a nice display (which the extra stderr then\nmakes uglier).\n\nI wonder if we could automate / formalize that. If we do this hacky\npatch on master:\n\ndiff --git a/t/Makefile b/t/Makefile\nindex ab8a5b54aa..f57180cc7b 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -79,7 +79,7 @@ prove: pre-clean $(TEST_LINT)\n \t$(MAKE) clean-except-prove-cache\n \n $(T):\n-\t@echo \"*** $@ ***\"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n+\techo \"*** $@ ***\"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS) 2>$@.stderr\n \n $(UNIT_TESTS):\n \t@echo \"*** $@ ***\"; $@\n\nthen:\n\n  cd t\n  make test\n  for i in *.stderr; do test -s $i && echo $i; done\n\ncatches the problem in t4014 and nothing else. Note that it _doesn't_\nwork with --verbose-log, though, as that redirects stderr to stdout\n(which is going to the log). It might be possible to do something\ncleaner and more clever within test-lib.sh, though.\n\n-Peff\n"},{"id":"539878","messageId":"xmqqy0jhxb3r.fsf@gitster.g","threadId":"65345","inReplyTo":"20260324193514.GA1870130@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: catch misspelt 'test_expect_successo'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T19:48:24Z","receivedAt":"2026-03-24T19:48:26Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 24, 2026 at 11:13:26AM -0700, Junio C Hamano wrote:\n>\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> Using \"-e\" makes me very nervous, given all of its quirks. Granted, most\n> of them are related to it _not_ kicking in when you'd want it to, but I\n> worry it will create false positive/negative headaches.\n\nAfter looking at a few scripts, I am not suffering from such\nheadaches yet; it does not look too bad.  I'll stop this effort for\nnow, but with a handful of patches I already sent, more than 80-90%\nof the entire test scripts that I run are now \"set -e\" clean, I\nthink.  Note that I do not run svn, cvs, or p4 tests ;-)\n\n> In the past I've caught errors outside of the test snippet by noticing\n> cruft on stderr. This is especially obvious if you use \"prove\", which\n> captures stdout and gives a nice display (which the extra stderr then\n> makes uglier).\n> I wonder if we could automate / formalize that.\n\nThat's a thought.\n"},{"id":"539895","messageId":"20260325054601.GA3701549@coredump.intra.peff.net","threadId":"65345","inReplyTo":"xmqqy0jhxb3r.fsf@gitster.g","subject":"Re: [PATCH] test-lib: catch misspelt 'test_expect_successo'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-25T05:46:01Z","receivedAt":"2026-03-25T05:46:10Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 12:48:24PM -0700, Junio C Hamano wrote:\n\n> > Using \"-e\" makes me very nervous, given all of its quirks. Granted, most\n> > of them are related to it _not_ kicking in when you'd want it to, but I\n> > worry it will create false positive/negative headaches.\n> \n> After looking at a few scripts, I am not suffering from such\n> headaches yet; it does not look too bad.  I'll stop this effort for\n> now, but with a handful of patches I already sent, more than 80-90%\n> of the entire test scripts that I run are now \"set -e\" clean, I\n> think.  Note that I do not run svn, cvs, or p4 tests ;-)\n\nClean in the sense that you don't _notice_ any problems. But there may\nbe lurking ones. For example, given this:\n\n  set -e\n  foo() {\n\tfalse\n\techo foo\n  }\n\nwhat would you expect the output to be for:\n\n  echo before &&\n  foo &&\n  echo after\n\nversus:\n\n  echo before &&\n  foo\n\nWhether that \"false\" triggers \"-e\" depends on where in the &&-chain the\ncall to the containing function is. So things that are not problems now\nmay suddenly become ones when far-away code is changed.\n\nMaybe it's enough that people would notice and debug them when they\nhappen (if \"set -e\" is in test-lib.sh), and they wouldn't come up all\nthat much. I dunno. I just have been bitten enough by \"-e\" quirks that\nI'm wary.\n\n-Peff\n"},{"id":"539911","messageId":"acOJmBluqb5SvjpW@pks.im","threadId":"65345","inReplyTo":"xmqqcy0t178a.fsf_-_@gitster.g","subject":"Re: Re* [PATCH] t4014: fix call to `test_expect_success ()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T07:07:04Z","receivedAt":"2026-03-25T07:07:15Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 10:13:09AM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I was wondering if we can make the test framework better so that a\n> > misspelt test_expect_success would cause a louder failure than what\n> > we have now, which is something like:\n> >\n> > \t...\n> >         ok 5 - check hash-object\n> >\n> >         t0002-gitfile.sh: line 46: test_expect_successo: command not found\n> >         expecting success of 0002.6 'check update-index':\n> >                 test_path_is_missing \"$REAL/index\" &&\n> >         ...\n> >         ok 13 - enter_repo strict mode\n> >\n> >         # passed all 13 test(s)\n> >         1..13\n> >\n> > when I corrupt the 6th test of a random script.\n> >\n> >         diff --git i/t/t0002-gitfile.sh w/t/t0002-gitfile.sh\n> >         index dfbcdddbcc..d65f664914 100755\n> >         --- i/t/t0002-gitfile.sh\n> >         +++ w/t/t0002-gitfile.sh\n> >         @@ -43,7 +43,7 @@ test_expect_success 'check hash-object' '\n> >                 test_path_is_file \"$REAL/objects/$(objpath $SHA)\"\n> >          '\n> >\n> >         -test_expect_success 'check cat-file' '\n> >         +test_expect_successo 'check cat-file' '\n> >                 git cat-file blob $SHA >actual &&\n> >                 test_cmp bar actual\n> >          '\n> >\n> > There is no indication of something bad happened, other than\n> > \"command not found\" and 13 tests passed instead of 14 the script\n> > has, which nobody knows.\n> >\n> > So, no, it hardly is your fault.\n> >\n> > I wonder if the test framework is safe to run with \"set -e\".\n> \n> It turns out that the test framework itself is not so clean.  If I\n> add \"set -e\" near the beginning of <t/test-lib.sh>, the first\n> roadblock we hit is this one:\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>         then\n> \t\t... complain that you haven't built and ...\n> \t\texit 1\n> \tfi\n> \n> With \"set -e\", \"$GIT_BINARY\" we expect to exit with status 1 (i.e.,\n> \"git<RETURN>\" that spits out the list of common commands) as a sign\n> that we have an instance of Git that we want to test is not even\n> allowed to do so.  \n> \n> I did this single liner at the end of <t/test-lib.sh>\n> \n>          t/test-lib.sh | 2 ++\n>          1 file changed, 2 insertions(+)\n> \n>         diff --git c/t/test-lib.sh w/t/test-lib.sh\n>         index 70fd3e9baf..4a80933487 100644\n>         --- c/t/test-lib.sh\n>         +++ w/t/test-lib.sh\n>         @@ -1971,3 +1971,5 @@ test_lazy_prereq FSMONITOR_DAEMON '\n>                 git version --build-options >output &&\n>                 grep \"feature: fsmonitor--daemon\" output\n>          '\n>         +\n>         +set -e\n> \n> and started running \"make test\".  I see some failures I haven't yet\n> looked into, but it seems promising.\n\nYeah, I was playing around with the same idea yesterday, but got pulled\ninto some meetings and thus couldn't finish that work.\n\n> Fixing all may involve finding and fixing little things like the\n> attached patch.  I am not sure if this would be a good microproject\n> canidate for the next year.  There are a handful of them that\n> multiple students can work on independently, but some of them\n> require familiarity with the test framework and shell scripting.\n\nI think it's overall not that bad, and I've got something that's almost\ndone. It's an easy win for students indeed, but in this case I'd rather\nmake our test suite a bit more robust sooner rather than later :)\n\nI'll likely have something later today. Thanks!\n\nPatrick\n"}]}