Re: [PATCH] Portability: returning void

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 30, 2011, 03:57 UTC
Message-ID: <20110330035733.GA2793@elie>
In-Reply-To: <20110330033017.GA18157@sigill.intra.peff.net>

Jeff King wrote:

Show 16 quoted lines

> 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