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

[Junio C Hamano <gitster@pobox.com>]

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

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