[PATCH] Portability: returning void

[PATCH] Portability: returning void

[Michael Witten]

Currently, building git with:

  CFLAGS="-std=c99 -pedantic -Wall -Werror -g -02"

causes gcc 4.5.2 to fail with:

  vcs-svn/svndump.c:217:3: error: ISO C forbids 'return' with
                                  expression, in function
                                  returning void

The line in question is this:

  return repo_delete(node_ctx.dst);

Because repo_delete returns void (vcs-svn/repo_tree.h:19):

  void repo_delete(uint32_t *path);

it would seem like it would be OK, but I guess the
C99 standard is quite particular:

  6.8.6.4.1:
  A return statement with an expression shall not appear in
  a function whose return type is void. A return statement
  without an expression shall only appear in a function
  whose return type is void.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 vcs-svn/svndump.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index eef49ca..572a995 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -214,7 +214,8 @@ static void handle_node(void)
 		if (have_text || have_props || node_ctx.srcRev)
 			die("invalid dump: deletion node has "
 				"copyfrom info, text, or properties");
-		return repo_delete(node_ctx.dst);
+		repo_delete(node_ctx.dst);
+		return;
 	}
 	if (node_ctx.action == NODEACT_REPLACE) {
 		repo_delete(node_ctx.dst);
-- 
1.7.4.2.417.g32d76d

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

Hi,

Michael Witten wrote:

> Currently, building git with:
>
>   CFLAGS="-std=c99 -pedantic -Wall -Werror -g -02"
>
> causes gcc 4.5.2 to fail with:
>
>   vcs-svn/svndump.c:217:3: error: ISO C forbids 'return' with
>                                   expression, in function
>                                   returning void
[...]
> Signed-off-by: Michael Witten <mfwitten@gmail.com>

Thanks, looks sensible (and thanks to Junio for a pointer).  Applied to

  git://repo.or.cz/git/jrn.git svn-fe

Next step is to figure out the longstanding mysterious bash + prove
hang in t0081.

Jonathan Nieder (2):
      vcs-svn: add missing cast to printf argument
      tests: make sure input to sed is newline terminated

Michael Witten (1):
      vcs-svn: a void function shouldn't try to return something

 t/t9010-svn-fe.sh     |    8 ++++++--
 vcs-svn/fast_export.c |    3 ++-
 vcs-svn/svndump.c     |    3 ++-
 3 files changed, 10 insertions(+), 4 deletions(-)

Re: [PATCH] Portability: returning void

[Jeff King]

On Tue, Mar 29, 2011 at 03:02:48PM -0500, Jonathan Nieder wrote:

> Next step is to figure out the longstanding mysterious bash + prove
> hang in t0081.

This patch solves it for me:

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 1dbe1c9..7ea1317 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -50,7 +50,7 @@ long_read_test () {
 		{
 			generate_tens_of_lines $tens_of_lines "$line" &&
 			sleep 100
-		} >input &
+		} >input 5>/dev/null &
 	} &&
 	test-line-buffer input <<-EOF >output &&
 	binary $readsize
@@ -84,7 +84,7 @@ test_expect_success PIPE '0-length read, no input available' '
 	rm -f input &&
 	mkfifo input &&
 	{
-		sleep 100 >input &
+		sleep 100 >input 5>/dev/null &
 	} &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 0
@@ -113,7 +113,7 @@ test_expect_success PIPE '1-byte read, no input available' '
 			printf "%s" a &&
 			printf "%s" b &&
 			sleep 100
-		} >input &
+		} >input 5>/dev/null &
 	} &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 1

The problem is that the sleeps hang around for 100 seconds, and they are
connected to the test script's stdout. It works to run "./t0081-*"
because bash sees the SIGCHLD and knows the script is done. But the
prove program actually ignore the SIGCHLD and waits until stdout and
stderr on the child are closed.

I'm not sure why it hangs with bash and not dash. Perhaps it has to do
with one of them using an internal "sleep" and the other not.
Double-weird is that if you "strace" the prove process, it will still
hang. But if you "strace -f", it _won't_ hang. Which makes no sense,
because the only extra thing happening is strace-ing the now-zombie bash
process and the sleeps which are, well, sleeping.

So there is definitely some deep mystery, but I think the fix can be
simpler: we should not leave processes hanging around that might have
descriptors open. Though the above works, I don't like scripts having to
know about the descriptor 5 magic above. The right solution to me is
either:

  1. Close descriptor 5 before running test code. But I'm not sure that
     will work, since we actually execute the test code in the _current_
     shell, not a subshell. Meaning we would be closing it for good.

     It also doesn't address descriptor 4, which might also go to
     stdout (if "-v" was used). I think the saving grace is that "prove"
     is the only thing that cares, and it doesn't use "-v".

  2. Tests should kill their backgrounded sleeps themselves. I think I
     saw some "kill $!" lines in there, but maybe we are missing one.

-Peff

Re: [PATCH] Portability: returning void

[Jeff King]

On Tue, Mar 29, 2011 at 06:16:52PM -0400, Jeff King wrote:

