1.7.2 cycle will open soon

1.7.2 cycle will open soon

[Junio C Hamano]

The usual post-release ritual.

 - The tip of 'next' has been rebuilt on top of the release, with topics
   that have been cooking;

 - The 'maint' branch is now for 1.7.1.X series; and

 - What have been cooking in 'next' during the pre-release freeze period
   for 1.7.1 should start graduating to 'master' soon.

It appears that the tip of 'maint' and 'next' (but curiously not 'master')
do not pass test t1504 on at least FreeBSD right now, but I don't have
enough energy to hunt things down tonight.

Re: 1.7.2 cycle will open soon

[Jeff King]

On Tue, May 04, 2010 at 10:39:59PM -0700, Junio C Hamano wrote:

> It appears that the tip of 'maint' and 'next' (but curiously not 'master')
> do not pass test t1504 on at least FreeBSD right now, but I don't have
> enough energy to hunt things down tonight.

The culprit is Jonathan's 3bf7886 (test-lib: Let tests specify commands
to be run at end of test, 2010-05-02). FreeBSD's /bin/sh doesn't
propagate $? over an eval:

  $ /bin/sh
  $ false; echo $?
  1
  $ false; eval 'echo $?'
  0
  $ eval 'false; echo $?' ;# but it still works inside, of course
  1

Looking at that patch, I don't see any reason that eval_ret needs to be
set inside the eval. If we have multiple test_when_finished calls, we
keep setting and propagating eval_ret, which doesn't make much sense to
me.  Why not just:

  test_run_ () {
    test_cleanup=
    eval >&3 2>&4 "$1"
    eval_ret=$?
    eval >&3 2>&4 "$test_cleanup"
    return 0
  }

  test_when_finished () {
    test_cleanup="$*; $test_cleanup"
  }

Am I missing something?

-Peff

Re: 1.7.2 cycle will open soon

[Jonathan Nieder]

Jeff King wrote:

> FreeBSD's /bin/sh doesn't propagate $? over an eval

Thanks for tracking it down.

> Looking at that patch, I don't see any reason that eval_ret needs to be
> set inside the eval. If we have multiple test_when_finished calls, we
> keep setting and propagating eval_ret, which doesn't make much sense to
> me.  Why not just:
> 
>   test_run_ () {
>     test_cleanup=
>     eval >&3 2>&4 "$1"
>     eval_ret=$?
>     eval >&3 2>&4 "$test_cleanup"
>     return 0
>   }
> 
>   test_when_finished () {
>     test_cleanup="$*; $test_cleanup"
>   }
> 
> Am I missing something?

If cleanup fails, I want to catch it.  Would something like this do?

 test_run_ () {
	test_cleanup=:
	eval >&3 2>&4 "$1"
	eval_ret=$?
	eval >&3 2>&4 "$test_cleanup" && (exit "$eval_ret")
	eval_ret=$?
	return 0
 }

 test_when_finished () {
	test_cleanup="$* && $test_cleanup"
 }

To permit a line break at the end of a cleanup command, one can do

 test_when_finished () {
	test_cleanup="{ $*
		} && $test_cleanup"
 }

but that might not be worth the ugliness.

Jonathan

Re: 1.7.2 cycle will open soon

[Jeff King]

On Thu, May 06, 2010 at 01:44:28AM -0500, Jonathan Nieder wrote:

> > Am I missing something?
> 
> If cleanup fails, I want to catch it.  Would something like this do?

Ah, I see.

>  test_run_ () {
> 	test_cleanup=:
> 	eval >&3 2>&4 "$1"
> 	eval_ret=$?
> 	eval >&3 2>&4 "$test_cleanup" && (exit "$eval_ret")
> 	eval_ret=$?
> 	return 0
>  }
> 
>  test_when_finished () {
> 	test_cleanup="$* && $test_cleanup"
>  }

Doesn't that violate your rule that the cleanup will _always_ be run?
Here, if the first cleanup fails, we don't run subsequent ones.

Perhaps the simplest would be to just keep a cleanup_ret in the second
eval (where you have eval_ret in the original patch), and then act
appropriately after the eval finishes. That would be easier on the eyes,
too, I think.

> To permit a line break at the end of a cleanup command, one can do
> 
>  test_when_finished () {
> 	test_cleanup="{ $*
> 		} && $test_cleanup"
>  }
> 
> but that might not be worth the ugliness.

I doubt anyone will put a linebreak in, but it is probably better to be
defensive, and the ugliness is at least contained only in that function.

-Peff

Re: 1.7.2 cycle will open soon

[Johannes Sixt]

