| Message ID | cover-v4-0.2-00000000000-20220531T173005Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hook API: connect hooks to the TTY again, fixes a v2.36.0 regression | expand |
Hi Ævar, On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote: > Æ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 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
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