> On Tue, Mar 29, 2011 at 03:02:48PM -0500, Jonathan Nieder wrote:
> 
> > Next step is to figure out the longstanding mysterious bash + prove
> > hang in t0081.
>
> [...]
> 
>   2. Tests should kill their backgrounded sleeps themselves. I think I
>      saw some "kill $!" lines in there, but maybe we are missing one.

Indeed, those kill lines aren't doing what you expect. Try instrumenting
like this:

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 1dbe1c9..3b2f8ce 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 
 test_description="Test the svn importer's input handling routines.
 
@@ -56,6 +56,7 @@ long_read_test () {
 	binary $readsize
 	copy $copysize
 	EOF
+	echo >>/tmp/foo killing $!
 	kill $! &&
 	test_line_count = $lines output &&
 	tail -n 1 <output >actual &&
@@ -90,6 +91,7 @@ test_expect_success PIPE '0-length read, no input available' '
 	binary 0
 	copy 0
 	EOF
+	echo >>/tmp/foo killing $!
 	kill $! &&
 	test_cmp expect actual
 '
@@ -119,6 +121,7 @@ test_expect_success PIPE '1-byte read, no input available' '
 	binary 1
 	copy 1
 	EOF
+	echo >>/tmp/foo killing $!
 	kill $! &&
 	test_cmp expect actual
 '

And then run the test under prove, and check "ps" for remaining sleep
processes. You will see that the killed process IDs do not match up with
the sleep processes. Except for one sleep process, which actually got
killed. That is the second one in the list above. It works because it's:

  {
    sleep 100 >input &
  } &&
  ...
  kill $!

whereas the other ones are like:

  {
    do_something &&
    sleep 100
  } >input &

So my guess is that we have to start a subshell for the latter ones, and
_that_ is what gets killed. And that may explain the bash vs dash
behavior; dash, upon receiving the kill signal, presumably kills its
child, but bash does not (I didn't confirm that; it's just a theory).

I'm not sure what the best solution is. Perhaps putting the subshell
into its own process group and killing the whole PGRP?

-Peff

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

Jeff King wrote:

> The problem is that the sleeps hang around for 100 seconds, and they are
> connected to the test script's stdout. It works to run "./t0081-*"
> because bash sees the SIGCHLD and knows the script is done. But the
> prove program actually ignore the SIGCHLD and waits until stdout and
> stderr on the child are closed.

Strange.  Why would prove tell its children to ignore SIGCHLD and
SIGTERM?

> Double-weird is that if you "strace" the prove process, it will still
> hang. But if you "strace -f", it _won't_ hang.

Well, it hangs for me. :)  The strangest aspect is that after 100
seconds, all is well again, which suggests that there's more happening
than an unreaped process.

After lowering the sleep to 15 seconds:

| kill: 19422
| bash(19422)---sleep(19424)
|
| ok 3 - 0-length read, no input available
[...]
| kill: 19431
| bash(19431)---sleep(19434)
| ok 5 - 1-byte read, no input available
[...]
| kill: 19438
| bash(19438)---sleep(19441)
| 
| ok 6 - long read (around 8192 bytes)
[...]
| # passed all 13 test(s)
[...]
| 19398 18:31:12 exit_group(0)            = ?
| 19397 18:31:12 <... select resumed> )   = ? ERESTARTNOHAND (To be restarted)
| 19397 18:31:12 --- SIGCHLD (Child exited) @ 0 (0) ---
| 19397 18:31:12 select(8, [4 6], NULL, NULL, NULL <unfinished ...>

The test script exits, but "prove" is stuck in select and does not
want to start reaping yet.  So presumably the test script's children
are adopted by init.  We wait around 13 seconds, and then:

| 19424 18:31:25 <... nanosleep resumed> NULL) = 0
| 19424 18:31:25 close(1)                 = 0
| 19424 18:31:25 close(2)                 = 0
| 19424 18:31:25 exit_group(0)            = ?
| 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)
| 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---

The first sleep wakes up and dies.  The corresponding subshell
wakes up, reaps the child, and finally accepts SIGTERM.

| 19434 18:31:26 <... nanosleep resumed> NULL) = 0
[...]
| 19438 18:31:26 --- SIGTERM (Terminated) @ 0 (0) ---

Likewise for the second and third sleeps.

| 19397 18:31:26 <... select resumed> )   = 2 (in [4 6])
| 19397 18:31:26 read(4, "", 65536)       = 0
| 19397 18:31:26 read(6, "", 65536)       = 0
| 19397 18:31:26 wait4(19398, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 19398

Now "prove" wakes up again.

The analagous experiment for dash, mksh, etc shows that their
subshells exec()'d /bin/sleep so the SIGTERM reaches sleep right
away with no trouble.

Re: [PATCH] Portability: returning void

[Jeff King]

On Tue, Mar 29, 2011 at 06:49:55PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > The problem is that the sleeps hang around for 100 seconds, and they are
> > connected to the test script's stdout. It works to run "./t0081-*"
> > because bash sees the SIGCHLD and knows the script is done. But the
> > prove program actually ignore the SIGCHLD and waits until stdout and
> > stderr on the child are closed.
> 
> Strange.  Why would prove tell its children to ignore SIGCHLD and
> SIGTERM?

No, you misunderstand. It is prove itself that ignores the SIGCHLD. It
is stuck in the loop in TAP::Parser::Iterator::Process::_next. It has
gotten SIGCHLD, but it keeps blocking waiting to get EOF on the child's
stdout.

> > Double-weird is that if you "strace" the prove process, it will still
> > hang. But if you "strace -f", it _won't_ hang.
> 
> Well, it hangs for me. :)  The strangest aspect is that after 100
> seconds, all is well again, which suggests that there's more happening
> than an unreaped process.

