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

From: Jeff King <peff@peff.net>
Date: May 6, 2010, 08:57 UTC
Message-ID: <20100506085720.GA31873@coredump.intra.peff.net>
In-Reply-To: <20100506084045.GA25917@progeny.tock>

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

Show 9 quoted lines

> 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