[PATCH] Fix race and deadlock when sending pack

[PATCH] Fix race and deadlock when sending pack

[Paul Serice]

Fix race and deadlock when sending pack.

The best way to reproduce the problem is to locally clone your
repository.  When you perform a push, git-send-pack will directly set
up pipes connected to stdin and stdout of git-receive-pack.  You
should then set up hook/post-update or hook/update to try to write
lots of text to stdout.  (You want to use the local protocol because
ssh is robust enough to mask the worst behavior.)

The first problem is that git-send-pack closes git-receive-pack's
stdout (which is inherited by the hooks) immediately after sending the
pack.  This almost always causes the hooks to receive SIGPIPE when
they try to write to stdout.

After fixing the SIGPIPE problem, you then run into a deadlock because
git-send-pack is blocked trying to reap git-receive-pack and
git-receive-pack (or one of its hooks) is blocked waiting for
git-send-pack to read its output.

I've also added an example a one-liner to both hooks demonstrating how
to redirect all subsequent output to stderr.  Because
git-receive-pack's stderr is not redirected, it has always been safe
to write to stderr.  Thus, all current status related output appears
on stderr.  This can lead to confusing ordering of messages if only
the hooks are using stdout.  The patch has the one-liner commented
out, but perhaps it should be enabled by default.

In addition, this commit reverts the work-around provided by
128aed684d0b3099092b7597c8644599b45b7503 which redirected both stdout
and stderr for the hooks to /dev/null.

Signed-off-by: Paul Serice <paul@serice.net>


---

 receive-pack.c               |    2 +
 run-command.c                |   27 +++++++++---------
 run-command.h                |    3 --
 send-pack.c                  |   64 +++++++++++++++++++++++++++++++++++++++++-
 templates/hooks--post-update |    4 +++
 templates/hooks--update      |    4 +++
 6 files changed, 85 insertions(+), 19 deletions(-)

36800ae8c6aa1427608a2d131b24986edba91bc9
diff --git a/receive-pack.c b/receive-pack.c
index cbe37e7..1873506 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -173,7 +173,7 @@ static void run_update_post_hook(struct 
 		argc++;
 	}
 	argv[argc] = NULL;
-	run_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);
+	run_command_v(argc, argv);
 }
 
 /*
diff --git a/run-command.c b/run-command.c
index 8bf5922..38cd6cb 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,19 +2,23 @@
 #include "run-command.h"
 #include <sys/wait.h>
 
-int run_command_v_opt(int argc, char **argv, int flags)
+int run_command_v(int argc, char **argv)
 {
-	pid_t pid = fork();
+       
+	pid_t pid = (pid_t)-1;
+
+	/* Because each process has independent buffering, if you
+	 * don't flush before the fork, it can seem like the new
+	 * output for the child occurs before the old output of the
+	 * parent which can be confusing at times. */
+	fflush(stdout);
+	fflush(stderr);
+
+	pid = fork();
 
 	if (pid < 0)
 		return -ERR_RUN_COMMAND_FORK;
 	if (!pid) {
-		if (flags & RUN_COMMAND_NO_STDIO) {
-			int fd = open("/dev/null", O_RDWR);
-			dup2(fd, 0);
-			dup2(fd, 1);
-			close(fd);			
-		}
 		execvp(argv[0], (char *const*) argv);
 		die("exec %s failed.", argv[0]);
 	}
@@ -42,11 +46,6 @@ int run_command_v_opt(int argc, char **a
 	}
 }
 
-int run_command_v(int argc, char **argv)
-{
-	return run_command_v_opt(argc, argv, 0);
-}
-
 int run_command(const char *cmd, ...)
 {
 	int argc;
@@ -65,5 +64,5 @@ int run_command(const char *cmd, ...)
 	va_end(param);
 	if (MAX_RUN_COMMAND_ARGS <= argc)
 		return error("too many args to run %s", cmd);
-	return run_command_v_opt(argc, argv, 0);
+	return run_command_v(argc, argv);
 }
diff --git a/run-command.h b/run-command.h
index 2469eea..5ee0972 100644
--- a/run-command.h
+++ b/run-command.h
@@ -11,9 +11,6 @@ enum {
 	ERR_RUN_COMMAND_WAITPID_NOEXIT,
 };
 
