* t5570 trap use in start/stop_git_daemon
@ 2015-02-12 20:31 Randall S. Becker
2015-02-13 7:44 ` Jeff King
0 siblings, 1 reply; 5+ messages in thread
From: Randall S. Becker @ 2015-02-12 20:31 UTC (permalink / raw)
To: 'Git Mailing List'; +Cc: 'Joachim Schmitz'
On the NonStop port, we found that trap was causing an issue with test
success for t5570. When start_git_daemon completes, the shell (ksh,bash) on
this platform is sending a signal 0 that is being caught and acted on by the
trap command within the start_git_daemon and stop_git_daemon functions. I am
taking this up with the operating system group, but in any case, it may be
appropriate to include a trap reset at the end of both functions, as below.
I verified this change on SUSE Linux.
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index bc4b341..543e98a 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -62,6 +62,7 @@ start_git_daemon() {
test_skip_or_die $GIT_TEST_GIT_DAEMON \
"git daemon failed to start"
fi
+ trap '' EXIT
}
stop_git_daemon() {
@@ -84,4 +85,6 @@ stop_git_daemon() {
fi
GIT_DAEMON_PID=
rm -f git_daemon_output
+
+ trap '' EXIT
}
Cheers,
Randall
-- Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)/NonStop(211288444200000000)
-- In real life, I talk too much.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: t5570 trap use in start/stop_git_daemon
2015-02-12 20:31 t5570 trap use in start/stop_git_daemon Randall S. Becker
@ 2015-02-13 7:44 ` Jeff King
2015-02-13 8:03 ` Jeff King
0 siblings, 1 reply; 5+ messages in thread
From: Jeff King @ 2015-02-13 7:44 UTC (permalink / raw)
To: Randall S. Becker; +Cc: 'Git Mailing List', 'Joachim Schmitz'
On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> On the NonStop port, we found that trap was causing an issue with test
> success for t5570. When start_git_daemon completes, the shell (ksh,bash) on
> this platform is sending a signal 0 that is being caught and acted on by the
> trap command within the start_git_daemon and stop_git_daemon functions. I am
> taking this up with the operating system group,
Yeah, that seems wrong. If it were a subshell, even, I could see some
argument for it, but it seems odd to trap 0 when a function returns
(bash does have a RETURN trap, which AFAIK is bash-specific, but it
should not trigger a 0-trap).
> but in any case, it may be
> appropriate to include a trap reset at the end of both functions, as below.
> I verified this change on SUSE Linux.
>
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index bc4b341..543e98a 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -62,6 +62,7 @@ start_git_daemon() {
> test_skip_or_die $GIT_TEST_GIT_DAEMON \
> "git daemon failed to start"
> fi
> + trap '' EXIT
> }
I don't think this is the right thing to do. That trap is meant to live
beyond the function's return. Without it, there is nothing to clean up
the running git-daemon if we exit the test script prematurely (e.g., by
a test failing in immediate-mode). We pollute the environment with a
running process which would cause subsequent test runs to fail.
> stop_git_daemon() {
> @@ -84,4 +85,6 @@ stop_git_daemon() {
> fi
> GIT_DAEMON_PID=
> rm -f git_daemon_output
> +
> + trap '' EXIT
> }
This one is slightly less bad, in that we are dropping our
daemon-specific cleanup here anyway. But the appropriate trap is still:
trap 'die' EXIT
which we set earlier in the function. Without it, the test harness's
ability to detect a premature failure is lost.
So I do not know quite what is going on with your shell, but turning off
the traps in these functions is definitely not an acceptable (general)
workaround; it makes things much worse on working platforms.
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: t5570 trap use in start/stop_git_daemon
2015-02-13 7:44 ` Jeff King
@ 2015-02-13 8:03 ` Jeff King
2015-02-13 8:57 ` Joachim Schmitz
0 siblings, 1 reply; 5+ messages in thread
From: Jeff King @ 2015-02-13 8:03 UTC (permalink / raw)
To: Randall S. Becker; +Cc: 'Git Mailing List', 'Joachim Schmitz'
On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:
> On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
>
> > On the NonStop port, we found that trap was causing an issue with test
> > success for t5570. When start_git_daemon completes, the shell (ksh,bash) on
> > this platform is sending a signal 0 that is being caught and acted on by the
> > trap command within the start_git_daemon and stop_git_daemon functions. I am
> > taking this up with the operating system group,
>
> Yeah, that seems wrong. If it were a subshell, even, I could see some
> argument for it, but it seems odd to trap 0 when a function returns
> (bash does have a RETURN trap, which AFAIK is bash-specific, but it
> should not trigger a 0-trap).
Hmm, today I learned something new about ksh. Apparently when you use
the "function" keyword to define a function like:
function foo {
trap 'echo trapped' EXIT
}
echo before
foo
echo after
then the trap runs when the function exits! If you declare the same
function as:
foo() {
trap 'echo trapped' EXIT
}
it behaves differently. POSIX shell does not have the function keyword,
of course, and we are not using it here. Bash _does_ have the function
keyword, but seems to behave POSIX-y even when it is present. I.e.,
running the first script:
$ ksh foo.sh
before
trapped
after
$ bash foo.sh
before
after
trapped
$ dash foo.sh
foo.sh: 3: foo.sh: function: not found
foo.sh: 5: foo.sh: Syntax error: "}" unexpected
Switching to the second form, all three produce:
before
after
trapped
I don't know if that is all helpful to your bug-tracking or analysis,
but for whatever reason it looks like your ksh is using localized traps
for both forms of function. But as far as I know, bash has never behaved
that way (I just grepped its CHANGES file for mentions of trap and found
nothing likely).
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: t5570 trap use in start/stop_git_daemon
2015-02-13 8:03 ` Jeff King
@ 2015-02-13 8:57 ` Joachim Schmitz
2015-02-13 12:27 ` Randall S. Becker
0 siblings, 1 reply; 5+ messages in thread
From: Joachim Schmitz @ 2015-02-13 8:57 UTC (permalink / raw)
To: git
Jeff King <peff <at> peff.net> writes:
>
> On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:
>
> > On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> >
<snip>
> Hmm, today I learned something new about ksh. Apparently when you use
> the "function" keyword to define a function like:
>
> function foo {
> trap 'echo trapped' EXIT
> }
> echo before
> foo
> echo after
>
> then the trap runs when the function exits! If you declare the same
> function as:
>
> foo() {
> trap 'echo trapped' EXIT
> }
>
> it behaves differently. POSIX shell does not have the function keyword,
> of course, and we are not using it here. Bash _does_ have the function
> keyword, but seems to behave POSIX-y even when it is present. I.e.,
> running the first script:
>
> $ ksh foo.sh
> before
> trapped
> after
>
> $ bash foo.sh
> before
> after
> trapped
>
> $ dash foo.sh
> foo.sh: 3: foo.sh: function: not found
> foo.sh: 5: foo.sh: Syntax error: "}" unexpected
>
> Switching to the second form, all three produce:
>
> before
> after
> trapped
>
> I don't know if that is all helpful to your bug-tracking or analysis,
> but for whatever reason it looks like your ksh is using localized traps
> for both forms of function. But as far as I know, bash has never behaved
> that way (I just grepped its CHANGES file for mentions of trap and found
> nothing likely).
>
> -Peff
>
Both versions produce your first output on our platform
$ ksh foo1.sh
before
trapped
after
$ bash foo1.sh
before
after
trapped
$ ksh foo2.sh
before
trapped
after
$ bash foo2.sh
before
after
trapped
$
This might have been one (or even _the_) reason why we picked bash as our
SHELL_PATH in config.mak.uname (I don't remember, it's more than 2 years
ago), not sure which shell Randall's test used?
bye, Jojo
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: t5570 trap use in start/stop_git_daemon
2015-02-13 8:57 ` Joachim Schmitz
@ 2015-02-13 12:27 ` Randall S. Becker
0 siblings, 0 replies; 5+ messages in thread
From: Randall S. Becker @ 2015-02-13 12:27 UTC (permalink / raw)
To: 'Joachim Schmitz', git
On 2015/02/13 3:58AM Joachim Schmitz wrote:
>Jeff King <peff <at> peff.net> writes:
> > On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:
> > On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> >
><snip>
>> Hmm, today I learned something new about ksh. Apparently when you use
>> the "function" keyword to define a function like:
>>
>> function foo {
>> trap 'echo trapped' EXIT
>> }
>> echo before
>> foo
>> echo after
>>
>> then the trap runs when the function exits! If you declare the same
>> function as:
>>
>> foo() {
>> trap 'echo trapped' EXIT
>> }
>>
>> it behaves differently. POSIX shell does not have the function keyword,
>> of course, and we are not using it here. Bash _does_ have the function
>> keyword, but seems to behave POSIX-y even when it is present. I.e.,
>> running the first script:
>>
>> $ ksh foo.sh
>> before
>> trapped
>> after
>>
>> $ bash foo.sh
> > before
>> after
> > trapped
>>
<snip>
>Both versions produce your first output on our platform
>$ ksh foo1.sh
>before
>trapped
>after
>$ bash foo1.sh
>before
>after
>trapped
>$ ksh foo2.sh
>before
>trapped
>after
>$ bash foo2.sh
>before
>after
>trapped
>$
>This might have been one (or even _the_) reason why we picked bash as our
>SHELL_PATH in config.mak.uname (I don't remember, it's more than 2 years
>ago), not sure which shell Randall's test used?
I tested both for trying to get t5570 to work. No matter which, without
resetting the trap, function return would kill the git-daemon and the test
would fail.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-02-13 12:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-12 20:31 t5570 trap use in start/stop_git_daemon Randall S. Becker
2015-02-13 7:44 ` Jeff King
2015-02-13 8:03 ` Jeff King
2015-02-13 8:57 ` Joachim Schmitz
2015-02-13 12:27 ` Randall S. Becker
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.