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

[Junio C Hamano <junkio@cox.net>]

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.