From: Jeff King <peff@peff.net>
Date: Wed, 14 Jan 2026 21:27:18 GMT
Subject: Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in
Message-ID: <20260114212718.GB1010080@coredump.intra.peff.net>
In-Reply-To: <20260114185731.2381550-3-adrian.ratiu@collabora.com>

On Wed, Jan 14, 2026 at 08:57:31PM +0200, Adrian Ratiu wrote:

> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),
> I accidentally made the ungroup option opt-in instead of opt-out and
> despite my best efforts to set it for all API users, I missed a case
> which requires it to be set: the pre-push hook which regressed.
> 
> The only thing I needed in that commit was a way to change the default,
> to convert the remaining receive-pack hooks which require ungroup == 0
> for sideband output, so it doesn't matter if it's on or off by default.
> 
> Bring back the original behavior by setting it for all hooks in the
> struct run_hooks_opt initializer, which nicely allows changing the
> default value only where needed, in receive-pack.c.

I think this is an improvement overall to what's currently in 'seen',
and the patch looks as I'd expect.

I have doubts in general about the approach taken by c65f26fca4
(receive-pack: convert receive hooks to hook API, 2025-12-26). We used
to use an async muxer thread, and now we are buffering hook stderr,
which to my mind is a regression (both in terms of real-time output, but
also the deadlock issues mentioned earlier).

I'd rather see us continue to set up a muxer thread, and then direct the
hook API to attach the stderr of the hook processes to that descriptor.
Then receive-pack would just work as before, without having to fiddle
with the ungroup flag at all.

You can take that with the appropriate size grain of salt from an
observer who has not been following the series (and is not really
interested in it, beyond making sure we do not introduce regressions).
But it is also an observer who has dealt with many I/O deadlocks in Git. ;)

-Peff