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

From: Junio C Hamano <gitster@pobox.com>
Date: May 27, 2022, 17:17 UTC
Message-ID: <xmqqa6b2520i.fsf@gitster.g>
In-Reply-To: <cover-v3-0.2-00000000000-20220527T090618Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 17 quoted lines

> 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.