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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 25, 2022, 11:30 UTC
Message-ID: <nycvar.QRO.7.76.6.2205251308381.352@tvgsbejvaqbjf.bet>
In-Reply-To: <cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com>

Hi Ævar,

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

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

Show 9 quoted lines

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