git 2.36.0 regression: pre-commit hooks no longer have stdout/stderr as tty

here's the shortest reproduction --

```console
$ cat ../testrepo/.git/hooks/pre-commit
#!/usr/bin/env bash
if [ -t 1 ]; then
    echo GOOD
fi
```

in previous git versions:

``` $ git commit -q --allow-empty -m foo GOOD $ ```

with git 2.36.0:

```` $ git commit -q --allow-empty -m foo $ ```

why I care: I maintain a git hooks framework which uses `isatty` to detect whether it's appropriate to color the output. many tools utilize the same check. in 2.36.0+ isatty is false for stdout and stderr causing coloring to be turned off.

I bisected this (it was a little complicated, needed to force a pty):

`../testrepo`: a git repo set up with the hook above

`../bisect.sh`:

```bash #!/usr/bin/env bash set -eux git clean -fxfd >& /dev/null make -j6 prefix="$PWD/prefix" NO_GETTEXT=1 NO_TCLTK=1 install >& /dev/null export PATH="$PWD/prefix/bin:$PATH" cd ../testrepo (../pty git commit -q --allow-empty -m foo || true) | grep GOOD ```

`../pty`:

```python #!/usr/bin/env python3 import errno import os import subprocess import sys

x: int = 'nope'

class Pty(object):
    def __init__(self):
        self.r = self.w = None

    def __enter__(self):
        self.r, self.w = os.openpty()

        return self

    def close_w(self):
        if self.w is not None:
            os.close(self.w)
            self.w = None

    def close_r(self):
        assert self.r is not None
        os.close(self.r)
        self.r = None

    def __exit__(self, exc_type, exc_value, traceback):
        self.close_w()
        self.close_r()

def cmd_output_p(*cmd, **kwargs):
    with open(os.devnull) as devnull, Pty() as pty:
        kwargs = {'stdin': devnull, 'stdout': pty.w, 'stderr': pty.w}
        proc = subprocess.Popen(cmd, **kwargs)
        pty.close_w()

        buf = b''
        while True:
            try:
                bts = os.read(pty.r, 4096)
            except OSError as e:
                if e.errno == errno.EIO:
                    bts = b''
                else:
                    raise
            else:
                buf += bts
            if not bts:
                break

    return proc.wait(), buf, None

if __name__ == '__main__':
    _, buf, _ = cmd_output_p(*sys.argv[1:])
    sys.stdout.buffer.write(buf)
```

the first commit it points out:

```
f443246b9f29b815f0b98a07bb2d425628ae6522 is the first bad commit
commit f443246b9f29b815f0b98a07bb2d425628ae6522
Author: Emily Shaffer <emilyshaffer@google.com>
Date:   Wed Dec 22 04:59:40 2021 +0100

    commit: convert {pre-commit,prepare-commit-msg} hook to hook.h

    Move these hooks hook away from run-command.h to and over to the new
    hook.h library.

    Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Acked-by: Emily Shaffer <emilyshaffer@google.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

 commit.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
bisect run success
```

Anthony

On Tue, Apr 19, 2022 at 02:59:36PM -0400, Anthony Sottile wrote:

Interesting. I'm surprised to see the tty-ness of hooks changing with this patch, as the way the hook is called is pretty much the same:

run_hook_ve() ("the old way") sets no_stdin, stdout_to_stderr, args, envvars, and some trace variables, and then runs 'run_command()'; run_command() invokes start_command().

run_hooks_opt ("the new way") ultimately kicks off the hook with a callback that sets up a child_process with no_stdin, stdout_to_stderr, args, envvars, and some trace variables (hook.c:pick_next_hook); the task queue manager also sets .err to -1 on that child_process; then it calls start_command() directly (run-command.c:pp_start_one()).

I'm not sure I see why the tty-ness would change between the two. If I'm being honest, I'm actually slightly surprised that `isatty` returned true for your hook before - since the hook process is a child of Git and its output is, presumably, being consumed by Git first rather than by an interactive user shell.

I suppose that with stdout_to_stderr being set, the tty-ness of the main process's stderr would then apply to the child process's stdout (we do that by calling `dup(2)`). But that's being set in both "the old way" and "the new way", so I'm pretty surprised to see a change here.

It *is* true that run-command.c:pp_start_one() sets child_process:err=-1 for the child and run-command.c:run_hook_ve() didn't do that; that -1 means that start_command() will create a new fd for the child's stderr. Since run_hook_ve() didn't care about the child's stderr before, I wonder if that is why? Could it be that now that we're processing the child's stderr, the child no longer thinks stderr is in tty, because the parent is consuming its output?

I think if that's the case, a fix would involve run-command.c:pp_start_one() not setting .err, .stdout_to_stderr, or .no_stdin at all on its own, and relying on the 'get_next_task' callback to set those things. It's a little more painful than I initially thought because the run_processes_parallel() library depends on that err capture to run pp_buffer_stderr() unconditionally; I guess it needs a tiny bit of shim logic to deal with callers who don't care to see their children's stderr.

All that said.... I'd expect that the dup() from the child's stdout to the parent's stderr would still result in a happy isatty(1). So I'm not convinced this is actually the right solution.... From your repro script, I can't quite tell which fd the isatty call is against (to be honest, I can't find the isatty call, either). So maybe I'm going the wrong direction :)

 - Emily

On Tue, Apr 19, 2022 at 7:37 PM Emily Shaffer <emilyshaffer@google.com> wrote:

ah, most of the repro script was just so I could bisect -- you can ignore pretty much all of it except for the `pre-commit` file:

#!/usr/bin/env bash
if [ -t 1 ]; then
   echo GOOD
fi

this is doing "isatty" against fd 1 which is stdout (it could also try the same against fd 2 which was also a tty previously)

Anthony

Hi Emily

On 20/04/2022 00:37, Emily Shaffer wrote:

 >

Exactly, stderr is redirected to a pipe so that we can buffer the output from each process and then write it to the real stdout when the process has finished to avoid the output from different processes getting mixed together. Ideally in this case we'd see that stdout is a tty and create a pty rather than a pipe when buffering the output from the process.

Best Wishes

Phillip

On Wed, Apr 20 2022, Phillip Wood wrote:

All: I have a fix for this, currently CI-ing, testing etc. Basically it
just adds an option to run_process_parallel() to stop doing the
stdout/stderr interception.

It means that for the current jobs=1 we'll behave as before.

For jobs >1 in the future we'll need to decide what we want to do, i.e. you can have TTY, or guaranteed non-interleaved output, but not both.

I'd think for hooks no interception makes sense, but in any case we can defer that until sometime later...

Preview of the fix below, this is on top of an earlier change to add the "struct run_process_parallel_opts" to pass such options along:

On Wed, Apr 20, 2022 at 5:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I'm curious what your reasoning is there. I rely on hooks which give me user-readable output quite frequently, so the interleaving is important to keep them from being useless if I trigger more than one hook (e.g. I have separate hooks to check for secret keys and for debug strings).

Would it make sense to start by setting it based on the number of hooks available?

Left a quick thought below, but please don't consider it as a full review - I haven't got time to look much more yet.

Is it not possible to let run_processes_parallel() callers set these flags manually (as they are providing a child_process in the "get next task" callback), and then to decide whether to buffer the output based on the fd status instead? I'd prefer that rather than an all-or-none option that may not apply to every process, I think... But I could be wrong :)

Phillip Wood <phillip.wood123@gmail.com> writes:

Ah, thanks, and sigh. That means this was an unintended regression caused by use of parallel infrastructure, mixed with a bit of "the original problem report wrote hook properly so that when it is not connected to a terminal (such as in this new implementation) it refrains to do terminal-y things like coloring, so everything is working as intended" ;-).

IIRC, the parallel subprocess stuff was invented to spawn multiple tasks we internally need (like "checkout these submodules") that are not interactive (hence does not need access to stdin) en masse, and the output buffering is there to avoid interleaving the output that would make it unreadable.

Use of the parallel subprocess API means that we inherently cannot give access to the standard input to the hooks. The users of the original run_hooks_ve() API would be OK with that, because it did .no_stdin=1 before the problematic hooks API rewrite, but I wonder what our plans should be for hooks that want to go interactive. They could open /dev/tty themselves (and that would have been the only way to go interactive even in the old world order, so it is perfectly acceptable to keep it that way with .no_stdin=1), but if they run in parallel, the end-user would not know whom they are typing to (and which output lines are the prompts they are expected to respond to).

In the longer term, there are multiple possible action items.

 * We probably would want to design a bit better anti-interleaving
   machinery than "buffer everything and show only after the process
   exists", if we want to keep using the parallel subprocess API.
   And that would help the original "do this thing in multiple
   submodules at the same time" use case, too.  

 * We should teach hooks API to make it _optional_ to use the
   parallel subprocess API.  If we are not spawning hooks in
   parallel today, there is no reason to incur this regression by
   using the parallel subprocess API---this was a needress bug, and
   I am angry.

 * the hooks API should learn a mechanism for multiple hooks to
   coordinate their executions.  Perhaps they indicate their
   preference if they are OK to be run in parallel, and those that
   want isolation will be run one-at-a-time before or after others
   run in parallel, or something.

 * The hooks API should learn a mechanism for us to tell what
   execution environment they are in.  Ideally, the hooks, if it is
   sane to run under the parallel subprocess API, shouldn't have
   been learning if they are talking to an interactive human user by
   looking at isatty(), but we should have been explicitly telling
   them that they are, perhaps by exporting an environment
   variable.  There may probably be more clue hooks writers want
   other than "am I talking to human user?" that we would want to
   enumerate before going this route.

Thanks for analyzing.

On Wed, Apr 20, 2022 at 9:42 AM Junio C Hamano <gitster@pobox.com> wrote:

I've noticed this too, but for very noisy things which are parallelized, I'm not sure a better user experience is possible. I suppose we could pick the "first" job in the task queue and print that output as it comes in, so that users are aware that *something* is happening?

[job 0 starts] [job 1 starts] job 0 says 0-foo [job 1 says 1-foo, but it's buffered] job 0 says 0-bar [job 1 says 1-bar, but it's buffered] [job 0 finishes] [we replay the buffer from job 1 so far:] job 1 says 1-foo job 1 says 1-bar job 1 says 1-baz [job 1 finishes]

I think it could be possible, but then job 1 still will never learn that it's a tty, because it's being buffered to prevent interleaving, even if we have the illusion of non-buffering.

To counter, I think that having hooks invoked via two different mechanisms depending on how many are provided or whether they are parallelized is a mess to debug and maintain. I still stand by the decision to use the parallel subprocess API, which I think was reasonable to expect to do the same thing when jobs=1, and I think we should continue to do so. It simplifies the hook code significantly.

There is such a mechanism for hooks overall, but not yet for individual hooks. I know we discussed it at length[1] before, and decided it would be okay to figure this out later on. I suppose "later on" may have come :)

Hm. I was going to mention that Ævar and I discussed the possibility of setting an environment variable for hook child processes, telling them which hook they are being run as - e.g. "GIT_HOOK=prepare-commit-msg" - but I suppose that relying on that alone doesn't tell us anything about whether the parent is being run in tty. I agree it could be very useful to simply pass GIT_PARENT_ISATTY to hooks (and I suppose other child processes). Could we simply do that from start_command() or something else deep in run-command.h machinery? Then Anthony's use case becomes

if [-t 1|| GIT_PARENT_ISATTY]
 ...

and no need to examine Git version.

 - Emily

1: https://lore.kernel.org/git/20210527000856.695702-2-emilyshaffer%40google.com under "Parallelization with dependencies" (and preceding conversations)

Emily Shaffer <emilyshaffer@google.com> writes:

A simple code that does not behave as it should and causes end-user regression is not a code worth defending. Admitting it was a bad move we made in the past is the first step to make it better.

The use of the parallel subprocess API in the hooks was prematurely done, before we had clear use cases for running multiple hooks in parallel, and due to the lack of use cases, we didn't have chance to think about the issues that need to be addressed before we can start using the parallel subprocess API. The message you are responding to was written with an explicit purpose of starting to list them.

This...

Yes, besides patching up this regression for short term, I listed it as a possible ation item for the longer term.

That...

But DO NOT call it ISATTY. "Are we showing the output to human end-users" is the question it is answering to, and isatty() happens to be an implementation detail on POSIXy system.

"This" and "That" above make it smell like discussion was done, but everybody got tired of discussing and the topic was shipped without necessary polishment? That sounds like a process failure, which we may want to address in the new development cycle, not limited to this particular topic.

Thanks.

On Wed, Apr 20, 2022 at 10:25 AM Junio C Hamano <gitster@pobox.com> wrote:

I am also sorry that this use case was broken. However, I don't see that it's documented in 'git help githooks' or elsewhere that we guarantee isatty() (or similar) of hooks matches that of the parent process. I think it is an accident that this worked before, and not something that was guaranteed by Git documentation - for example, we also do not have regression tests ensuring that behavior for hooks today, either, or else we would not be having this conversation. (If I simply missed the documentation promising that behavior, then I am sorry, and please point me to it.)

I think, rather, during discussion we said "without knowing how real users want to use hooks, it's not possible for us to make a good design for individual hooks to state whether they need to be parallelized or not." Perhaps that means this body of work should have stayed in 'next' longer, rather than making it to a release?

For what it's worth, Google internally has been using multiple hooks via config for something like a year, with this design, from a combination of 'next' and pending hooks patches. But we haven't imagined the need to color hook output for users and check isatty() or similar. I think there are not many other consumers of 'next' besides the Google internal release. So I'm not sure that longer time in 'next' would have allowed us to see this issue, either.

 - Emily

On Wed, Apr 20 2022, Emily Shaffer wrote:

[I'll reply to most of this & other questions in the form of patches, just on some of this]

You're correct that it wasn't documented, and as regressions go that makes it *slightly* better. I.e. at least it's not a publicly documented promise.

Anyone using this part of the interface would have discovered it by experimentation, or (reasonably) assumed that git was invoking the hook without any special redirection or buffering.

And you're also right that we didn't have any test coverage for this, actually before the t/t1800-hook.sh we didn't have any test coverage at all on stdout_to_stderr for hooks (at least those converted to the API so far), which is pretty fundimental.

But none of that (except perhaps the doc omission) makes this any less of a regression. We don't have 100% test coverage, and can't assume that just because something isn't documented or tested for that it's not being relied on in the wild. It is, as this upthread report indicates.

In this case "100% test coverage" in the "make coverage" sense wouldn't help, this is part of 200% test coverage. I.e. it's in how an external user expects to use and interact with the command. So it can remain uncovered even if our own tests touch 100% of our own code.

Just to clarify this a bit, we discussed passing down GIT_HOOK so that you could e.g. symlink all your hooks and dispatch to some "hook router".

Which right now you can do with the file-based hooks, because you'll need to symlink them to such a router, but couldn't with future config-based hooks.

IOW it's entirely separate conceptually from a "how does this hook expect to behave" vis-a-vis calling isatty() or whatever. It would just be working around or own implementation details, i.e. whether we invoke a path or a configured command.

We're both thoroughly "on inside" of this particular process failure, so we're both bound to have biases here.

But having said that I agree with you here. I.e. as a mechanism for mitigating mistakes and catching obscure edge cases just being more careful or having things sit in 'next' for longer has, I think, proved itself to not be an effective method (not just in this case, but a few similar cases).

I'm not sure what the solution is exactly, but I'm pretty sure it involves more controlled exposure to the wild (e.g. shipping certain things as feature flags first), not deferring that exposure for long periods, which is what having things sit it "next" for longer amounts to.

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

I tend to agree. Given that people do not discover possible regression that will affect even after a change hits 'master', cooking in 'next' alone would not be all that effective.

But that does not mean we shouldn't cook in 'next' at all.

Especially the previous cycle, I was experimenting with a tweak in my workflow to have topics in 'next' to cook for one week and have them graduate to 'master' unless we saw regression in a week. Previously, I tended to keep topics on the larger side in 'next' and we did see "oops we found this after the topic hit 'next' and here is a fix-up" to them, which I think helped to catch bugs before it broke 'master'.

To be fair, "is the hook invoked with its standard output stream connected to the original standard output of the main process?" is not something either of you cared while working on and reviewing the changes, and releases based on 'next' $BIGCOMPANY have its users use internally wouldn't have helped to catch this particular regression, as the primary reason we didn't think of it as a problem is because internal users of $BIGCOMPANY tend to be more monoculture than folks in the wild.

So the fundamental solution would be to find a way to involve those who found the regression after a release was done in the development process at a much earlier stage. I do not offhand know how to get there.

It may be very hard for us to do with end-users, who typically have only one instance of Git they use and a single valuable repository they cannot subject to "experiments". They may depend on certain aspects of the behaviour of the current version, but they lack an environment to try out our new version to see if we broke them. They probably may not even be aware of what they are relying on, just as we are unaware of their dependence.

But for toolsmiths who integrate their gears with Git, there may be something we can do. Perhaps we can start giving "works with Git" badge out (we need to control its use with some kind of trademark registration), to those who "maintain X that enhances Git" when they regularly test their ware with 'next' or much earlier. And when they stop us before unleashing a possible regression by reporting bugs early, give them a "star", so that their "works with Git *****" logo can boast how much contribution in testing they are maing upstream.

Or something like that, perhaps?

Junio C Hamano <gitster@pobox.com> writes:

"and mechanically have them graduate" is what I meant.  Also

"in 'next' longer, and" is what I meant.

Anthony Sottile <asottile@umich.edu> writes:

Nicely bisected.  Thanks.

I have a feeling that it may have been a deliberate design decision when Ævar revamped the code that drives the hook invocation based on Emily's code. Ævar, Emily, do any of you remember why we did this, or is this a mere regression?

Thanks.

This fixes the regression reported by Anthony Sottile[1] with hooks not being connected to a TTY. See 6/6 for details.

It would also have been possible to rip the run_processes_parallel_tr2() out of hook.c, as we currently only use it for nproc=1. However it's the plan to have it run multiple hooks, and as 3/6 argues it's a good idea in general for our parallel execution API to learn a mode similar to GNU parallel's "--ungroup", even though at the conclusion of this series the hook API is its only user.

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Ævar Arnfjörð Bjarmason (6):
  run-command API: replace run_processes_parallel_tr2() with opts struct
  run-command tests: test stdout of run_command_parallel()
  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

 builtin/fetch.c             |  15 ++--
 builtin/submodule--helper.c |  12 ++--
 hook.c                      |  19 ++---
 run-command.c               | 135 +++++++++++++++++++++++++++---------
 run-command.h               |  56 +++++++++++----
 submodule.c                 |  13 ++--
 t/helper/test-run-command.c |  44 ++++++++----
 t/t0061-run-command.sh      |  45 ++++++++++--
 t/t1800-hook.sh             |  39 ++++++++++-
 9 files changed, 287 insertions(+), 91 deletions(-)

Add a new "struct run_process_parallel_opts" to cover the trace2 use-case added in ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). A subsequent commit will add more options, and having a proliferation of new functions or extra parameters would result in needless churn.

