git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Emily Shaffer <emilyshaffer@google.com>
Date
Apr 20, 2022, 16:22 UTC
Message-ID
<CAJoAoZ=ysz6GDjUVCzUC-4OEwkyfrUzDyAws1xPbKXeLufUa0w@mail.gmail.com>
In-Reply-To
<220420.86wnfk6isy.gmgdl@evledraar.gmail.com>

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

Show 27 quoted lines
> >> 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?
> >
> > 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.
>
> 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...

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.

Show 42 quoted lines
>
> 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:
>
> diff --git a/hook.c b/hook.c
> index eadb2d58a7b..1f20e5db447 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -126,6 +126,7 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
>         struct run_process_parallel_opts run_opts = {
>                 .tr2_category = "hook",
>                 .tr2_label = hook_name,
> +               .no_buffering = 1,
>         };
>
>         if (!options)
> diff --git a/run-command.c b/run-command.c
> index 2383375ee07..0f9d84433ad 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1604,7 +1604,7 @@ static void pp_cleanup(struct parallel_processes *pp)
>   * <0 no new job was started, user wishes to shutdown early. Use negative code
>   *    to signal the children.
>   */
> -static int pp_start_one(struct parallel_processes *pp)
> +static int pp_start_one(struct parallel_processes *pp, const int no_buffering)
>  {
>         int i, code;
>
> @@ -1623,9 +1623,12 @@ static int pp_start_one(struct parallel_processes *pp)
>                 strbuf_reset(&pp->children[i].err);
>                 return 1;
>         }
> -       pp->children[i].process.err = -1;
> -       pp->children[i].process.stdout_to_stderr = 1;
> -       pp->children[i].process.no_stdin = 1;
> +
> +       if (!no_buffering) {
> +               pp->children[i].process.err = -1;
> +               pp->children[i].process.stdout_to_stderr = 1;
> +               pp->children[i].process.no_stdin = 1;
> +       }

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 :)

