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 25, 2022, 16:57 UTC
Message-ID: <xmqqbkvl8s88.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2205251308381.352@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

I share this sentiment.

Show 15 quoted lines

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

True.

Show 5 quoted lines

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

I had a similar impression before I reviewed the code after the regression report, but if I read the code before the breakage correctly, I think we didn't change the handling of the standard input stream with the series from Emily/Ævar that broke the hooks.

The regression is the output streams are no longer _directly_ connected to the outside world, and instead to our internal relay that buffers. The run_hook_ve() helper did set .no_stdin to 1 before doing run_command() in Git 2.35. The series with regression does the same in pick_next_hook() callback in hook.c. Both also set .stdout_to_stderr to 1, so the apparent output should not change.

> 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()`.

My take on it is slightly different.

I personally do not think we should run hooks in parallel ourselves, but if hook-like things, which Emily and Ævar want, want run in parallel, we can safely allow them to do so. No current users have ever seen such hook-like things specified in their configuration files---as long as it is clearly documented that these hook-like things are not connected to the original standard output or error, and they may run in parallel and whatever inter-process coordination is their responsibility, there is no regression. It is a brand new feature.

The mechanism that supports that hook-like things should have a compatibility mode, if it ever wants to take responsibility of running the traditional hooks as part of its offering. I think the right way to do so is follows:

 - Unless each hook-like thing explicitly asks, it does not run in
   parallel with other hook-like things, and its output stream is
   connected directly to the original output stream.  They can run
   without involving the run_processes_parallel() at all.

 - When the traditional on-disk hooks are treated as if it is one of
   these hook-like things, the compatibility mode should be set to
   on for them without any user interaction.

 - Only the new stuff written specifically to be used as these shiny
   new hook-like things would explicitly ask to run in parallel and
   emit to the output multiplexer.

Doing things that way would pave the way forward to allow new stuff to work differently, without breaking existing stuff people have, wouldn't it?

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

Absolutely. I wonder how involved is would be to revert the merge of the whole thing from 'master'. It may give us a clean slate to rethink the whole mess and redo it without breaking the existing users' hooks.