Am 5/6/2010 7:52, schrieb Jeff King:
> Why not just:
> 
>   test_run_ () {
>     test_cleanup=
>     eval >&3 2>&4 "$1"
>     eval_ret=$?
>     eval >&3 2>&4 "$test_cleanup"
>     return 0
>   }
> 
>   test_when_finished () {
>     test_cleanup="$*; $test_cleanup"
>   }
> 
> Am I missing something?

I ask myself the same question.

For my taste, this change went production far too fast. Did I miss any
discussion on it? (I didn't speak up when it passed by on the ML because I
was on a short trip at that time.)

-- Hannes

Re: 1.7.2 cycle will open soon

[Johannes Sixt]

Am 5/6/2010 8:44, schrieb Jonathan Nieder:
>  test_when_finished () {
> 	test_cleanup="$* && $test_cleanup"
>  }

I'm wondering why you want this test_cleanup at all?

Is it so that subsequent tests can succeed even if an earlier test failed
before its regular cleanup?

I don't see what this buys you. If a test case uncovers a regression, you
got to fix it - who cares how many later tests fail or not? Once you are
finished with your change, all tests will pass anyway (including their
regular cleanups).

-- Hannes

Re: 1.7.2 cycle will open soon

[Jeff King]

On Thu, May 06, 2010 at 09:06:43AM +0200, Johannes Sixt wrote:

> Am 5/6/2010 8:44, schrieb Jonathan Nieder:
> >  test_when_finished () {
> > 	test_cleanup="$* && $test_cleanup"
> >  }
> 
> I'm wondering why you want this test_cleanup at all?
> 
> Is it so that subsequent tests can succeed even if an earlier test failed
> before its regular cleanup?
> 
> I don't see what this buys you. If a test case uncovers a regression, you
> got to fix it - who cares how many later tests fail or not? Once you are
> finished with your change, all tests will pass anyway (including their
> regular cleanups).

I have to agree. Yes, using test_when_finished can make _some_ tests
more robust, but there will still be many tests whose breakage will
break future tests. And many of those will never be fixed, because the
tests simply build on one another.

So I don't think we will ever "solve" this problem, which means testers
will continue to have to fix early failures before looking at later
ones, because they don't know if the later test is a false negative or
not. And as you point out, it is simply not that big a deal in the first
place.

-Peff

Re: 1.7.2 cycle will open soon

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 5/6/2010 8:44, schrieb Jonathan Nieder:
>>  test_when_finished () {
>> 	test_cleanup="$* && $test_cleanup"
>>  }
>
> I'm wondering why you want this test_cleanup at all?
> 
> Is it so that subsequent tests can succeed even if an earlier test failed
> before its regular cleanup?

Yes.  In some cases (permissions-related), if a test fails, even a
‘make clean’ afterwards fails.

> I don't see what this buys you. If a test case uncovers a regression, you
> got to fix it - who cares how many later tests fail or not? Once you are
> finished with your change, all tests will pass anyway (including their
> regular cleanups).

Why do we support the non --immediate mode at all, then?  Just like it can
be easier to understand the result when a compile uncovers more than one
error, it can help in debugging to see which later tests were broken.

If there is a consensus that this is not worth it, I am fine with
that, though.  The current status is that each test where this matters
does things its own way.

Jonathan

[PATCH] test-lib: some shells do not let $? propagate into an eval

[Jonathan Nieder]

Jeff King wrote:
> On Thu, May 06, 2010 at 01:44:28AM -0500, Jonathan Nieder wrote:

>>  test_when_finished () {
>> 	test_cleanup="$* && $test_cleanup"
>>  }
>
> Doesn't that violate your rule that the cleanup will _always_ be run?

Thanks for the sanity check.

> Perhaps the simplest would be to just keep a cleanup_ret in the second
> eval (where you have eval_ret in the original patch), and then act
> appropriately after the eval finishes. That would be easier on the eyes,
> too, I think.

I tried the cleanup_ret idea; test_run_ ended up looking like this:

	test_cleanup=:
	eval >&3 2>&4 "$1"
	eval_ret=$?
	eval >&3 2>&4 ":; $test_cleanup"
	cleanup_ret=?
	(exit "$test_ret") && (exit "$cleanup_ret")
	eval_ret=$?
	return 0

That breaks the principle of keeping the ugliness in test_when_finished.
So here’s the minimal fix.

-- 8< --
Subject: test-lib: some shells do not let $? propagate into an eval

In 3bf7886 (test-lib: Let tests specify commands to be run at end of
test, 2010-05-02), the git test harness learned to run cleanup
commands unconditionally at the end of a test.  During each test,
the intended cleanup actions are collected in the test_cleanup variable
and evaluated.  That variable looks something like this:

	eval_ret=$?; clean_something && (exit "$eval_ret")
	eval_ret=$?; clean_something_else && (exit "$eval_ret")
	eval_ret=$?; final_cleanup && (exit "$eval_ret")
	eval_ret=$?

