From a3d1f391d35762162356201028fb73774a6c4a8b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 15 Jan 2026 11:12:53 -0800 Subject: Revert "Merge branch 'ar/run-command-hook'" This reverts commit f406b8955295d01089ba2baf35eceadff2d11cae, reversing changes made to 1627809eeff75e6ec936fc609e7be46d5eb2fa9e. It seems to have caused a few regressions, two of the three known ones we have proposed solutions for. Let's give ourselves a bit more room to maneuver during the pre-release freeze period and restart once the 2.53 ships. --- run-command.c | 142 ++++++++++------------------------------------------------ 1 file changed, 24 insertions(+), 118 deletions(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index 2d3c2ac55c..e3e02475cc 100644 --- a/run-command.c +++ b/run-command.c @@ -1478,32 +1478,15 @@ enum child_state { GIT_CP_WAIT_CLEANUP, }; -struct parallel_child { - enum child_state state; - struct child_process process; - struct strbuf err; - void *data; -}; - -static int child_is_working(const struct parallel_child *pp_child) -{ - return pp_child->state == GIT_CP_WORKING; -} - -static int child_is_ready_for_cleanup(const struct parallel_child *pp_child) -{ - return child_is_working(pp_child) && !pp_child->process.in; -} - -static int child_is_receiving_input(const struct parallel_child *pp_child) -{ - return child_is_working(pp_child) && pp_child->process.in > 0; -} - struct parallel_processes { size_t nr_processes; - struct parallel_child *children; + struct { + enum child_state state; + struct child_process process; + struct strbuf err; + void *data; + } *children; /* * The struct pollfd is logically part of *children, * but the system call expects it as its own array. @@ -1526,7 +1509,7 @@ static void kill_children(const struct parallel_processes *pp, int signo) { for (size_t i = 0; i < opts->processes; i++) - if (child_is_working(&pp->children[i])) + if (pp->children[i].state == GIT_CP_WORKING) kill(pp->children[i].process.pid, signo); } @@ -1595,10 +1578,7 @@ 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. */ - if (opts->consume_output) - opts->consume_output(&pp->buffered_output, opts->data); - else - strbuf_write(&pp->buffered_output, stderr); + strbuf_write(&pp->buffered_output, stderr); strbuf_release(&pp->buffered_output); sigchain_pop_common(); @@ -1672,44 +1652,6 @@ static int pp_start_one(struct parallel_processes *pp, return 0; } -static void pp_buffer_stdin(struct parallel_processes *pp, - const struct run_process_parallel_opts *opts) -{ - /* Buffer stdin for each pipe. */ - for (size_t i = 0; i < opts->processes; i++) { - struct child_process *proc = &pp->children[i].process; - int ret; - - if (!child_is_receiving_input(&pp->children[i])) - continue; - - /* - * child input is provided via path_to_stdin when the feed_pipe cb is - * missing, so we just signal an EOF. - */ - if (!opts->feed_pipe) { - close(proc->in); - proc->in = 0; - continue; - } - - /** - * Feed the pipe: - * ret < 0 means error - * ret == 0 means there is more data to be fed - * ret > 0 means feeding finished - */ - ret = opts->feed_pipe(proc->in, opts->data, pp->children[i].data); - if (ret < 0) - die_errno("feed_pipe"); - - if (ret) { - close(proc->in); - proc->in = 0; - } - } -} - static void pp_buffer_stderr(struct parallel_processes *pp, const struct run_process_parallel_opts *opts, int output_timeout) @@ -1723,7 +1665,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp, /* Buffer output from all pipes. */ for (size_t i = 0; i < opts->processes; i++) { - if (child_is_working(&pp->children[i]) && + if (pp->children[i].state == GIT_CP_WORKING && pp->pfd[i].revents & (POLLIN | POLLHUP)) { int n = strbuf_read_once(&pp->children[i].err, pp->children[i].process.err, 0); @@ -1737,17 +1679,13 @@ static void pp_buffer_stderr(struct parallel_processes *pp, } } -static void pp_output(const struct parallel_processes *pp, - const struct run_process_parallel_opts *opts) +static void pp_output(const struct parallel_processes *pp) { size_t i = pp->output_owner; - if (child_is_working(&pp->children[i]) && + if (pp->children[i].state == GIT_CP_WORKING && pp->children[i].err.len) { - if (opts->consume_output) - opts->consume_output(&pp->children[i].err, opts->data); - else - strbuf_write(&pp->children[i].err, stderr); + strbuf_write(&pp->children[i].err, stderr); strbuf_reset(&pp->children[i].err); } } @@ -1784,7 +1722,6 @@ static int pp_collect_finished(struct parallel_processes *pp, pp->children[i].state = GIT_CP_FREE; if (pp->pfd) pp->pfd[i].fd = -1; - pp->children[i].process.in = 0; child_process_init(&pp->children[i].process); if (opts->ungroup) { @@ -1795,15 +1732,11 @@ static int pp_collect_finished(struct parallel_processes *pp, } else { const size_t n = opts->processes; - /* Output errors, then all other finished child processes */ - if (opts->consume_output) { - opts->consume_output(&pp->children[i].err, opts->data); - opts->consume_output(&pp->buffered_output, opts->data); - } else { - strbuf_write(&pp->children[i].err, stderr); - strbuf_write(&pp->buffered_output, stderr); - } + strbuf_write(&pp->children[i].err, stderr); strbuf_reset(&pp->children[i].err); + + /* Output all other finished child processes */ + strbuf_write(&pp->buffered_output, stderr); strbuf_reset(&pp->buffered_output); /* @@ -1815,7 +1748,7 @@ static int pp_collect_finished(struct parallel_processes *pp, * running process time. */ for (i = 0; i < n; i++) - if (child_is_working(&pp->children[(pp->output_owner + i) % n])) + if (pp->children[(pp->output_owner + i) % n].state == GIT_CP_WORKING) break; pp->output_owner = (pp->output_owner + i) % n; } @@ -1823,27 +1756,6 @@ static int pp_collect_finished(struct parallel_processes *pp, return result; } -static void pp_handle_child_IO(struct parallel_processes *pp, - const struct run_process_parallel_opts *opts, - int output_timeout) -{ - /* - * First push input, if any (it might no-op), to child tasks to avoid them blocking - * after input. This also prevents deadlocks when ungrouping below, if a child blocks - * while the parent also waits for them to finish. - */ - pp_buffer_stdin(pp, opts); - - if (opts->ungroup) { - for (size_t i = 0; i < opts->processes; i++) - if (child_is_ready_for_cleanup(&pp->children[i])) - pp->children[i].state = GIT_CP_WAIT_CLEANUP; - } else { - pp_buffer_stderr(pp, opts, output_timeout); - pp_output(pp, opts); - } -} - void run_processes_parallel(const struct run_process_parallel_opts *opts) { int i, code; @@ -1863,16 +1775,6 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) "max:%"PRIuMAX, (uintmax_t)opts->processes); - if (opts->ungroup && opts->consume_output) - BUG("ungroup and reading output are mutualy exclusive"); - - /* - * Child tasks might receive input via stdin, terminating early (or not), so - * ignore the default SIGPIPE which gets handled by each feed_pipe_fn which - * actually writes the data to children stdin fds. - */ - sigchain_push(SIGPIPE, SIG_IGN); - pp_init(&pp, opts, &pp_sig); while (1) { for (i = 0; @@ -1890,7 +1792,13 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) } if (!pp.nr_processes) break; - pp_handle_child_IO(&pp, opts, output_timeout); + if (opts->ungroup) { + for (size_t i = 0; i < opts->processes; i++) + pp.children[i].state = GIT_CP_WAIT_CLEANUP; + } else { + pp_buffer_stderr(&pp, opts, output_timeout); + pp_output(&pp); + } code = pp_collect_finished(&pp, opts); if (code) { pp.shutdown = 1; @@ -1901,8 +1809,6 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) pp_cleanup(&pp, opts); - sigchain_pop(SIGPIPE); - if (do_trace2) trace2_region_leave(tr2_category, tr2_label, NULL); } -- cgit v1.3