[PATCH] t0005: add test for trap handling from deeply nested function calls

[PATCH] t0005: add test for trap handling from deeply nested function calls

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

The /usr/xpg4/bin/sh shell on Solaris only executes a trap handler set
within a function from the first-level function call made from within the
function that set the trap.  In other words, if func1 sets a trap, then
calls func2 which triggers the trap, then the trap handler will be executed
correctly.  If instead of exiting, func2 calls func3, and func3 satisfies
the conditions for triggering the trap, the trap handler will NOT be
executed.

This trap sequence exists in git-bisect.sh and is exercised by tests
t6030.12 and t6030.13 which fail when the /usr/xpg4/bin/sh shell is used.

Add a test that will fail sooner and more precisely than the t6030 tests.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


Improvements to the commit message are welcome.

-brandon


 t/t0005-signals.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 09f855a..cb68d1f 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -19,4 +19,12 @@ test_expect_success 'sigchain works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'trap triggered in deeply nested function works correctly' '
+	(atrap () { exit 0; }
+	 func3 () { exit 1; }
+	 func2 () { func3; }
+	 func1 () { trap atrap EXIT; func2; }
+	 func1)
+'
+
 test_done
-- 
1.6.6.2

Re: [PATCH] t0005: add test for trap handling from deeply nested function calls

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> From: Brandon Casey <drafnel@gmail.com>
>
> The /usr/xpg4/bin/sh shell on Solaris only executes a trap handler set
> within a function from the first-level function call made from within the
> function that set the trap.  In other words, if func1 sets a trap, then
> calls func2 which triggers the trap, then the trap handler will be executed
> correctly.  If instead of exiting, func2 calls func3, and func3 satisfies
> the conditions for triggering the trap, the trap handler will NOT be
> executed.
>
> This trap sequence exists in git-bisect.sh and is exercised by tests
> t6030.12 and t6030.13 which fail when the /usr/xpg4/bin/sh shell is used.
>
> Add a test that will fail sooner and more precisely than the t6030 tests.

That would be useful if you run all the tests in sequence.

It smells like the real solution is to fix git-bisect.sh not to do that
trap trick, if we can.

Or perhaps have this not as an individual test but as a code to set a test
prerequiste token (e.g. NESTED_TRAP) to run tests of bisect conditionally?

>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>
>
> Improvements to the commit message are welcome.
>
> -brandon
>
>
>  t/t0005-signals.sh |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
> index 09f855a..cb68d1f 100755
> --- a/t/t0005-signals.sh
> +++ b/t/t0005-signals.sh
> @@ -19,4 +19,12 @@ test_expect_success 'sigchain works' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'trap triggered in deeply nested function works correctly' '
> +	(atrap () { exit 0; }
> +	 func3 () { exit 1; }
> +	 func2 () { func3; }
> +	 func1 () { trap atrap EXIT; func2; }
> +	 func1)
> +'
> +
>  test_done
> -- 
> 1.6.6.2

Re: [PATCH] t0005: add test for trap handling from deeply nested function calls

[Johannes Sixt]

On Freitag, 26. März 2010, Brandon Casey wrote:
> +test_expect_success 'trap triggered in deeply nested function works
> correctly' ' +	(atrap () { exit 0; }
> +	 func3 () { exit 1; }
> +	 func2 () { func3; }
> +	 func1 () { trap atrap EXIT; func2; }
> +	 func1)
> +'

What do you want to achieve with this test? That /usr/xpg4/bin/sh is not used 
on Solaris?

If git-bisect triggers this bug in /usr/xpg4/bin/sh and if this is our shell 
of choice on Solaris, wouldn't it be better to work around it in git-bisect?

-- Hannes

Re: [PATCH] t0005: add test for trap handling from deeply nested function calls

[Brandon Casey]