-#define RUN_COMMAND_NO_STDIO 1
-
-int run_command_v_opt(int argc, char **argv, int opt);
 int run_command_v(int argc, char **argv);
 int run_command(const char *cmd, ...);
 
diff --git a/send-pack.c b/send-pack.c
index 6ce0d9f..efc66ca 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -274,6 +274,55 @@ static int send_pack(int in, int out, in
 	return ret;
 }
 
+/* This function copies the data from in_fd to out_fd. It returns 1 if
+ * all data was successfully copied; otherwise, it returns 0, and
+ * errno will be set depending on how read() or write() failed. */
+static int cpfd(int in_fd, int out_fd)
+{
+	int rv = 0;
+	ssize_t rcount = -1;
+	ssize_t wcount = 0;
+	ssize_t wtmp = -1;
+	static char cpfd_buf[4096];
+
+	for (;;) {
+
+		/* Read buffer. */
+		rcount = read(in_fd, cpfd_buf, sizeof(cpfd_buf));
+
+		/* Done. */
+		if (rcount == 0) {
+			rv = 1;
+			goto out;
+		}
+
+		/* Error. */
+		if (rcount < 0) {
+			if (errno == EINTR) {
+				continue;
+			}
+			goto out;
+		}
+
+		/* Write buffer. */
+		wcount = 0;
+		while (wcount < rcount) {
+			wtmp = write(out_fd,
+			             cpfd_buf + wcount,
+			             rcount - wcount);
+			if (wtmp < 0) {
+				if (errno == EINTR) {
+					continue;
+				}
+				goto out;
+			}
+			wcount += wtmp;
+		}
+	}
+
+ out:
+	return rv;
+}
 
 int main(int argc, char **argv)
 {
@@ -319,8 +368,21 @@ int main(int argc, char **argv)
 	if (pid < 0)
 		return 1;
 	ret = send_pack(fd[0], fd[1], nr_heads, heads);
-	close(fd[0]);
+
+	/* git_connect() sets up a pipe between this program and
+	 * "exec" (typically git-receive-pack).	 The git-receive-pack
+	 * program in turn executes the "update" and "post-update"
+	 * hooks which might write to this program's fd[0].  To avoid
+	 * deadlock, this program must consume all of the data on
+	 * fd[0].  (The hooks are called after the pack has been
+	 * transfered so it should it should be safe to allow them to
+	 * write to stdout because it should not interfere with the
+	 * transfer protocol which also occurs on stdout).  */
+	cpfd(fd[0], fileno(stdout));
+
 	close(fd[1]);
 	finish_connect(pid);
+	close(fd[0]);
+
 	return ret;
 }
diff --git a/templates/hooks--post-update b/templates/hooks--post-update
index bcba893..d470dcc 100644
--- a/templates/hooks--post-update
+++ b/templates/hooks--post-update
@@ -5,4 +5,8 @@
 #
 # To enable this hook, make this file executable by "chmod +x post-update".
 
+# If your stdout and stderr messages are interleaved, uncomment the
+# following line.
+#exec 1>&2
+
 exec git-update-server-info
diff --git a/templates/hooks--update b/templates/hooks--update
index 6db555f..6199deb 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -8,6 +8,10 @@
 # (2) make this file executable by "chmod +x update".
 #
 
+# If your stdout and stderr messages are interleaved, uncomment the
+# following line.
+#exec 1>&2
+
 recipient="commit-list@example.com"
 
 if expr "$2" : '0*$' >/dev/null
-- 
0.99.9.GIT

Re: [PATCH] Fix race and deadlock when sending pack

[Junio C Hamano]

Paul Serice <paul@serice.net> writes:

> The best way to reproduce the problem is to locally clone your
> repository.  When you perform a push, git-send-pack will directly set
> up pipes connected to stdin and stdout of git-receive-pack.  You
> should then set up hook/post-update or hook/update to try to write
> lots of text to stdout.  (You want to use the local protocol because
> ssh is robust enough to mask the worst behavior.)

My immediate reaction was "do not do it then", but you are
right.  Hooks are run after all the protocol exchanges are done,
so they should be free to throw any garbage at the other end.

