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

From: Junio C Hamano <gitster@pobox.com>
Date: May 26, 2022, 19:56 UTC
Message-ID: <xmqqsfowxe2p.fsf@gitster.g>
In-Reply-To: <220526.86ee0g3y1k.gmgdl@evledraar.gmail.com>

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

Show 23 quoted lines

>> I wonder if rebuilding from scratch is easier to review, then?  The
>> first three patches of such a series would be
>>
>>  - Revert cb3b3974 (Merge branch 'ab/racy-hooks', 2022-03-30)
>>  - Revert 7431379a (Merge branch 'ab/racy-hooks', 2022-03-16)
>>  - Revert c70bc338 (Merge branch 'ab/config-based-hooks-2', 2022-02-09)
>>
>> and then the rest would rebuild what used to be in the original
>> series on top.  There will be a lot of duplicate patches between
>> that "the rest" and the patches in the original series (e.g. I would
>> imagine that the resulting hook.h would look more or less
>> identical), but "git range-diff" may be able to trim it down by
>> comparing between "the rest" and "c70bc338^..c70bc338^2" (aka
>> ab/config-based-hooks-2).  I dunno.
>
> I'm still happy to and planning to send a re-roll of this to try to
> address outstanding comments/concerns, but am holding off for now
> because it's not clear to me if you're already planning to discard any
> such re-roll in favor of a revert.
>
> Or do you mean to create a point release with such revert(s) and have
> master free to move forward with a fix for the outstanding issue, but
> not to use that for a point release?

If a maintenance release will have reverts with adjustment, then the solution that will be only merged to master should still be built on top. So if we were to go the route above, the early part (the first three that are reverts above, and possibly a couple more directly on top just to address "did we really run hook?") would be merged to the maintenance track, while the whole thing that rebuilds on top of the reverted one would be merged to 'master', I would imagine.

It all depends on how involved it is to get to where we want to be, between

 (1) starting from 'master' and working backwards, removing the use of
     the run_parallel stuff and replacing it with the run_command API, or

 (2) bringing us back to pre-c70bc338 state first and then building
     up what we would have built if we didn't use run_parallel stuff
     in the original series.

As you were saying that what you would produce with the former approach would be, compared to the initial "regress fix" that still used the run_parallel stuff, a large and unreviewable mess, I was throwing out a different approach as a potential alternative, with the hope that the resulting series may make it reviewable, as long as the early "straight revert" part is straight-forward.

If we take the "start from 'master' and fix minimally" approach, the whole thing would be both in the maintenance track and in the track for the next release, I would imagine.

So, in short, either way, we would not run hooks in parallel, and we would not run hooks with run_parallel API castrated to a single process usage only, in the version we will merge to the maintenance track and also to the master track. The latter may get an update to re-attempt reusing run_parallel API in a way that is less hostile to existing users, but I do not think we should make users wait by spending more time on it than necessary right now, before we get the regression fix ready.

Thanks.