Subject: Re: [PATCH v3 0/2] hook API: connect hooks to the TTY again, fixes a v2.36.0 regression

From: Junio C Hamano <[email-masked]>
Date: Fri, 27 May 2022 10:17:01 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> A re-roll of [1] which aims to address the concerns about the previous
> 8-part series being too large to fix a release regression. "If it
> isn't bolted down, throw it overboard!".
>
> The main change here is:
>
>  * The new "ungroup" parameter is now passed via an "extern" parameter.
>  * Tests for existing run-command.c behavior (not narrowly needed for
>    the regression fix) are gone.
>  * Adding an INIT macro is gone, instead we explicitly  initialize to NULL.
>  * Stray bugfix for existing hook test is gone.
>
> etc. I think all of those still make sense, but they're something I
> can rebase on this topic once it (hopefully) lands. In the meantime
> the updated commit messages for the remaining two (see start of the
> range-diff below) argue for this being a a safe API change, even if
> the interface is a bit nasty.

So the approach taken here is that we assume the reported one is the
only regression and keep going with run_process_parallel() API.

I still share the sentiment with Dscho that it is generally a bad
idea, when dealing with a regression, to double-down and dig in
your heels to keep the change that caused a regression with paper
over patches, but too much time has passed since the release, and a
patch or two on top does look like a quicker way forward.

I left a few comments on the implementation, but modulo these small
details, the code looks OK (provided that the assumption holds true,
that is, of course).

Thanks.