Doesn't that point to an unreaped process? After 100 seconds the sleep
process closes, prove gets EOF, and it completes. Lowering the "100" to
"1" caused a 1-second hang for me.

> | 19398 18:31:12 exit_group(0)            = ?
> | 19397 18:31:12 <... select resumed> )   = ? ERESTARTNOHAND (To be restarted)
> | 19397 18:31:12 --- SIGCHLD (Child exited) @ 0 (0) ---
> | 19397 18:31:12 select(8, [4 6], NULL, NULL, NULL <unfinished ...>
> 
> The test script exits, but "prove" is stuck in select and does not
> want to start reaping yet.  So presumably the test script's children
> are adopted by init.  We wait around 13 seconds, and then:

Right, prove is stuck in the select. You can see it even got SIGCHLD
above, and if you check your process list, you will probably see the
defunct bash process. But instead of realizing its child has died, it
insists on waiting until the pipe is closed. Nothing has to be adopted
by init. There are simply still processes with the pipe open.

> | 19424 18:31:25 <... nanosleep resumed> NULL) = 0
> | 19424 18:31:25 close(1)                 = 0
> | 19424 18:31:25 close(2)                 = 0
> | 19424 18:31:25 exit_group(0)            = ?
> | 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)
> | 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---
> 
> The first sleep wakes up and dies.  The corresponding subshell
> wakes up, reaps the child, and finally accepts SIGTERM.

Hrm. That's different than what happens on my system. On my system, the
bash process is _already_ dead during the whole procedure, and it is
just the stray sleeps that keep prove waiting.

Maybe different bash versions? Mine is 4.1.5(1) (from debian unstable,
bash_4.1-3).

> | 19397 18:31:26 <... select resumed> )   = 2 (in [4 6])
> | 19397 18:31:26 read(4, "", 65536)       = 0
> | 19397 18:31:26 read(6, "", 65536)       = 0
> | 19397 18:31:26 wait4(19398, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 19398
> 
> Now "prove" wakes up again.

Right, because the pipe is finally closed.

Did you try my 5>/dev/null patch? With it, I get no hang at all.

-Peff

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

(-cc: Michael, to stop spamming a kind bug reporter :))
Jeff King wrote:
> On Tue, Mar 29, 2011 at 06:49:55PM -0500, Jonathan Nieder wrote:

>> | 19424 18:31:25 <... nanosleep resumed> NULL) = 0
>> | 19424 18:31:25 close(1)                 = 0
>> | 19424 18:31:25 close(2)                 = 0
>> | 19424 18:31:25 exit_group(0)            = ?
>> | 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)
>> | 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---
>> 
>> The first sleep wakes up and dies.  The corresponding subshell
>> wakes up, reaps the child, and finally accepts SIGTERM.
>
> Hrm. That's different than what happens on my system. On my system, the
> bash process is _already_ dead during the whole procedure, and it is
> just the stray sleeps that keep prove waiting.
>
> Maybe different bash versions? Mine is 4.1.5(1) (from debian unstable,
> bash_4.1-3).

$ dpkg-query -W perl bash
bash	4.1-3
perl	5.10.1-18

Same version here, but I had modified the test a little.  *tries the
stock version again*  Same behavior still.  FWIW I am using the patch
below[1] and invoking the tests as

	strace -f -o trace.out prove --exec=bash -v t0081-line-buffer.sh :: -v -i

> Did you try my 5>/dev/null patch? With it, I get no hang at all.

Haven't tried it yet but will try.

I really don't like that as a long-term solution.  Yes, it gets prove
to stop hanging, but meanwhile we have no control over the child
processes we have spawned.  I'd rather just drop the tests.
---
diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 1dbe1c9..054bffa 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -49,13 +49,14 @@ long_read_test () {
 	{
 		{
 			generate_tens_of_lines $tens_of_lines "$line" &&
-			sleep 100
+			sleep 15
 		} >input &
 	} &&
 	test-line-buffer input <<-EOF >output &&
 	binary $readsize
 	copy $copysize
 	EOF
+	pstree -p $! &&
 	kill $! &&
 	test_line_count = $lines output &&
 	tail -n 1 <output >actual &&
@@ -84,12 +85,13 @@ test_expect_success PIPE '0-length read, no input available' '
 	rm -f input &&
 	mkfifo input &&
 	{
-		sleep 100 >input &
+		sleep 15 >input &
 	} &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 0
 	copy 0
 	EOF
+	pstree -p $! &&
 	kill $! &&
 	test_cmp expect actual
 '
@@ -112,13 +114,14 @@ test_expect_success PIPE '1-byte read, no input available' '
 		{
 			printf "%s" a &&
 			printf "%s" b &&
-			sleep 100
+			sleep 15
 		} >input &
 	} &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 1
 	copy 1
 	EOF
