From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Wed, 14 Jan 2026 17:56:23 GMT
Subject: Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr
Message-ID: <87tswokri0.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <20260114171929.GC885771@coredump.intra.peff.net>

On Wed, 14 Jan 2026, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 14, 2026 at 12:08:49PM -0500, Jeff King wrote:
>
>> I looked at what feed_receive_hook_cb() is doing and...it's kind of
>> horrifying. It arbitrarily sends 500 lines, and then yields to the
>> caller to pump stderr (assuming ungroup=0). So:
>> 
>>   1. It is assuming that 500 lines of input won't fill up the pipe
>>      buffer and block. Even if we compute the size of 500 lines we're
>>      sending, we don't know if the caller has cleared anything from the
>>      pipe in the last call. There might be zero bytes available!
>> 
>>   2. After 500 lines we'll go back to the caller, which will then
>>      poll(). But if there's nothing to read on stderr, it will wait for
>>      the 100ms timeout. So if you have, say, 501 lines to send, then
>>      there will be a pointless 100ms pause in the middle.
>> 
>> So here's an example hook setup that will deadlock due to (1):
>
> And just for fun, here's an example that shows problem (2):
>
> -- >8 --
> rm -rf repo
> git init repo
> cd repo
> git commit --allow-empty -m foo
> git init --bare dst.git
>
> cat >dst.git/hooks/pre-receive <<\EOF
> #!/bin/sh
> # We don't even need to do anything interesting here! Git
> # will send us 500 lines, then block waiting for stderr which
> # we'll never send, and then send us another batch of 500.
> cat >/dev/null
> EOF
> chmod +x dst.git/hooks/pre-receive
>
> # Now do a moderate push of 500 branches.
> seq --format='create refs/heads/small-%g HEAD' 500 |
> git update-ref --stdin
> time git push -q dst.git refs/heads/small-*
>
> # And compare with one that sends just one more.
> seq --format='create refs/heads/large-%g HEAD' 501 |
> git update-ref --stdin
> time git push -q dst.git refs/heads/large-*
> -- >8 --
>
> The second push always takes 100ms more! If we run the server side under
> strace by replacing the final line with this:
>
>   git push -q --receive-pack='strace -T git-receive-pack' dst.git refs/heads/large-*
>
> we can see the stall here as we write to the hook:
>
>   write(4, "00000000000000000000000000000000"..., 51393) = 51393 <0.000011>
>   poll([{fd=5, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) <0.100506>
>   write(4, "00000000000000000000000000000000"..., 102) = 102 <0.000057>
>
> That would likewise be solved by using ungroup=1 (in which case we do
> not poll, but just call the feed function immediately again) or by using
> a real poll() loop (which would see immediately that the hook is ready
> for more input, rather than hitting the 100ms timeout).

Thanks for the detailed examples.

For the server-side hooks, I think the way forward is to implement the
poll loop as you suggested so we can buffer stderr and for a single
(non-parallel) hook, we can keep the existing behavior (still need to
test this). I'll do that in a separate patch and drop the batching.

For the client side hooks, I'll send v3 of this series which fixes the
two regressions reported by Chris and Kristoffer.