All cleanup actions are run unconditionally but if one of them fails
it is properly reported through $eval_ret.

On FreeBSD, unfortunately, $? is set at the beginning of an ‘eval’
to 0 instead of the exit status of the previous command.  This results
in tests using test_expect_code appearing to fail and all others
appearing to pass, unless their cleanup fails.  Avoid the problem by
setting eval_ret before the ‘eval’ begins.

Thanks to Jeff King for the explanation.

Cc: Jeff King <peff@peff.net>
Cc: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I was also surprised to see this migrate to maint so quickly, but I
was happy to see it broke early and loudly.

Because there is some unhappiness with the feature[1], it might make
more sense to revert it for now.  If that discussion can be taken as
license to write tests that take down other tests with them on
failure, then such a revert would not interfere with other work.

[1] http://thread.gmane.org/gmane.comp.version-control.git/146375/focus=146451

 t/t0000-basic.sh |   21 +++++++++++++++++++++
 t/test-lib.sh    |    7 ++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f4ca4fc..3ec9cbe 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -73,6 +73,27 @@ then
 	exit 1
 fi
 
+clean=no
+test_expect_success 'tests clean up after themselves' '
+    test_when_finished clean=yes
+'
+
+cleaner=no
+test_expect_code 1 'tests clean up even after a failure' '
+    test_when_finished cleaner=yes &&
+    (exit 1)
+'
+
+if test $clean$cleaner != yesyes
+then
+	say "bug in test framework: cleanup commands do not work reliably"
+	exit 1
+fi
+
+test_expect_code 2 'failure to clean up causes the test to fail' '
+    test_when_finished "(exit 2)"
+'
+
 ################################################################
 # Basics of the basics
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index acce3d0..7422bba 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -366,8 +366,9 @@ test_debug () {
 }
 
 test_run_ () {
-	test_cleanup='eval_ret=$?'
+	test_cleanup=:
 	eval >&3 2>&4 "$1"
+	eval_ret=$?
 	eval >&3 2>&4 "$test_cleanup"
 	return 0
 }
@@ -567,8 +568,8 @@ test_cmp() {
 # the test to pass.
 
 test_when_finished () {
-	test_cleanup="eval_ret=\$?; { $*
-		} && (exit \"\$eval_ret\"); $test_cleanup"
+	test_cleanup="{ $*
+		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
 
 # Most tests can use the created repository, but some may need to create more.
-- 
1.7.1

Re: [PATCH] test-lib: some shells do not let $? propagate into an eval

[Jeff King]

On Thu, May 06, 2010 at 03:41:10AM -0500, Jonathan Nieder wrote:

> I tried the cleanup_ret idea; test_run_ ended up looking like this:
> 
> 	test_cleanup=:
> 	eval >&3 2>&4 "$1"
> 	eval_ret=$?
> 	eval >&3 2>&4 ":; $test_cleanup"
> 	cleanup_ret=?
> 	(exit "$test_ret") && (exit "$cleanup_ret")
> 	eval_ret=$?

Maybe it is just me, but those last two lines would be much more
readable as:

  if test "$eval_ret" = 0; then
    eval_ret=$cleanup_ret
  fi

assuming your "$test_ret" was supposed to be $eval_ret.

> That breaks the principle of keeping the ugliness in test_when_finished.
> So here’s the minimal fix.

I don't know that we have necessarily have that principle. The most
important thing is that ugliness not escape to the test scripts
themselves, where we have to write many thousands of (hopefully
readable) lines.

But...

> -- 8< --
> Subject: test-lib: some shells do not let $? propagate into an eval

This fix looks fine to me, and I tested it on FreeBSD 8.0. So:

 Acked-by: Jeff King <peff@peff.net>

> I was also surprised to see this migrate to maint so quickly, but I
> was happy to see it broke early and loudly.

I think Junio is much more carefree with changes that only touch test
infrastructure, as they cannot possibly be breaking git itself for
anybody.

> Because there is some unhappiness with the feature[1], it might make

To clarify my position, I am not that negative on the feature. I just
think it is a bit of a fool's errand, and I don't want to personally
spend any time on making it happen. If you think it is worth proceeding
and you can do so without breaking anything[1], I won't object.

[1] Yes, I am teasing you. But probably the complex part is now done,
and you and others can use test_when_finished() at will now, and we can
see if it actually makes anybody's life better.

-Peff

Re: [PATCH] test-lib: some shells do not let $? propagate into an eval

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think Junio is much more carefree with changes that only touch test
> infrastructure, as they cannot possibly be breaking git itself for
> anybody.

Yes, and also I deliberately queued this with a known breakage because
I've been swamped with non-git issues (sorry), I knew it would be very
isolated issue, and it was the easiest way to get people's attention.

Thanks for helping.