It appears cpfd() seems to mostly duplicate what is in copy.c;
is there any particular reason?

Re: [PATCH] Fix race and deadlock when sending pack

[Daniel Barkalow]

On Sun, 18 Dec 2005, Paul Serice wrote:

> Fix race and deadlock when sending pack.
> 
> The best way to reproduce the problem is to locally clone your
> repository.  When you perform a push, git-send-pack will directly set
> up pipes connected to stdin and stdout of git-receive-pack.  You
> should then set up hook/post-update or hook/update to try to write
> lots of text to stdout.  (You want to use the local protocol because
> ssh is robust enough to mask the worst behavior.)
> 
> The first problem is that git-send-pack closes git-receive-pack's
> stdout (which is inherited by the hooks) immediately after sending the
> pack.  This almost always causes the hooks to receive SIGPIPE when
> they try to write to stdout.
> 
> After fixing the SIGPIPE problem, you then run into a deadlock because
> git-send-pack is blocked trying to reap git-receive-pack and
> git-receive-pack (or one of its hooks) is blocked waiting for
> git-send-pack to read its output.
> 
> I've also added an example a one-liner to both hooks demonstrating how
> to redirect all subsequent output to stderr.  Because
> git-receive-pack's stderr is not redirected, it has always been safe
> to write to stderr.  Thus, all current status related output appears
> on stderr.  This can lead to confusing ordering of messages if only
> the hooks are using stdout.  The patch has the one-liner commented
> out, but perhaps it should be enabled by default.
> 
> In addition, this commit reverts the work-around provided by
> 128aed684d0b3099092b7597c8644599b45b7503 which redirected both stdout
> and stderr for the hooks to /dev/null.

Actually, that was stdin and stdout. If, for some reason, a hook looked at 
stdin, it could get surprising results. I don't think that it's actually a 
good idea to have output to stdout from hooks go to git-send-pack's 
stdout, since we may want to have git-send-pack report some sort of 
information of its own to stdout, which would then get confused with 
output from hooks. I think /dev/null, a log file, and stderr are the 
reasonable choices for what happens to output (and input pretty much has 
to be /dev/null).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix race and deadlock when sending pack

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> ... I don't think that it's actually a 
> good idea to have output to stdout from hooks go to git-send-pack's 
> stdout, since we may want to have git-send-pack report some sort of 
> information of its own to stdout,...

I admit that I haven't thought things through yet, but I do not
offhand think of an argument against Paul's patch (a scenario
that may be broken by the patch, that is), so I am inclined to
take it, perhaps after hearing about the cpfd() thing I
mentioned in the previous response to Paul.

It is conceivable that we may want to later extend the protocol
so that the receiver can tell the sender the result of what
happened to each of the ref-update request.  Right now, the
sender refuses to listen to what receiver says after it learns
the current object names, but after pack transfer finishes and
receiver decides what to do with each ref update request, we
might want to add status, like this:

	# Tell the pusher what commits we have and what their names are
	R: SHA1 name
	R: ...
	R: SHA1 name
	R: # flush -- it's your turn
	# Tell the puller what the pusher wants to happen
	S: old-SHA1 new-SHA1 name
	S: old-SHA1 new-SHA1 name
	S: ...
	S: # flush -- done with the list
	S: XXXXXXX --- packfile contents.
	# current protocol exchange ends here, but we could add...

        # ... what happened to each ref-update request.
	R: name OK
        R: name FAIL
        R: ...

If we do something like this, we might want to say why things
failed on "FAIL" line, and the output from hooks/update that
prevented the ref-update would probably belong there.

Re: [PATCH] Fix race and deadlock when sending pack

[Paul Serice]

> It appears cpfd() seems to mostly duplicate what is in copy.c;
> is there any particular reason?

No, I just wasn't aware of it.  I've made another patch to account for
it.  I wonder why copy_fd() (usually) closes ifd though.  I also
wonder why, if it is going to close ifd, doesn't it do so when there
an error is detected after the write() call.

I would also like to mention that I'm on the fence about all of this.
I think a lot of it is a matter of policy, and I'm just not sure what
the policy is regarding the hooks.  I wanted to submit the patch in it
full form to show that it is possible to redirect stdout of the hooks
to the terminal.  I also feel that, even if the patch is ultimately
rejected (no hard feelings :-), it would be interesting to know what's
the underlying problem.