+	pstree -p $! &&
 	kill $! &&
 	test_cmp expect actual
 '
-- 

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

Jeff King wrote:

> No, you misunderstand. It is prove itself that ignores the SIGCHLD. It
> is stuck in the loop in TAP::Parser::Iterator::Process::_next. It has
> gotten SIGCHLD, but it keeps blocking waiting to get EOF on the child's
> stdout.

Thanks for clarifying.

> Doesn't that point to an unreaped process? After 100 seconds the sleep
> process closes, prove gets EOF, and it completes. Lowering the "100" to
> "1" caused a 1-second hang for me.

I guess it's just a matter of terminology.  To me, it points to an
undelivered signal, since the problem is not a zombie waiting to be
wait-ed on but a process still alive and sleeping.

> But instead of realizing its child has died, it
> insists on waiting until the pipe is closed. Nothing has to be adopted
> by init. There are simply still processes with the pipe open.

I meant that the sleep process is not a descendant of prove any
more.

| $ ps ax | egrep 'sleep|prove'
| 20203 pts/3    T      0:00 vim utils/prove
| 22654 pts/1    S+     0:00 /usr/bin/perl /usr/bin/prove --exec=bash
| t0081-line-buffer.sh
| 22688 pts/1    S+     0:00 sleep 100
| 22694 pts/1    S+     0:00 sleep 100
| 22727 pts/3    S+     0:00 egrep sleep|prove
| $ pstree -p 22654
| prove(22654)───bash(22655)

> Did you try my 5>/dev/null patch? With it, I get no hang at all.

Now I've tested it and it indeed works as advertised.

Sorry for the lack of clarity.

Re: [PATCH] Portability: returning void

[Jeff King]

On Tue, Mar 29, 2011 at 07:29:21PM -0500, Jonathan Nieder wrote:

> > Did you try my 5>/dev/null patch? With it, I get no hang at all.
> 
> Haven't tried it yet but will try.
> 
> I really don't like that as a long-term solution.  Yes, it gets prove
> to stop hanging, but meanwhile we have no control over the child
> processes we have spawned.  I'd rather just drop the tests.

Agreed, it's a horrible fix. I just wanted to make sure it fixed it for
you, too. After thinking on it more, I figured out an elegant fix.
Patch is below.

-- >8 --
Subject: [PATCH] t0081: kill backgrounded processes more robustly

This test creates several background processes that write to
a fifo and then go to sleep for a while (so the reader of
the fifo does not see EOF).

Each background process is made in a curly-braced block in
the shell, and after we are done reading from the fifo, we
use "kill $!" to kill it off.

For a simple, single-command process, this works reliably
and kills the child sleep process. But for more complex
commands like "make_some_output && sleep", the results are
less predictable. When executing under bash, we end up with
a subshell that gets killed by the $! but leaves the sleep
process still alive.

This is bad not only for process hygeine (we are leaving
random sleep processes to expire after a while), but also
interacts badly with the "prove" command. When prove
executes a test, it does not realize the test is done when
it sees SIGCHLD, but rather waits until the test's stdout
pipe is closed. The orphaned sleep process may keep that
pipe open via test-lib's file descriptor 5, causing prove to
hang for 100 seconds.

The solution is to explicitly use a subshell and to exec the
final sleep process, so that when we "kill $!" we get the
process id of the sleep process.

While we're at it, let's make the "kill" process a little
more robust. Specifically:

  1. Wrap the "kill" in a test_when_finished, since we want
     to clean up the process whether the test succeeds or not.

  2. The "kill" is part of our && chain for test success. It
     probably won't fail, but it can if the process has
     expired before we manage to kill it. So let's mark it
     as OK to fail.

Signed-off-by: Jeff King <peff@peff.net>
---
Actually, my (2) above is unlikely to trigger, since the test would have
to hang for 100 seconds first, which probably means it is failing
anyway. But I did run across it when I screwed up my fix.

