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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

Hi Ævar,

as promised in the Git IRC Standup [*1*], a review.

On Wed, 18 May 2022, Ævar Arnfjörð Bjarmason wrote:

> Ævar Arnfjörð Bjarmason (8):
>   run-command tests: change if/if/... to if/else if/else
>   run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
>   run-command tests: test stdout of run_command_parallel()
>   run-command.c: add an initializer for "struct parallel_processes"
>   run-command: add an "ungroup" option to run_process_parallel()
>   hook tests: fix redirection logic error in 96e7225b310
>   hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
>   hook API: fix v2.36.0 regression: hooks should be connected to a TTY

I started reviewing the patches individually, but have some higher-level
concerns that put my per-patch review on hold.

Keeping in mind that the intention is to fix a regression that was
introduced by way of refactoring (most of our recent regressions seem to
share that trait [*2*]), I strongly advise against another round of
refactoring [*3*], especially against tying it to fix a regression.

In this instance, it would be very easy to fix the bug without any
refactoring. In a nutshell, the manifestation of the bug amplifies this
part of the commit message of 96e7225b310 (hook: add 'run' subcommand,
2021-12-22):

    Some of the implementation here, such as a function being named
    run_hooks_opt() when it's tasked with running one hook, to using the
    run_processes_parallel_tr2() API to run with jobs=1 is somewhere
    between a bit odd and and an overkill for the current features of this
    "hook run" command and the hook.[ch] API.

It is this switch to `run_processes_parallel()` that is the root cause of
the regression.

The current iteration of the patch series does not fix that.

In the commit message from which I quoted, the plan is laid out to
eventually run more than one hook. If that is still the plan, we will be
presented with the unfortunate choice to either never running them in
parallel, or alternatively reintroducing the regression where the hooks
run detached from stdin/stdout/stderr.

It is pretty clear that there is no actual choice, and the hooks will
never be able to run in parallel. Therefore, the fix should move
`run_hooks_opt()` away from calling `run_processes_parallel()`.

In any case, regression fixes should not be mixed with refactorings unless
the latter make the former easier, which is not the case here.

Ciao,
Johannes

Footnote *1*:
https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-05-23#l44

Footnote *2*: I say "seem" because it would take a proper retro to analyze
what was the reason for the uptick in regressions, and even more
importantly to analyze what we can learn from the experience.

Footnote *3*: The refactoring, as Junio suspected, might very well go a
bit over board. Even if a new variation of the `run_processes_parallel()`
function that takes a struct should be necessary, it would be easy -- and
desirable -- to keep the current function signatures unchanged and simply
turn them into shims that then call the new variant. That would make the
refactoring much easier to review, and in turn it would make it less
likely to introduce another regression.