Paul Serice

========================================================================

Subject: [PATCH] Fix race and deadlock when sending pack.

The best way to reproduce the problem is to locally clone your
repository.  When you perform a push, git-send-pack will directly set
up pipes connected to stdin and stdout of git-receive-pack.  You
should then set up hook/post-update or hook/update to try to write
lots of text to stdout.  (You want to use the local protocol because
ssh is robust enough to mask the worst behavior.)

The first problem is that git-send-pack closes git-receive-pack's
stdout (which is inherited by the hooks) immediately after sending the
pack.  This almost always causes the hooks to receive SIGPIPE when
they try to write to stdout.

After fixing the SIGPIPE problem, you then run into a deadlock because
git-send-pack is blocked trying to reap git-receive-pack and
git-receive-pack (or one of its hooks) is blocked waiting for
git-send-pack to read its output.

I've also added an example a one-liner to both hooks demonstrating how
to redirect all subsequent output to stderr.  Because
git-receive-pack's stderr is not redirected, it has always been safe
to write to stderr.  Thus, all current status related output appears
on stderr.  This can lead to confusing ordering of messages if only
the hooks are using stdout.  The patch has the one-liner commented
out, but perhaps it should be enabled by default.

In addition, this commit reverts the work-around provided by
128aed684d0b3099092b7597c8644599b45b7503 which redirected stdin and
stdout to /dev/null.

Signed-off-by: Paul Serice <paul@serice.net>


---

 receive-pack.c               |    2 +-
 run-command.c                |   27 +++++++++++++--------------
 run-command.h                |    3 ---
 send-pack.c                  |    9 ++++++++-
 templates/hooks--post-update |    4 ++++
 templates/hooks--update      |    4 ++++
 6 files changed, 30 insertions(+), 19 deletions(-)

87b7ee91cbe1b90bbd0937a85595de3933fc9459
diff --git a/receive-pack.c b/receive-pack.c
index cbe37e7..1873506 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -173,7 +173,7 @@ static void run_update_post_hook(struct 
 		argc++;
 	}
 	argv[argc] = NULL;
-	run_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);
+	run_command_v(argc, argv);
 }
 
 /*
diff --git a/run-command.c b/run-command.c
index 8bf5922..38cd6cb 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,19 +2,23 @@
 #include "run-command.h"
 #include <sys/wait.h>
 
-int run_command_v_opt(int argc, char **argv, int flags)
+int run_command_v(int argc, char **argv)
 {
-	pid_t pid = fork();
+       
+	pid_t pid = (pid_t)-1;
+
+	/* Because each process has independent buffering, if you
+	 * don't flush before the fork, it can seem like the new
+	 * output for the child occurs before the old output of the
+	 * parent which can be confusing at times. */
+	fflush(stdout);
+	fflush(stderr);
+
+	pid = fork();
 
 	if (pid < 0)
 		return -ERR_RUN_COMMAND_FORK;
 	if (!pid) {
-		if (flags & RUN_COMMAND_NO_STDIO) {
-			int fd = open("/dev/null", O_RDWR);
-			dup2(fd, 0);
-			dup2(fd, 1);
-			close(fd);			
-		}
 		execvp(argv[0], (char *const*) argv);
 		die("exec %s failed.", argv[0]);
 	}
@@ -42,11 +46,6 @@ int run_command_v_opt(int argc, char **a
 	}
 }
 
-int run_command_v(int argc, char **argv)
-{
-	return run_command_v_opt(argc, argv, 0);
-}
-
 int run_command(const char *cmd, ...)
 {
 	int argc;
@@ -65,5 +64,5 @@ int run_command(const char *cmd, ...)
 	va_end(param);
 	if (MAX_RUN_COMMAND_ARGS <= argc)
 		return error("too many args to run %s", cmd);
-	return run_command_v_opt(argc, argv, 0);
+	return run_command_v(argc, argv);
 }
diff --git a/run-command.h b/run-command.h
index 2469eea..5ee0972 100644
--- a/run-command.h
+++ b/run-command.h
@@ -11,9 +11,6 @@ enum {
 	ERR_RUN_COMMAND_WAITPID_NOEXIT,
 };
 
