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

[Clemens Buchacher <clemens.buchacher@intel.com>]

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