Show 206 quoted lines
>
>         if (start_command(&pp->children[i].process)) {
>                 code = pp->start_failure(&pp->children[i].err,
> @@ -1681,12 +1684,17 @@ static void pp_output(struct parallel_processes *pp)
>         }
>  }
>
> -static int pp_collect_finished(struct parallel_processes *pp)
> +static int pp_collect_finished(struct parallel_processes *pp,
> +                              const int no_buffering)
>  {
>         int i, code;
>         int n = pp->max_processes;
>         int result = 0;
>
> +       if (no_buffering)
> +               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)
> @@ -1741,7 +1749,7 @@ static int pp_collect_finished(struct parallel_processes *pp)
>  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)
> +                                   void *pp_cb, const int no_buffering)
>  {
>         int i, code;
>         int output_timeout = 100;
> @@ -1754,7 +1762,7 @@ static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
>                     i < spawn_cap && !pp.shutdown &&
>                     pp.nr_processes < pp.max_processes;
>                     i++) {
> -                       code = pp_start_one(&pp);
> +                       code = pp_start_one(&pp, no_buffering);
>                         if (!code)
>                                 continue;
>                         if (code < 0) {
> @@ -1765,9 +1773,11 @@ static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
>                 }
>                 if (!pp.nr_processes)
>                         break;
> -               pp_buffer_stderr(&pp, output_timeout);
> -               pp_output(&pp);
> -               code = pp_collect_finished(&pp);
> +               if (!no_buffering) {
> +                       pp_buffer_stderr(&pp, output_timeout);
> +                       pp_output(&pp);
> +               }
> +               code = pp_collect_finished(&pp, no_buffering);
>                 if (code) {
>                         pp.shutdown = 1;
>                         if (code < 0)
> @@ -1783,7 +1793,8 @@ 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, const char *tr2_category,
> -                                     const char *tr2_label)
> +                                     const char *tr2_label,
> +                                     const int no_buffering)
>  {
>         int result;
>
> @@ -1791,7 +1802,7 @@ static int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
>                                    ((n < 1) ? online_cpus() : n));
>
>         result = run_processes_parallel_1(n, get_next_task, start_failure,
> -                                         task_finished, pp_cb);
> +                                         task_finished, pp_cb, no_buffering);
>
>         trace2_region_leave(tr2_category, tr2_label, NULL);
>
> @@ -1803,6 +1814,8 @@ int run_processes_parallel(int n, get_next_task_fn get_next_task,
>                            task_finished_fn task_finished, void *pp_cb,
>                            struct run_process_parallel_opts *opts)
>  {
> +       const int no_buffering = opts && opts->no_buffering;
> +
>         if (!opts)
>                 goto no_opts;
>
> @@ -1811,12 +1824,13 @@ int run_processes_parallel(int n, get_next_task_fn get_next_task,
>                 return run_processes_parallel_tr2(n, get_next_task,
>                                                   start_failure, task_finished,
>                                                   pp_cb, opts->tr2_category,
> -                                                 opts->tr2_label);
> +                                                 opts->tr2_label,
> +                                                 no_buffering);
>         }
>
>  no_opts:
>         return run_processes_parallel_1(n, get_next_task, start_failure,
> -                                       task_finished, pp_cb);
> +                                       task_finished, pp_cb, no_buffering);
>  }
>
>
> diff --git a/run-command.h b/run-command.h
> index 9ec57a25de4..062eff81e17 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -463,11 +463,17 @@ 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.
> + *
> + * no_buffering: Don't redirect stderr to stdout, and don't "buffer"
> + * the output of the N children started. The output will not be
> + * deterministic and may be interleaved, but we won't interfere with
> + * the connection to the TTY.
>   */
>  struct run_process_parallel_opts
>  {
>         const char *tr2_category;
>         const char *tr2_label;
> +       unsigned int no_buffering:1;
>  };
>
>  /**
> @@ -477,7 +483,8 @@ 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
> - * from different tasks does not interleave.
> + * from different tasks does not interleave. This can be disabled by setting
> + * "no_buffering" in "struct run_process_parallel_opts".
>   *
>   * start_failure_fn and task_finished_fn can be NULL to omit any
>   * special handling.
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index ee281909bc3..fb6ad0bf4f7 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -130,7 +130,7 @@ World
>  EOF
>
>  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" >actual 2>&1 &&
>         test_cmp expect actual
>  '
>
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 26ed5e11bc8..c0eda4e9237 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -4,6 +4,7 @@ test_description='git-hook command'
>
>  TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-terminal.sh
>
>  test_expect_success 'git hook usage' '
>         test_expect_code 129 git hook &&
> @@ -120,4 +121,49 @@ test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
>         test_cmp expect actual
>  '
>
> +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY: STDOUT redirect' '
> +       rm -rf .git &&
> +       test_when_finished "rm -rf .git" &&
> +       git init . &&
> +
> +       test_hook pre-commit <<-EOF &&
> +       {
> +               test -t 1 && echo STDOUT TTY || echo STDOUT NO TTY &&
> +               test -t 2 && echo STDERR TTY || echo STDERR NO TTY
> +       } >actual
> +       EOF
> +
> +       test_commit A &&
> +       test_commit B &&
> +       git reset --soft HEAD^ &&
> +       cat >expect <<-\EOF &&
> +       STDOUT NO TTY
> +       STDERR TTY
> +       EOF
> +       test_terminal git commit -m"msg" &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY: STDERR redirect' '
> +       test_when_finished "rm -rf .git" &&
> +       git init . &&
> +
> +       test_hook pre-commit <<-EOF &&
> +       {
> +               test -t 1 && echo >&2 STDOUT TTY || echo >&2 STDOUT NO TTY &&
> +               test -t 2 && echo >&2 STDERR TTY || echo >&2 STDERR NO TTY
> +       } 2>actual
> +       EOF
> +
> +       test_commit A &&
> +       test_commit B &&
> +       git reset --soft HEAD^ &&
> +       cat >expect <<-\EOF &&
> +       STDOUT TTY
> +       STDERR NO TTY
> +       EOF
> +       test_terminal git commit -m"msg" &&
> +       test_cmp expect actual
> +'
> +
>  test_done
Previous: Ævar Arnfjörð BjarmasonNext: Junio C Hamano
Message 6 of 85 in “git 2.36.0 regression: pre-commit hooks no longer have stdout/stderr as tty”
  1. Anthony SottileApr 19, 2022
  2. Emily ShafferApr 19, 2022
  3. Anthony SottileApr 19, 2022
  4. Phillip WoodApr 20, 2022
  5. Ævar Arnfjörð BjarmasonApr 20, 2022
  6. Emily ShafferApr 20, 2022
  7. Junio C HamanoApr 20, 2022
  8. Emily ShafferApr 20, 2022
  9. Junio C HamanoApr 20, 2022
  10. Emily ShafferApr 20, 2022
  11. Ævar Arnfjörð BjarmasonApr 21, 2022
  12. Junio C HamanoApr 21, 2022
  13. Junio C HamanoApr 21, 2022
  14. Junio C HamanoApr 20, 2022
  15. 0/6 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, Apr 21, 2022
  16. 1/6 run-command API: replace run_processes_parallel_tr2() with opts structÆvar Arnfjörð Bjarmason, Apr 21, 2022
  17. Junio C HamanoApr 23, 2022
  18. Emily ShafferApr 28, 2022
  19. Junio C HamanoApr 29, 2022
  20. 2/6 run-command tests: test stdout of run_command_parallel()Ævar Arnfjörð Bjarmason, Apr 21, 2022
  21. Junio C HamanoApr 23, 2022
  22. 6/6 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, Apr 21, 2022
  23. Emily ShafferApr 28, 2022
  24. Junio C HamanoApr 29, 2022
  25. 3/6 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, Apr 21, 2022
  26. Junio C HamanoApr 23, 2022
  27. Emily ShafferApr 28, 2022
  28. 4/6 hook tests: fix redirection logic error in 96e7225b310Ævar Arnfjörð Bjarmason, Apr 21, 2022
  29. Junio C HamanoApr 23, 2022
  30. 5/6 hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"Ævar Arnfjörð Bjarmason, Apr 21, 2022
  31. Junio C HamanoApr 29, 2022
  32. Junio C HamanoApr 21, 2022
  33. Ævar Arnfjörð BjarmasonApr 21, 2022
  34. 0/8 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, May 18, 2022
  35. 1/8 run-command tests: change if/if/... to if/else if/elseÆvar Arnfjörð Bjarmason, May 18, 2022
  36. 2/8 run-command API: use "opts" struct for run_processes_parallel{,_tr2}()Ævar Arnfjörð Bjarmason, May 18, 2022
  37. Junio C HamanoMay 18, 2022
  38. Emily ShafferMay 25, 2022
  39. 3/8 run-command tests: test stdout of run_command_parallel()Ævar Arnfjörð Bjarmason, May 18, 2022
  40. 4/8 run-command.c: add an initializer for "struct parallel_processes"Ævar Arnfjörð Bjarmason, May 18, 2022
  41. 7/8 hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"Ævar Arnfjörð Bjarmason, May 18, 2022
  42. 6/8 hook tests: fix redirection logic error in 96e7225b310Ævar Arnfjörð Bjarmason, May 18, 2022
  43. 8/8 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, May 18, 2022
  44. Junio C HamanoMay 18, 2022
  45. Emily ShafferMay 26, 2022
  46. Ævar Arnfjörð BjarmasonMay 26, 2022
  47. Emily ShafferMay 26, 2022
  48. 5/8 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, May 18, 2022
  49. Junio C HamanoMay 18, 2022
  50. Emily ShafferMay 26, 2022
  51. Junio C HamanoMay 27, 2022
  52. Johannes SchindelinMay 25, 2022
  53. Ævar Arnfjörð BjarmasonMay 25, 2022
  54. Junio C HamanoMay 25, 2022
  55. Junio C HamanoMay 26, 2022
  56. Ævar Arnfjörð BjarmasonMay 26, 2022
  57. Junio C HamanoMay 26, 2022
  58. Ævar Arnfjörð BjarmasonMay 26, 2022
  59. Junio C HamanoMay 26, 2022
  60. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, May 27, 2022
  61. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, May 27, 2022
  62. Junio C HamanoMay 27, 2022
  63. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, May 27, 2022
  64. Junio C HamanoMay 27, 2022
  65. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, May 31, 2022
  66. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, May 31, 2022
  67. Johannes SchindelinJun 1, 2022
  68. Junio C HamanoJun 1, 2022
  69. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, May 31, 2022
  70. Johannes SchindelinJun 1, 2022
  71. Johannes SchindelinJun 1, 2022
  72. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, Jun 2, 2022
  73. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, Jun 2, 2022
  74. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, Jun 2, 2022
  75. Junio C HamanoJun 2, 2022
  76. Phillip WoodJun 3, 2022
  77. Ævar Arnfjörð BjarmasonJun 3, 2022
  78. Phillip WoodJun 3, 2022
  79. Ævar Arnfjörð BjarmasonJun 7, 2022
  80. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, Jun 7, 2022
  81. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, Jun 7, 2022
  82. Junio C HamanoJun 7, 2022
  83. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, Jun 7, 2022
  84. Emily ShafferJun 17, 2022
  85. Junio C HamanoJun 7, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.