Also, is it just me, or does it seem weird that test_when_finished
blocks failing can produce test failure? I would think you would be able
to put any old cleanup crap into them and not care whether it worked or
not.

 t/t0081-line-buffer.sh |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 1dbe1c9..416ccc1 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -47,16 +47,16 @@ long_read_test () {
 	rm -f input &&
 	mkfifo input &&
 	{
-		{
+		(
 			generate_tens_of_lines $tens_of_lines "$line" &&
-			sleep 100
-		} >input &
+			exec sleep 100
+		) >input &
 	} &&
+	test_when_finished "kill $! || true" &&
 	test-line-buffer input <<-EOF >output &&
 	binary $readsize
 	copy $copysize
 	EOF
-	kill $! &&
 	test_line_count = $lines output &&
 	tail -n 1 <output >actual &&
 	test_cmp expect actual
@@ -86,11 +86,11 @@ test_expect_success PIPE '0-length read, no input available' '
 	{
 		sleep 100 >input &
 	} &&
+	test_when_finished "kill $! || true" &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 0
 	copy 0
 	EOF
-	kill $! &&
 	test_cmp expect actual
 '
 
@@ -109,17 +109,17 @@ test_expect_success PIPE '1-byte read, no input available' '
 	rm -f input &&
 	mkfifo input &&
 	{
-		{
+		(
 			printf "%s" a &&
 			printf "%s" b &&
-			sleep 100
-		} >input &
+			exec sleep 100
+		) >input &
 	} &&
+	test_when_finished "kill $! || true" &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 1
 	copy 1
 	EOF
-	kill $! &&
 	test_cmp expect actual
 '
 
-- 
1.7.4.2.7.g9407

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

Jeff King wrote:

> While we're at it, let's make the "kill" process a little
> more robust. Specifically:
>
>   1. Wrap the "kill" in a test_when_finished, since we want
>      to clean up the process whether the test succeeds or not.
>
>   2. The "kill" is part of our && chain for test success. It
>      probably won't fail, but it can if the process has
>      expired before we manage to kill it. So let's mark it
>      as OK to fail.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, my (2) above is unlikely to trigger, since the test would have
> to hang for 100 seconds first, which probably means it is failing
> anyway. But I did run across it when I screwed up my fix.

That is actually how the tests distinguish between success and
failure.  Any idea about a better way?

I was in the process of writing a commit message for the same "exec"
trick, but I'm glad you got to it first. ;-)

> Also, is it just me, or does it seem weird that test_when_finished
> blocks failing can produce test failure? I would think you would be able
> to put any old cleanup crap into them and not care whether it worked or
> not.

I'm generally happy that it catches mistakes in cleanup code, but I
could easily be convinced to change it anyway.

> --- a/t/t0081-line-buffer.sh
> +++ b/t/t0081-line-buffer.sh
> @@ -47,16 +47,16 @@ long_read_test () {
[...]
> +		) >input &
>  	} &&
> +	test_when_finished "kill $! || true" &&

The $! gets expanded when this is executed, so later background
processes won't interfere.  Good.

Maybe it would be good to factor out a helper to make future
improvements a little easier, like so:

-- 8< --
Subject: t0081: introduce helper to fill a pipe in the background

The fill_input function generates a fifo and runs a command to write
to it and wait a while.  The intended use is to test programs that
need to be able to cope with input of limited length without relying
on EOF or SIGPIPE to detect its end.

For example:

	fill_input "echo hi" &&
	head -1 input

will succeed, while

	fill_input "echo hi" &&
	head -2 input

will hang waiting for more input.

It works by running the indicated commands followed by
"exec sleep 100" in a background process and registering a clean-up
command to kill it and check if it was killed by SIGPIPE (good) or
ran to completion (bad).  This patch adds a definition for this
function and updates existing tests that used that trick to use it.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The commit message assumes test_when_finished "kill $!", while the
patch uses "kill $! || :"; one of the two would need to be fixed
before applying this.

 t/t0081-line-buffer.sh |   51 +++++++++++++++++++----------------------------
 1 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 416ccc1..218da98 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -14,6 +14,21 @@ correctly.
 
 test -n "$GIT_REMOTE_SVN_TEST_BIG_FILES" && test_set_prereq EXPENSIVE
 
+fill_input () {
+	if ! test_declared_prereq PIPE
+	then
+		echo >&4 "fill_input: need to declare PIPE prerequisite"
+		return 127
+	fi &&
+	rm -f input &&
+	mkfifo input &&
+	(
+		eval "$*" &&
+		exec sleep 100
+	) >input &
+	test_when_finished "kill $! || true"
+}
+
 generate_tens_of_lines () {
 	tens=$1 &&
 	line=$2 &&
@@ -35,24 +50,11 @@ long_read_test () {
 	line=abcdefghi &&
 	echo "$line" >expect &&
 
-	if ! test_declared_prereq PIPE
-	then
-		echo >&4 "long_read_test: need to declare PIPE prerequisite"
-		return 127
-	fi &&
 	tens_of_lines=$(($1 / 100 + 1)) &&
 	lines=$(($tens_of_lines * 10)) &&
 	readsize=$((($lines - 1) * 10 + 3)) &&
 	copysize=7 &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		(
-			generate_tens_of_lines $tens_of_lines "$line" &&
-			exec sleep 100
-		) >input &
-	} &&
-	test_when_finished "kill $! || true" &&
+	fill_input "generate_tens_of_lines $tens_of_lines $line" &&
 	test-line-buffer input <<-EOF >output &&
 	binary $readsize
 	copy $copysize
@@ -81,12 +83,7 @@ test_expect_success 'hello world' '
 
 test_expect_success PIPE '0-length read, no input available' '
 	printf ">" >expect &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		sleep 100 >input &
-	} &&
-	test_when_finished "kill $! || true" &&
+	fill_input : &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 0
 	copy 0
@@ -106,16 +103,10 @@ test_expect_success '0-length read, send along greeting' '
 
 test_expect_success PIPE '1-byte read, no input available' '
 	printf ">%s" ab >expect &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		(
-			printf "%s" a &&
-			printf "%s" b &&
-			exec sleep 100
-		) >input &
-	} &&
-	test_when_finished "kill $! || true" &&
+	fill_input "
+		printf a &&
+		printf b
+	" &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 1
 	copy 1