On 03/26/2010 03:19 PM, Johannes Sixt wrote:
> On Freitag, 26. März 2010, Brandon Casey wrote:
>> +test_expect_success 'trap triggered in deeply nested function works
>> correctly' ' +	(atrap () { exit 0; }
>> +	 func3 () { exit 1; }
>> +	 func2 () { func3; }
>> +	 func1 () { trap atrap EXIT; func2; }
>> +	 func1)
>> +'
> 
> What do you want to achieve with this test? That /usr/xpg4/bin/sh is not used 
> on Solaris?

Yeah, pretty much.  At least until bisect is rewritten in c.

> If git-bisect triggers this bug in /usr/xpg4/bin/sh and if this is our shell 
> of choice on Solaris,

It's not currently git's shell of choice on Solaris.  The Makefile still sets
SHELL_PATH = /bin/bash in the SunOS section.

> wouldn't it be better to work around it in git-bisect?

Yes.  You and Junio are giving the same advice, but I don't know how to work
around it.  I don't think there is anything wrong or tricky about what
bisect does.  I just think the shell is broken with respect to this
behavior.

This shell ignores traps that are set in the second function level away from
where the trap was set (I found this difficult to describe in my commit
message).  So even if the trap in my test above was set at the top level of
the script, it would not be executed after a call to exit from func3 or from
func2.

Take a look at this script:

--- >8 ---
#!/bin/ksh

(atrap () { exit 0; }
 func3 () { exit 1; }
 func2 () { func3; }
 func1 () { trap atrap EXIT; func2; }
 func1) &&
 echo 'SUCCESS: Trap set in function catches exit two levels deeper' ||
 echo 'FAILURE: Trap set in function fails to catch exit two levels deeper'

(atrap () { exit 0; }
 func2 () { exit 1; }
 func1 () { trap atrap EXIT; func2; }
 func1) &&
 echo 'SUCCESS: Trap set in function catches exit one level deeper' ||
 echo 'FAILURE: Trap set in function fails to catch exit one level deeper'

(atrap () { exit 0; }
 func3 () { exit 1; }
 func2 () { func3; }
 func1 () { func2; }
 trap atrap EXIT
 func1) &&
 echo 'SUCCESS: Trap set at top-level catches exit three levels deeper' ||
 echo 'FAILURE: Trap set at top-level fails to catch exit three levels deeper'

(atrap () { exit 0; }
 func2 () { exit 1; }
 func1 () { func2; }
 trap atrap EXIT
 func1) &&
 echo 'SUCCESS: Trap set at top-level catches exit two levels deeper' ||
 echo 'FAILURE: Trap set at top-level fails to catch exit two levels deeper'

(atrap () { exit 0; }
 func1 () { exit 1; }
 trap atrap EXIT
 func1) &&
 echo 'SUCCESS: Trap set at top-level catches exit one level deeper' ||
 echo 'FAILURE: Trap set at top-level fails to catch exit one level deeper'
--- >8 ---

On Linux or IRIX with bash or ksh, it prints
SUCCESS: Trap set in function catches exit two levels deeper
SUCCESS: Trap set in function catches exit one level deeper
SUCCESS: Trap set at top-level catches exit three levels deeper
SUCCESS: Trap set at top-level catches exit two levels deeper
SUCCESS: Trap set at top-level catches exit one level deeper

Using Solaris's /usr/xpg4/bin/sh it prints:
FAILURE: Trap set in function fails to catch exit two levels deeper
SUCCESS: Trap set in function catches exit one level deeper
FAILURE: Trap set at top-level fails to catch exit three levels deeper
FAILURE: Trap set at top-level fails to catch exit two levels deeper
SUCCESS: Trap set at top-level catches exit one level deeper

Funny enough, Solaris's /bin/sh works correctly.

So, I don't see any way to catch a termination by signal in the scripts
that are currently using trap and which may make a function call from
within another function.

The explicit exit's within the script could be handled without a trap
though.  Maybe that is enough.  We'll see how ugly it is...

-brandon