Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr

From: Jeff King <peff@peff.net>
Date: Jan 14, 2026, 17:08 UTC
Message-ID: <20260114170849.GB885771@coredump.intra.peff.net>
In-Reply-To: <878qe0zimo.fsf@gentoo.mail-host-address-is-not-set>

On Wed, Jan 14, 2026 at 10:46:39AM +0200, Adrian Ratiu wrote:

Show 35 quoted lines

> > So we are hitting the poll timeout for each ref we consider, and it
> > takes forever to actually write the whole input stream. Which seems like
> > a bug in using feed_pipe without ungroup. Either:
> >
> >   1. We should write everything to the child as quickly as possible,
> >      assuming that we do not have to worry about reading back from it to
> >      avoid deadlock.
> >
> >   2. We should add the child's input pipes to our poll() call so that we
> >      can tell it is ready for more input (without hitting the timeout).
> >
> > Setting ungroup=1 saves us from this because it means that we'll skip
> > the poll() call entirely in pp_handle_child_IO(). So we end up
> > effectively doing (1), which is OK because ungroup means we are not
> > reading stdout or stderr from the child at all.
> >
> > But it feels like this is papering over a bug, or at least providing a
> > dangerous interface. AFAICT you _must_ set ungroup if you are going to
> > use the feed_pipe callback. And it does not really have anything to do
> > with the stdout_to_stderr flag at all.
> >
> > It looks like feed_pipe feature is new-ish in your series. Maybe it
> > should just be a BUG() to use it without ungroup?
> 
> This is all very useful and it proves there are 2 separate bugs here,
> requiring two separate fixes for both Chris and Kristoffer.
> 
> The logic in v1 (without ungroup) is enough to fix Chris' issue with
> stdin and for Kristoffer I will do a smarter fix which implements your
> (1) suggestion: batch more than a single stdin fd write in each poll
> call so we achieve comparable throughtput (no added poll latency).
> 
> We already do this for the receive hook in feed_receive_hook_cb(). In
> this case we just need the callback to process more than just 1 ref at a
> time.

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):

-- >8 -- #!/bin/sh

# make two repos: one to push from, and one to push into rm -rf repo git init repo cd repo git init --bare dst.git

# And here's our pre-receive hook that will cause problems. cat >dst.git/hooks/pre-receive <<\EOF #!/bin/sh

# Imagine we write a lot of output to stderr. For example, progress # reporting for some kind of setup procedure (but it could be anything). # The key thing is that it is enough to fill up the pipe buffer going # back to git. for i in $(seq 10000); do printf "\rprocessing $i..."; done echo done

# and now we are ready to read the input from the caller. A real hook # would do something useful with the input, but we'll just read it # and discard. cat >/dev/null EOF chmod +x dst.git/hooks/pre-receive

# And now do a big push. 1000 ref updates seems to be enough to fill up # the pipe buffer (each one is 2 oids plus the ref name plus whitespace, # which is 100+ bytes each). git commit --allow-empty -m foo seq --format='create refs/heads/branch-%g HEAD' 1000 | git update-ref --stdin git push -q --all dst.git -- >8 --

This will deadlock when run using the ar/run-command-hook topic. What happens is this:

  1. Git writes out the first 500 lines to the hook. This partially
     fills the pipe buffer going to the hook.

  2. The hook writes to stderr, filling up the pipe buffer back to Git
     and blocking.

  3. Git does its poll() and sees that there is data to read on stderr.
     It reads some of it (8k, I think, due to strbuf_read_once).

  4. The hook sees more room in the pipe, so it writes another 8k. But
     it blocks again, still not having read any of its stdin.

  5. Git, having done one round of poll(), goes back to trying to write
     to the hook's stdin, and tries for another 500 lines. But since the
     hook did not read anything from stdin, this fills up the pipe
     buffer.

  6. Now we are deadlocked. Git is blocked trying to write to the hook's
     stdin, but the hook is blocked trying to write to stderr.

To solve this we must either:

  a. Make sure ungroup=1 is set, which means that Git does not read back
     stderr. In which case the 500-line batching is pointless. We can
     just write everything! But I assume you do not want to do this, as I'd
     guess the point of the series is that we want to buffer the stderr
     of each hook so that multiple hooks can be run in parallel without
     stomping on each other's output.

  b. Do a real poll() loop that checks both for incoming data on stderr
     from the hook, but also for the ability to write to the hook's
     stdin. Look at how pipe_command() and pump_io() do this, for
     example. You'd want something like that, but extended across
     multiple sub-processes running at once.

-Peff

PS If the goal of the series is to buffer stderr, that has another side
   effect: hooks can no longer produce real-time progress updates. Maybe
   losing that ability is a good tradeoff to keep the stderr output from
   multiple hooks from stomping on each other. But for a single hook,
   should we retain the existing behavior?