-#define RUN_COMMAND_NO_STDIO 1
-
-int run_command_v_opt(int argc, char **argv, int opt);
 int run_command_v(int argc, char **argv);
 int run_command(const char *cmd, ...);
 
diff --git a/send-pack.c b/send-pack.c
index 6ce0d9f..5a99ba9 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -319,8 +319,15 @@ int main(int argc, char **argv)
 	if (pid < 0)
 		return 1;
 	ret = send_pack(fd[0], fd[1], nr_heads, heads);
-	close(fd[0]);
+
+	/* Close our side of the conversation.	Wait for the child to
+	 * close its side of the conversation (copying the remainder
+	 * to our stdout).  Note that copy_fd() has the side effect of
+	 * closing fd[0]. */
 	close(fd[1]);
+	copy_fd(fd[0], fileno(stdout));
+
 	finish_connect(pid);
+
 	return ret;
 }
diff --git a/templates/hooks--post-update b/templates/hooks--post-update
index bcba893..d470dcc 100644
--- a/templates/hooks--post-update
+++ b/templates/hooks--post-update
@@ -5,4 +5,8 @@
 #
 # To enable this hook, make this file executable by "chmod +x post-update".
 
+# If your stdout and stderr messages are interleaved, uncomment the
+# following line.
+#exec 1>&2
+
 exec git-update-server-info
diff --git a/templates/hooks--update b/templates/hooks--update
index 6db555f..6199deb 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -8,6 +8,10 @@
 # (2) make this file executable by "chmod +x update".
 #
 
+# If your stdout and stderr messages are interleaved, uncomment the
+# following line.
+#exec 1>&2
+
 recipient="commit-list@example.com"
 
 if expr "$2" : '0*$' >/dev/null
-- 
0.99.9.GIT

Re: [PATCH] Fix race and deadlock when sending pack

[Paul Serice]

> If, for some reason, a hook looked at stdin, it could get surprising
> results.

In the first patch, the hook would hang waiting for input.  That was a
bug.  In the second patch, the hook's standard input has been closed
by git-send-pack (which is the standard thing for the parent to do in
a pipeline), thus if a hook looks at stdin it will get EOF (the same
as if connected to /dev/null).


Paul

Re: [PATCH] Fix race and deadlock when sending pack

[Daniel Barkalow]

On Sun, 18 Dec 2005, Junio C Hamano wrote:

> Paul Serice <paul@serice.net> writes:
> 
> > The best way to reproduce the problem is to locally clone your
> > repository.  When you perform a push, git-send-pack will directly set
> > up pipes connected to stdin and stdout of git-receive-pack.  You
> > should then set up hook/post-update or hook/update to try to write
> > lots of text to stdout.  (You want to use the local protocol because
> > ssh is robust enough to mask the worst behavior.)
> 
> My immediate reaction was "do not do it then", but you are
> right.  Hooks are run after all the protocol exchanges are done,
> so they should be free to throw any garbage at the other end.

If we extend it to transfer multiple things, wouldn't we want to run hooks 
after each of them, rather than all at the end?

As for the policy:

We definitely want to let hooks write to stdout, because git programs that 
you might want to run in hooks write to stdout. I can't figure out what 
"cvs" does with trigger script output and "at" and "cron" email the output 
to the owners. I'd sort of like to avoid making people expect that there 
is necessarily a path for text going back to the user directly. We may, 
for example, want to support these hooks with pushes over HTTP(/WebDAV). I 
also think that messages are likely to be at least as useful to the owner 
of the target repository as the person pushing, which is why I'd prefer a 
log file. E.g., if you've got a group central repository that different 
people push to, it may be other group members who want to know what 
happened with the output from a post-update hook, not the group member who 
pushed.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix race and deadlock when sending pack

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

>> My immediate reaction was "do not do it then", but you are
>> right.  Hooks are run after all the protocol exchanges are done,
>> so they should be free to throw any garbage at the other end.
>
> If we extend it to transfer multiple things, wouldn't we want to run hooks 
> after each of them, rather than all at the end?

