[PATCH] allow hooks to ignore their standard input stream

[PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input
stream) the pre-receive and post-receive hooks ignore SIGPIPE. Do the
same for the remaining hooks pre-push and post-rewrite, which read from
standard input. The same arguments for ignoring SIGPIPE apply.

Signed-off-by: Clemens Buchacher <clemens.buchacher@intel.com>
---
 builtin/commit.c |  3 +++
 transport.c      | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 		return code;
 	n = snprintf(buf, sizeof(buf), "%s %s\n",
 		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(proc.in, buf, n);
 	close(proc.in);
+	sigchain_pop(SIGPIPE);
 	return finish_command(&proc);
 }
 
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
 		return -1;
 	}
 
+	sigchain_push(SIGPIPE, SIG_IGN);
+
 	strbuf_init(&buf, 256);
 
 	for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 			 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 			 r->name, sha1_to_hex(r->old_sha1));
 
-		if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-			ret = -1;
+		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+			/* We do not mind if a hook does not read all refs. */
+			if (errno != EPIPE)
+				ret = -1;
 			break;
 		}
 	}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
 	if (!ret)
 		ret = x;
 
+	sigchain_pop(SIGPIPE);
+
 	x = finish_command(&proc);
 	if (!ret)
 		ret = x;
-- 
1.9.4