-- 
1.7.4.2

Re: [PATCH] Portability: returning void

[Jeff King]

On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > While we're at it, let's make the "kill" process a little
> > more robust. Specifically:
> >
> >   1. Wrap the "kill" in a test_when_finished, since we want
> >      to clean up the process whether the test succeeds or not.
> >
> >   2. The "kill" is part of our && chain for test success. It
> >      probably won't fail, but it can if the process has
> >      expired before we manage to kill it. So let's mark it
> >      as OK to fail.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Actually, my (2) above is unlikely to trigger, since the test would have
> > to hang for 100 seconds first, which probably means it is failing
> > anyway. But I did run across it when I screwed up my fix.
> 
> That is actually how the tests distinguish between success and
> failure.  Any idea about a better way?

Ah. I was trying not to look too hard at how the tests actually work, so
I completely missed that. Yes, if the "kill" is part of the actual test,
then it should stay in the test. We can also put in a test_when_finished
to kill it should the test fail to make it that far. If the cleanup one
does an extra kill, it shouldn't be a big deal.

> I was in the process of writing a commit message for the same "exec"
> trick, but I'm glad you got to it first. ;-)

I don't know why I didn't think of it sooner. :)

> > Also, is it just me, or does it seem weird that test_when_finished
> > blocks failing can produce test failure? I would think you would be able
> > to put any old cleanup crap into them and not care whether it worked or
> > not.
> 
> I'm generally happy that it catches mistakes in cleanup code, but I
> could easily be convinced to change it anyway.

I don't think it's a big deal. It just surprised me.

> Maybe it would be good to factor out a helper to make future
> improvements a little easier, like so:
> 
> -- 8< --
> Subject: t0081: introduce helper to fill a pipe in the background
> 
> The fill_input function generates a fifo and runs a command to write
> to it and wait a while.  The intended use is to test programs that
> need to be able to cope with input of limited length without relying
> on EOF or SIGPIPE to detect its end.

Yeah, that looks much nicer. Do you want to just re-roll that on top of
what's in 'master', with the "exec" magic and the defensive
test_when_finished in it (or as a separate patch on top if you want to
split the refactor and fix)? Feel free to borrow from my commit message.

-Peff

[PULL svn-fe] Re: Portability: returning void

[Jonathan Nieder]

Jeff King wrote:

> After thinking on it more, I figured out an elegant fix.

All right, thanks again for this.

I really do want to address your "while at it".  I'm pretty
uncomfortable with the "sleep 100" --- it might be better to do "exec
perl -e sleep" so there is no chance that some incredibly slow system
won't make the race.  Or a failure mode that does not involve a long
hang would be even better.

Anyway, to make others' life better quickly I've pushed out the easy
part of your fix.  It sits with the fixes to other embarrasing bugs.

  git://repo.or.cz/git/jrn.git svn-fe

Jeff King (1):
      tests: kill backgrounded processes more robustly

Jonathan Nieder (2):
      vcs-svn: add missing cast to printf argument
      tests: make sure input to sed is newline terminated

Michael Witten (1):
      vcs-svn: a void function shouldn't try to return something

 t/t0081-line-buffer.sh |   12 ++++++------
 t/t9010-svn-fe.sh      |    8 ++++++--
 vcs-svn/fast_export.c  |    3 ++-
 vcs-svn/svndump.c      |    3 ++-
 4 files changed, 16 insertions(+), 10 deletions(-)

Re: [PATCH] Portability: returning void

[Johannes Sixt]

Am 3/30/2011 6:13, schrieb Jeff King:
> On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:
>> I was in the process of writing a commit message for the same "exec"
>> trick, but I'm glad you got to it first. ;-)
> 
> I don't know why I didn't think of it sooner. :)

Note that tests that depend on ( exec ... ) & kill $! must be marked with
the EXECKEEPSPID prerequisite.

-- Hannes

[PATCH/RFC svn-fe] tests: introduce helper to fill a pipe in the background

[Jonathan Nieder]

The fill_input function generates a fifo and runs a command to write
to it and wait.  The intended use is to check that specialized
programs can find the end of their input without reading too much or
relying on EOF or SIGPIPE.  For example:

	fill_input "echo hi" &&
	head -1 input

will succeed, while

	fill_input "echo hi" &&
	head -2 input

will hang.

It works by running the indicated commands followed by
"exec sleep 100" in a background process; the process ID is later
passed to "kill" to avoid leaving behind random processes.

Several tests already did something like that but this adds some
improvements:

  1. Wrap the "kill" in a test_when_finished, since we want
     to clean up the process whether the test succeeds or not.

  2. Mark the kill as OK to fail.  These tests are not about
     whether the input generating function dies due to SIGPIPE
     or the system is slow enough for the timer to expire early
     but about what happens on the downstream end.

  3. Mark the relevant tests with the EXECKEEPSPID prerequisite.

Based-on-patch-by: Jeff King <peff@peff.net>
Improved-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Johannes Sixt wrote:

> Note that tests that depend on ( exec ... ) & kill $! must be marked with
> the EXECKEEPSPID prerequisite.

Hmm, that's a shame, since the point of writing t0081 was to make sure
some assumptions the line-buffer lib makes are valid on Windows.  So I
suspect a better thing to do would be to remove t0081 --- the
line-buffer lib is simple, everyday use exercises it pretty well
already, and I can't imagine this script catching a bug.

 t/t0081-line-buffer.sh |   61 ++++++++++++++++++++++--------------------------
 1 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 5067d1e..fb09ff1 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -14,6 +14,25 @@ correctly.
 
 test -n "$GIT_REMOTE_SVN_TEST_BIG_FILES" && test_set_prereq EXPENSIVE
 
+fill_input () {
+	if
+		! test_declared_prereq PIPE ||
+		! test_declared_prereq EXECKEEPSPID
+	then
+		echo >&4 "fill_input requires PIPE,EXECKEEPSPID prerequisites"
+		return 127
+	fi &&
+	rm -f input &&
+	mkfifo input &&
+	{
+		(
+			eval "$*" &&
+			exec sleep 100
+		) >input &
+	} &&
+	test_when_finished "kill $!"
+}
+
 generate_tens_of_lines () {
 	tens=$1 &&
 	line=$2 &&
@@ -35,28 +54,15 @@ long_read_test () {
 	line=abcdefghi &&
 	echo "$line" >expect &&
 
-	if ! test_declared_prereq PIPE
-	then
-		echo >&4 "long_read_test: need to declare PIPE prerequisite"
-		return 127
-	fi &&
 	tens_of_lines=$(($1 / 100 + 1)) &&
 	lines=$(($tens_of_lines * 10)) &&
 	readsize=$((($lines - 1) * 10 + 3)) &&
 	copysize=7 &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		(
-			generate_tens_of_lines $tens_of_lines "$line" &&
-			exec sleep 100
-		) >input &
-	} &&
+	fill_input "generate_tens_of_lines $tens_of_lines $line" &&
 	test-line-buffer input <<-EOF >output &&
 	binary $readsize
 	copy $copysize
 	EOF
-	kill $! &&
 	test_line_count = $lines output &&
 	tail -n 1 <output >actual &&
 	test_cmp expect actual
@@ -79,18 +85,13 @@ test_expect_success 'hello world' '
 	test_cmp expect actual
 '
 
-test_expect_success PIPE '0-length read, no input available' '
+test_expect_success PIPE,EXECKEEPSPID '0-length read, no input available' '
 	printf ">" >expect &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		sleep 100 >input &
-	} &&
+	fill_input : &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 0
 	copy 0
 	EOF
-	kill $! &&
 	test_cmp expect actual
 '
 
@@ -104,26 +105,20 @@ test_expect_success '0-length read, send along greeting' '
 	test_cmp expect actual
 '
 
-test_expect_success PIPE '1-byte read, no input available' '
+test_expect_success PIPE,EXECKEEPSPID '1-byte read, no input available' '
 	printf ">%s" ab >expect &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		(
-			printf "%s" a &&
-			printf "%s" b &&
-			exec sleep 100
-		) >input &
-	} &&
+	fill_input "
+		printf a &&
+		printf b
+	" &&
 	test-line-buffer input <<-\EOF >actual &&
 	binary 1
 	copy 1
 	EOF
-	kill $! &&
 	test_cmp expect actual
 '
 
-test_expect_success PIPE 'long read (around 8192 bytes)' '
+test_expect_success PIPE,EXECKEEPSPID 'long read (around 8192 bytes)' '
 	long_read_test 8192
 '
 
-- 
1.7.4.2

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

Jeff King wrote:

> Ah. I was trying not to look too hard at how the tests actually work, so
> I completely missed that. Yes, if the "kill" is part of the actual test,
> then it should stay in the test.

Heh, what was the person writing that test thinking?  Allowing the
kill to fail would be better --- some future test might have the
generate_lots_of_text part dying of SIGPIPE, which would make later
attempts to kill it fail.  Totally cryptic.

What are these tests make to check, anyway?

 - "0-length read, no input available" makes some sense.  It's checking
   that the platform fread can handle requests to read nothing (or that
   line-buffer works around it if some esoteric platform cannot).

 - "1-byte read, no input available" makes less sense.  I just can't
   see any reason for it to fail.  And it "tests" two functions at
   once, for crazy historical reasons.

 - "long read" checks that we can read something around the pipe size
   and not get totally confused.  I can imagine it failing but it
   wouldn't be git's fault.  It also doesn't seem like a useful test.

I don't think any of them is worth the fuss.  So how about this?

-- 8< --
Subject: Revert "t0081 (line-buffer): add buffering tests"

This (morally) reverts commit d280f68313eecb8b3838c70641a246382d5e5343,
which added some tests that are a pain to maintain and are not likely
to find bugs in git.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0081-line-buffer.sh |  106 +-----------------------------------------------
 1 files changed, 2 insertions(+), 104 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 5067d1e..bd83ed3 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -2,74 +2,14 @@
 
 test_description="Test the svn importer's input handling routines.
 