We do transfer multiple things already, and all protocol
exchange happens before everything is transferred.  And hooks
are run for each refs being updated, one by one.  What we do not
have is a reporting mechanism that says "we refused to update
this ref because of the hooks/update policy return value for
it".  Even if we later add that reporting mechanism, as I
outlined in a separate message earlier, I think it is OK to keep
running the update hooks after the pack transfer part.

> As for the policy:
>
> We definitely want to let hooks write to stdout, because git programs that 
> you might want to run in hooks write to stdout.
> ... I'd sort of like to avoid making people expect that there 
> is necessarily a path for text going back to the user directly.
> ... I 
> also think that messages are likely to be at least as useful to the owner 
> of the target repository as the person pushing, which is why I'd prefer a 
> log file.

This part I mostly agree with.  Will have to think about the
details but probably I'd punt this for now and declare it post
1.0 ;-).

Re: [PATCH] Fix race and deadlock when sending pack

[Daniel Barkalow]

On Mon, 19 Dec 2005, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> >> My immediate reaction was "do not do it then", but you are
> >> right.  Hooks are run after all the protocol exchanges are done,
> >> so they should be free to throw any garbage at the other end.
> >
> > If we extend it to transfer multiple things, wouldn't we want to run hooks 
> > after each of them, rather than all at the end?
> 
> We do transfer multiple things already, and all protocol
> exchange happens before everything is transferred.  And hooks
> are run for each refs being updated, one by one.  What we do not
> have is a reporting mechanism that says "we refused to update
> this ref because of the hooks/update policy return value for
> it".  Even if we later add that reporting mechanism, as I
> outlined in a separate message earlier, I think it is OK to keep
> running the update hooks after the pack transfer part.

If we have the reporting mechanism, that will effectively be part of the 
protocol. It's obviously done transferring the pack at that point, but it 
still wants fixed-format communication, so switching over to being the 
stardard output of the hooks would cause problems with this.

> > As for the policy:
> >
> > We definitely want to let hooks write to stdout, because git programs that 
> > you might want to run in hooks write to stdout.
> > ... I'd sort of like to avoid making people expect that there 
> > is necessarily a path for text going back to the user directly.
> > ... I 
> > also think that messages are likely to be at least as useful to the owner 
> > of the target repository as the person pushing, which is why I'd prefer a 
> > log file.
> 
> This part I mostly agree with.  Will have to think about the
> details but probably I'd punt this for now and declare it post
> 1.0 ;-).

It's probably worth making sure that all the hooks run with something 
sane, and punt making it configurable andnice until post-1.0. I was only 
really looking at post-update, so I don't know how the others run.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix race and deadlock when sending pack

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> If we have the reporting mechanism, that will effectively be part of the 
> protocol. It's obviously done transferring the pack at that point, but it 
> still wants fixed-format communication, so switching over to being the 
> stardard output of the hooks would cause problems with this.

In order to add reporting mechanism later, I think we need to be
able to identify the protocol version in a backward compatible
way, something like the "server capabilities hidden behind the
NUL" trick we did for fetch-pack/upload-pack protocol.  Once
that is in place, it does not cause harm even if the current
protocol program connects hooks' stdout to send-pack, at least
in theory.  If we take Paul's patch now, however, it would add
more work for us later when we do that protocol change, because
we will need to wrap the output from the hook in the pkt-line
interface in the new protocol, in order to give that back to the
stdout of send-pack.  Considering that, I think we may want to
drop Paul's patch and declare that hooks stdout does not come
back to send-pack.

Honestly speaking, I do not really care where stdout of hooks go
as long as that does not cause breakage/deadlocks, and I think
your earlier patch on December 7th is serving us well enough; we
needed to have told users to do an "exec 1>somewhere" in their
hooks before that fix, which was not nice at all (and we even
forgot to tell them that).  If people want to send the output to
a log file, they can do so; if they want e-mails, they can do
so; if they want to show the output to the pusher, they can do
1>&2; all inside their hooks.  I do "echo nitfol | at now" and
love the way that I do not have to worry about how "at" command
gives me back execution report via e-mail at all ;-).

> It's probably worth making sure that all the hooks run with something 
> sane, and punt making it configurable andnice until post-1.0.

I think we agree that /dev/null is one of the sane choices as
you did in your earlier fix.  Duping stderr would have been
another sane choice, but I honestly do not think we care much
either way.