It makes for a smaller change to make run_processes_parallel() and run_processes_parallel_tr2() wrapper functions for the new "static" run_processes_parallel_1(), which contains the main logic. We pass down "opts" to the *_1() function even though it isn't used there yet (only in the *_tr2() function), a subsequent commit will make more use of it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c             | 15 ++++++++------
 builtin/submodule--helper.c | 12 +++++++----
 hook.c                      | 13 ++++++------
 run-command.c               | 40 +++++++++++++++++++++++++++----------
 run-command.h               | 26 ++++++++++++++++--------
 submodule.c                 | 13 ++++++------
 t/helper/test-run-command.c | 13 ++++++------
 7 files changed, 84 insertions(+), 48 deletions(-)

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

If the idea is that with unset .tr2_* members we can silently bypass the overhead to invoke trace2 machinery without changing much in the caller side (or even better, instead of doing this as run_opts but as tr2_opts, and allow the caller to pass NULL to decline tracing at runtime), that would be wonderful.

If we are going to throw random other members into the struct that are unrelated to tr2, then it makes it unclear why we have the three *_fn and its callback state still passed as separate parameters, rather than making them members of the struct. After all, it is clear that this new struct is designed to be used only with the run_process_parallel() API, so it is doubly dubious why these three *_fn and callback state are not members.

So, I dunno.  

Either

 (1) making it to very clear that this is only about trace2 and name
     the type as such, or

 (2) making it about run_process_parallel (and keep the name), and
     move the *_fn parameters to it (which will allow us to add more
optional callbacks if needed),

would make it better (simply by clarifying why we have this extra structure and what it is meant to be used for), but the interface as posted is halfway between the two, and does not look well suited for either purpose, making the reader feel somewhat frustrating.

On Thu, Apr 21, 2022 at 02:25:26PM +0200, Ævar Arnfjörð Bjarmason wrote:

Hm, so now it is possible for a callsite to forget to set these, rather than to grep for "run_processes_parallel" and notice that everybody else is already calling "run_processes_parallel_tr2". There are not any callers of 'run_processes_parallel()' except for a test helper today, so why do we make this seem optional?

If I'm being honest, I'd rather see everything _but_ the trace2 stuff go into an opts struct, and then see the same entry points we have today (run_processes_parallel that takes a struct, run_processes_parallel_tr2 that takes a struct and two tr2 string args). Or, I guess, a single run_processes_parallel() that only takes a struct, does the right thing with the trace args, and entirely removes the run_processes_parallel_tr2 call.

What is the point for this extra layer of indirection? I am confused why we might not just change the arg list for run_processes_parallel_tr2 and call it good.

If the final result was to reduce the number of run_processes_parallel.* functions available I'd be happy to see this change, but as it introduces even more various entry points into run_processes_parallel.* I'm not so sure.

Emily Shaffer <emilyshaffer@google.com> writes:

Yup, it was the impression I had when I saw this patch for the first time.

Extend the tests added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to test stdout in addition to stderr. A subsequent commit will add additional related tests for a new feature, making it obvious how the output of the two compares on both stdout and stderr will make this easier to reason about.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0061-run-command.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

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

OK.

The original cared only about the standard error stream, and it was sensible to name the actual output there "actual" and the correct output "expect" to be compared.

But now we care _both_ the standard output and the standard error streams, so if we call one "out", wouldn't we want to call the other one "err", I wonder? If it makes sense, it still is OK if we are not doing so inside this step or in the series, but then it would make sense to leave ourselves a NEEDSWORK: note to do so later when the tree is quiescent.

Other than that, this one is pretty much boringly OK, and boring is good.

Fix a regression reported[1] in f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook.

As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster:

	$ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook'
	#!/bin/sh

	seq 100000000
	Benchmark 1: ./git hook run seq-hook' in 'origin/master
	  Time (mean ± σ):     787.1 ms ±  13.6 ms    [User: 701.6 ms, System: 534.4 ms]
	  Range (min … max):   773.2 ms … 806.3 ms    10 runs

	Benchmark 2: ./git hook run seq-hook' in 'HEAD~0
	  Time (mean ± σ):     603.4 ms ±   1.6 ms    [User: 573.1 ms, System: 30.3 ms]
	  Range (min … max):   601.0 ms … 606.2 ms    10 runs

	Summary
	  './git hook run seq-hook' in 'HEAD~0' ran
	    1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'

In the preceding commit we removed the "no_stdin=1" and "stdout_to_stderr=1" assignments. This change brings them back as with ".ungroup=1" the run_process_parallel() function doesn't provide them for us implicitly.

As an aside omitting the stdout_to_stderr=1 here would have all tests pass, except those that test "git hook run" itself in t1800-hook.sh. But our tests passing is the result of another test blind spot, as was the case with the regression being fixed here. The "stdout_to_stderr=1" for hooks is long-standing behavior, see e.g. 1d9e8b56fe3 (Split back out update_hook handling in receive-pack, 2007-03-10) and other follow-up commits (running "git log" with "--reverse -p -Gstdout_to_stderr" is a good start).

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c          |  8 +++++++-
 t/t1800-hook.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

On Thu, Apr 21, 2022 at 02:25:31PM +0200, Ævar Arnfjörð Bjarmason wrote:

So here we do set .ungroup based only on the job count - but we do that only for hooks, which means someone else could conceivably come across similar bug in their later use of run_processes_parallel. Is the reason for doing this in the context of the hook library instead of in the context of run_processes_parallel library just because we are not sure if it will break other parallel callers? Or some other reason?

 - Emily

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

Wait.

No hunk in this step updates how pick_next_hook() is called (presumably "with .ungroup=1"), and there is no change to the code that uses the cp structure prepared by pick_next_hook() further, so why does the change in the previous step need to be reverted in this step? Does it mean if we apply patches 1-5 without this step, because of step 5, the contents of cp structure returned by pick_next_hook() is broken? But we clearly saw a claim that these assignments are redundant and unnecessary.

So I am confused as to what change in this step makes these assignments necessary again? There is no removal of assignments to these two members that we used to have in this patch. There is no addition of a new caller that calls pick_next_hook and uses it differently (i.e. the other existing caller(s) made assignments to these two members, which allowed 5/6 to remove the assignments, but if this step adds a different caller that uses the struct without making these assignments, then we do need to add them back).

Either I am reading a wrong patch, or the steps 5 & 6 confused me beyond repair, or perhaps a bit of both?

If [5/6] were not there, and [6/6] was added because [5/6] broke it by forgetting that some caller that already exists after applying [5/6] did not make assignments to these two members (iow, the assignment removed by that step were not redundant and [5/6] was buggy in removing them), then I would understand it, but that does not seem to be the case...

Puzzled and utterly confused I am.

Extend the parallel execution API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to support a mode where the stdout and stderr of the processes isn't captured and output in a deterministic order, instead we'll leave it to the kernel and stdio to sort it out.

This gives the API same functionality as GNU parallel's --ungroup option. As we'll see in a subsequent commit the main reason to want this is to support stdout and stderr being connected to the TTY in the case of jobs=1, demonstrated here with GNU parallel:

	$ parallel --ungroup 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	TTY
	TTY
	$ parallel 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	NTTY
	NTTY

Another is as GNU parallel's documentation notes a potential for optimization. Our results will be a bit different, but in cases where you want to run processes in parallel where the exact order isn't important this can be a lot faster:

	$ hyperfine -r 3 -L o ,--ungroup 'parallel {o} seq ::: 10000000 >/dev/null '
	Benchmark 1: parallel  seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     220.2 ms ±   9.3 ms    [User: 124.9 ms, System: 96.1 ms]
	  Range (min … max):   212.3 ms … 230.5 ms    3 runs

	Benchmark 2: parallel --ungroup seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     154.7 ms ±   0.9 ms    [User: 136.2 ms, System: 25.1 ms]
	  Range (min … max):   153.9 ms … 155.7 ms    3 runs

	Summary
	  'parallel --ungroup seq ::: 10000000 >/dev/null ' ran
	    1.42 ± 0.06 times faster than 'parallel  seq ::: 10000000 >/dev/null '

A large part of the juggling in the API is to make the API safer for its maintenance and consumers alike.

For the maintenance of the API we e.g. avoid malloc()-ing the "pp->pfd", ensuring that SANITIZE=address and other similar tools will catch any unexpected misuse.

For API consumers we take pains to never pass the non-NULL "out" buffer to an API user that provided the "ungroup" option. The resulting code in t/helper/test-run-command.c isn't typical of such a user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c               | 95 ++++++++++++++++++++++++++++---------
 run-command.h               | 32 +++++++++----
 t/helper/test-run-command.c | 31 +++++++++---
 t/t0061-run-command.sh      | 30 ++++++++++++
 4 files changed, 151 insertions(+), 37 deletions(-)

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

Match the style with the above (either with or without SP consistently, I would choose to match existing one if I were doing this myself).

OK, now this makes it clear that the new structure introduced in the first step is about run_process_parallel() and not about trace2, so it would probably make sense to go back to that step and throw these *_fn callbacks and callback state to the structure, too.

OK, we will not poll under ungroup option, so we do not need pfd[] in that case. It would be cleaner to clear pp->pfd = NULL if not done already when ungroup is in effect.

This does not make practical difference _right_ _now_, but as a general code hygiene discipline, it would be more future-proof not to rely on "ungroup" being the _only_ thing that allows us to omit allocating pfd. IOW, conditional allocation of pp->pfd based on ungroup before the loop is perfectly fine, but inside the loop, it would be better to say "if pp->pfd is not there, no matter the reason why pp->pfd is missing, we refrain from filling the array because we are not polling".

		if (!pp->pfd)
			continue;

We may know that the only reason we decided not to poll is with the ungroup bit in the current code, but we do not have to depend on the knowledge. The only thing we need to know, in order to refrain from setting POLLIN/POLLHUP bits, is that we decided that we will not poll, and the decision should be more directly found in pp->pfd than inferring what the ungroup says.

Likewise, the NULLness of pp->pfd should be what matters.

OK, this need to happen only when we are buffering.

OK.

OK, except for .no_stdin bit. Even before the "we started using the parallel running API to drive hooks, losing the direct access to the real standard output from the hooks" regression, we didn't expose the input side to the hooks. run_hook_ve() did set .no_stdin and I do not think we want to change that with "--ungroup". If there is any reason why you needed not to keep .no_stdin bit set in the ungroup mode, deviating from what the code before the regression did, that needs to be explained (but I suspect this was simply a bug in this round of the patch, not a deliberate behaviour change).

Make it a habit to keep ";" semicolon when writing an empty statement, i.e.

			; /* some comments */

This will help when other else/if body becomes shorter and we can lose the {} around here.

I cannot quite shake the feeling that this step is doing so much only because it wants to coax "run in parallel" infrastructure to do what it is not suited to do, i.e. drive hooks that wants to directly face the end-user output channel, and it might make a conceptually cleaner fix to simply revert the root cause, but it is getting late so...

On Thu, Apr 21, 2022 at 02:25:28PM +0200, Ævar Arnfjörð Bjarmason wrote:

Interesting! It is separate from whether there are >1 jobs in the task queue. I like that approach - but I do also think we could set ungroup opportunistically if the task list has only one entry or the job number is 1, no? I'd like to see that, too. I guess that we can't really tell how many tasks are available (because it's a callback, not a list) but setting ungroup if jobs=1 sounds like a reasonable improvement to me.

Otherwise, this patch's code is very straightforward and looks fine.

The tests added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) were redirecting to "actual" both in the body of the hook itself and in the testing code below.

The net result was that the "2>>actual" redirection later in the test wasn't doing anything. Let's have those redirection do what it looks like they're doing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1800-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

And the error didn't affect the outcome of the tests? This is fun.

Nicely spotted.

Amend code added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) top stop setting these two flags. We use the run_process_parallel() API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15), which always sets these in pp_start_one() (in addition to setting .err = -1).

Note that an assert() to check that these values are already what we're setting them to here would fail. That's because in pp_start_one() we'll set these after calling this "get_next_task" callback (which we call pick_next_hook()). But the only case where we weren't setting these just after returning from this function was if we took the "return 0" path here, in which case we wouldn't have set these.

So while this code wasn't wrong, it was entirely redundant. The run_process_parallel() also can't work with a generic "struct child_process", it needs one that's behaving in a way that it expects when it comes to stderr/stdout. So we shouldn't be changing these values, or in this case keeping around code that gives the impression that doing in the general case is OK.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c | 2 --
 1 file changed, 2 deletions(-)

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

[jc: I stopped reviewing at 4/6 in the last batch, thought that it was enough for a reroll and shifted my attention to address other regressions. Let me come back to this topic to finish commenting before a reroll comes.]

"top stop"?  -ECANNOTPARSE.