Re: [PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

On Wed, Nov 11, 2015 at 03:39:20PM +0100, Clemens Buchacher wrote:
> +		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
> +			/* We do not mind if a hook does not read all refs. */
> +			if (errno != EPIPE)
> +				ret = -1;

I can reproduce the pipe error reliably with the test below. I did not
include it in the patch since I am in doubt if we should add an optional
sleep to the code.

-->o--
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..8cfe59a 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,13 @@ test_expect_success 'push to URL' '
        diff expected actual
 '

-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-       printf 'parent1\nrepo1\n' >expected
-       nr=1000
-       while test $nr -lt 2000
-       do
-               nr=$(( $nr + 1 ))
-               git branch b/$nr $COMMIT3
-               echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" >>expected
-       done
-
-       test_expect_success 'push many refs' '
-               git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-               diff expected actual
-       '
-fi
+write_script "$HOOK" <<\EOF
+exit 0
+EOF
+
+test_expect_success 'hook does not consume input' '
+    git branch noinput &&
+    GIT_TEST_SIGPIPE=t git push parent1 noinput
+'

 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..d83ef1c 100644
--- a/transport.c
+++ b/transport.c
@@ -1129,6 +1129,8 @@ static int run_pre_push_hook(struct transport *transport,

        strbuf_init(&buf, 256);

+       if (getenv("GIT_TEST_SIGPIPE"))
+               sleep_millisec(10);
        for (r = remote_refs; r; r = r->next) {
                if (!r->peer_ref) continue;
                if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;

Re: [PATCH] allow hooks to ignore their standard input stream

[Jeff King]

On Wed, Nov 11, 2015 at 03:42:22PM +0100, Clemens Buchacher wrote:

> On Wed, Nov 11, 2015 at 03:39:20PM +0100, Clemens Buchacher wrote:
> > +		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
> > +			/* We do not mind if a hook does not read all refs. */
> > +			if (errno != EPIPE)
> > +				ret = -1;
> 
> I can reproduce the pipe error reliably with the test below. I did not
> include it in the patch since I am in doubt if we should add an optional
> sleep to the code.

Yeah, we definitely do not want to do so. The reliable way to get a
SIGPIPE is:

  1. Have one process write more than a pipe buffer's worth of data.

  2. Have the other process exit without reading anything.

Then even if the writer process goes first, it will block, and
eventually get EPIPE when the other side closes the descriptor.

Unfortunately, the pipe buffer size varies from system to system. It's
128K by default on modern Linux platforms. Using that is probably
enough. The worst case is that the system has a larger buffer, and the
test will succeed even without the fix. But that is OK as long as
somebody somewhere is running the test with a smaller buffer and would
catch a regression.

Of course, convincing git to generate 128K worth of hook input is often
quite inefficient. The existing test in t5571 is quite painful because
it calls "git branch" in a loop. We can do much better than that.
The test below reliably fails without your patch and passes with it, and
seems to run reasonably quickly for me:

diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..61df2f9 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,28 @@ test_expect_success 'push to URL' '
 	diff expected actual
 '
 
-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-	printf 'parent1\nrepo1\n' >expected
-	nr=1000
-	while test $nr -lt 2000
-	do
-		nr=$(( $nr + 1 ))
-		git branch b/$nr $COMMIT3
-		echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" >>expected
-	done
-
-	test_expect_success 'push many refs' '
-		git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-		diff expected actual
-	'
-fi
+test_expect_success 'set up many-ref tests' '
+	{
+		echo >&3 parent1 &&
+		echo >&3 repo1 &&
+		nr=1000
+		while test $nr -lt 2000
+		do
+			nr=$(( $nr + 1 ))
+			echo "create refs/heads/b/$nr $COMMIT3"
+			echo >&3 "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40"
+		done
+	} 3>expected | git update-ref --stdin
+'
+
+test_expect_success 'filling pipe buffer does not cause failure' '
+	git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
+	test_cmp expected actual
+'
+
+test_expect_success 'sigpipe does not cause pre-push hook failure' '
+	echo "exit 0" | write_script "$HOOK" &&
+	git push parent1 "refs/heads/b/*:refs/heads/c/*"
+'
 
 test_done

Re: [PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

On Fri, Nov 13, 2015 at 01:17:29AM -0500, Jeff King wrote:
> 
> The test below reliably fails without your patch and passes with it, and
> seems to run reasonably quickly for me:

Thank you. I confirm the same behavior on my system. Below I have added
your change to the patch.

-->o--
Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input stream)
the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
remaining hooks pre-push and post-rewrite, which read from standard input. The
same arguments for ignoring SIGPIPE apply.

Performance improvements which allow us to enable the test by
default by Jeff King.

Signed-off-by: Clemens Buchacher <clemens.buchacher@intel.com>
---
 builtin/commit.c         |  3 +++
 t/t5571-pre-push-hook.sh | 41 +++++++++++++++++++++++------------------
 transport.c              | 11 +++++++++--
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 		return code;
 	n = snprintf(buf, sizeof(buf), "%s %s\n",
 		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(proc.in, buf, n);
 	close(proc.in);
+	sigchain_pop(SIGPIPE);
 	return finish_command(&proc);
 }
 
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..61df2f9 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,28 @@ test_expect_success 'push to URL' '
 	diff expected actual
 '
 
-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-	printf 'parent1\nrepo1\n' >expected
-	nr=1000
-	while test $nr -lt 2000
-	do
-		nr=$(( $nr + 1 ))
-		git branch b/$nr $COMMIT3
-		echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" >>expected
-	done
-
-	test_expect_success 'push many refs' '
-		git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-		diff expected actual
-	'
-fi
+test_expect_success 'set up many-ref tests' '
+	{
+		echo >&3 parent1 &&
+		echo >&3 repo1 &&
+		nr=1000
+		while test $nr -lt 2000
+		do
+			nr=$(( $nr + 1 ))
+			echo "create refs/heads/b/$nr $COMMIT3"
+			echo >&3 "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40"
+		done
+	} 3>expected | git update-ref --stdin
+'
+
+test_expect_success 'filling pipe buffer does not cause failure' '
+	git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
+	test_cmp expected actual
+'
+
+test_expect_success 'sigpipe does not cause pre-push hook failure' '
+	echo "exit 0" | write_script "$HOOK" &&
+	git push parent1 "refs/heads/b/*:refs/heads/c/*"
+'
 
 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
 		return -1;
 	}
 
+	sigchain_push(SIGPIPE, SIG_IGN);
+
 	strbuf_init(&buf, 256);
 
 	for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 			 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 			 r->name, sha1_to_hex(r->old_sha1));
 
-		if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-			ret = -1;
+		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+			/* We do not mind if a hook does not read all refs. */
+			if (errno != EPIPE)
+				ret = -1;
 			break;
 		}
 	}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
 	if (!ret)
 		ret = x;
 
+	sigchain_pop(SIGPIPE);
+
 	x = finish_command(&proc);
 	if (!ret)
 		ret = x;
-- 
1.9.4

Re: [PATCH] allow hooks to ignore their standard input stream

[Jeff King]

On Fri, Nov 13, 2015 at 10:33:03AM +0100, Clemens Buchacher wrote:

> Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input stream)
> the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
> remaining hooks pre-push and post-rewrite, which read from standard input. The
> same arguments for ignoring SIGPIPE apply.
> 
> Performance improvements which allow us to enable the test by
> default by Jeff King.

I actually did add a new test. The existing one was basically this:

> +test_expect_success 'filling pipe buffer does not cause failure' '
> +	git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
> +	test_cmp expected actual
> +'