-These tests exercise the line_buffer library, but their real purpose
-is to check the assumptions that library makes of the platform's input
-routines.  Processes engaged in bi-directional communication would
-hang if fread or fgets is too greedy.
+These tests provide some simple checks that the line_buffer API
+behaves as advertised.
 
 While at it, check that input of newlines and null bytes are handled
 correctly.
 "
 . ./test-lib.sh
 
-test -n "$GIT_REMOTE_SVN_TEST_BIG_FILES" && test_set_prereq EXPENSIVE
-
-generate_tens_of_lines () {
-	tens=$1 &&
-	line=$2 &&
-
-	i=0 &&
-	while test $i -lt "$tens"
-	do
-		for j in a b c d e f g h i j
-		do
-			echo "$line"
-		done &&
-		: $((i = $i + 1)) ||
-		return
-	done
-}
-
-long_read_test () {
-	: each line is 10 bytes, including newline &&
-	line=abcdefghi &&
-	echo "$line" >expect &&
-
-	if ! test_declared_prereq PIPE
-	then
-		echo >&4 "long_read_test: need to declare PIPE prerequisite"
-		return 127
-	fi &&
-	tens_of_lines=$(($1 / 100 + 1)) &&
-	lines=$(($tens_of_lines * 10)) &&
-	readsize=$((($lines - 1) * 10 + 3)) &&
-	copysize=7 &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		(
-			generate_tens_of_lines $tens_of_lines "$line" &&
-			exec sleep 100
-		) >input &
-	} &&
-	test-line-buffer input <<-EOF >output &&
-	binary $readsize
-	copy $copysize
-	EOF
-	kill $! &&
-	test_line_count = $lines output &&
-	tail -n 1 <output >actual &&
-	test_cmp expect actual
-}
-
-test_expect_success 'setup: have pipes?' '
-      rm -f frob &&
-      if mkfifo frob
-      then
-		test_set_prereq PIPE
-      fi
-'
-
 test_expect_success 'hello world' '
 	echo ">HELLO" >expect &&
 	test-line-buffer <<-\EOF >actual &&
@@ -79,21 +19,6 @@ test_expect_success 'hello world' '
 	test_cmp expect actual
 '
 
-test_expect_success PIPE '0-length read, no input available' '
-	printf ">" >expect &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		sleep 100 >input &
-	} &&
-	test-line-buffer input <<-\EOF >actual &&
-	binary 0
-	copy 0
-	EOF
-	kill $! &&
-	test_cmp expect actual
-'
-
 test_expect_success '0-length read, send along greeting' '
 	echo ">HELLO" >expect &&
 	test-line-buffer <<-\EOF >actual &&
@@ -104,33 +29,6 @@ test_expect_success '0-length read, send along greeting' '
 	test_cmp expect actual
 '
 
-test_expect_success PIPE '1-byte read, no input available' '
-	printf ">%s" ab >expect &&
-	rm -f input &&
-	mkfifo input &&
-	{
-		(
-			printf "%s" a &&
-			printf "%s" b &&
-			exec sleep 100
-		) >input &
-	} &&
-	test-line-buffer input <<-\EOF >actual &&
-	binary 1
-	copy 1
-	EOF
-	kill $! &&
-	test_cmp expect actual
-'
-
-test_expect_success PIPE 'long read (around 8192 bytes)' '
-	long_read_test 8192
-'
-
-test_expect_success PIPE,EXPENSIVE 'longer read (around 65536 bytes)' '
-	long_read_test 65536
-'
-
 test_expect_success 'read from file descriptor' '
 	rm -f input &&
 	echo hello >expect &&
-- 
1.7.4.2.660.g270d4b.dirty

Re: [PATCH] Portability: returning void

[Jeff King]

On Wed, Mar 30, 2011 at 03:41:48AM -0500, Jonathan Nieder wrote:

> I don't think any of them is worth the fuss.  So how about this?
> 
> -- 8< --
> Subject: Revert "t0081 (line-buffer): add buffering tests"

That's OK with me. Should it drop test-line-buffer, too?

-Peff

Re: [PATCH] Portability: returning void

[Jonathan Nieder]

Jeff King wrote:

> That's OK with me. Should it drop test-line-buffer, too?

For now, test-line-buffer is still being used to check the API wrt
early EOF.

In the long term, yes, I would like to replace those tests with tests
of svn-fe itself.  The test suite should not be an obstacle to
changing the line-buffer API as long as svn-fe is adjusted correctly
at the same time.

Thanks for looking it over.

Re: [PULL svn-fe] Re: Portability: returning void

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I really do want to address your "while at it".  I'm pretty
> uncomfortable with the "sleep 100" --- it might be better to do "exec
> perl -e sleep" so there is no chance that some incredibly slow system
> won't make the race.  Or a failure mode that does not involve a long
> hang would be even better.
>
> Anyway, to make others' life better quickly I've pushed out the easy
> part of your fix.  It sits with the fixes to other embarrasing bugs.
>
>   git://repo.or.cz/git/jrn.git svn-fe

Pulled, thanks.