OK. As long as we set these two fields correctly (i.e. the hooks do not read from the standard input, and stdout_to_stderr is in effect (i.e. dup2(2, 1) is done), by the time we pass this into run_command() API, this step would be a benign no-op. Good.

Not that it is something I would expect to see in a rather urgent post-release regression fix, though.

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

It is surprising that it takes 6 patches that rewrites ~100 lines and adds ~200 new lines, which would need to be treated as a new development with its own risk of regressions (hence going through the normal review cycle, starting out of 'next', and gradually getting merged down), instead of being able to be fast-tracked.

Let's see who comments on the patches first and perhaps people may shoot to gain "works with Git" star by testing it ;-)

Thanks.

On Thu, Apr 21 2022, Junio C Hamano wrote:

Yes, it's unfortunate. Perhaps there's some way I'm missing in coming up with a smaller isolated fix, but I wasn't able to come up with one.

It's not just the pre-commit hook, that's just where the problem was discovered, but the dozen or so other run_hook*() users (not all of whom may be practically impacted).

Rewriting the hook.c API to use another "runner" would probably be the smallest change, but even if that's going to be smaller by line-count I'd think it's much bigger in terms of review time. Most of this series is boilerplate changes, or changes where it's easy to review that no caller of run-command.[ch] that isn't hook.c will be impacted by them...

A re-roll of v1[1]. I believe this addresses all comments on the v1 (but perhaps I missed something). Changes:

 * The run_processes_parallel() now takes only one argument, the new
   "opts" struct which has options, callbacks etc. This will also make
   the subsequent config-based hooks topic less churny (it needs new
   callbacks).

   As a result the whole internal *_tr2() wrapper/static function are
   gone.

 * Replaced checks of "ungroup" with whether we have a NULL or not
   (e.g. for pp->pfd), also for free().

 * Typo/grammar fixes in commit messages.

 * Hopefully the 8/8 is less confusing vis-a-vis
   https://lore.kernel.org/git/xmqqfslva3mx.fsf@gitster.g/; I.e. now
   we only add "stdout_to_stderr".

 * The 01/08 and 04/08 are new: Splitting those out made subsequent
   diffs smaller.

 * Tweaked 5/8 a bit to make the diff smaller.

 * Used "err" and "out", not "actual" and "out" in tests, per Junio's
   suggestion.

Passing CI for this series at: https://github.com/avar/git/actions/runs/2346571047

1. https://lore.kernel.org/git/cover-0.6-00000000000-20220421T122108Z-avarab@gmail.com/

Æ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

 builtin/fetch.c             |  18 +++--
 builtin/submodule--helper.c |  15 ++--
 hook.c                      |  28 +++++---
 run-command.c               | 132 +++++++++++++++++++++++-------------
 run-command.h               |  66 ++++++++++++++----
 submodule.c                 |  18 +++--
 t/helper/test-run-command.c |  65 ++++++++++++------
 t/t0061-run-command.sh      |  55 ++++++++++++---
 t/t1800-hook.sh             |  39 ++++++++++-
 9 files changed, 316 insertions(+), 120 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  26a81eff267 run-command tests: change if/if/... to if/else if/else
1:  8bf71ce63dd ! 2:  5f0a6e9925f run-command API: replace run_processes_parallel_tr2() with opts struct
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    run-command API: replace run_processes_parallel_tr2() with opts struct
    +    run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
     
    -    Add a new "struct run_process_parallel_opts" to cover the trace2
    -    use-case added in ee4512ed481 (trace2: create new combined trace
    -    facility, 2019-02-22). A subsequent commit will add more options, and
    -    having a proliferation of new functions or extra parameters would
    -    result in needless churn.
    +    Add a new "struct run_process_parallel_opts" to replace the growing
    +    run_processes_parallel() and run_processes_parallel_tr2() argument
    +    lists. This refactoring makes it easier to add new options and
    +    parameters easier.
     
    -    It makes for a smaller change to make run_processes_parallel() and
    -    run_processes_parallel_tr2() wrapper functions for the new "static"
    -    run_processes_parallel_1(), which contains the main logic. We pass
    -    down "opts" to the *_1() function even though it isn't used there
    -    yet (only in the *_tr2() function), a subsequent commit will make more
    -    use of it.
    +    The *_tr2() variant of the function was added in ee4512ed481 (trace2:
    +    create new combined trace facility, 2019-02-22), and has subsequently
    +    been used by every caller except t/helper/test-run-command.c.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
     +		struct run_process_parallel_opts run_opts = {
     +			.tr2_category = "fetch",
     +			.tr2_label = "parallel/fetch",
    ++
    ++			.jobs = max_children,
    ++
    ++			.get_next_task = &fetch_next_remote,
    ++			.start_failure = &fetch_failed_to_start,
    ++			.task_finished = &fetch_finished,
    ++			.data = &state,
     +		};
      
      		strvec_push(&argv, "--end-of-options");
    @@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
     -						    &fetch_finished,
     -						    &state,
     -						    "fetch", "parallel/fetch");
    -+		result = run_processes_parallel(max_children,
    -+						&fetch_next_remote,
    -+						&fetch_failed_to_start,
    -+						&fetch_finished, &state,
    -+						&run_opts);
    ++		result = run_processes_parallel(&run_opts);
      
      		if (!result)
      			result = state.result;
    @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
     +	struct run_process_parallel_opts run_opts = {
     +		.tr2_category = "submodule",
     +		.tr2_label = "parallel/update",
    ++
    ++		.get_next_task = update_clone_get_next_task,
    ++		.start_failure = update_clone_start_failure,
    ++		.task_finished = update_clone_task_finished,
    ++		.data = &suc,
     +	};
      
      	suc.update_data = update_data;
    @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
     -				   update_clone_start_failure,
     -				   update_clone_task_finished, &suc, "submodule",
     -				   "parallel/update");
    -+	run_processes_parallel(suc.update_data->max_jobs,
    -+			       update_clone_get_next_task,
    -+			       update_clone_start_failure,
    -+			       update_clone_task_finished, &suc, &run_opts);
    ++	run_opts.jobs = suc.update_data->max_jobs;
    ++	run_processes_parallel(&run_opts);
      
      	/*
      	 * We saved the output and put it out all at once now.
     
      ## hook.c ##
     @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
    + 		.options = options,
    + 	};
      	const char *const hook_path = find_hook(hook_name);
    - 	int jobs = 1;
    +-	int jobs = 1;
    ++	const int jobs = 1;
      	int ret = 0;
     +	struct run_process_parallel_opts run_opts = {
     +		.tr2_category = "hook",
     +		.tr2_label = hook_name,
    ++
    ++		.jobs = jobs,
    ++
    ++		.get_next_task = pick_next_hook,
    ++		.start_failure = notify_start_failure,
    ++		.task_finished = notify_hook_finished,
    ++		.data = &cb_data,
     +	};
      
      	if (!options)
    @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
     -				   &cb_data,
     -				   "hook",
     -				   hook_name);
    -+	run_processes_parallel(jobs, pick_next_hook, notify_start_failure,
    -+			       notify_hook_finished, &cb_data, &run_opts);
    ++	run_processes_parallel(&run_opts);
      	ret = cb_data.rc;
      cleanup:
      	strbuf_release(&abs_path);
     
      ## run-command.c ##
    +@@ run-command.c: static void handle_children_on_signal(int signo)
    + }
    + 
    + static void pp_init(struct parallel_processes *pp,
    +-		    int n,
    +-		    get_next_task_fn get_next_task,
    +-		    start_failure_fn start_failure,
    +-		    task_finished_fn task_finished,
    +-		    void *data)
    ++		    struct run_process_parallel_opts *opts)
    + {
    + 	int i;
    ++	int n = opts->jobs;
    ++	void *data = opts->data;
    ++	get_next_task_fn get_next_task = opts->get_next_task;
    ++	start_failure_fn start_failure = opts->start_failure;
    ++	task_finished_fn task_finished = opts->task_finished;
    + 
    + 	if (n < 1)
    + 		n = online_cpus();
     @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      	return result;
      }
    @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
     -			   start_failure_fn start_failure,
     -			   task_finished_fn task_finished,
     -			   void *pp_cb)
    -+static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
    -+				    start_failure_fn start_failure,
    -+				    task_finished_fn task_finished,
    -+				    void *pp_cb,
    -+				    struct run_process_parallel_opts *opts)
    ++int run_processes_parallel(struct run_process_parallel_opts *opts)
      {
      	int i, code;
      	int output_timeout = 100;
    + 	int spawn_cap = 4;
    + 	struct parallel_processes pp;
    ++	const char *tr2_category = opts->tr2_category;
    ++	const char *tr2_label = opts->tr2_label;
    ++	const int do_trace2 = tr2_category && tr2_label;
    ++	const int n = opts->jobs;
    + 
    +-	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
    ++	if (do_trace2)
    ++		trace2_region_enter_printf(tr2_category, tr2_label, NULL,
    ++					   "max:%d", ((n < 1) ? online_cpus()
    ++						      : n));
    ++
    ++	pp_init(&pp, opts);
    + 	while (1) {
    + 		for (i = 0;
    + 		    i < spawn_cap && !pp.shutdown &&
     @@ run-command.c: int run_processes_parallel(int n,
    - 	return 0;
    - }
    + 	}
      
    + 	pp_cleanup(&pp);
    +-	return 0;
    +-}
    +-
     -int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
     -			       start_failure_fn start_failure,
     -			       task_finished_fn task_finished, void *pp_cb,
     -			       const char *tr2_category, const char *tr2_label)
    -+static int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
    -+				      start_failure_fn start_failure,
    -+				      task_finished_fn task_finished,
    -+				      void *pp_cb,
    -+				      struct run_process_parallel_opts *opts)
    - {
    -+	const char *tr2_category = opts->tr2_category;
    -+	const char *tr2_label = opts->tr2_label;
    - 	int result;
    +-{
    +-	int result;
      
    - 	trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
    - 				   ((n < 1) ? online_cpus() : n));
    +-	trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
    +-				   ((n < 1) ? online_cpus() : n));
    ++	if (do_trace2)
    ++		trace2_region_leave(tr2_category, tr2_label, NULL);
      
     -	result = run_processes_parallel(n, get_next_task, start_failure,
     -					task_finished, pp_cb);
    -+	result = run_processes_parallel_1(n, get_next_task, start_failure,
    -+					  task_finished, pp_cb, opts);
    - 
    - 	trace2_region_leave(tr2_category, tr2_label, NULL);
    - 
    - 	return result;
    +-
    +-	trace2_region_leave(tr2_category, tr2_label, NULL);
    +-
    +-	return result;
    ++	return 0;
      }
      
    -+int run_processes_parallel(int n, get_next_task_fn get_next_task,
    -+			   start_failure_fn start_failure,
    -+			   task_finished_fn task_finished, void *pp_cb,
    -+			   struct run_process_parallel_opts *opts)
    -+{
    -+	if (opts->tr2_category && opts->tr2_label)
    -+		return run_processes_parallel_tr2(n, get_next_task,
    -+						  start_failure, task_finished,
    -+						  pp_cb, opts);
    -+
    -+	return run_processes_parallel_1(n, get_next_task, start_failure,
    -+					task_finished, pp_cb, opts);
    -+}
    -+
    -+
      int run_auto_maintenance(int quiet)
    - {
    - 	int enabled;
     
      ## run-command.h ##
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
    - 				void *pp_cb,
      				void *pp_task_cb);
      
    -+/**
    + /**
     + * Options to pass to run_processes_parallel(), { 0 }-initialized
     + * means no options. Fields:
     + *
     + * tr2_category & tr2_label: sets the trace2 category and label for
     + * logging. These must either be unset, or both of them must be set.
    ++ *
    ++ * jobs: see 'n' in run_processes_parallel() below.
    ++ *
    ++ * *_fn & data: see run_processes_parallel() below.
     + */
     +struct run_process_parallel_opts
     +{
     +	const char *tr2_category;
     +	const char *tr2_label;
    ++
    ++	int jobs;
    ++
    ++	get_next_task_fn get_next_task;
    ++	start_failure_fn start_failure;
    ++	task_finished_fn task_finished;
    ++	void *data;
     +};
     +
    - /**
    ++/**
    ++ * Options are passed via the "struct run_process_parallel_opts" above.
    ++
       * Runs up to n processes at the same time. Whenever a process can be
       * started, the callback get_next_task_fn is called to obtain the data
    +  * required to start another child process.
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
    -  *
       * start_failure_fn and task_finished_fn can be NULL to omit any
       * special handling.
    -+ *
    -+ * Options are passed via a "struct run_process_parallel_opts".
       */
     -int run_processes_parallel(int n,
     -			   get_next_task_fn,
    @@ run-command.h: typedef int (*task_finished_fn)(int result,
     -int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
     -			       task_finished_fn, void *pp_cb,
     -			       const char *tr2_category, const char *tr2_label);
    -+int run_processes_parallel(int n, get_next_task_fn, start_failure_fn,
    -+			   task_finished_fn, void *pp_cb,
    -+			   struct run_process_parallel_opts *opts);
    ++int run_processes_parallel(struct run_process_parallel_opts *opts);
      
      /**
       * Convenience function which prepares env_array for a command to be run in a
    @@ submodule.c: int fetch_submodules(struct repository *r,
     +	struct run_process_parallel_opts run_opts = {
     +		.tr2_category = "submodule",
     +		.tr2_label = "parallel/fetch",
    ++
    ++		.jobs = max_parallel_jobs,
    ++
    ++		.get_next_task = get_next_submodule,
    ++		.start_failure = fetch_start_failure,
    ++		.task_finished = fetch_finish,
    ++		.data = &spf,
     +	};
      
      	spf.r = r;
    @@ submodule.c: int fetch_submodules(struct repository *r,
     -				   fetch_finish,
     -				   &spf,
     -				   "submodule", "parallel/fetch");
    -+	run_processes_parallel(max_parallel_jobs, get_next_submodule,
    -+			       fetch_start_failure, fetch_finish, &spf,
    -+			       &run_opts);
    ++	run_processes_parallel(&run_opts);
      
      	if (spf.submodules_with_errors.len > 0)
      		fprintf(stderr, _("Errors during submodule fetch:\n%s"),
     
      ## t/helper/test-run-command.c ##
     @@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
    + 			 "write JUnit-style XML files"),
    + 		OPT_END()
    + 	};
    ++	struct run_process_parallel_opts run_opts = {
    ++		.get_next_task = next_test,
    ++		.start_failure = test_failed,
    ++		.task_finished = test_finished,
    ++		.data = &suite,
    ++	};
    + 
    + 	argc = parse_options(argc, argv, NULL, options,
    + 			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
    +@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
    + 
    + 	fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
      		(uintmax_t)suite.tests.nr, max_jobs);
    ++	run_opts.jobs = max_jobs;
      
    - 	ret = run_processes_parallel(max_jobs, next_test, test_failed,
    +-	ret = run_processes_parallel(max_jobs, next_test, test_failed,
     -				     test_finished, &suite);
    -+				     test_finished, &suite, NULL);
    ++	ret = run_processes_parallel(&run_opts);
      
      	if (suite.failed.nr > 0) {
      		ret = 1;
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    - {
    - 	struct child_process proc = CHILD_PROCESS_INIT;
      	int jobs;
    + 	get_next_task_fn next_fn = NULL;
    + 	task_finished_fn finished_fn = NULL;
     +	struct run_process_parallel_opts opts = { 0 };
      
      	if (argc > 1 && !strcmp(argv[1], "testsuite"))
      		exit(testsuite(argc - 1, argv + 1));
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    + 	jobs = atoi(argv[2]);
    + 	strvec_clear(&proc.args);
    + 	strvec_pushv(&proc.args, (const char **)argv + 3);
    ++	opts.jobs = jobs;
    ++	opts.data = &proc;
      
    - 	if (!strcmp(argv[1], "run-command-parallel"))
    - 		exit(run_processes_parallel(jobs, parallel_next,
    --					    NULL, NULL, &proc));
    -+					    NULL, NULL, &proc, &opts));
    - 
    - 	if (!strcmp(argv[1], "run-command-abort"))
    --		exit(run_processes_parallel(jobs, parallel_next,
    --					    NULL, task_finished, &proc));
    -+		exit(run_processes_parallel(jobs, parallel_next, NULL,
    -+					    task_finished, &proc, &opts));
    - 
    - 	if (!strcmp(argv[1], "run-command-no-jobs"))
    --		exit(run_processes_parallel(jobs, no_job,
    --					    NULL, task_finished, &proc));
    -+		exit(run_processes_parallel(jobs, no_job, NULL, task_finished,
    -+					    &proc, &opts));
    + 	if (!strcmp(argv[1], "run-command-parallel")) {
    + 		next_fn = parallel_next;
    +@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    + 		return 1;
    + 	}
      
    - 	fprintf(stderr, "check usage\n");
    - 	return 1;
    +-	exit(run_processes_parallel(jobs, next_fn, NULL, finished_fn, &proc));
    ++	opts.get_next_task = next_fn;
    ++	opts.task_finished = finished_fn;
    ++	exit(run_processes_parallel(&opts));
    + }
2:  d9c9b158130 ! 3:  a8e1fc07b65 run-command tests: test stdout of run_command_parallel()
    @@ t/t0061-run-command.sh: World
      
      test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
     -	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    -+	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect err
      '
      
      test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
     -	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    -+	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect err
      '
      
      test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
     -	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    -+	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect err
      '
      
    + cat >expect <<-EOF
     @@ t/t0061-run-command.sh: asking for a quick stop
      EOF
      
      test_expect_success 'run_command is asked to abort gracefully' '
     -	test-tool run-command run-command-abort 3 false 2>actual &&
    -+	test-tool run-command run-command-abort 3 false >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-abort 3 false >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect err
      '
      
    + cat >expect <<-EOF
     @@ t/t0061-run-command.sh: no further jobs available
      EOF
      
      test_expect_success 'run_command outputs ' '
     -	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    -+	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect err
      '
      
    + test_trace () {
-:  ----------- > 4:  663936fb4ad run-command.c: add an initializer for "struct parallel_processes"
3:  d76f63c2948 ! 5:  c2e015ed840 run-command: add an "ungroup" option to run_process_parallel()
    @@ run-command.c: struct parallel_processes {
      	struct pollfd *pfd;
      
      	unsigned shutdown : 1;
    -+	unsigned ungroup:1;
    ++	unsigned ungroup : 1;
      
      	int output_owner;
      	struct strbuf buffered_output; /* of finished children */
     @@ run-command.c: static void pp_init(struct parallel_processes *pp,
    - 		    get_next_task_fn get_next_task,
    - 		    start_failure_fn start_failure,
    - 		    task_finished_fn task_finished,
    --		    void *data)
    -+		    void *data, struct run_process_parallel_opts *opts)
    - {
    -+	const int ungroup = opts->ungroup;
    - 	int i;
    - 
    - 	if (n < 1)
    -@@ run-command.c: static void pp_init(struct parallel_processes *pp,
    - 	pp->start_failure = start_failure ? start_failure : default_start_failure;
    - 	pp->task_finished = task_finished ? task_finished : default_task_finished;
    - 
    -+	pp->ungroup = ungroup;
    -+
      	pp->nr_processes = 0;
      	pp->output_owner = 0;
      	pp->shutdown = 0;
    ++	pp->ungroup = opts->ungroup;
      	CALLOC_ARRAY(pp->children, n);
     -	CALLOC_ARRAY(pp->pfd, n);
    -+	if (!ungroup)
    ++	if (!pp->ungroup)
     +		CALLOC_ARRAY(pp->pfd, n);
    -+
    - 	strbuf_init(&pp->buffered_output, 0);
      
      	for (i = 0; i < n; i++) {
      		strbuf_init(&pp->children[i].err, 0);
      		child_process_init(&pp->children[i].process);
    -+		if (ungroup)
    ++		if (!pp->pfd)
     +			continue;
      		pp->pfd[i].events = POLLIN | POLLHUP;
      		pp->pfd[i].fd = -1;
      	}
    -@@ run-command.c: static void pp_init(struct parallel_processes *pp,
    - 
    - static void pp_cleanup(struct parallel_processes *pp)
    - {
    -+	const int ungroup = pp->ungroup;
    - 	int i;
    - 
    - 	trace_printf("run_processes_parallel: done");
     @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
    - 	}
    - 
    - 	free(pp->children);
    --	free(pp->pfd);
    -+	if (!ungroup)
    -+		free(pp->pfd);
    - 
    - 	/*
      	 * When get_next_task added messages to the buffer in its last
      	 * iteration, the buffered output is non empty.
      	 */
     -	strbuf_write(&pp->buffered_output, stderr);
    --	strbuf_release(&pp->buffered_output);
    -+	if (!ungroup) {
    ++	if (!pp->ungroup)
     +		strbuf_write(&pp->buffered_output, stderr);
    -+		strbuf_release(&pp->buffered_output);
    -+	}
    + 	strbuf_release(&pp->buffered_output);
      
      	sigchain_pop_common();
    - }
     @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
       */
      static int pp_start_one(struct parallel_processes *pp)
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      	}
     -	pp->children[i].process.err = -1;
     -	pp->children[i].process.stdout_to_stderr = 1;
    --	pp->children[i].process.no_stdin = 1;
    -+
     +	if (!ungroup) {
     +		pp->children[i].process.err = -1;
     +		pp->children[i].process.stdout_to_stderr = 1;
    -+		pp->children[i].process.no_stdin = 1;
     +	}
    + 	pp->children[i].process.no_stdin = 1;
      
      	if (start_command(&pp->children[i].process)) {
     -		code = pp->start_failure(&pp->children[i].err,
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      	pp->nr_processes++;
      	pp->children[i].state = GIT_CP_WORKING;
     -	pp->pfd[i].fd = pp->children[i].process.err;
    -+	if (!ungroup)
    ++	if (pp->pfd)
     +		pp->pfd[i].fd = pp->children[i].process.err;
      	return 0;
      }
    @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      		pp->nr_processes--;
      		pp->children[i].state = GIT_CP_FREE;
     -		pp->pfd[i].fd = -1;
    -+		if (!ungroup)
    ++		if (pp->pfd)
     +			pp->pfd[i].fd = -1;
      		child_process_init(&pp->children[i].process);
      
     -		if (i != pp->output_owner) {
     +		if (ungroup) {
    -+			/* no strbuf_*() work to do here */
    ++			; /* no strbuf_*() work to do here */
     +		} else if (i != pp->output_owner) {
      			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
      			strbuf_reset(&pp->children[i].err);
      		} else {
    -@@ run-command.c: static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
    - 				    void *pp_cb,
    - 				    struct run_process_parallel_opts *opts)
    - {
    -+	const int ungroup = opts->ungroup;
    - 	int i, code;
    - 	int output_timeout = 100;
    - 	int spawn_cap = 4;
    - 	struct parallel_processes pp;
    - 
    --	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
    -+	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
    -+		opts);
    - 	while (1) {
    - 		for (i = 0;
    - 		    i < spawn_cap && !pp.shutdown &&
    -@@ run-command.c: static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
    +@@ run-command.c: int run_processes_parallel(struct run_process_parallel_opts *opts)
      		}
      		if (!pp.nr_processes)
      			break;
     -		pp_buffer_stderr(&pp, output_timeout);
     -		pp_output(&pp);
    -+		if (ungroup) {
    ++		if (opts->ungroup) {
     +			pp_mark_working_for_cleanup(&pp);
     +		} else {
     +			pp_buffer_stderr(&pp, output_timeout);
    @@ run-command.h: typedef int (*start_failure_fn)(struct strbuf *out,
       * pp_task_cb is the callback cookie as passed into get_next_task_fn.
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
       *
    -  * tr2_category & tr2_label: sets the trace2 category and label for
    -  * logging. These must either be unset, or both of them must be set.
    -+ *
    +  * jobs: see 'n' in run_processes_parallel() below.
    +  *
     + * ungroup: Ungroup output. Output is printed as soon as possible and
     + * bypasses run-command's internal processing. This may cause output
     + * from different commands to be mixed.
    ++ *
    +  * *_fn & data: see run_processes_parallel() below.
       */
      struct run_process_parallel_opts
    - {
    - 	const char *tr2_category;
    +@@ run-command.h: struct run_process_parallel_opts
      	const char *tr2_label;
    + 
    + 	int jobs;
     +	unsigned int ungroup:1;
    - };
      
    - /**
    + 	get_next_task_fn get_next_task;
    + 	start_failure_fn start_failure;
     @@ run-command.h: struct run_process_parallel_opts
       *
       * The children started via this function run in parallel. Their output
    @@ run-command.h: struct run_process_parallel_opts
       *
       * start_failure_fn and task_finished_fn can be NULL to omit any
       * special handling.
    -  *
    -- * Options are passed via a "struct run_process_parallel_opts".
    -+ * Options are passed via a "struct run_process_parallel_opts". If the
    -+ * "ungroup" option isn't specified the callbacks will get a pointer
    -+ * to a "struct strbuf *out", and must not write to stdout or stderr
    -+ * as such output will mess up the output of the other parallel
    ++ *
    ++ * If the "ungroup" option isn't specified the callbacks will get a
    ++ * pointer to a "struct strbuf *out", and must not write to stdout or
    ++ * stderr as such output will mess up the output of the other parallel
     + * processes. If "ungroup" option is specified callbacks will get a
     + * NULL "struct strbuf *out" parameter, and are responsible for
     + * emitting their own output, including dealing with any race
     + * conditions due to writing in parallel to stdout and stderr.
       */
    - int run_processes_parallel(int n, get_next_task_fn, start_failure_fn,
    - 			   task_finished_fn, void *pp_cb,
    + int run_processes_parallel(struct run_process_parallel_opts *opts);
    + 
     
      ## t/helper/test-run-command.c ##
     @@ t/helper/test-run-command.c: static int parallel_next(struct child_process *cp,
    @@ t/helper/test-run-command.c: static int task_finished(int result,
      }
      
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    - 	strvec_clear(&proc.args);
    - 	strvec_pushv(&proc.args, (const char **)argv + 3);
    + 	opts.jobs = jobs;
    + 	opts.data = &proc;
      
    --	if (!strcmp(argv[1], "run-command-parallel"))
    +-	if (!strcmp(argv[1], "run-command-parallel")) {
     +	if (!strcmp(argv[1], "run-command-parallel") ||
     +	    !strcmp(argv[1], "run-command-parallel-ungroup")) {
    -+		opts.ungroup = !strcmp(argv[1], "run-command-parallel-ungroup");
    - 		exit(run_processes_parallel(jobs, parallel_next,
    - 					    NULL, NULL, &proc, &opts));
    -+	}
    - 
    --	if (!strcmp(argv[1], "run-command-abort"))
    -+	if (!strcmp(argv[1], "run-command-abort") ||
    -+	    !strcmp(argv[1], "run-command-abort-ungroup")) {
    -+		opts.ungroup = !strcmp(argv[1], "run-command-abort-ungroup");
    - 		exit(run_processes_parallel(jobs, parallel_next, NULL,
    - 					    task_finished, &proc, &opts));
    -+	}
    - 
    --	if (!strcmp(argv[1], "run-command-no-jobs"))
    -+	if (!strcmp(argv[1], "run-command-no-jobs") ||
    -+	    !strcmp(argv[1], "run-command-no-jobs-ungroup")) {
    -+		opts.ungroup = !strcmp(argv[1], "run-command-no-jobs-ungroup");
    - 		exit(run_processes_parallel(jobs, no_job, NULL, task_finished,
    - 					    &proc, &opts));
    -+	}
    + 		next_fn = parallel_next;
    +-	} else if (!strcmp(argv[1], "run-command-abort")) {
    ++	} else if (!strcmp(argv[1], "run-command-abort") ||
    ++		   !strcmp(argv[1], "run-command-abort-ungroup")) {
    + 		next_fn = parallel_next;
    + 		finished_fn = task_finished;
    +-	} else if (!strcmp(argv[1], "run-command-no-jobs")) {
    ++	} else if (!strcmp(argv[1], "run-command-no-jobs") ||
    ++		   !strcmp(argv[1], "run-command-no-jobs-ungroup")) {
    + 		next_fn = no_job;
    + 		finished_fn = task_finished;
    + 	} else {
    +@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    + 		return 1;
    + 	}
      
    - 	fprintf(stderr, "check usage\n");
    - 	return 1;
    ++	opts.ungroup = ends_with(argv[1], "-ungroup");
    + 	opts.get_next_task = next_fn;
    + 	opts.task_finished = finished_fn;
    + 	exit(run_processes_parallel(&opts));
     
      ## t/t0061-run-command.sh ##
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with more jobs available than
    - 	test_cmp expect actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
     +'
     +
      test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
    - 	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    + 	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
      	test_must_be_empty out &&
    - 	test_cmp expect actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
     +'
     +
      test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
    - 	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    + 	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
      	test_must_be_empty out &&
    - 	test_cmp expect actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
      preloaded output of a child
      asking for a quick stop
     @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort gracefully' '
    - 	test_cmp expect actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
    @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort grace
      no further jobs available
      EOF
     @@ t/t0061-run-command.sh: test_expect_success 'run_command outputs ' '
    - 	test_cmp expect actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command outputs (ungroup) ' '
    -+	test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    ++	test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    -+	test_cmp expect actual
    ++	test_cmp expect err
     +'
     +
      test_trace () {
4:  cf62569b2e0 = 6:  84e92c6f7c7 hook tests: fix redirection logic error in 96e7225b310
5:  98c26c9917b ! 7:  bf7d871565f hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
    @@ Commit message
         hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
     
         Amend code added in 96e7225b310 (hook: add 'run' subcommand,
    -    2021-12-22) top stop setting these two flags. We use the
    +    2021-12-22) to stop setting these two flags. We use the
         run_process_parallel() API added in c553c72eed6 (run-command: add an
         asynchronous parallel child processor, 2015-12-15), which always sets
         these in pp_start_one() (in addition to setting .err = -1).
6:  de3664f6d2b ! 8:  238155fcb9d hook API: fix v2.36.0 regression: hooks should be connected to a TTY
    @@ Commit message
                   './git hook run seq-hook' in 'HEAD~0' ran
                     1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'
     
    -    In the preceding commit we removed the "no_stdin=1" and
    -    "stdout_to_stderr=1" assignments. This change brings them back as with
    -    ".ungroup=1" the run_process_parallel() function doesn't provide them
    -    for us implicitly.
    +    In the preceding commit we removed the "stdout_to_stderr=1" assignment
    +    as being redundant. This change brings it back as with ".ungroup=1"
    +    the run_process_parallel() function doesn't provide them for us
    +    implicitly.
     
         As an aside omitting the stdout_to_stderr=1 here would have all tests
         pass, except those that test "git hook run" itself in
    @@ Commit message
     
      ## hook.c ##
     @@ hook.c: static int pick_next_hook(struct child_process *cp,
    - 	if (!hook_path)
      		return 0;
      
    -+	cp->no_stdin = 1;
      	strvec_pushv(&cp->env_array, hook_cb->options->env.v);
    -+	cp->stdout_to_stderr = 1;
    ++	cp->stdout_to_stderr = 1; /* because of .ungroup = 1 */
      	cp->trace2_hook_name = hook_cb->hook_name;
      	cp->dir = hook_cb->options->dir;
      
     @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
    - 		.options = options,
    - 	};
    - 	const char *const hook_path = find_hook(hook_name);
    --	int jobs = 1;
    -+	const int jobs = 1;
    - 	int ret = 0;
    - 	struct run_process_parallel_opts run_opts = {
    - 		.tr2_category = "hook",
      		.tr2_label = hook_name,
    + 
    + 		.jobs = jobs,
     +		.ungroup = jobs == 1,
    - 	};
      
    + 		.get_next_task = pick_next_hook,
    + 		.start_failure = notify_start_failure,
    +@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
      	if (!options)
      		BUG("a struct run_hooks_opt must be provided to run_hooks");
      

Refactor the code in cmd__run_command() to make a subsequent changes smaller by reducing duplication.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-run-command.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

Add a new "struct run_process_parallel_opts" to replace the growing run_processes_parallel() and run_processes_parallel_tr2() argument lists. This refactoring makes it easier to add new options and parameters easier.

The *_tr2() variant of the function was added in ee4512ed481 (trace2: create new combined trace facility, 2019-02-22), and has subsequently been used by every caller except t/helper/test-run-command.c.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c             | 18 +++++++++-----
 builtin/submodule--helper.c | 15 ++++++++----
 hook.c                      | 21 +++++++++-------
 run-command.c               | 48 ++++++++++++++++---------------------
 run-command.h               | 35 ++++++++++++++++++++-------
 submodule.c                 | 18 +++++++++-----
 t/helper/test-run-command.c | 17 ++++++++++---
 7 files changed, 109 insertions(+), 63 deletions(-)

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

;-)

Can't tell if this is going overboard, but it probably is better than piling more parameter on top of existing ones.

I would have written "unset" -> "set to NULL" if I were writing this, but it should hopefully be obvious from the context, so it is OK.

OK.

Beyond the post context follows this text.

    * The children started via this function run in parallel. Their output
    * (both stdout and stderr) is routed to stderr in a manner that output
    * from different tasks does not interleave.
    *
    * start_failure_fn and task_finished_fn can be NULL to omit any
    * special handling.
    */
   int run_processes_parallel(struct run_process_parallel_opts *opts);

The forward reference of 'n' we saw earlier does have matching 'n' here, but the 'n' no longer exists, so it probably is a good idea to rewrite the comment before this function.

    Runs up to opts->jobs processes at the time.  Whenever a process
    can be started, the callback opts->get_next_task_fn is called to
    obtain the data required to start another child process. ...

The forward reference of 'data' we saw earlier does not have any matching description here (it is a flaw in the original and not the problem with this patch). The description of get_next_task_fn that appears much earlier in this file talks about two "callback cookies", pp_cb and pp_task_cb, but it is unclear how opts->data (after this patch) relates to either of these two. Presumably the calling convention around the "callback cookie" is the same across get_next_task_fn, start_failure_fn, and task_finished_fn? If so, perhaps this is a good place to describe how opts->data is fed into them.

On Wed, May 18, 2022 at 10:05:18PM +0200, Ævar Arnfjörð Bjarmason wrote:

[...]

I see this comment...

[moved snippet]

...but it's not actually very well enforced here. That is, it seems I can set one or the other but not both, and nothing bad will happen, except that I am wasting my time setting one. If you want to enforce them both to be set, then why not use a BUG()? But otherwise the comment could be reworded, I think.

[/moved snippet]

Otherwise, although the number of lines of code is often higher, I find the named initializers in the struct much easier to read at the callsites, so I like this change.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

Extend the tests added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to test stdout in addition to stderr. A subsequent commit will add additional related tests for a new feature, making it obvious how the output of the two compares on both stdout and stderr will make this easier to reason about.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0061-run-command.sh | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

Add a PARALLEL_PROCESSES_INIT macro for "struct parallel_processes", this allows us to do away with a call to strbuf_init(), in subsequent commits we'll be able to rely on other fields being NULL'd.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Amend code added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) to stop setting these two flags. We use the run_process_parallel() API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15), which always sets these in pp_start_one() (in addition to setting .err = -1).

Note that an assert() to check that these values are already what we're setting them to here would fail. That's because in pp_start_one() we'll set these after calling this "get_next_task" callback (which we call pick_next_hook()). But the only case where we weren't setting these just after returning from this function was if we took the "return 0" path here, in which case we wouldn't have set these.

So while this code wasn't wrong, it was entirely redundant. The run_process_parallel() also can't work with a generic "struct child_process", it needs one that's behaving in a way that it expects when it comes to stderr/stdout. So we shouldn't be changing these values, or in this case keeping around code that gives the impression that doing in the general case is OK.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c | 2 --
 1 file changed, 2 deletions(-)

The tests added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) were redirecting to "actual" both in the body of the hook itself and in the testing code below.

The net result was that the "2>>actual" redirection later in the test wasn't doing anything. Let's have those redirection do what it looks like they're doing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1800-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Fix a regression reported[1] in f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook.

As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster:

	$ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook'
	#!/bin/sh

	seq 100000000
	Benchmark 1: ./git hook run seq-hook' in 'origin/master
	  Time (mean ± σ):     787.1 ms ±  13.6 ms    [User: 701.6 ms, System: 534.4 ms]
	  Range (min … max):   773.2 ms … 806.3 ms    10 runs

	Benchmark 2: ./git hook run seq-hook' in 'HEAD~0
	  Time (mean ± σ):     603.4 ms ±   1.6 ms    [User: 573.1 ms, System: 30.3 ms]
	  Range (min … max):   601.0 ms … 606.2 ms    10 runs

	Summary
	  './git hook run seq-hook' in 'HEAD~0' ran
	    1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'

In the preceding commit we removed the "stdout_to_stderr=1" assignment as being redundant. This change brings it back as with ".ungroup=1" the run_process_parallel() function doesn't provide them for us implicitly.

As an aside omitting the stdout_to_stderr=1 here would have all tests pass, except those that test "git hook run" itself in t1800-hook.sh. But our tests passing is the result of another test blind spot, as was the case with the regression being fixed here. The "stdout_to_stderr=1" for hooks is long-standing behavior, see e.g. 1d9e8b56fe3 (Split back out update_hook handling in receive-pack, 2007-03-10) and other follow-up commits (running "git log" with "--reverse -p -Gstdout_to_stderr" is a good start).

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c          |  5 +++++
 t/t1800-hook.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

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

This part I recall commenting on the earlier round. The above message and the change in the patch makes sense.

Thanks.

On Wed, May 18, 2022 at 10:05:24PM +0200, Ævar Arnfjörð Bjarmason wrote:

I mentioned it on patch 5, but I actually do not see a reason why we shouldn't do this logic in run_processes_parallel instead of just for the hooks. If someone can mention a reason we want to buffer child processes we're running in series I'm all ears, of course.

Doesn't this mean we're actually disallowing parallel hooks entirely? I don't think that's necessary or desired. I guess right now when the config isn't used, there's not really a way to provide parallel hooks, but I also think this will cause unnecessary conflicts for Google who is carrying config hooks downstream. I know that's not such a great reason. But it seems weird to be explicitly using the parallel processing framework, but then say, "oh, but we actually don't want to run in parallel, that's a BUG()".

On Thu, May 26 2022, Emily Shaffer wrote:

I can just drop this paranoia. I figured it was prudent to leave this landmine in place so we'd definitely remember to re-visit this aspect of it, but I think there's 0% that we'll forget. So I'll make it less paranoid.

On Thu, May 26, 2022 at 08:23:23PM +0200, Ævar Arnfjörð Bjarmason wrote:

Thanks. With that change the series looks good to me otherwise, although if you're rerolling to drop it, maybe consider some of the other little nits I left elsewhere. ;)

 - Emily

Extend the parallel execution API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to support a mode where the stdout and stderr of the processes isn't captured and output in a deterministic order, instead we'll leave it to the kernel and stdio to sort it out.

This gives the API same functionality as GNU parallel's --ungroup option. As we'll see in a subsequent commit the main reason to want this is to support stdout and stderr being connected to the TTY in the case of jobs=1, demonstrated here with GNU parallel:

	$ parallel --ungroup 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	TTY
	TTY
	$ parallel 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	NTTY
	NTTY

Another is as GNU parallel's documentation notes a potential for optimization. Our results will be a bit different, but in cases where you want to run processes in parallel where the exact order isn't important this can be a lot faster:

	$ hyperfine -r 3 -L o ,--ungroup 'parallel {o} seq ::: 10000000 >/dev/null '
	Benchmark 1: parallel  seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     220.2 ms ±   9.3 ms    [User: 124.9 ms, System: 96.1 ms]
	  Range (min … max):   212.3 ms … 230.5 ms    3 runs

	Benchmark 2: parallel --ungroup seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     154.7 ms ±   0.9 ms    [User: 136.2 ms, System: 25.1 ms]
	  Range (min … max):   153.9 ms … 155.7 ms    3 runs

	Summary
	  'parallel --ungroup seq ::: 10000000 >/dev/null ' ran
	    1.42 ± 0.06 times faster than 'parallel  seq ::: 10000000 >/dev/null '

A large part of the juggling in the API is to make the API safer for its maintenance and consumers alike.

For the maintenance of the API we e.g. avoid malloc()-ing the "pp->pfd", ensuring that SANITIZE=address and other similar tools will catch any unexpected misuse.

For API consumers we take pains to never pass the non-NULL "out" buffer to an API user that provided the "ungroup" option. The resulting code in t/helper/test-run-command.c isn't typical of such a user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c               | 76 ++++++++++++++++++++++++++++---------
 run-command.h               | 31 +++++++++++----
 t/helper/test-run-command.c | 26 ++++++++++---
 t/t0061-run-command.sh      | 30 +++++++++++++++
 4 files changed, 132 insertions(+), 31 deletions(-)

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

This illustrates the intended use of !err and !!err pretty well ;-).

On Wed, May 18, 2022 at 10:05:21PM +0200, Ævar Arnfjörð Bjarmason wrote:

Ah, interesting, so it's a little finer grained than my suggestion of "just always ungroup when jobs=1". Since it's an option directly available to the caller, that means they could use it for something where the ordering really doesn't matter as much as the rate, such as if they wanted a bunch of subprocesses to split up some work in parallel or something. I like the flexibility, even though I know before I said "why don't we hide this functionality". So I was wrong, and this looks nice to me :)

I think we actually could even automatically set ungroup if jobs=1 as well, because then there is no reason to buffer the output - it uses additional memory for us, and it makes output slower to see for the end user. But I do not really mind enough to want a reroll.

I was worried about what happens if the caller changes the value of run_processes_parallel_opt.ungroup in the middle of execution, but since we're copying the value away during init, I think it will have no effect. OK.

Hm, so for now if ungroup=1, we ignore the stdout entirely? It looks like in patch 8 we're relying on the get_next_task callback to set these instead, right? Or am I misunderstanding it, and the child's stderr goes to our stderr by default?

I think we are losing some useful information here ("do not write directly to stdout or stderr from here"). Same for below.

Hm, I think it would be more accurate to say, "By default, their output (blah blah)..." because in fact when we set ungroup, it does interleave. But this is a small nit, from reading "ungroup" it becomes clear.

Emily Shaffer <emilyshaffer@google.com> writes:

Not doing so would protect us from future end-user complaints, similar to the way that made us consider the change in 2.36 to be a regression. Those who are used to see their stuff run in submodules (which I recall was the original purpose of run_processes_parallel was invented for) with their standard output and error streams not directly connected to the original end-user terminal will start seeing the expectation broken but only when there is only one submodule, no?

Doing it when an explicit "ungroup" was called for would hopefully avoid such an inconsistent behaviour.

Hi Ævar,

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

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

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.

On Wed, May 25 2022, Johannes Schindelin wrote:

Yes, or more generally to the new hook API which makes use of it.

Because the plan is still to continue in this direction and go for Emily's config-based hooks, which will run in parallel.

And to fix that would at this point be a larger functional change, because we'd be running with more code we haven't tested before, i.e. hook.[ch] on some new backend. So just passing down the appropriate flags to have run-command.[ch] do the right thing for us seemed to be the least bad option.

No, because you can have N processes all connected to a terminal with "ungroup", what you can't do is guarantee that they won't interleave.

But as discussed in some previous threads that would be OK, since that would come as an opt-in to parallel hook execution. I.e. you could pick one of:

 1. Current behavior
 2. Our parallel hook execution (whatever "ungroup" etc. settings that entails)
 3. Your own parallel hook execution

It only matters that we don't regress in #1, for #2 we could have different behavior, but just document the caveats as such.

IOW it's OK if you run parallel hooks and we decide that they won't be connected to a terminal, because that's a new feature we don't have yet, one you'd need to opt into.

They will run in parallel, see above.

I noted upthread/side-thread (in any case, in discussions around this) that I wished I'd come up with something smaller, but couldn't.

If you want to try your hand at that I'd love to see it.

But basically migrating the hook API to a new "backend" would also be a large change, so would making the bare minumum change in run-command.[ch].

But hey, I might be wrong. So if you think it's obvious that this could be much smaller I'd love to see patches for it...

Yes, that might be interesting.

I'll only note that I think you're focusing on the wrong thing here with "refactorings".

If you look at the history of this hooks API topic it started early on with a version where the config-based hooks + parallelism (currently not here yet) were combined with making the existing hook users use the new API (partially here now).

Now, I suggested that be split up so that we'd first re-implement all existing hooks on the new API, and *then* perform any feature changes.

Except of course by doing so that alters the nature of those changes in your definition, I assume, i.e. it goes from a feature series to "refactorings".

Whereas I think the important thing to optimize for is to make smaller incremental changes. Here we had a bug, and it's relatively easy to fix it, it would be much harder if we had a bigger delta in v2.36 with not only this bug, but some other regressions.

Which isn't hypothetical b.t.w., until 3-4 months ago nobody had seen that the config-based hooks topic we had kicking around had a severe performance regression. I found it and Emily & I have been kicking around a fix for it (mostly off-list).

But if we'd done that we'd have a more broken release, but we also wouldn't have "refactorings". I.e. the run_parallel API would actually be used, but we'd have this breakage plus some others.

Anyway, I think there's lots of things we could probably do better in delivering more reliable software. I'm just pointing out that here that I think focusing on a part of a larger progression from A..B and saying that it refactored something as being bad is to make a categorical mistake. Because a re-doing of that state to make each step not have any of those would result in larger change deltas.

Sure, we could instead add a third variant to it in addition to the two on "master", instead of unifying them as is done here.

But per the v1 feedback the consensus seemed to be that this was a good direction, and to the extent that there were objections it was that I should add the rest of the arguments to the "opts" struct.

But again, I'm fully open to that, I tried that and didn't think the end result was any simpler to review, but perhaps you'd like to try...

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

I share this sentiment.

True.

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.

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?

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.

Junio C Hamano <gitster@pobox.com> writes:

I tried the revert, and the result compiled and tested OK, but I am tempted to say that it looks as if the topic was deliberately designed to make it hard to revert by taking as much stuff hostage as possible.

At least one fix that depends on the run_hooks_opt structure introduced by c70bc338 (Merge branch 'ab/config-based-hooks-2', 2022-02-09) needs to be discarded. 7431379a (Merge branch 'ab/racy-hooks', 2022-03-16) did address an issue worth addressing, so even if we revert the whole c70bc338, we would want to redo the fix, possibly in some other way. But it also needed an "oops that was wrong, here is an attempt to fix it again" by cb3b3974 (Merge branch 'ab/racy-hooks', 2022-03-30). The situation is quite ugly.

As you hinted in the message I responded to in the message I am responding to, if we can make a surgical fix to make the new and improved run_hooks_opt() API build on top of run_command(), instead on top of run_processes_parallel(), that would give us a cleaner way out than discarding everything and redoing them "the right way". At least, the external interface into the API (read: the impression you would get by "less hook.h") does not look too bad.

Thanks.

On Wed, May 25 2022, Junio C Hamano wrote:

No, it's just that...

...we've made some use of the API since then, including for that bug fix...

...although for that last one if you're considering reverting that fix too to back out of the topic(s) it should be relatively easy to deal with that one.

I have a pending re-roll of this topic structured the way it is now (but with fixes for outstanding issues).

I understand your suggestion here to use the non-parallel API, and the reluctance to have a relatively large regression fix.

I haven't come up with a patch in this direction, and I'll try before a re-roll, but I can't see how we wouldn't end up with code that's an even larger logical change as a result.

I.e. this would require rewriting a large part of hook.[ch] which is currently structured around the callback API, and carefully coming up with the equivalent non-parallel API pattern for it.

Whereas the current direction is more boilerplate for sure, but keeps all of that existing behavior, and just narrowly adjust what options we pass down to the "struct child_process" in that case.

I can try to come up with it (and delay the current re-roll I have that's almost ready), but I really think that reviewing such a change will be much harder.

The current proposal is large by line count, but it's relatively easy to skim it and assure oneself that a new parameter is being passed in, and that all the proposed behavior change applies only to the one caller that passes in that new parameter.

Whereas switching to a new non-callback based API will require carefully going over the parallel API line-by-line, assuring oneself that the non-callback version is really doing the same thing etc.

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

I was worried about something like that when I wrote (admittedly unfairly, in a somewhat frustrated state) that the series was designed to be hard to revert. The reverting itself was reasonably easy if the "did we invoke the hook, really?" topic is discarded at the same time, but if was done with too much rearchitecting, it is understandable to become cumbersome to review X-<.

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.

Thanks.

On Thu, May 26 2022, Junio C Hamano wrote:

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?

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

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.

A re-roll of [1] which aims to address the concerns about the previous 8-part series being too large to fix a release regression. "If it isn't bolted down, throw it overboard!".

The main change here is:

 * The new "ungroup" parameter is now passed via an "extern" parameter.
 * Tests for existing run-command.c behavior (not narrowly needed for
   the regression fix) are gone.
 * Adding an INIT macro is gone, instead we explicitly  initialize to NULL.
 * Stray bugfix for existing hook test is gone.

etc. I think all of those still make sense, but they're something I can rebase on this topic once it (hopefully) lands. In the meantime the updated commit messages for the remaining two (see start of the range-diff below) argue for this being a a safe API change, even if the interface is a bit nasty.

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

 hook.c                      |  1 +
 run-command.c               | 88 ++++++++++++++++++++++++++++---------
 run-command.h               | 31 ++++++++++---
 t/helper/test-run-command.c | 19 ++++++--
 t/t0061-run-command.sh      | 35 +++++++++++++++
 t/t1800-hook.sh             | 37 ++++++++++++++++
 6 files changed, 181 insertions(+), 30 deletions(-)

Range-diff against v2:
1:  26a81eff267 < -:  ----------- run-command tests: change if/if/... to if/else if/else
2:  5f0a6e9925f < -:  ----------- run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
3:  a8e1fc07b65 < -:  ----------- run-command tests: test stdout of run_command_parallel()
4:  663936fb4ad < -:  ----------- run-command.c: add an initializer for "struct parallel_processes"
5:  c2e015ed840 ! 1:  aabd99de680 run-command: add an "ungroup" option to run_process_parallel()
    @@ Commit message
         user, i.e. they'd typically use one mode or the other, and would know
         whether they'd provided "ungroup" or not.
     
    +    We could also avoid the strbuf_init() for "buffered_output" by having
    +    "struct parallel_processes" use a static PARALLEL_PROCESSES_INIT
    +    initializer, but let's leave that cleanup for later.
    +
    +    Using a global "run_processes_parallel_ungroup" variable to enable
    +    this option is rather nasty, but is being done here to produce as
    +    minimal of a change as possible for a subsequent regression fix. This
    +    change is extracted from a larger initial version[1] which ends up
    +    with a better end-state for the API, but in doing so needed to modify
    +    all existing callers of the API. Let's defer that for now, and
    +    narrowly focus on what we need for fixing the regression in the
    +    subsequent commit.
    +
    +    It's safe to do this with a global variable because:
    +
    +     A) hook.c is the only user of it that sets it to non-zero, and before
    +        we'll get any other API users we'll refactor away this method of
    +        passing in the option, i.e. re-roll [1].
    +
    +     B) Even if hook.c wasn't the only user we don't have callers of this
    +        API that concurrently invoke this parallel process starting API
    +        itself in parallel.
    +
    +    As noted above "A" && "B" are rather nasty, and we don't want to live
    +    with those caveats long-term, but for now they should be an acceptable
    +    compromise.
    +
    +    1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## run-command.c ##
    @@ run-command.c: int pipe_command(struct child_process *cmd,
     +	GIT_CP_WAIT_CLEANUP, /* only for !ungroup */
      };
      
    ++int run_processes_parallel_ungroup;
      struct parallel_processes {
    + 	void *data;
    + 
     @@ run-command.c: struct parallel_processes {
      	struct pollfd *pfd;
      
    @@ run-command.c: struct parallel_processes {
      
      	int output_owner;
      	struct strbuf buffered_output; /* of finished children */
    +@@ run-command.c: static void pp_init(struct parallel_processes *pp,
    + 		    get_next_task_fn get_next_task,
    + 		    start_failure_fn start_failure,
    + 		    task_finished_fn task_finished,
    +-		    void *data)
    ++		    void *data,  const int ungroup)
    + {
    + 	int i;
    + 
     @@ run-command.c: static void pp_init(struct parallel_processes *pp,
      	pp->nr_processes = 0;
      	pp->output_owner = 0;
      	pp->shutdown = 0;
    -+	pp->ungroup = opts->ungroup;
    ++	pp->ungroup = ungroup;
      	CALLOC_ARRAY(pp->children, n);
     -	CALLOC_ARRAY(pp->pfd, n);
    -+	if (!pp->ungroup)
    ++	if (pp->ungroup)
    ++		pp->pfd = NULL;
    ++	else
     +		CALLOC_ARRAY(pp->pfd, n);
    + 	strbuf_init(&pp->buffered_output, 0);
      
      	for (i = 0; i < n; i++) {
      		strbuf_init(&pp->children[i].err, 0);
    @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
      			strbuf_reset(&pp->children[i].err);
      		} else {
    -@@ run-command.c: int run_processes_parallel(struct run_process_parallel_opts *opts)
    +@@ run-command.c: int run_processes_parallel(int n,
    + 	int output_timeout = 100;
    + 	int spawn_cap = 4;
    + 	struct parallel_processes pp;
    ++	const int ungroup = run_processes_parallel_ungroup;
    + 
    +-	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
    ++	/* unset for the next API user */
    ++	run_processes_parallel_ungroup = 0;
    ++
    ++	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
    ++		ungroup);
    + 	while (1) {
    + 		for (i = 0;
    + 		    i < spawn_cap && !pp.shutdown &&
    +@@ run-command.c: int run_processes_parallel(int n,
      		}
      		if (!pp.nr_processes)
      			break;
     -		pp_buffer_stderr(&pp, output_timeout);
     -		pp_output(&pp);
    -+		if (opts->ungroup) {
    ++		if (ungroup) {
     +			pp_mark_working_for_cleanup(&pp);
     +		} else {
     +			pp_buffer_stderr(&pp, output_timeout);
    @@ run-command.h: typedef int (*start_failure_fn)(struct strbuf *out,
       * pp_cb is the callback cookie as passed into run_processes_parallel,
       * pp_task_cb is the callback cookie as passed into get_next_task_fn.
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
    -  *
    -  * jobs: see 'n' in run_processes_parallel() below.
    -  *
    -+ * ungroup: Ungroup output. Output is printed as soon as possible and
    -+ * bypasses run-command's internal processing. This may cause output
    -+ * from different commands to be mixed.
    -+ *
    -  * *_fn & data: see run_processes_parallel() below.
    -  */
    - struct run_process_parallel_opts
    -@@ run-command.h: struct run_process_parallel_opts
    - 	const char *tr2_label;
    - 
    - 	int jobs;
    -+	unsigned int ungroup:1;
    - 
    - 	get_next_task_fn get_next_task;
    - 	start_failure_fn start_failure;
    -@@ run-command.h: struct run_process_parallel_opts
       *
       * The children started via this function run in parallel. Their output
       * (both stdout and stderr) is routed to stderr in a manner that output
    @@ run-command.h: struct run_process_parallel_opts
     + * NULL "struct strbuf *out" parameter, and are responsible for
     + * emitting their own output, including dealing with any race
     + * conditions due to writing in parallel to stdout and stderr.
    ++ * The "ungroup" option can be enabled by setting the global
    ++ * "run_processes_parallel_ungroup" to "1" before invoking
    ++ * run_processes_parallel(), it will be set back to "0" as soon as the
    ++ * API reads that setting.
       */
    - int run_processes_parallel(struct run_process_parallel_opts *opts);
    - 
    ++extern int run_processes_parallel_ungroup;
    + int run_processes_parallel(int n,
    + 			   get_next_task_fn,
    + 			   start_failure_fn,
     
      ## t/helper/test-run-command.c ##
     @@ t/helper/test-run-command.c: static int parallel_next(struct child_process *cp,
    @@ t/helper/test-run-command.c: static int task_finished(int result,
      }
      
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    - 	opts.jobs = jobs;
    - 	opts.data = &proc;
    + 	strvec_clear(&proc.args);
    + 	strvec_pushv(&proc.args, (const char **)argv + 3);
      
    --	if (!strcmp(argv[1], "run-command-parallel")) {
    -+	if (!strcmp(argv[1], "run-command-parallel") ||
    -+	    !strcmp(argv[1], "run-command-parallel-ungroup")) {
    - 		next_fn = parallel_next;
    --	} else if (!strcmp(argv[1], "run-command-abort")) {
    -+	} else if (!strcmp(argv[1], "run-command-abort") ||
    -+		   !strcmp(argv[1], "run-command-abort-ungroup")) {
    - 		next_fn = parallel_next;
    - 		finished_fn = task_finished;
    --	} else if (!strcmp(argv[1], "run-command-no-jobs")) {
    -+	} else if (!strcmp(argv[1], "run-command-no-jobs") ||
    -+		   !strcmp(argv[1], "run-command-no-jobs-ungroup")) {
    - 		next_fn = no_job;
    - 		finished_fn = task_finished;
    - 	} else {
    -@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    - 		return 1;
    - 	}
    - 
    -+	opts.ungroup = ends_with(argv[1], "-ungroup");
    - 	opts.get_next_task = next_fn;
    - 	opts.task_finished = finished_fn;
    - 	exit(run_processes_parallel(&opts));
    ++	if (getenv("RUN_PROCESSES_PARALLEL_UNGROUP"))
    ++		run_processes_parallel_ungroup = 1;
    ++
    + 	if (!strcmp(argv[1], "run-command-parallel"))
    + 		exit(run_processes_parallel(jobs, parallel_next,
    + 					    NULL, NULL, &proc));
     
      ## t/t0061-run-command.sh ##
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with more jobs available than
    - 	test_cmp expect err
    + 	test_cmp expect actual
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
    -+	test-tool run-command run-command-parallel-ungroup 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    ++	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_line_count = 8 out &&
     +	test_line_count = 4 err
     +'
     +
      test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
    - 	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    - 	test_must_be_empty out &&
    - 	test_cmp expect err
    + 	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    + 	test_cmp expect actual
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
    -+	test-tool run-command run-command-parallel-ungroup 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    ++	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_line_count = 8 out &&
     +	test_line_count = 4 err
     +'
     +
      test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
    - 	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    - 	test_must_be_empty out &&
    - 	test_cmp expect err
    + 	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    + 	test_cmp expect actual
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
    -+	test-tool run-command run-command-parallel-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    ++	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_line_count = 8 out &&
     +	test_line_count = 4 err
     +'
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
      preloaded output of a child
      asking for a quick stop
     @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort gracefully' '
    - 	test_cmp expect err
    + 	test_cmp expect actual
      '
      
     +test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
    -+	test-tool run-command run-command-abort-ungroup 3 false >out 2>err &&
    ++	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    ++	test-tool run-command run-command-abort 3 false >out 2>err &&
     +	test_must_be_empty out &&
     +	test_line_count = 6 err
     +'
    @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort grace
      no further jobs available
      EOF
     @@ t/t0061-run-command.sh: test_expect_success 'run_command outputs ' '
    - 	test_cmp expect err
    + 	test_cmp expect actual
      '
      
     +test_expect_success 'run_command outputs (ungroup) ' '
    -+	test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    ++	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
     +	test_cmp expect err
     +'
6:  84e92c6f7c7 < -:  ----------- hook tests: fix redirection logic error in 96e7225b310
7:  bf7d871565f < -:  ----------- hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
8:  238155fcb9d ! 2:  ec27e3906e1 hook API: fix v2.36.0 regression: hooks should be connected to a TTY
    @@ Commit message
                   './git hook run seq-hook' in 'HEAD~0' ran
                     1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'
     
    -    In the preceding commit we removed the "stdout_to_stderr=1" assignment
    -    as being redundant. This change brings it back as with ".ungroup=1"
    -    the run_process_parallel() function doesn't provide them for us
    -    implicitly.
    -
    -    As an aside omitting the stdout_to_stderr=1 here would have all tests
    -    pass, except those that test "git hook run" itself in
    -    t1800-hook.sh. But our tests passing is the result of another test
    -    blind spot, as was the case with the regression being fixed here. The
    -    "stdout_to_stderr=1" for hooks is long-standing behavior, see
    -    e.g. 1d9e8b56fe3 (Split back out update_hook handling in receive-pack,
    -    2007-03-10) and other follow-up commits (running "git log" with
    -    "--reverse -p -Gstdout_to_stderr" is a good start).
    -
         1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/
     
         Reported-by: Anthony Sottile <asottile@umich.edu>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## hook.c ##
    -@@ hook.c: static int pick_next_hook(struct child_process *cp,
    - 		return 0;
    - 
    - 	strvec_pushv(&cp->env_array, hook_cb->options->env.v);
    -+	cp->stdout_to_stderr = 1; /* because of .ungroup = 1 */
    - 	cp->trace2_hook_name = hook_cb->hook_name;
    - 	cp->dir = hook_cb->options->dir;
    - 
     @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
    - 		.tr2_label = hook_name,
    - 
    - 		.jobs = jobs,
    -+		.ungroup = jobs == 1,
    - 
    - 		.get_next_task = pick_next_hook,
    - 		.start_failure = notify_start_failure,
    -@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
    - 	if (!options)
    - 		BUG("a struct run_hooks_opt must be provided to run_hooks");
    - 
    -+	if (jobs != 1 || !run_opts.ungroup)
    -+		BUG("TODO: think about & document order & interleaving of parallel hook output");
    -+
    - 	if (options->invoked_hook)
    - 		*options->invoked_hook = 0;
    + 		cb_data.hook_path = abs_path.buf;
    + 	}
      
    ++	run_processes_parallel_ungroup = 1;
    + 	run_processes_parallel_tr2(jobs,
    + 				   pick_next_hook,
    + 				   notify_start_failure,
     
      ## t/t1800-hook.sh ##
     @@ t/t1800-hook.sh: test_description='git-hook command'

Extend the parallel execution API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to support a mode where the stdout and stderr of the processes isn't captured and output in a deterministic order, instead we'll leave it to the kernel and stdio to sort it out.

This gives the API same functionality as GNU parallel's --ungroup option. As we'll see in a subsequent commit the main reason to want this is to support stdout and stderr being connected to the TTY in the case of jobs=1, demonstrated here with GNU parallel:

	$ parallel --ungroup 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	TTY
	TTY
	$ parallel 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	NTTY
	NTTY

Another is as GNU parallel's documentation notes a potential for optimization. Our results will be a bit different, but in cases where you want to run processes in parallel where the exact order isn't important this can be a lot faster:

	$ hyperfine -r 3 -L o ,--ungroup 'parallel {o} seq ::: 10000000 >/dev/null '
	Benchmark 1: parallel  seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     220.2 ms ±   9.3 ms    [User: 124.9 ms, System: 96.1 ms]
	  Range (min … max):   212.3 ms … 230.5 ms    3 runs

	Benchmark 2: parallel --ungroup seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     154.7 ms ±   0.9 ms    [User: 136.2 ms, System: 25.1 ms]
	  Range (min … max):   153.9 ms … 155.7 ms    3 runs

	Summary
	  'parallel --ungroup seq ::: 10000000 >/dev/null ' ran
	    1.42 ± 0.06 times faster than 'parallel  seq ::: 10000000 >/dev/null '

A large part of the juggling in the API is to make the API safer for its maintenance and consumers alike.

For the maintenance of the API we e.g. avoid malloc()-ing the "pp->pfd", ensuring that SANITIZE=address and other similar tools will catch any unexpected misuse.

For API consumers we take pains to never pass the non-NULL "out" buffer to an API user that provided the "ungroup" option. The resulting code in t/helper/test-run-command.c isn't typical of such a user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not.

We could also avoid the strbuf_init() for "buffered_output" by having "struct parallel_processes" use a static PARALLEL_PROCESSES_INIT initializer, but let's leave that cleanup for later.

Using a global "run_processes_parallel_ungroup" variable to enable this option is rather nasty, but is being done here to produce as minimal of a change as possible for a subsequent regression fix. This change is extracted from a larger initial version[1] which ends up with a better end-state for the API, but in doing so needed to modify all existing callers of the API. Let's defer that for now, and narrowly focus on what we need for fixing the regression in the subsequent commit.

It's safe to do this with a global variable because:

 A) hook.c is the only user of it that sets it to non-zero, and before
    we'll get any other API users we'll refactor away this method of
    passing in the option, i.e. re-roll [1].

 B) Even if hook.c wasn't the only user we don't have callers of this
    API that concurrently invoke this parallel process starting API
    itself in parallel.

As noted above "A" && "B" are rather nasty, and we don't want to live with those caveats long-term, but for now they should be an acceptable compromise.

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c               | 88 ++++++++++++++++++++++++++++---------
 run-command.h               | 31 ++++++++++---
 t/helper/test-run-command.c | 19 ++++++--
 t/t0061-run-command.sh      | 35 +++++++++++++++
 4 files changed, 143 insertions(+), 30 deletions(-)

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

A few comments on these below.

It is unusual in this codebase to pass "const" non-pointer as a parameter, but OK.

Micronit. If buffered_output is empty, whether it is because we are in the ungroup mode and haven't buffered anything there, or because our subprocess didn't emit anything, we do not have to do this write. So it looks to me that it would be conceptually much cleaner to do

	if (pp->buffered_output.len)
		strbuf_write(&pp->buffered_output, stderr);

or just to let the strbuf_write() worry about it, as this is an I/O codepath and the overhead of a no-op function call may be negligible.

Or is there a reason to believe that pp->buffered_output is in an undefined state when in the ungroup mode? If so, we probably should fix that. The fewer special rules like "in X mode, members Y, Z and W are left uninitialized so do not even look at them", the better off we will be, especially when Y, Z and W have their own natural "initialized and untouched" state. Allow users of Y to decide how they do things with Y without having to worry about X that they do not have to worry about when doing their job.

And this unconditinal call indicates buffered_output is never in an undefined state and it is safe to call _release even in the ungroup mode.

OK, any process taken from the pp struct with the ungroup bit on does not get its output stolen. Makes sense.

OK.

OK.

This is shared between the two modes, and is unchanged from the run_hook_ve() days. Good.

OK.

OK.

This thing is new. I do not see a corresponding removal of a similar loop that used to be done unconditionally that was turned into a call to this helper only under the non-ungroup mode, or anything like that, so it is a bit puzzling.

Sensible.  Or even "if (pp->ungroup) BUG()".

The new helper does this only for those in the WORKING state, but this one does so unconditionally. It's not like we leave the .state of our subprocesses unspecified when we start them---we set to WORKING whether we are in the ungroup mode or not. So it is also puzzling why we are not calling the helper function here.

By the way, if we use WAIT_CLEANUP state like this in the ungroup mode, shouldn't we lose the "only for !ungroup" comment from the enum definition?

OK.

Of course ;-)

This way, you do not have to touch existing calls to this function that do not (yet) want to know about the ungroup mode.

That makes a confusing API, but the trade-off feels OK.

This comment appears just before the typedef of get_next_task_fn function type, presumably to explain the parameters involved in calling such a function, and it does talk about pp_cb and pp_task_cb. The new paragraph, however, looks out of place. There is no err parameter. The existing text (before the pre-context) mentions "preload the error channel" but it is left unclear what that means. Does that "err" non-parameter the new paragraph talks about have some connection to the thing that receives the preloaded error channel contents? Puzzled.

This comment is for start_failure_fn and has the same issue. The text removed gives a readable/understandable explanation for developers who are writing for non-ungrouped mode, though.

Ditto for task_finished_fn.

I think you meant "below" here, not "above".

"specified callbacks" -> "specified, callbacks"

This new paragraph is well written.

Fix a regression reported[1] in f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook.

As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster:

	$ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook'
	#!/bin/sh

	seq 100000000
	Benchmark 1: ./git hook run seq-hook' in 'origin/master
	  Time (mean ± σ):     787.1 ms ±  13.6 ms    [User: 701.6 ms, System: 534.4 ms]
	  Range (min … max):   773.2 ms … 806.3 ms    10 runs

	Benchmark 2: ./git hook run seq-hook' in 'HEAD~0
	  Time (mean ± σ):     603.4 ms ±   1.6 ms    [User: 573.1 ms, System: 30.3 ms]
	  Range (min … max):   601.0 ms … 606.2 ms    10 runs

	Summary
	  './git hook run seq-hook' in 'HEAD~0' ran
	    1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c          |  1 +
 t/t1800-hook.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

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

So the approach taken here is that we assume the reported one is the only regression and keep going with run_process_parallel() API.

I still share the sentiment with Dscho that it is generally a bad idea, when dealing with a regression, to double-down and dig in your heels to keep the change that caused a regression with paper over patches, but too much time has passed since the release, and a patch or two on top does look like a quicker way forward.

I left a few comments on the implementation, but modulo these small details, the code looks OK (provided that the assumption holds true, that is, of course).

Thanks.

A re-roll of [1] fixing issues pointed out by Junio in the last re-roll:

 * A minor whitespace parameter fix.

 * Clear up confusion about GIT_CP_WAIT_CLEANUP, and remove the
   duplicate "mark ungroup children as GIT_CP_WAIT_CLEANUP" code.

 * Droped a conditional strbuf_write() on an always-empty string under
   "ungroup".

 * Correct "err" parameter name to "out" in the new API docs.

 * Replace assert() with BUG().

 * Address other minor issues noted by Junio.

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

 hook.c                      |  1 +
 run-command.c               | 83 +++++++++++++++++++++++++++++--------
 run-command.h               | 30 ++++++++++----
 t/helper/test-run-command.c | 19 +++++++--
 t/t0061-run-command.sh      | 35 ++++++++++++++++
 t/t1800-hook.sh             | 37 +++++++++++++++++
 6 files changed, 177 insertions(+), 28 deletions(-)

Range-diff against v3:
1:  aabd99de680 ! 1:  f1170b02553 run-command: add an "ungroup" option to run_process_parallel()
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## run-command.c ##
    -@@ run-command.c: int pipe_command(struct child_process *cmd,
    - enum child_state {
    - 	GIT_CP_FREE,
    - 	GIT_CP_WORKING,
    --	GIT_CP_WAIT_CLEANUP,
    -+	GIT_CP_WAIT_CLEANUP, /* only for !ungroup */
    +@@ run-command.c: enum child_state {
    + 	GIT_CP_WAIT_CLEANUP,
      };
      
     +int run_processes_parallel_ungroup;
    @@ run-command.c: static void pp_init(struct parallel_processes *pp,
      		    start_failure_fn start_failure,
      		    task_finished_fn task_finished,
     -		    void *data)
    -+		    void *data,  const int ungroup)
    ++		    void *data, const int ungroup)
      {
      	int i;
      
    @@ run-command.c: static void pp_init(struct parallel_processes *pp,
      		pp->pfd[i].events = POLLIN | POLLHUP;
      		pp->pfd[i].fd = -1;
      	}
    -@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
    - 	 * When get_next_task added messages to the buffer in its last
    - 	 * iteration, the buffered output is non empty.
    - 	 */
    --	strbuf_write(&pp->buffered_output, stderr);
    -+	if (!pp->ungroup)
    -+		strbuf_write(&pp->buffered_output, stderr);
    - 	strbuf_release(&pp->buffered_output);
    - 
    - 	sigchain_pop_common();
     @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
       */
      static int pp_start_one(struct parallel_processes *pp)
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      	return 0;
      }
      
    -+static void pp_mark_working_for_cleanup(struct parallel_processes *pp)
    ++static void pp_mark_ungrouped_for_cleanup(struct parallel_processes *pp)
     +{
     +	int i;
     +
    ++	if (!pp->ungroup)
    ++		BUG("only reachable if 'ungrouped'");
    ++
     +	for (i = 0; i < pp->max_processes; i++)
    -+		if (pp->children[i].state == GIT_CP_WORKING)
    -+			pp->children[i].state = GIT_CP_WAIT_CLEANUP;
    ++		pp->children[i].state = GIT_CP_WAIT_CLEANUP;
     +}
     +
      static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
      {
      	int i;
      
    -+	assert(!pp->ungroup);
    ++	if (pp->ungroup)
    ++		BUG("unreachable with 'ungrouped'");
     +
      	while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
      		if (errno == EINTR)
    @@ run-command.c: static void pp_buffer_stderr(struct parallel_processes *pp, int o
      {
      	int i = pp->output_owner;
     +
    -+	assert(!pp->ungroup);
    ++	if (pp->ungroup)
    ++		BUG("unreachable with 'ungrouped'");
     +
      	if (pp->children[i].state == GIT_CP_WORKING &&
      	    pp->children[i].err.len) {
    @@ run-command.c: static void pp_output(struct parallel_processes *pp)
      	int i, code;
      	int n = pp->max_processes;
      	int result = 0;
    - 
    -+	if (ungroup)
    -+		for (i = 0; i < pp->max_processes; i++)
    -+			pp->children[i].state = GIT_CP_WAIT_CLEANUP;
    -+
    - 	while (pp->nr_processes > 0) {
    - 		for (i = 0; i < pp->max_processes; i++)
    - 			if (pp->children[i].state == GIT_CP_WAIT_CLEANUP)
     @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      		code = finish_command(&pp->children[i].process);
      
    @@ run-command.c: int run_processes_parallel(int n,
     -		pp_buffer_stderr(&pp, output_timeout);
     -		pp_output(&pp);
     +		if (ungroup) {
    -+			pp_mark_working_for_cleanup(&pp);
    ++			pp_mark_ungrouped_for_cleanup(&pp);
     +		} else {
     +			pp_buffer_stderr(&pp, output_timeout);
     +			pp_output(&pp);
    @@ run-command.h: void check_pipe(int err);
       * pp_cb is the callback cookie as passed to run_processes_parallel.
       * You can store a child process specific callback cookie in pp_task_cb.
       *
    -+ * The "struct strbuf *err" parameter is either a pointer to a string
    -+ * to write errors to, or NULL if the "ungroup" option was
    -+ * provided. See run_processes_parallel() below.
    ++ * See run_processes_parallel() below for a discussion of the "struct
    ++ * strbuf *out" parameter.
     + *
       * Even after returning 0 to indicate that there are no more processes,
       * this function will be called again until there are no more running
    @@ run-command.h: typedef int (*get_next_task_fn)(struct child_process *cp,
     - * You must not write to stdout or stderr in this function. Add your
     - * message to the strbuf out instead, which will be printed without
     - * messing up the output of the other parallel processes.
    -+ * The "struct strbuf *err" parameter is either a pointer to a string
    -+ * to write errors to, or NULL if the "ungroup" option was
    -+ * provided. See run_processes_parallel() below.
    ++ * See run_processes_parallel() below for a discussion of the "struct
    ++ * strbuf *out" parameter.
       *
       * pp_cb is the callback cookie as passed into run_processes_parallel,
       * pp_task_cb is the callback cookie as passed into get_next_task_fn.
    @@ run-command.h: typedef int (*start_failure_fn)(struct strbuf *out,
     - * You must not write to stdout or stderr in this function. Add your
     - * message to the strbuf out instead, which will be printed without
     - * messing up the output of the other parallel processes.
    -+ * The "struct strbuf *err" parameter is either a pointer to a string
    -+ * to write errors to, or NULL if the "ungroup" option was
    -+ * provided. See run_processes_parallel() below.
    ++ * See run_processes_parallel() below for a discussion of the "struct
    ++ * strbuf *out" parameter.
       *
       * pp_cb is the callback cookie as passed into run_processes_parallel,
       * pp_task_cb is the callback cookie as passed into get_next_task_fn.
    @@ run-command.h: typedef int (*task_finished_fn)(int result,
       * The children started via this function run in parallel. Their output
       * (both stdout and stderr) is routed to stderr in a manner that output
     - * from different tasks does not interleave.
    -+ * from different tasks does not interleave (but see "ungroup" above).
    ++ * from different tasks does not interleave (but see "ungroup" below).
       *
       * start_failure_fn and task_finished_fn can be NULL to omit any
       * special handling.
     + *
    -+ * If the "ungroup" option isn't specified the callbacks will get a
    -+ * pointer to a "struct strbuf *out", and must not write to stdout or
    ++ * If the "ungroup" option isn't specified, the API will set the
    ++ * "stdout_to_stderr" parameter in "struct child_process" and provide
    ++ * the callbacks with a "struct strbuf *out" parameter to write output
    ++ * to. In this case the callbacks must not write to stdout or
     + * stderr as such output will mess up the output of the other parallel
     + * processes. If "ungroup" option is specified callbacks will get a
     + * NULL "struct strbuf *out" parameter, and are responsible for
2:  ec27e3906e1 = 2:  8ab09f28729 hook API: fix v2.36.0 regression: hooks should be connected to a TTY

Extend the parallel execution API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to support a mode where the stdout and stderr of the processes isn't captured and output in a deterministic order, instead we'll leave it to the kernel and stdio to sort it out.

This gives the API same functionality as GNU parallel's --ungroup option. As we'll see in a subsequent commit the main reason to want this is to support stdout and stderr being connected to the TTY in the case of jobs=1, demonstrated here with GNU parallel:

	$ parallel --ungroup 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	TTY
	TTY
	$ parallel 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	NTTY
	NTTY

Another is as GNU parallel's documentation notes a potential for optimization. Our results will be a bit different, but in cases where you want to run processes in parallel where the exact order isn't important this can be a lot faster:

	$ hyperfine -r 3 -L o ,--ungroup 'parallel {o} seq ::: 10000000 >/dev/null '
	Benchmark 1: parallel  seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     220.2 ms ±   9.3 ms    [User: 124.9 ms, System: 96.1 ms]
	  Range (min … max):   212.3 ms … 230.5 ms    3 runs

	Benchmark 2: parallel --ungroup seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     154.7 ms ±   0.9 ms    [User: 136.2 ms, System: 25.1 ms]
	  Range (min … max):   153.9 ms … 155.7 ms    3 runs

	Summary
	  'parallel --ungroup seq ::: 10000000 >/dev/null ' ran
	    1.42 ± 0.06 times faster than 'parallel  seq ::: 10000000 >/dev/null '

A large part of the juggling in the API is to make the API safer for its maintenance and consumers alike.

For the maintenance of the API we e.g. avoid malloc()-ing the "pp->pfd", ensuring that SANITIZE=address and other similar tools will catch any unexpected misuse.

For API consumers we take pains to never pass the non-NULL "out" buffer to an API user that provided the "ungroup" option. The resulting code in t/helper/test-run-command.c isn't typical of such a user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not.

We could also avoid the strbuf_init() for "buffered_output" by having "struct parallel_processes" use a static PARALLEL_PROCESSES_INIT initializer, but let's leave that cleanup for later.

Using a global "run_processes_parallel_ungroup" variable to enable this option is rather nasty, but is being done here to produce as minimal of a change as possible for a subsequent regression fix. This change is extracted from a larger initial version[1] which ends up with a better end-state for the API, but in doing so needed to modify all existing callers of the API. Let's defer that for now, and narrowly focus on what we need for fixing the regression in the subsequent commit.

It's safe to do this with a global variable because:

 A) hook.c is the only user of it that sets it to non-zero, and before
    we'll get any other API users we'll refactor away this method of
    passing in the option, i.e. re-roll [1].

 B) Even if hook.c wasn't the only user we don't have callers of this
    API that concurrently invoke this parallel process starting API
    itself in parallel.

As noted above "A" && "B" are rather nasty, and we don't want to live with those caveats long-term, but for now they should be an acceptable compromise.

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c               | 83 +++++++++++++++++++++++++++++--------
 run-command.h               | 30 ++++++++++----
 t/helper/test-run-command.c | 19 +++++++--
 t/t0061-run-command.sh      | 35 ++++++++++++++++
 4 files changed, 139 insertions(+), 28 deletions(-)

Hi Ævar,

what you work on here is important. Git v2.36.0 unfortunately had a couple of bugs that were quickly discovered, and this here regression is the one of them. The other known ones had been fixed after a little more than two weeks, but another four weeks later, this regression still awaits fixing.

On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote:

It might be worth picking a better name than "ungroup". Maybe something like "interleaved_output".

This commit message talks a lot about GNU parallel.

It would make more sense to measure Git with that new mode, though, and talk about that instead.

This is an uncomfortably large diffstat for a patch series that is supposed to fix a regression. That makes it harder than necessary to review, and hence unnecessarily blocks v2.36.2 (at least it was my expectation that we would release that version relatively quickly, as the regression fix already missed the v2.36.1 boat).

This global variable seems to exist solely to avoid extending the signature of `run_processes_parallel_tr2()`. Let's not do that.

It would be more logical to test for `pp->ungroup` than for `!pp->pfd`. In other instances below, the patch uses `if (ungroup)` instead. Let's not flip-flop between those two conditions, but the latter consistently.

This avoids indenting the following two lines, at the price of readability. The code would be more obvious if it made those two lines contingent upon `!pp->ungroup`.

It costs readers a couple of moments when they stumble over code that is inconsistent with the existing code. In this instance, I find very little value in the `const` qualifier. Actually, this entire line is probably not worth having because `pp->ungroup` is just 4 characters longer than `ungroup`.

This same comment applies to another hunk below, too.

Things like this do take focus away from reviewing the interesting part of the contribution, which in particular in the case of a regression fix that many are waiting for is something to avoid.

In contrast to the change in `pp_init()`, this hunk is good, because it makes the intention and implementation quite clear.

Here, the patch uses `pp->pfd` instead of `pp->ungroup` again. It should use only one of them for the many conditions that are added, not flip-flop between them.

This function has but a single caller. It would improve readability to insert the loop directly instead.

Better: `BUG("pp_buffer_stderr() called in ungrouped mode")`

A similar issue exists in the next hunk.

A better idea would be to describe the "interleaved_output" mode first, and then say "the rest of this comment deals with the non-interleaved mode".

This `if (err) strbuf_add... else fprintf...` pattern is a bit repetitive, but in the interest of fixing a regression without risking to introduce another regression, I agree that this should wait for later to be addressed.

On the other hand, this issue is indicating that the API could be designed better, e.g. by letting the `parallel_processes` struct provide a callback function for printing or buffering messages.

At this stage, it is concerning that we introduce a feature that needs to be designed well and therefore needs more time to be fleshed out, when a regression fix rides on it that should be integrated swiftly and does not allow for said time to flesh things out.

It is really unfortunate that the hook changes that made it into v2.36.0 are in such an unrevertable state. It would really make most sense, as Junio suggested elsewhere in this thread, to roll back the changes that introduced the regression, then spend the time it actually takes to design the feature properly how hooks could be run via the `run_processes_parallel()` API. Or spend the time to figure out that not using the parallel API at all might be the best course of action, instead executing the hook directly, using the standard `run_command()` API. That may very well turn out to avoid some over-engineering, as an added benefit.

Apart from using a naming scheme that is inconsistent with how Git does similar things elsewhere, this environment variable seems to have been introduced for the sole purpose of testing the `ungroup` mode.

Let's instead introduce a new `test-tool run-command` subcommand for that specific purpose.

Good. Testing for the line count avoids getting confused by interleaved output.

Having said that, adding seven (!) new test cases merely to verify that the `ungroup` mode does not break things is a bit excessive, in particular since none of the test cases seem to _actually_ verify that the output is interleaved. Remember, adding test cases is not free.

Ciao, Johannes

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

It may make the change even noisier, though.

Marking incoming parameter as const is probably a misfeature in C, but doing so with file-scope static would not hurt too much, so if this series needs no further reroll, I'd let it pass.

I'd be somewhat sympathetic to the aversion to "flip-flop", but I strongly disagree with you here.

"ungroup" does not have to stay to be the only reason why we do not allocate the pp->pfd[] array, and what we care here is "if we are polling for events, then do this initialization to the array", not "if ungroup -> we must not have the pfd[] array -> so let's skip it". We do not have to add more code that depends on that two step inference when we do not need to.

OK.

Thanks.

Fix a regression reported[1] in f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook.

As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster:

	$ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook'
	#!/bin/sh

	seq 100000000
	Benchmark 1: ./git hook run seq-hook' in 'origin/master
	  Time (mean ± σ):     787.1 ms ±  13.6 ms    [User: 701.6 ms, System: 534.4 ms]
	  Range (min … max):   773.2 ms … 806.3 ms    10 runs

	Benchmark 2: ./git hook run seq-hook' in 'HEAD~0
	  Time (mean ± σ):     603.4 ms ±   1.6 ms    [User: 573.1 ms, System: 30.3 ms]
	  Range (min … max):   601.0 ms … 606.2 ms    10 runs

	Summary
	  './git hook run seq-hook' in 'HEAD~0' ran
	    1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c          |  1 +
 t/t1800-hook.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Hi Ævar,

On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote:

The regression was not reported in f443247b9f2.

It is an unwanted distraction to talk about the speed here, when the entire purpose of the patch series is to fix the regression that stdio are no longer connected when running hooks.

Instead of spreading the regression test out over so many lines, a single test case that verifies what needs to be verified succinctly should be plenty sufficient. Something along these lines:

	test_expect_success TTY 'hooks are conencted to stdio' '
		test_when_finished "rm .git/hooks/pre-commit" &&

		write_script .git/hooks/pre-commit <<-EOF
		test -t 1 && echo "stdout is a TTY" >out
		test -t 2 && echo "stderr is a TTY" >>out
		EOF

		test_terminal git commit --allow-empty -m hooks-and-stdio &&
		grep stdout out &&
		grep stderr out
	'

Not only is this much easier to review, not only is it more obvious what is being tested, it is also much quicker to debug in case it fails.

Ciao, Johannes

Hi Ævar,

On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote:

As I mentioned in the review of the first patch, this introduces a feature with enough code that it is quite easy for more regressions to lurk in there.

One thing that is notably missing from the cover letter is a discussion how the current approach compares to either reverting the patches that introduced the regression or alternatively patching the code in `hook.c` to avoid using the `run_processes_parallel()` API altogether.

Ciao, Johannes

This series fixes a v2.36.0 regression[1]. See [2] for the v4. The reasons for why a regression needs this relatively large change to move forward is discussed in past rounds, e.g. around [3]. CI at https://github.com/avar/git/actions/runs/2428475773

Changes since v4, mainly to address comments by Johannes (thanks for the review!):

 * First, some things like renaming "ungroup" to something else &
   rewriting the tests I didn't do because I thought keeping the
   inter/range-diff down in size outweighed re-arranging or changing
   the code at this late stage.

   In the case of the suggested shorter test in
   https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206011827300.349@tvgsbejvaqbjf.bet/
   the replacement wasn't testing the same thing. I.e. we don't see
   what's connected to a TTY if we redirect one of stdout or stderr
   anymore, which is important to get right.

 * Ditto the suggestion to e.g. add a parameter for "ungroup". I agree
   that's better, but that approach was in the earlier and much larger
   round[4], here we're trying to aim for the smallest possible
   regression fix by line count & complexity.

 * I retained the performance test(s) for "parallel" and "git hook
   run" in 1/2 and 2/2. Yes, the former isn't ours, but I think it
   helps to explain the code, implementation and resulting performance
   with reference to existing well-known software that's doing the
   exact same thing we're doing here.

 * Stopped using "const" in "const int ungroup", and dropped some of
   those variables entirely.

 * Inlined the pp_mark_ungrouped_for_cleanup() function. I added an
   "int i" in the inner scope in run_processes_parallel() even though
   we have one in the outer, just to make it clear that we're not
   caring about the other one (or clobbering it).

 * I just got rid of the two added BUG(). It's obvious enough from the
   calling code that those two functions are !ungroup only, so we can
   do without the sprinkling of BUG() and larger resulting diff.

 * Passed an --ungroup parameter in the tests instead of passing a
   parameter by environment variable.

 * Fixed a minor s/reported in/reported against/ phrasing in the 2/2
   commit message.

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/
2. https://lore.kernel.org/git/cover-v4-0.2-00000000000-20220531T173005Z-avarab@gmail.com/
3. https://lore.kernel.org/git/220526.86pmk060xa.gmgdl@evledraar.gmail.com/
4. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

 hook.c                      |  1 +
 run-command.c               | 70 +++++++++++++++++++++++++++----------
 run-command.h               | 30 ++++++++++++----
 t/helper/test-run-command.c | 22 ++++++++++--
 t/t0061-run-command.sh      | 30 ++++++++++++++++
 t/t1800-hook.sh             | 37 ++++++++++++++++++++
 6 files changed, 161 insertions(+), 29 deletions(-)

Range-diff against v4:
1:  f1170b02553 ! 1:  d018b7c4441 run-command: add an "ungroup" option to run_process_parallel()
    @@ Commit message
                 NTTY
     
         Another is as GNU parallel's documentation notes a potential for
    -    optimization. Our results will be a bit different, but in cases where
    +    optimization. As demonstrated in next commit our results with "git
    +    hook run" will be similar, but generally speaking this shows that if
         you want to run processes in parallel where the exact order isn't
         important this can be a lot faster:
     
    @@ run-command.c: static void pp_init(struct parallel_processes *pp,
      		    start_failure_fn start_failure,
      		    task_finished_fn task_finished,
     -		    void *data)
    -+		    void *data, const int ungroup)
    ++		    void *data, int ungroup)
      {
      	int i;
      
    @@ run-command.c: static void pp_init(struct parallel_processes *pp,
      	for (i = 0; i < n; i++) {
      		strbuf_init(&pp->children[i].err, 0);
      		child_process_init(&pp->children[i].process);
    -+		if (!pp->pfd)
    -+			continue;
    - 		pp->pfd[i].events = POLLIN | POLLHUP;
    - 		pp->pfd[i].fd = -1;
    +-		pp->pfd[i].events = POLLIN | POLLHUP;
    +-		pp->pfd[i].fd = -1;
    ++		if (pp->pfd) {
    ++			pp->pfd[i].events = POLLIN | POLLHUP;
    ++			pp->pfd[i].fd = -1;
    ++		}
      	}
    -@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
    -  */
    - static int pp_start_one(struct parallel_processes *pp)
    - {
    -+	const int ungroup = pp->ungroup;
    - 	int i, code;
      
    - 	for (i = 0; i < pp->max_processes; i++)
    + 	pp_for_signal = pp;
     @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      		BUG("bookkeeping is hard");
      
      	code = pp->get_next_task(&pp->children[i].process,
     -				 &pp->children[i].err,
    -+				 ungroup ? NULL : &pp->children[i].err,
    ++				 pp->ungroup ? NULL : &pp->children[i].err,
      				 pp->data,
      				 &pp->children[i].data);
      	if (!code) {
     -		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
     -		strbuf_reset(&pp->children[i].err);
    -+		if (!ungroup) {
    ++		if (!pp->ungroup) {
     +			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
     +			strbuf_reset(&pp->children[i].err);
     +		}
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      	}
     -	pp->children[i].process.err = -1;
     -	pp->children[i].process.stdout_to_stderr = 1;
    -+	if (!ungroup) {
    ++	if (!pp->ungroup) {
     +		pp->children[i].process.err = -1;
     +		pp->children[i].process.stdout_to_stderr = 1;
     +	}
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      
      	if (start_command(&pp->children[i].process)) {
     -		code = pp->start_failure(&pp->children[i].err,
    -+		code = pp->start_failure(ungroup ? NULL : &pp->children[i].err,
    ++		code = pp->start_failure(pp->ungroup ? NULL :
    ++					 &pp->children[i].err,
      					 pp->data,
      					 pp->children[i].data);
     -		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
     -		strbuf_reset(&pp->children[i].err);
    -+		if (!ungroup) {
    ++		if (!pp->ungroup) {
     +			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
     +			strbuf_reset(&pp->children[i].err);
     +		}
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      	return 0;
      }
      
    -+static void pp_mark_ungrouped_for_cleanup(struct parallel_processes *pp)
    -+{
    -+	int i;
    -+
    -+	if (!pp->ungroup)
    -+		BUG("only reachable if 'ungrouped'");
    -+
    -+	for (i = 0; i < pp->max_processes; i++)
    -+		pp->children[i].state = GIT_CP_WAIT_CLEANUP;
    -+}
    -+
    - static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
    - {
    - 	int i;
    - 
    -+	if (pp->ungroup)
    -+		BUG("unreachable with 'ungrouped'");
    -+
    - 	while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
    - 		if (errno == EINTR)
    - 			continue;
     @@ run-command.c: static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
      static void pp_output(struct parallel_processes *pp)
      {
      	int i = pp->output_owner;
    -+
    -+	if (pp->ungroup)
    -+		BUG("unreachable with 'ungrouped'");
     +
      	if (pp->children[i].state == GIT_CP_WORKING &&
      	    pp->children[i].err.len) {
      		strbuf_write(&pp->children[i].err, stderr);
    -@@ run-command.c: static void pp_output(struct parallel_processes *pp)
    - 
    - static int pp_collect_finished(struct parallel_processes *pp)
    - {
    -+	const int ungroup = pp->ungroup;
    - 	int i, code;
    - 	int n = pp->max_processes;
    - 	int result = 0;
     @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
    + 
      		code = finish_command(&pp->children[i].process);
      
    - 		code = pp->task_finished(code,
    --					 &pp->children[i].err, pp->data,
    --					 pp->children[i].data);
    -+					 ungroup ? NULL : &pp->children[i].err,
    -+					 pp->data, pp->children[i].data);
    +-		code = pp->task_finished(code,
    ++		code = pp->task_finished(code, pp->ungroup ? NULL :
    + 					 &pp->children[i].err, pp->data,
    + 					 pp->children[i].data);
      
    - 		if (code)
    - 			result = code;
     @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      
      		pp->nr_processes--;
    @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      		child_process_init(&pp->children[i].process);
      
     -		if (i != pp->output_owner) {
    -+		if (ungroup) {
    ++		if (pp->ungroup) {
     +			; /* no strbuf_*() work to do here */
     +		} else if (i != pp->output_owner) {
      			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
      			strbuf_reset(&pp->children[i].err);
      		} else {
     @@ run-command.c: int run_processes_parallel(int n,
    + 	int i, code;
      	int output_timeout = 100;
      	int spawn_cap = 4;
    ++	int ungroup = run_processes_parallel_ungroup;
      	struct parallel_processes pp;
    -+	const int ungroup = run_processes_parallel_ungroup;
      
     -	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
     +	/* unset for the next API user */
    @@ run-command.c: int run_processes_parallel(int n,
     -		pp_buffer_stderr(&pp, output_timeout);
     -		pp_output(&pp);
     +		if (ungroup) {
    -+			pp_mark_ungrouped_for_cleanup(&pp);
    ++			int i;
    ++
    ++			for (i = 0; i < pp.max_processes; i++)
    ++				pp.children[i].state = GIT_CP_WAIT_CLEANUP;
     +		} else {
     +			pp_buffer_stderr(&pp, output_timeout);
     +			pp_output(&pp);
    @@ t/helper/test-run-command.c: static int task_finished(int result,
      }
      
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    - 	strvec_clear(&proc.args);
    - 	strvec_pushv(&proc.args, (const char **)argv + 3);
    + 	if (!strcmp(argv[1], "run-command"))
    + 		exit(run_command(&proc));
      
    -+	if (getenv("RUN_PROCESSES_PARALLEL_UNGROUP"))
    ++	if (!strcmp(argv[1], "--ungroup")) {
    ++		argv += 1;
    ++		argc -= 1;
     +		run_processes_parallel_ungroup = 1;
    ++	}
     +
    - 	if (!strcmp(argv[1], "run-command-parallel"))
    - 		exit(run_processes_parallel(jobs, parallel_next,
    - 					    NULL, NULL, &proc));
    + 	jobs = atoi(argv[2]);
    + 	strvec_clear(&proc.args);
    + 	strvec_pushv(&proc.args, (const char **)argv + 3);
     
      ## t/t0061-run-command.sh ##
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with more jobs available than
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
    -+	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    -+	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command --ungroup run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_line_count = 8 out &&
     +	test_line_count = 4 err
     +'
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
    -+	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    -+	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command --ungroup run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_line_count = 8 out &&
     +	test_line_count = 4 err
     +'
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
    -+	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    -+	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command --ungroup run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_line_count = 8 out &&
     +	test_line_count = 4 err
     +'
    @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort grace
      '
      
     +test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
    -+	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    -+	test-tool run-command run-command-abort 3 false >out 2>err &&
    ++	test-tool run-command --ungroup run-command-abort 3 false >out 2>err &&
     +	test_must_be_empty out &&
     +	test_line_count = 6 err
     +'
    @@ t/t0061-run-command.sh: test_expect_success 'run_command outputs ' '
      '
      
     +test_expect_success 'run_command outputs (ungroup) ' '
    -+	RUN_PROCESSES_PARALLEL_UNGROUP=1 \
    -+	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command --ungroup run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
     +	test_cmp expect err
     +'
2:  8ab09f28729 ! 2:  b0f0dc7492a hook API: fix v2.36.0 regression: hooks should be connected to a TTY
    @@ Metadata
      ## Commit message ##
         hook API: fix v2.36.0 regression: hooks should be connected to a TTY
     
    -    Fix a regression reported[1] in f443246b9f2 (commit: convert
    +    Fix a regression reported[1] against f443246b9f2 (commit: convert
         {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to
         using the run_process_parallel() API in the earlier 96e7225b310 (hook:
         add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and

Extend the parallel execution API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to support a mode where the stdout and stderr of the processes isn't captured and output in a deterministic order, instead we'll leave it to the kernel and stdio to sort it out.

This gives the API same functionality as GNU parallel's --ungroup option. As we'll see in a subsequent commit the main reason to want this is to support stdout and stderr being connected to the TTY in the case of jobs=1, demonstrated here with GNU parallel:

	$ parallel --ungroup 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	TTY
	TTY
	$ parallel 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	NTTY
	NTTY

Another is as GNU parallel's documentation notes a potential for optimization. As demonstrated in next commit our results with "git hook run" will be similar, but generally speaking this shows that if you want to run processes in parallel where the exact order isn't important this can be a lot faster:

	$ hyperfine -r 3 -L o ,--ungroup 'parallel {o} seq ::: 10000000 >/dev/null '
	Benchmark 1: parallel  seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     220.2 ms ±   9.3 ms    [User: 124.9 ms, System: 96.1 ms]
	  Range (min … max):   212.3 ms … 230.5 ms    3 runs

	Benchmark 2: parallel --ungroup seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     154.7 ms ±   0.9 ms    [User: 136.2 ms, System: 25.1 ms]
	  Range (min … max):   153.9 ms … 155.7 ms    3 runs

	Summary
	  'parallel --ungroup seq ::: 10000000 >/dev/null ' ran
	    1.42 ± 0.06 times faster than 'parallel  seq ::: 10000000 >/dev/null '

A large part of the juggling in the API is to make the API safer for its maintenance and consumers alike.

For the maintenance of the API we e.g. avoid malloc()-ing the "pp->pfd", ensuring that SANITIZE=address and other similar tools will catch any unexpected misuse.

For API consumers we take pains to never pass the non-NULL "out" buffer to an API user that provided the "ungroup" option. The resulting code in t/helper/test-run-command.c isn't typical of such a user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not.

We could also avoid the strbuf_init() for "buffered_output" by having "struct parallel_processes" use a static PARALLEL_PROCESSES_INIT initializer, but let's leave that cleanup for later.

Using a global "run_processes_parallel_ungroup" variable to enable this option is rather nasty, but is being done here to produce as minimal of a change as possible for a subsequent regression fix. This change is extracted from a larger initial version[1] which ends up with a better end-state for the API, but in doing so needed to modify all existing callers of the API. Let's defer that for now, and narrowly focus on what we need for fixing the regression in the subsequent commit.

It's safe to do this with a global variable because:

 A) hook.c is the only user of it that sets it to non-zero, and before
    we'll get any other API users we'll refactor away this method of
    passing in the option, i.e. re-roll [1].

 B) Even if hook.c wasn't the only user we don't have callers of this
    API that concurrently invoke this parallel process starting API
    itself in parallel.

As noted above "A" && "B" are rather nasty, and we don't want to live with those caveats long-term, but for now they should be an acceptable compromise.

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c               | 70 +++++++++++++++++++++++++++----------
 run-command.h               | 30 ++++++++++++----
 t/helper/test-run-command.c | 22 ++++++++++--
 t/t0061-run-command.sh      | 30 ++++++++++++++++
 4 files changed, 123 insertions(+), 29 deletions(-)

Fix a regression reported[1] against f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook.

As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster:

	$ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook'
	#!/bin/sh

	seq 100000000
	Benchmark 1: ./git hook run seq-hook' in 'origin/master
	  Time (mean ± σ):     787.1 ms ±  13.6 ms    [User: 701.6 ms, System: 534.4 ms]
	  Range (min … max):   773.2 ms … 806.3 ms    10 runs

	Benchmark 2: ./git hook run seq-hook' in 'HEAD~0
	  Time (mean ± σ):     603.4 ms ±   1.6 ms    [User: 573.1 ms, System: 30.3 ms]
	  Range (min … max):   601.0 ms … 606.2 ms    10 runs

	Summary
	  './git hook run seq-hook' in 'HEAD~0' ran
	    1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c          |  1 +
 t/t1800-hook.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

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

This version looks good to me.

This change is merely a personal taste---it does not match mine but that is Meh ;-)

It may have made the resulting code easier to read if the local variable was kept as a synonym as "pp->" is short enough but is repeated often, but what is written is good enough and I do not see a need to flip-flop.

Good to see this inlined. I find the caller easier to follow without it.

Thanks for a quick succession of rerolling.  Will queue.

Hi Ævar

On 02/06/2022 15:07, Ævar Arnfjörð Bjarmason wrote:

I'm a bit confused by this, the proposed test uses this hook script

	write_script .git/hooks/pre-commit <<-EOF
	test -t 1 && echo "stdout is a TTY" >out
	test -t 2 && echo "stderr is a TTY" >>out
	EOF

if either of stderr or stdout is redirected then the corresponding "test -t" should fail and so we will detect that it is not a tty.

Best Wishes

Phillip

On Fri, Jun 03 2022, Phillip Wood wrote:

Yes, exactly, but the proposed test doesn't test that, in that case both of them are connected, the test in 2/2 does test that case.

Can that snippet bebe made to work? Sure, but I know the test I have works, and that proposed replacement didn't even pass chainlint (i.e. hasn't been run even once in our test suite). So I didn't think that trying to micro-optimize the test length was worth it in this case.

It's also getting much of that length reduction e.g. by not cleaning up after itself, which the test in 2/2 does.

Hi Ævar

On 03/06/2022 10:20, Ævar Arnfjörð Bjarmason wrote:

I think I must be missing something. As I understand it we want to check that the hook can see a tty on stdout and stderr. In the test above we'll get a line printed for each fd that is a tty. Your test always redirects one of stdout and stderr - why is it important to test that? - it feels like it is testing the shell's redirection code rather than git.

I was concerned that we had also regressed the handling of stdin but looking at (the now deleted) run_hook_ve() it used to set .no_stdin = 1 so that is unchanged in the new code.

Best Wishes

Phillip

On Fri, Jun 03 2022, Phillip Wood wrote:

Yes, I think I'm the one who was missing something.

I looked at this again and I thought I'd been testing that e.g. one of the two not returning true from isatty() wasn't making both "not TTY", i.e. that run-command.c wasn't performing some shenanigans.

But that was probably too paranoid, and in any case I couldn't find a good way to test it.

*nod*

I re-rolled a v6 just now which I think should address your comments here: https://lore.kernel.org/git/cover-v6-0.2-00000000000-20220606T170356Z-avarab@gmail.com/

I've still kept the "clean up after yourself" etc. behavior in the test, and since it was easy we now test both "git hook run" and "git commit".

Thanks a lot for the careful review.

This series fixes a v2.36.0 regression[1]. See [2] for the v5. The reasons for why a regression needs this relatively large change to move forward is discussed in past rounds, e.g. around [3]. CI at https://github.com/avar/git/actions/runs/2448496389

Changes since v5:

 * Make the hook run test more meaningful, we now test with "-t" in
   the hook, instead of redirecting one of STDOUT or STDERR.

 * Add a test for both "git hook run" and "git commit", to showh that
   the "git hook run" command and one "real" user of it agree.

1. https://lore.kernel.org/git/cover-v5-0.2-00000000000-20220602T131858Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

 hook.c                      |  1 +
 run-command.c               | 70 +++++++++++++++++++++++++++----------
 run-command.h               | 30 ++++++++++++----
 t/helper/test-run-command.c | 22 ++++++++++--
 t/t0061-run-command.sh      | 30 ++++++++++++++++
 t/t1800-hook.sh             | 31 ++++++++++++++++
 6 files changed, 155 insertions(+), 29 deletions(-)

Range-diff against v5:
1:  d018b7c4441 = 1:  45248c786d7 run-command: add an "ungroup" option to run_process_parallel()
2:  b0f0dc7492a ! 2:  503ef241a52 hook API: fix v2.36.0 regression: hooks should be connected to a TTY
    @@ t/t1800-hook.sh: test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
      '
      
     +test_hook_tty() {
    -+	local fd="$1" &&
    -+
    -+	cat >expect &&
    ++	cat >expect <<-\EOF
    ++	STDOUT TTY
    ++	STDERR TTY
    ++	EOF
     +
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +
    -+	test_hook -C repo pre-commit <<-EOF &&
    -+	{
    -+		test -t 1 && echo >&$fd STDOUT TTY || echo >&$fd STDOUT NO TTY &&
    -+		test -t 2 && echo >&$fd STDERR TTY || echo >&$fd STDERR NO TTY
    -+	} $fd>actual
    -+	EOF
    -+
     +	test_commit -C repo A &&
     +	test_commit -C repo B &&
     +	git -C repo reset --soft HEAD^ &&
    -+	test_terminal git -C repo commit -m"B.new" &&
    ++
    ++	test_hook -C repo pre-commit <<-EOF &&
    ++	test -t 1 && echo STDOUT TTY >>actual || echo STDOUT NO TTY >>actual &&
    ++	test -t 2 && echo STDERR TTY >>actual || echo STDERR NO TTY >>actual
    ++	EOF
    ++
    ++	test_terminal git "$@" &&
     +	test_cmp expect repo/actual
     +}
     +
    -+test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY: STDOUT redirect' '
    -+	test_hook_tty 1 <<-\EOF
    -+	STDOUT NO TTY
    -+	STDERR TTY
    -+	EOF
    ++test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY' '
    ++	test_hook_tty -C repo hook run pre-commit
     +'
     +
    -+test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY: STDERR redirect' '
    -+	test_hook_tty 2 <<-\EOF
    -+	STDOUT TTY
    -+	STDERR NO TTY
    -+	EOF
    ++test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' '
    ++	test_hook_tty -C repo commit -m"B.new"
     +'
     +
      test_done

Fix a regression reported[1] against f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook.

As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster:

	$ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook'
	#!/bin/sh

	seq 100000000
	Benchmark 1: ./git hook run seq-hook' in 'origin/master
	  Time (mean ± σ):     787.1 ms ±  13.6 ms    [User: 701.6 ms, System: 534.4 ms]
	  Range (min … max):   773.2 ms … 806.3 ms    10 runs

	Benchmark 2: ./git hook run seq-hook' in 'HEAD~0
	  Time (mean ± σ):     603.4 ms ±   1.6 ms    [User: 573.1 ms, System: 30.3 ms]
	  Range (min … max):   601.0 ms … 606.2 ms    10 runs

	Summary
	  './git hook run seq-hook' in 'HEAD~0' ran
	    1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'

1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/

Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 hook.c          |  1 +
 t/t1800-hook.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

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

Style.

So, when this hook is run, we'd see if STDOUT and STDERR are connected to a tty in the "actual" file.

And we run the test and see 

what happens. The test_cmp knows that the git command runs in "repo" by hardcoding repo/actual, and this helper is full of the same knowledge, so it would be easier to see what is going on if you removed "-C repo" from the two callers (below) and instead added it to where you run "git" under test_terminal (above).

Other than that, looking good.

Thanks.

Extend the parallel execution API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to support a mode where the stdout and stderr of the processes isn't captured and output in a deterministic order, instead we'll leave it to the kernel and stdio to sort it out.

This gives the API same functionality as GNU parallel's --ungroup option. As we'll see in a subsequent commit the main reason to want this is to support stdout and stderr being connected to the TTY in the case of jobs=1, demonstrated here with GNU parallel:

	$ parallel --ungroup 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	TTY
	TTY
	$ parallel 'test -t {} && echo TTY || echo NTTY' ::: 1 2
	NTTY
	NTTY

Another is as GNU parallel's documentation notes a potential for optimization. As demonstrated in next commit our results with "git hook run" will be similar, but generally speaking this shows that if you want to run processes in parallel where the exact order isn't important this can be a lot faster:

	$ hyperfine -r 3 -L o ,--ungroup 'parallel {o} seq ::: 10000000 >/dev/null '
	Benchmark 1: parallel  seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     220.2 ms ±   9.3 ms    [User: 124.9 ms, System: 96.1 ms]
	  Range (min … max):   212.3 ms … 230.5 ms    3 runs

	Benchmark 2: parallel --ungroup seq ::: 10000000 >/dev/null
	  Time (mean ± σ):     154.7 ms ±   0.9 ms    [User: 136.2 ms, System: 25.1 ms]
	  Range (min … max):   153.9 ms … 155.7 ms    3 runs

	Summary
	  'parallel --ungroup seq ::: 10000000 >/dev/null ' ran
	    1.42 ± 0.06 times faster than 'parallel  seq ::: 10000000 >/dev/null '

A large part of the juggling in the API is to make the API safer for its maintenance and consumers alike.

For the maintenance of the API we e.g. avoid malloc()-ing the "pp->pfd", ensuring that SANITIZE=address and other similar tools will catch any unexpected misuse.

For API consumers we take pains to never pass the non-NULL "out" buffer to an API user that provided the "ungroup" option. The resulting code in t/helper/test-run-command.c isn't typical of such a user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not.

We could also avoid the strbuf_init() for "buffered_output" by having "struct parallel_processes" use a static PARALLEL_PROCESSES_INIT initializer, but let's leave that cleanup for later.

Using a global "run_processes_parallel_ungroup" variable to enable this option is rather nasty, but is being done here to produce as minimal of a change as possible for a subsequent regression fix. This change is extracted from a larger initial version[1] which ends up with a better end-state for the API, but in doing so needed to modify all existing callers of the API. Let's defer that for now, and narrowly focus on what we need for fixing the regression in the subsequent commit.

It's safe to do this with a global variable because:

 A) hook.c is the only user of it that sets it to non-zero, and before
    we'll get any other API users we'll refactor away this method of
    passing in the option, i.e. re-roll [1].

 B) Even if hook.c wasn't the only user we don't have callers of this
    API that concurrently invoke this parallel process starting API
    itself in parallel.

As noted above "A" && "B" are rather nasty, and we don't want to live with those caveats long-term, but for now they should be an acceptable compromise.

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c               | 70 +++++++++++++++++++++++++++----------
 run-command.h               | 30 ++++++++++++----
 t/helper/test-run-command.c | 22 ++++++++++--
 t/t0061-run-command.sh      | 30 ++++++++++++++++
 4 files changed, 123 insertions(+), 29 deletions(-)

On Tue, Jun 07, 2022 at 10:48:19AM +0200, Ævar Arnfjörð Bjarmason wrote:

FYI, this broke for us downstream where we are carrying patches adding a 'pp_buffer_stdin()' and friends to enable stdin buffering to parallel processes. It also appears to break when pp.max_processes exceeds the number of actual tasks provided. I needed to add something like this

+    if (pp.children[i].state == GIT_CP_WORKING &&
+        !pp.children[i].process.in)
+            pp.children[i].state = GIT_CP_WAIT_CLEANUP;

That is, only set the WAIT_CLEANUP state if the task wasn't waiting to be given work (GIT_CP_FREE -> GIT_CP_WAIT_CLEANUP leads to some "error: waitpid is confused" errors) and if stdin is not currently being buffered. In the case where .process.in is > 0, GIT_CP_WAIT_CLEANUP causes pp_collect_finished() to stop spinning in this IO buffer loop, but there is still stdin to pass along.

Anyway, I think the first part (GIT_CP_FREE -> GIT_CP_WAIT_CLEANUP) is a bug that matters for the patch as it is now; the stdin bit will not matter until the later config-based-hooks patches which introduce stdin buffering anyway.

Thanks very much for this fix otherwise.

 - Emily

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

Thanks for a careful review and a timely response.

Puzzling.  Perhaps copy-and-paste mistake we can ignore.