It actually _does_ read all of the input, but I guess is making sure we
call write() in a loop. I don't know if this is even worth keeping.

Can you think of a good reason that it is checking something
interesting?

> +test_expect_success 'sigpipe does not cause pre-push hook failure' '
> +	echo "exit 0" | write_script "$HOOK" &&
> +	git push parent1 "refs/heads/b/*:refs/heads/c/*"
> +'

This is the new one which checks your code.

-Peff

Re: [PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

Hi Junio,

I believe we have finalized the discussion on this patch. Please apply

On Fri, Nov 13, 2015 at 06:23:20PM -0500, Jeff King wrote:
> 
> > +test_expect_success 'filling pipe buffer does not cause failure' '
> > +	git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
> > +	test_cmp expected actual
> > +'
> 
> It actually _does_ read all of the input, but I guess is making sure we
> call write() in a loop. I don't know if this is even worth keeping.
> 
> Can you think of a good reason that it is checking something
> interesting?

No, I also cannot think of a good reason to keep it. Here is the patch
with the test above removed.

Best regards,
Clemens

--o<--
Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input stream)
the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
remaining hooks pre-push and post-rewrite, which read from standard input. The
same arguments for ignoring SIGPIPE apply.

Include test by Jeff King which checks that SIGPIPE does not cause
pre-push hook failure. With the use of git update-ref --stdin it is fast
enough to be enabled by default.

Signed-off-by: Clemens Buchacher <clemens.buchacher@intel.com>
---
 builtin/commit.c         |  3 +++
 t/t5571-pre-push-hook.sh | 33 +++++++++++++++------------------
 transport.c              | 11 +++++++++--
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 		return code;
 	n = snprintf(buf, sizeof(buf), "%s %s\n",
 		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(proc.in, buf, n);
 	close(proc.in);
+	sigchain_pop(SIGPIPE);
 	return finish_command(&proc);
 }
 
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..ba975bb 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,20 @@ test_expect_success 'push to URL' '
 	diff expected actual
 '
 
-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-	printf 'parent1\nrepo1\n' >expected
-	nr=1000
-	while test $nr -lt 2000
-	do
-		nr=$(( $nr + 1 ))
-		git branch b/$nr $COMMIT3
-		echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" >>expected
-	done
-
-	test_expect_success 'push many refs' '
-		git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-		diff expected actual
-	'
-fi
+test_expect_success 'set up many-ref tests' '
+	{
+		nr=1000
+		while test $nr -lt 2000
+		do
+			nr=$(( $nr + 1 ))
+			echo "create refs/heads/b/$nr $COMMIT3"
+		done
+	} | git update-ref --stdin
+'
+
+test_expect_success 'sigpipe does not cause pre-push hook failure' '
+	echo "exit 0" | write_script "$HOOK" &&
+	git push parent1 "refs/heads/b/*:refs/heads/b/*"
+'
 
 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
 		return -1;
 	}
 
+	sigchain_push(SIGPIPE, SIG_IGN);
+
 	strbuf_init(&buf, 256);
 
 	for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 			 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 			 r->name, sha1_to_hex(r->old_sha1));
 
-		if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-			ret = -1;
+		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+			/* We do not mind if a hook does not read all refs. */
+			if (errno != EPIPE)
+				ret = -1;
 			break;
 		}
 	}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
 	if (!ret)
 		ret = x;
 
+	sigchain_pop(SIGPIPE);
+
 	x = finish_command(&proc);
 	if (!ret)
 		ret = x;
-- 
1.9.4

Re: [PATCH] allow hooks to ignore their standard input stream

[Jeff King]

On Mon, Nov 16, 2015 at 09:05:58AM +0100, Clemens Buchacher wrote:

> Hi Junio,
> 
> I believe we have finalized the discussion on this patch. Please apply

Junio is out for a bit, and I'm acting as maintainer. I've picked up
your patch but haven't pushed anything out yet.

> No, I also cannot think of a good reason to keep it. Here is the patch
> with the test above removed.

Thanks, this looks good to me.

-Peff

Re: [PATCH] allow hooks to ignore their standard input stream

[Jeff King]

On Wed, Nov 11, 2015 at 03:39:20PM +0100, Clemens Buchacher wrote:

> Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input
> stream) the pre-receive and post-receive hooks ignore SIGPIPE. Do the
> same for the remaining hooks pre-push and post-rewrite, which read from
> standard input. The same arguments for ignoring SIGPIPE apply.

The reasoning and the patch itself look good to me. I posted some
thoughts on tests in response to your other message.

-Peff