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>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Anthony Sottile <asottile@umich.edu>,
	Emily Shaffer <emilyshaffer@google.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v3 0/2] hook API: connect hooks to the TTY again, fixes a v2.36.0 regression
Date: Fri, 27 May 2022 10:17:01 -0700	[thread overview]
Message-ID: <xmqqa6b2520i.fsf@gitster.g> (raw)
In-Reply-To: <cover-v3-0.2-00000000000-20220527T090618Z-avarab@gmail.com> (=?utf-8?B?IsOGdmFyIEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Fri, 27 May 2022 11:14:29 +0200")

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.