We have added a couple of new tests to t4014 in 6005932d95 (format-patch: add ability to use alt cover format, 2026-03-07). One of the tests has typoed the call to `test_expect_success ()` and instead invokes `test_expected_success ()`. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Hi,

this fixes a test bug in one of the new tests introduced via "mf/format-patch-cover-letter-format". Thanks!

Patrick
---
 t/t4014-format-patch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Mar 24, 2026 at 03:52:30PM +0100, Patrick Steinhardt wrote:

This has already been fixed in a follow-up series under mf/format-patch-cover-letter-format [1].

Thank you

[1] https://lore.kernel.org/git/5d061d6398bae368a7cc95700b5df44854d1d8e8.1774284699.git.mroik@delayed.space

Mirko Faina <mroik@delayed.space> writes:

Could either of you remind us why "make test" did not catch this?

Thanks.

On Tue, Mar 24, 2026 at 08:38:49AM -0700, Junio C Hamano wrote:

My bad. At the time when I ran it I simply saw no failing tests and assumed everything worked fine. Next time I'll check that the actual name of the test is present in the output.

Mirko Faina <mroik@delayed.space> writes:

No, I wasn't complaining a human tester not running tests.

I was wondering if we can make the test framework better so that a misspelt test_expect_success would cause a louder failure than what we have now, which is something like:

	...
        ok 5 - check hash-object

        t0002-gitfile.sh: line 46: test_expect_successo: command not found
        expecting success of 0002.6 'check update-index':
                test_path_is_missing "$REAL/index" &&
                rm -f "$REAL/objects/$(objpath $SHA)" &&
                git update-index --add bar &&
                test_path_is_file "$REAL/index" &&
                test_path_is_file "$REAL/objects/$(objpath $SHA)"

        ok 6 - check update-index
        ...
        expecting success of 0002.13 'enter_repo strict mode':
                head=$(git -C enter_repo rev-parse HEAD) &&
                ...
                test_cmp expected actual

        ok 13 - enter_repo strict mode

        # passed all 13 test(s)
        1..13

when I corrupt the 6th test of a random script.

        diff --git i/t/t0002-gitfile.sh w/t/t0002-gitfile.sh
        index dfbcdddbcc..d65f664914 100755
        --- i/t/t0002-gitfile.sh
        +++ w/t/t0002-gitfile.sh
        @@ -43,7 +43,7 @@ test_expect_success 'check hash-object' '
                test_path_is_file "$REAL/objects/$(objpath $SHA)"
         '

        -test_expect_success 'check cat-file' '
        +test_expect_successo 'check cat-file' '
                git cat-file blob $SHA >actual &&
                test_cmp bar actual
         '

There is no indication of something bad happened, other than "command not found" and 13 tests passed instead of 14 the script has, which nobody knows.

So, no, it hardly is your fault.

I wonder if the test framework is safe to run with "set -e".

Junio C Hamano <gitster@pobox.com> writes:

It turns out that the test framework itself is not so clean. If I add "set -e" near the beginning of <t/test-lib.sh>, the first roadblock we hit is this one:

        # 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
        then
		... complain that you haven't built and ...
		exit 1
	fi

With "set -e", "$GIT_BINARY" we expect 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 is not even allowed to do so.

I did this single liner at the end of <t/test-lib.sh>

         t/test-lib.sh | 2 ++
         1 file changed, 2 insertions(+)

        diff --git c/t/test-lib.sh w/t/test-lib.sh
        index 70fd3e9baf..4a80933487 100644
        --- c/t/test-lib.sh
        +++ w/t/test-lib.sh
        @@ -1971,3 +1971,5 @@ test_lazy_prereq FSMONITOR_DAEMON '
                git version --build-options >output &&
                grep "feature: fsmonitor--daemon" output
         '
        +
        +set -e

and started running "make test". I see some failures I haven't yet looked into, but it seems promising.

Fixing all may involve finding and fixing little things like the attached patch. I am not sure if this would be a good microproject canidate for the next year. There are a handful of them that multiple students can work on independently, but some of them require familiarity with the test framework and shell scripting.

I'll stop at marking this #leftoverbits but it probably is not for microproject.

---- >8 ----
Subject: [PATCH] t4032: make test "set -e" clean

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

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

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

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

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

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

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

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

 * It was fun to figure this one out.

 t/t6002-rev-list-bisect.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

 * As we look into other breakages, we may discover more breakages
   in the test framework that need to be fixed, but this change
   alone seems to get thing going for many test scripts.

 t/test-lib.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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

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

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

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

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

On Tue, Mar 24, 2026 at 2:32 PM Junio C Hamano <gitster@pobox.com> wrote:

I suppose this is the absolute minimum change to make this work, but typically we would handle this sort of case by defining a PREREQ, wouldn't we? Using a PREREQ would also set a better example for those new to the codebase.

Eric Sunshine <sunshine@sunshineco.com> writes:

In some situations, maybe, but I do not think this one is a good fit for a prerequisite, whose typical pattern is "let's see what we have in the executing platform environment just once, and act accordingly".

This is a "the outside helper function is repeatedly called, and the caller may or may not call it with an option, depending on which this extra test may or may not make sense to run, so run this one conditionally".

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

Using "-e" makes me very nervous, given all of its quirks. Granted, most of them are related to it _not_ kicking in when you'd want it to, but I worry it will create false positive/negative headaches.

In the past I've caught errors outside of the test snippet by noticing cruft on stderr. This is especially obvious if you use "prove", which captures stdout and gives a nice display (which the extra stderr then makes uglier).

I wonder if we could automate / formalize that. If we do this hacky patch on master:

Jeff King <peff@peff.net> writes:

After looking at a few scripts, I am not suffering from such headaches yet; it does not look too bad. I'll stop this effort for now, but with a handful of patches I already sent, more than 80-90% of the entire test scripts that I run are now "set -e" clean, I think. Note that I do not run svn, cvs, or p4 tests ;-)

That's a thought.

On Tue, Mar 24, 2026 at 12:48:24PM -0700, Junio C Hamano wrote:

Clean in the sense that you don't _notice_ any problems. But there may be lurking ones. For example, given this:

  set -e
  foo() {
	false
	echo foo
  }

what would you expect the output to be for:

  echo before &&
  foo &&
  echo after

versus:

  echo before &&
  foo

Whether that "false" triggers "-e" depends on where in the &&-chain the call to the containing function is. So things that are not problems now may suddenly become ones when far-away code is changed.

Maybe it's enough that people would notice and debug them when they happen (if "set -e" is in test-lib.sh), and they wouldn't come up all that much. I dunno. I just have been bitten enough by "-e" quirks that I'm wary.

-Peff

On Tue, Mar 24, 2026 at 10:13:09AM -0700, Junio C Hamano wrote:

Yeah, I was playing around with the same idea yesterday, but got pulled into some meetings and thus couldn't finish that work.

I think it's overall not that bad, and I've got something that's almost done. It's an easy win for students indeed, but in this case I'd rather make our test suite a bit more robust sooner rather than later :)

I'll likely have something later today. Thanks!

Patrick