From c9ef360403e0f64a5d4c70f66a9b25f02a98fda6 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:16 +0200 Subject: [PATCH 01/12] t1800: add hook output stream tests Lack of test coverage in this area led to some regressions while converting the remaining hooks to the newer hook.[ch] API. Add some tests to verify hooks write to the expected output streams. Suggested-by: Patrick Steinhardt Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- t/t1800-hook.sh | 137 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 4feaf0d7be..ed28a2fadb 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -184,4 +184,141 @@ test_expect_success 'stdin to hooks' ' test_cmp expect actual ' +check_stdout_separate_from_stderr () { + for hook in "$@" + do + # Ensure hook's stdout is only in stdout, not stderr + test_grep "Hook $hook stdout" stdout.actual || return 1 + test_grep ! "Hook $hook stdout" stderr.actual || return 1 + + # Ensure hook's stderr is only in stderr, not stdout + test_grep "Hook $hook stderr" stderr.actual || return 1 + test_grep ! "Hook $hook stderr" stdout.actual || return 1 + done +} + +check_stdout_merged_to_stderr () { + for hook in "$@" + do + # Ensure hook's stdout is only in stderr, not stdout + test_grep "Hook $hook stdout" stderr.actual || return 1 + test_grep ! "Hook $hook stdout" stdout.actual || return 1 + + # Ensure hook's stderr is only in stderr, not stdout + test_grep "Hook $hook stderr" stderr.actual || return 1 + test_grep ! "Hook $hook stderr" stdout.actual || return 1 + done +} + +setup_hooks () { + for hook in "$@" + do + test_hook $hook <<-EOF + echo >&1 Hook $hook stdout + echo >&2 Hook $hook stderr + EOF + done +} + +test_expect_success 'client hooks: pre-push expects separate stdout and stderr' ' + test_when_finished "rm -f stdout.actual stderr.actual" && + git init --bare remote && + git remote add origin remote && + test_commit A && + setup_hooks pre-push && + git push origin HEAD:main >stdout.actual 2>stderr.actual && + check_stdout_separate_from_stderr pre-push +' + +test_expect_success 'client hooks: commit hooks expect stdout redirected to stderr' ' + hooks="pre-commit prepare-commit-msg \ + commit-msg post-commit \ + reference-transaction" && + setup_hooks $hooks && + test_when_finished "rm -f stdout.actual stderr.actual" && + git checkout -B main && + git checkout -b branch-a && + test_commit commit-on-branch-a && + git commit --allow-empty -m "Test" >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr $hooks +' + +test_expect_success 'client hooks: checkout hooks expect stdout redirected to stderr' ' + setup_hooks post-checkout reference-transaction && + test_when_finished "rm -f stdout.actual stderr.actual" && + git checkout -b new-branch main >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr post-checkout reference-transaction +' + +test_expect_success 'client hooks: merge hooks expect stdout redirected to stderr' ' + setup_hooks pre-merge-commit post-merge reference-transaction && + test_when_finished "rm -f stdout.actual stderr.actual" && + test_commit new-branch-commit && + git merge --no-ff branch-a >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr pre-merge-commit post-merge reference-transaction +' + +test_expect_success 'client hooks: post-rewrite hooks expect stdout redirected to stderr' ' + setup_hooks post-rewrite reference-transaction && + test_when_finished "rm -f stdout.actual stderr.actual" && + git commit --amend --allow-empty --no-edit >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr post-rewrite reference-transaction +' + +test_expect_success 'client hooks: applypatch hooks expect stdout redirected to stderr' ' + setup_hooks applypatch-msg pre-applypatch post-applypatch && + test_when_finished "rm -f stdout.actual stderr.actual" && + git checkout -b branch-b main && + test_commit branch-b && + git format-patch -1 --stdout >patch && + git checkout -b branch-c main && + git am patch >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr applypatch-msg pre-applypatch post-applypatch +' + +test_expect_success 'client hooks: rebase hooks expect stdout redirected to stderr' ' + setup_hooks pre-rebase && + test_when_finished "rm -f stdout.actual stderr.actual" && + git checkout -b branch-d main && + test_commit branch-d && + git checkout main && + test_commit diverge-main && + git checkout branch-d && + git rebase main >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr pre-rebase +' + +test_expect_success 'client hooks: post-index-change expects stdout redirected to stderr' ' + setup_hooks post-index-change && + test_when_finished "rm -f stdout.actual stderr.actual" && + oid=$(git hash-object -w --stdin stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr post-index-change +' + +test_expect_success 'server hooks expect stdout redirected to stderr' ' + test_when_finished "rm -f stdout.actual stderr.actual" && + git init --bare remote-server && + git remote add origin-server remote-server && + cd remote-server && + setup_hooks pre-receive update post-receive post-update && + cd .. && + git push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr pre-receive update post-receive post-update +' + +test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' ' + test_when_finished "rm -f stdout.actual stderr.actual" && + git init server && + git -C server checkout -b main && + test_config -C server receive.denyCurrentBranch updateInstead && + git remote add origin-server-2 server && + cd server && + setup_hooks push-to-checkout && + cd .. && + git push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr push-to-checkout +' + test_done From dee82860688cdcd35cbf0ce7e74736e473c5b697 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:17 +0200 Subject: [PATCH 02/12] run-command: add helper for pp child states There is a recurring pattern of testing parallel process child states and file descriptors to determine if a child is running, receiving any input or if it's ready for cleanup. Name the pp_child structure and introduce a helper to make the checks more readable. Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- run-command.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/run-command.c b/run-command.c index e3e02475cc..3989673569 100644 --- a/run-command.c +++ b/run-command.c @@ -1478,15 +1478,22 @@ 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; +} + struct parallel_processes { size_t nr_processes; - struct { - enum child_state state; - struct child_process process; - struct strbuf err; - void *data; - } *children; + struct parallel_child *children; /* * The struct pollfd is logically part of *children, * but the system call expects it as its own array. @@ -1509,7 +1516,7 @@ static void kill_children(const struct parallel_processes *pp, int signo) { for (size_t i = 0; i < opts->processes; i++) - if (pp->children[i].state == GIT_CP_WORKING) + if (child_is_working(&pp->children[i])) kill(pp->children[i].process.pid, signo); } @@ -1665,7 +1672,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 (pp->children[i].state == GIT_CP_WORKING && + if (child_is_working(&pp->children[i]) && pp->pfd[i].revents & (POLLIN | POLLHUP)) { int n = strbuf_read_once(&pp->children[i].err, pp->children[i].process.err, 0); @@ -1683,7 +1690,7 @@ static void pp_output(const struct parallel_processes *pp) { size_t i = pp->output_owner; - if (pp->children[i].state == GIT_CP_WORKING && + if (child_is_working(&pp->children[i]) && pp->children[i].err.len) { strbuf_write(&pp->children[i].err, stderr); strbuf_reset(&pp->children[i].err); @@ -1748,7 +1755,7 @@ static int pp_collect_finished(struct parallel_processes *pp, * running process time. */ for (i = 0; i < n; i++) - if (pp->children[(pp->output_owner + i) % n].state == GIT_CP_WORKING) + if (child_is_working(&pp->children[(pp->output_owner + i) % n])) break; pp->output_owner = (pp->output_owner + i) % n; } From ec0becacc9847406f2b0147a81f62e023b006351 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Wed, 28 Jan 2026 23:39:18 +0200 Subject: [PATCH 03/12] run-command: add stdin callback for parallelization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a user of the run_processes_parallel() API wants to pipe a large amount of information to the stdin of each parallel command, that data could exceed the pipe buffer of the process's stdin and can be too big to store in-memory via strbuf & friends or to slurp to a file. Generally this is solved by repeatedly writing to child_process.in between calls to start_command() and finish_command(). For a specific pre-existing example of this, see transport.c:run_pre_push_hook(). This adds a generic callback API to run_processes_parallel() to do exactly that in a unified manner, similar to the existing callback APIs, which can then be used by hooks.h to convert the remaining hooks to the new, simpler parallel interface. Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- run-command.c | 87 ++++++++++++++++++++++++++++++++++--- run-command.h | 21 +++++++++ t/helper/test-run-command.c | 52 +++++++++++++++++++++- t/t0061-run-command.sh | 31 +++++++++++++ 4 files changed, 182 insertions(+), 9 deletions(-) diff --git a/run-command.c b/run-command.c index 3989673569..aaf0e4ecee 100644 --- a/run-command.c +++ b/run-command.c @@ -1490,6 +1490,16 @@ 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; @@ -1659,6 +1669,44 @@ 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) @@ -1729,6 +1777,7 @@ 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) { @@ -1763,6 +1812,27 @@ 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); + } +} + void run_processes_parallel(const struct run_process_parallel_opts *opts) { int i, code; @@ -1782,6 +1852,13 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) "max:%"PRIuMAX, (uintmax_t)opts->processes); + /* + * 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; @@ -1799,13 +1876,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) } if (!pp.nr_processes) break; - 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); - } + pp_handle_child_IO(&pp, opts, output_timeout); code = pp_collect_finished(&pp, opts); if (code) { pp.shutdown = 1; @@ -1816,6 +1887,8 @@ 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); } diff --git a/run-command.h b/run-command.h index 0df25e445f..e1ca965b5b 100644 --- a/run-command.h +++ b/run-command.h @@ -420,6 +420,21 @@ typedef int (*start_failure_fn)(struct strbuf *out, void *pp_cb, void *pp_task_cb); +/** + * This callback is repeatedly called on every child process who requests + * start_command() to create a pipe by setting child_process.in < 0. + * + * pp_cb is the callback cookie as passed into run_processes_parallel, and + * pp_task_cb is the callback cookie as passed into get_next_task_fn. + * + * Returns < 0 for error + * Returns == 0 when there is more data to be fed (will be called again) + * Returns > 0 when finished (child closed fd or no more data to be fed) + */ +typedef int (*feed_pipe_fn)(int child_in, + void *pp_cb, + void *pp_task_cb); + /** * This callback is called on every child process that finished processing. * @@ -473,6 +488,12 @@ struct run_process_parallel_opts */ start_failure_fn start_failure; + /* + * feed_pipe: see feed_pipe_fn() above. This can be NULL to omit any + * special handling. + */ + feed_pipe_fn feed_pipe; + /** * task_finished: See task_finished_fn() above. This can be * NULL to omit any special handling. diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 3719f23cc2..4a56456894 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -23,19 +23,26 @@ static int number_callbacks; static int parallel_next(struct child_process *cp, struct strbuf *err, void *cb, - void **task_cb UNUSED) + void **task_cb) { struct child_process *d = cb; if (number_callbacks >= 4) return 0; strvec_pushv(&cp->args, d->args.v); + cp->in = d->in; + cp->no_stdin = d->no_stdin; if (err) strbuf_addstr(err, "preloaded output of a child\n"); else fprintf(stderr, "preloaded output of a child\n"); number_callbacks++; + + /* test_stdin callback will use this to count remaining lines */ + *task_cb = xmalloc(sizeof(int)); + *(int*)(*task_cb) = 2; + return 1; } @@ -54,15 +61,48 @@ static int no_job(struct child_process *cp UNUSED, static int task_finished(int result UNUSED, struct strbuf *err, void *pp_cb UNUSED, - void *pp_task_cb UNUSED) + void *pp_task_cb) { if (err) strbuf_addstr(err, "asking for a quick stop\n"); else fprintf(stderr, "asking for a quick stop\n"); + + FREE_AND_NULL(pp_task_cb); + return 1; } +static int task_finished_quiet(int result UNUSED, + struct strbuf *err UNUSED, + void *pp_cb UNUSED, + void *pp_task_cb) +{ + FREE_AND_NULL(pp_task_cb); + return 0; +} + +static int test_stdin_pipe_feed(int hook_stdin_fd, void *cb UNUSED, void *task_cb) +{ + int *lines_remaining = task_cb; + + if (*lines_remaining) { + struct strbuf buf = STRBUF_INIT; + strbuf_addf(&buf, "sample stdin %d\n", --(*lines_remaining)); + if (write_in_full(hook_stdin_fd, buf.buf, buf.len) < 0) { + if (errno == EPIPE) { + /* child closed stdin, nothing more to do */ + strbuf_release(&buf); + return 1; + } + die_errno("write"); + } + strbuf_release(&buf); + } + + return !(*lines_remaining); +} + struct testsuite { struct string_list tests, failed; int next; @@ -157,6 +197,7 @@ static int testsuite(int argc, const char **argv) struct run_process_parallel_opts opts = { .get_next_task = next_test, .start_failure = test_failed, + .feed_pipe = test_stdin_pipe_feed, .task_finished = test_finished, .data = &suite, }; @@ -460,12 +501,19 @@ int cmd__run_command(int argc, const char **argv) if (!strcmp(argv[1], "run-command-parallel")) { opts.get_next_task = parallel_next; + opts.task_finished = task_finished_quiet; } else if (!strcmp(argv[1], "run-command-abort")) { opts.get_next_task = parallel_next; opts.task_finished = task_finished; } else if (!strcmp(argv[1], "run-command-no-jobs")) { opts.get_next_task = no_job; opts.task_finished = task_finished; + } else if (!strcmp(argv[1], "run-command-stdin")) { + proc.in = -1; + proc.no_stdin = 0; + opts.get_next_task = parallel_next; + opts.task_finished = task_finished_quiet; + opts.feed_pipe = test_stdin_pipe_feed; } else { ret = 1; fprintf(stderr, "check usage\n"); diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 76d4936a87..2f77fde0d9 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -164,6 +164,37 @@ test_expect_success 'run_command runs ungrouped in parallel with more tasks than test_line_count = 4 err ' +test_expect_success 'run_command listens to stdin' ' + cat >expect <<-\EOF && + preloaded output of a child + listening for stdin: + sample stdin 1 + sample stdin 0 + preloaded output of a child + listening for stdin: + sample stdin 1 + sample stdin 0 + preloaded output of a child + listening for stdin: + sample stdin 1 + sample stdin 0 + preloaded output of a child + listening for stdin: + sample stdin 1 + sample stdin 0 + EOF + + write_script stdin-script <<-\EOF && + echo "listening for stdin:" + while read line + do + echo "$line" + done + EOF + test-tool run-command run-command-stdin 2 ./stdin-script 2>actual && + test_cmp expect actual +' + cat >expect <<-EOF preloaded output of a child asking for a quick stop From 33e5914863ab232164571c2e259cfa1e9c3f30dd Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Wed, 28 Jan 2026 23:39:19 +0200 Subject: [PATCH 04/12] hook: provide stdin via callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds a callback mechanism for feeding stdin to hooks alongside the existing path_to_stdin (which slurps a file's content to stdin). The advantage of this new callback is that it can feed stdin without going through the FS layer. This helps when feeding large amount of data and uses the run-command parallel stdin callback introduced in the preceding commit. Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 23 ++++++++++++++++++++++- hook.h | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/hook.c b/hook.c index b3de1048bf..5ddd7678d1 100644 --- a/hook.c +++ b/hook.c @@ -55,7 +55,7 @@ int hook_exists(struct repository *r, const char *name) static int pick_next_hook(struct child_process *cp, struct strbuf *out UNUSED, void *pp_cb, - void **pp_task_cb UNUSED) + void **pp_task_cb) { struct hook_cb_data *hook_cb = pp_cb; const char *hook_path = hook_cb->hook_path; @@ -65,11 +65,22 @@ static int pick_next_hook(struct child_process *cp, cp->no_stdin = 1; strvec_pushv(&cp->env, hook_cb->options->env.v); + + if (hook_cb->options->path_to_stdin && hook_cb->options->feed_pipe) + BUG("options path_to_stdin and feed_pipe are mutually exclusive"); + /* reopen the file for stdin; run_command closes it. */ if (hook_cb->options->path_to_stdin) { cp->no_stdin = 0; cp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY); } + + if (hook_cb->options->feed_pipe) { + cp->no_stdin = 0; + /* start_command() will allocate a pipe / stdin fd for us */ + cp->in = -1; + } + cp->stdout_to_stderr = 1; cp->trace2_hook_name = hook_cb->hook_name; cp->dir = hook_cb->options->dir; @@ -77,6 +88,12 @@ static int pick_next_hook(struct child_process *cp, strvec_push(&cp->args, hook_path); strvec_pushv(&cp->args, hook_cb->options->args.v); + /* + * Provide per-hook internal state via task_cb for easy access, so + * hook callbacks don't have to go through hook_cb->options. + */ + *pp_task_cb = hook_cb->options->feed_pipe_cb_data; + /* * This pick_next_hook() will be called again, we're only * running one hook, so indicate that no more work will be @@ -140,6 +157,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name, .get_next_task = pick_next_hook, .start_failure = notify_start_failure, + .feed_pipe = options->feed_pipe, .task_finished = notify_hook_finished, .data = &cb_data, @@ -148,6 +166,9 @@ int run_hooks_opt(struct repository *r, const char *hook_name, if (!options) BUG("a struct run_hooks_opt must be provided to run_hooks"); + if (options->path_to_stdin && options->feed_pipe) + BUG("options path_to_stdin and feed_pipe are mutually exclusive"); + if (options->invoked_hook) *options->invoked_hook = 0; diff --git a/hook.h b/hook.h index 11863fa734..2169d4a6bd 100644 --- a/hook.h +++ b/hook.h @@ -1,6 +1,7 @@ #ifndef HOOK_H #define HOOK_H #include "strvec.h" +#include "run-command.h" struct repository; @@ -37,6 +38,43 @@ struct run_hooks_opt * Path to file which should be piped to stdin for each hook. */ const char *path_to_stdin; + + /** + * Callback used to incrementally feed a child hook stdin pipe. + * + * Useful especially if a hook consumes large quantities of data + * (e.g. a list of all refs in a client push), so feeding it via + * in-memory strings or slurping to/from files is inefficient. + * While the callback allows piecemeal writing, it can also be + * used for smaller inputs, where it gets called only once. + * + * Add hook callback initalization context to `feed_pipe_ctx`. + * Add hook callback internal state to `feed_pipe_cb_data`. + * + */ + feed_pipe_fn feed_pipe; + + /** + * Opaque data pointer used to pass context to `feed_pipe_fn`. + * + * It can be accessed via the second callback arg 'pp_cb': + * ((struct hook_cb_data *) pp_cb)->hook_cb->options->feed_pipe_ctx; + * + * The caller is responsible for managing the memory for this data. + * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. + */ + void *feed_pipe_ctx; + + /** + * Opaque data pointer used to keep internal state across callback calls. + * + * It can be accessed directly via the third callback arg 'pp_task_cb': + * struct ... *state = pp_task_cb; + * + * The caller is responsible for managing the memory for this data. + * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. + */ + void *feed_pipe_cb_data; }; #define RUN_HOOKS_OPT_INIT { \ From e8ee80cfb692139d0ca305995effe253b7a00463 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Wed, 28 Jan 2026 23:39:20 +0200 Subject: [PATCH 05/12] hook: convert 'post-rewrite' hook in sequencer.c to hook API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the custom run-command calls used by post-rewrite with the newer and simpler hook_run_opt(), which does not need to create a custom 'struct child_process' or call find_hook(). Another benefit of using the hook API is that hook_run_opt() handles the SIGPIPE toggle logic. Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- sequencer.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/sequencer.c b/sequencer.c index 5476d39ba9..71ed31c774 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1292,32 +1292,40 @@ int update_head_with_reflog(const struct commit *old_head, return ret; } +static int pipe_from_strbuf(int hook_stdin_fd, void *pp_cb, void *pp_task_cb UNUSED) +{ + struct hook_cb_data *hook_cb = pp_cb; + struct strbuf *to_pipe = hook_cb->options->feed_pipe_ctx; + int ret; + + if (!to_pipe) + BUG("pipe_from_strbuf called without feed_pipe_ctx"); + + ret = write_in_full(hook_stdin_fd, to_pipe->buf, to_pipe->len); + if (ret < 0 && errno != EPIPE) + return ret; + + return 1; /* done writing */ +} + static int run_rewrite_hook(const struct object_id *oldoid, const struct object_id *newoid) { - struct child_process proc = CHILD_PROCESS_INIT; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; int code; struct strbuf sb = STRBUF_INIT; - const char *hook_path = find_hook(the_repository, "post-rewrite"); - if (!hook_path) - return 0; - - strvec_pushl(&proc.args, hook_path, "amend", NULL); - proc.in = -1; - proc.stdout_to_stderr = 1; - proc.trace2_hook_name = "post-rewrite"; - - code = start_command(&proc); - if (code) - return code; strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid)); - sigchain_push(SIGPIPE, SIG_IGN); - write_in_full(proc.in, sb.buf, sb.len); - close(proc.in); + + opt.feed_pipe_ctx = &sb; + opt.feed_pipe = pipe_from_strbuf; + + strvec_push(&opt.args, "amend"); + + code = run_hooks_opt(the_repository, "post-rewrite", &opt); + strbuf_release(&sb); - sigchain_pop(SIGPIPE); - return finish_command(&proc); + return code; } void commit_post_rewrite(struct repository *r, From d816637f6c253e3829c4c6e817c7749b3a4f8bf4 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:21 +0200 Subject: [PATCH 06/12] hook: allow separate std[out|err] streams The hook API assumes that all hooks merge stdout to stderr. This assumption is proven wrong by pre-push: some of its users actually expect separate stdout and stderr streams and merging them will cause a regression. Therefore this adds a mechanism to allow pre-push to separate the streams, which will be used in the next commit. The mechanism is generic via struct run_hooks_opt just in case there are any more surprise exceptions like this. Reported-by: Chris Darroch Suggested-by: brian m. carlson Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 2 +- hook.h | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/hook.c b/hook.c index 5ddd7678d1..fde1f88ce8 100644 --- a/hook.c +++ b/hook.c @@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp, cp->in = -1; } - cp->stdout_to_stderr = 1; + cp->stdout_to_stderr = hook_cb->options->stdout_to_stderr; cp->trace2_hook_name = hook_cb->hook_name; cp->dir = hook_cb->options->dir; diff --git a/hook.h b/hook.h index 2169d4a6bd..2c8a23a569 100644 --- a/hook.h +++ b/hook.h @@ -34,6 +34,15 @@ struct run_hooks_opt */ int *invoked_hook; + /** + * Send the hook's stdout to stderr. + * + * This is the default behavior for all hooks except pre-push, + * which has separate stdout and stderr streams for backwards + * compatibility reasons. + */ + unsigned int stdout_to_stderr:1; + /** * Path to file which should be piped to stdin for each hook. */ @@ -80,6 +89,7 @@ struct run_hooks_opt #define RUN_HOOKS_OPT_INIT { \ .env = STRVEC_INIT, \ .args = STRVEC_INIT, \ + .stdout_to_stderr = 1, \ } struct hook_cb_data { From 9ac9612c828fc8c28afc833ca0a3511f2f406628 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Wed, 28 Jan 2026 23:39:22 +0200 Subject: [PATCH 07/12] transport: convert pre-push to hook API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the pre-push hook from custom run-command invocations to the new hook API which doesn't require a custom child_process structure and signal toggling. Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- transport.c | 107 ++++++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/transport.c b/transport.c index c7f06a7382..e876cc9189 100644 --- a/transport.c +++ b/transport.c @@ -1316,65 +1316,72 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing) die(_("Aborting.")); } +struct feed_pre_push_hook_data { + struct strbuf buf; + const struct ref *refs; +}; + +static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_task_cb) +{ + struct feed_pre_push_hook_data *data = pp_task_cb; + const struct ref *r = data->refs; + int ret = 0; + + if (!r) + return 1; /* no more refs */ + + data->refs = r->next; + + switch (r->status) { + case REF_STATUS_REJECT_NONFASTFORWARD: + case REF_STATUS_REJECT_REMOTE_UPDATED: + case REF_STATUS_REJECT_STALE: + case REF_STATUS_UPTODATE: + return 0; /* skip refs which won't be pushed */ + default: + break; + } + + if (!r->peer_ref) + return 0; + + strbuf_reset(&data->buf); + strbuf_addf(&data->buf, "%s %s %s %s\n", + r->peer_ref->name, oid_to_hex(&r->new_oid), + r->name, oid_to_hex(&r->old_oid)); + + ret = write_in_full(hook_stdin_fd, data->buf.buf, data->buf.len); + if (ret < 0 && errno != EPIPE) + return ret; /* We do not mind if a hook does not read all refs. */ + + return 0; +} + static int run_pre_push_hook(struct transport *transport, struct ref *remote_refs) { - int ret = 0, x; - struct ref *r; - struct child_process proc = CHILD_PROCESS_INIT; - struct strbuf buf; - const char *hook_path = find_hook(the_repository, "pre-push"); + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct feed_pre_push_hook_data data; + int ret = 0; - if (!hook_path) - return 0; + strvec_push(&opt.args, transport->remote->name); + strvec_push(&opt.args, transport->url); - strvec_push(&proc.args, hook_path); - strvec_push(&proc.args, transport->remote->name); - strvec_push(&proc.args, transport->url); + strbuf_init(&data.buf, 0); + data.refs = remote_refs; - proc.in = -1; - proc.trace2_hook_name = "pre-push"; + opt.feed_pipe = pre_push_hook_feed_stdin; + opt.feed_pipe_cb_data = &data; - if (start_command(&proc)) { - finish_command(&proc); - return -1; - } + /* + * pre-push hooks expect stdout & stderr to be separate, so don't merge + * them to keep backwards compatibility with existing hooks. + */ + opt.stdout_to_stderr = 0; - sigchain_push(SIGPIPE, SIG_IGN); + ret = run_hooks_opt(the_repository, "pre-push", &opt); - strbuf_init(&buf, 256); - - for (r = remote_refs; r; r = r->next) { - if (!r->peer_ref) continue; - if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue; - if (r->status == REF_STATUS_REJECT_STALE) continue; - if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) continue; - if (r->status == REF_STATUS_UPTODATE) continue; - - strbuf_reset(&buf); - strbuf_addf( &buf, "%s %s %s %s\n", - r->peer_ref->name, oid_to_hex(&r->new_oid), - r->name, oid_to_hex(&r->old_oid)); - - if (write_in_full(proc.in, buf.buf, buf.len) < 0) { - /* We do not mind if a hook does not read all refs. */ - if (errno != EPIPE) - ret = -1; - break; - } - } - - strbuf_release(&buf); - - x = close(proc.in); - if (!ret) - ret = x; - - sigchain_pop(SIGPIPE); - - x = finish_command(&proc); - if (!ret) - ret = x; + strbuf_release(&data.buf); return ret; } From b7b2157d0d23f6ed98da6fce548d614c5198713d Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:23 +0200 Subject: [PATCH 08/12] reference-transaction: use hook API instead of run-command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert the reference-transaction hook to the new hook API, so it doesn't need to set up a struct child_process, call find_hook or toggle the pipe signals. The stdin feed callback is processing one ref update per call. I haven't noticed any performance degradation due to this, however we can batch as many we want in each call, to ensure a good pipe throughtput (i.e. the child does not wait after stdin). Helped-by: Emily Shaffer Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- refs.c | 112 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 57 insertions(+), 55 deletions(-) diff --git a/refs.c b/refs.c index 046b695bb2..965b232a06 100644 --- a/refs.c +++ b/refs.c @@ -15,7 +15,6 @@ #include "iterator.h" #include "refs.h" #include "refs/refs-internal.h" -#include "run-command.h" #include "hook.h" #include "object-name.h" #include "odb.h" @@ -26,7 +25,6 @@ #include "strvec.h" #include "repo-settings.h" #include "setup.h" -#include "sigchain.h" #include "date.h" #include "commit.h" #include "wildmatch.h" @@ -2422,68 +2420,72 @@ static int ref_update_reject_duplicates(struct string_list *refnames, return 0; } +struct transaction_feed_cb_data { + size_t index; + struct strbuf buf; +}; + +static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_task_cb) +{ + struct hook_cb_data *hook_cb = pp_cb; + struct ref_transaction *transaction = hook_cb->options->feed_pipe_ctx; + struct transaction_feed_cb_data *feed_cb_data = pp_task_cb; + struct strbuf *buf = &feed_cb_data->buf; + struct ref_update *update; + size_t i = feed_cb_data->index++; + int ret; + + if (i >= transaction->nr) + return 1; /* No more refs to process */ + + update = transaction->updates[i]; + + if (update->flags & REF_LOG_ONLY) + return 0; + + strbuf_reset(buf); + + if (!(update->flags & REF_HAVE_OLD)) + strbuf_addf(buf, "%s ", oid_to_hex(null_oid(the_hash_algo))); + else if (update->old_target) + strbuf_addf(buf, "ref:%s ", update->old_target); + else + strbuf_addf(buf, "%s ", oid_to_hex(&update->old_oid)); + + if (!(update->flags & REF_HAVE_NEW)) + strbuf_addf(buf, "%s ", oid_to_hex(null_oid(the_hash_algo))); + else if (update->new_target) + strbuf_addf(buf, "ref:%s ", update->new_target); + else + strbuf_addf(buf, "%s ", oid_to_hex(&update->new_oid)); + + strbuf_addf(buf, "%s\n", update->refname); + + ret = write_in_full(hook_stdin_fd, buf->buf, buf->len); + if (ret < 0 && errno != EPIPE) + return ret; + + return 0; /* no more input to feed */ +} + static int run_transaction_hook(struct ref_transaction *transaction, const char *state) { - struct child_process proc = CHILD_PROCESS_INIT; - struct strbuf buf = STRBUF_INIT; - const char *hook; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct transaction_feed_cb_data feed_ctx = { 0 }; int ret = 0; - hook = find_hook(transaction->ref_store->repo, "reference-transaction"); - if (!hook) - return ret; + strvec_push(&opt.args, state); - strvec_pushl(&proc.args, hook, state, NULL); - proc.in = -1; - proc.stdout_to_stderr = 1; - proc.trace2_hook_name = "reference-transaction"; + opt.feed_pipe = transaction_hook_feed_stdin; + opt.feed_pipe_ctx = transaction; + opt.feed_pipe_cb_data = &feed_ctx; - ret = start_command(&proc); - if (ret) - return ret; + strbuf_init(&feed_ctx.buf, 0); - sigchain_push(SIGPIPE, SIG_IGN); + ret = run_hooks_opt(transaction->ref_store->repo, "reference-transaction", &opt); - for (size_t i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - - if (update->flags & REF_LOG_ONLY) - continue; - - strbuf_reset(&buf); - - if (!(update->flags & REF_HAVE_OLD)) - strbuf_addf(&buf, "%s ", oid_to_hex(null_oid(the_hash_algo))); - else if (update->old_target) - strbuf_addf(&buf, "ref:%s ", update->old_target); - else - strbuf_addf(&buf, "%s ", oid_to_hex(&update->old_oid)); - - if (!(update->flags & REF_HAVE_NEW)) - strbuf_addf(&buf, "%s ", oid_to_hex(null_oid(the_hash_algo))); - else if (update->new_target) - strbuf_addf(&buf, "ref:%s ", update->new_target); - else - strbuf_addf(&buf, "%s ", oid_to_hex(&update->new_oid)); - - strbuf_addf(&buf, "%s\n", update->refname); - - if (write_in_full(proc.in, buf.buf, buf.len) < 0) { - if (errno != EPIPE) { - /* Don't leak errno outside this API */ - errno = 0; - ret = -1; - } - break; - } - } - - close(proc.in); - sigchain_pop(SIGPIPE); - strbuf_release(&buf); - - ret |= finish_command(&proc); + strbuf_release(&feed_ctx.buf); return ret; } From c549a40547fb9765a9ea78dbe4a3397dbee715b2 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:24 +0200 Subject: [PATCH 09/12] hook: add jobs option Allow the API callers to specify the number of jobs across which hook execution can be parallelized. It defaults to 1 and no hook currently changes it, so all hooks run sequentially as before. This allows us to both pave the way for parallel hook execution (that will be a follow-up patch series building upon this) and to finish the API conversion of builtin/receive-pack.c, keeping the output async sideband thread ("muxer") design as Peff suggested. When .jobs==1 nothing changes, the "copy_to_sideband" async thread still outputs directly via sideband channel 2, keeping the current (mostly) real-time output characteristics, avoids unnecessary poll delays or deadlock risks. When .jobs > 1, a more complex muxer is needed to buffer the hook output and avoid interleaving. After working on this mux I quickly realized I was re-implementing run-command with ungroup=0 so that idea was dropped in favor of run-command which outputs to stderr. In other words, run-command itself already can buffer/deinterleave pp child outputs (ungroup=0), so we can just connect its stderr to the sideband async task when jobs > 1. Maybe it helps to illustrate how it works with ascii graphics: [ Sequential (jobs = 1) ] [ Parallel (jobs > 1) ] +--------------+ +--------+ +--------+ | Hook Process | | Hook 1 | | Hook 2 | +--------------+ +--------+ +--------+ | | | | stderr (inherited) | stderr pipe | | | (captured) | v v v +-------------------------------------------------------------+ | Parent Process | | | | (direct write) [run-command (buffered)] | | | | | | | | writes | | v v | | +-------------------------------------------+ | | | stderr (FD 2) | | | +-------------------------------------------+ | | | | | | (dup2'd to pipe) | | v | | +-----------------------+ | | | sideband async thread | | | +-----------------------+ | +-------------------------------------------------------------+ When use_sideband == 0, the sideband async thread is missing, so this same architecture just outputs via the parent stderr stream. See the following commits for the hook API conversions doing this, using pre-existing sideband thread logic from `copy_to_sideband`. Suggested-by: Jeff King Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 7 +++++-- hook.h | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/hook.c b/hook.c index fde1f88ce8..cde7198412 100644 --- a/hook.c +++ b/hook.c @@ -152,8 +152,8 @@ int run_hooks_opt(struct repository *r, const char *hook_name, .tr2_category = "hook", .tr2_label = hook_name, - .processes = 1, - .ungroup = 1, + .processes = options->jobs, + .ungroup = options->jobs == 1, .get_next_task = pick_next_hook, .start_failure = notify_start_failure, @@ -169,6 +169,9 @@ int run_hooks_opt(struct repository *r, const char *hook_name, if (options->path_to_stdin && options->feed_pipe) BUG("options path_to_stdin and feed_pipe are mutually exclusive"); + if (!options->jobs) + BUG("run_hooks_opt must be called with options.jobs >= 1"); + if (options->invoked_hook) *options->invoked_hook = 0; diff --git a/hook.h b/hook.h index 2c8a23a569..20eb56fd63 100644 --- a/hook.h +++ b/hook.h @@ -16,6 +16,14 @@ struct run_hooks_opt /* Emit an error if the hook is missing */ unsigned int error_if_missing:1; + /** + * Number of processes to parallelize across. + * + * If > 1, output will be buffered and de-interleaved (ungroup=0). + * If == 1, output will be real-time (ungroup=1). + */ + unsigned int jobs; + /** * An optional initial working directory for the hook, * translates to "struct child_process"'s "dir" member. @@ -90,6 +98,7 @@ struct run_hooks_opt .env = STRVEC_INIT, \ .args = STRVEC_INIT, \ .stdout_to_stderr = 1, \ + .jobs = 1, \ } struct hook_cb_data { From c45a34e12e8699f656ec3613b6ba158c1a57c5e8 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:25 +0200 Subject: [PATCH 10/12] run-command: poll child input in addition to output Child input feeding might hit the 100ms output poll timeout as a side-effect of the ungroup=0 design when feeding multiple children in parallel and buffering their outputs. This throttles the write throughput as reported by Kristoffer. Peff also noted that the parent might block if the write pipe is full and cause a deadlock if both parent + child wait for one another. Thus we refactor the run-command I/O loop so it polls on both child input and output fds to eliminate the risk of artificial 100ms latencies and unnecessarily blocking the main process. This ensures that parallel hooks are fed data ASAP while maintaining responsiveness for (sideband) output. It's worth noting that in our current design, sequential execution is not affected by this because it still uses the ungroup=1 behavior, so there are no run-command induced buffering delays since the child sequentially outputs directly to the parent-inherited fds. Reported-by: Kristoffer Haugsbakk Suggested-by: Jeff King Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- run-command.c | 80 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/run-command.c b/run-command.c index aaf0e4ecee..3356463d43 100644 --- a/run-command.c +++ b/run-command.c @@ -1499,6 +1499,14 @@ static int child_is_receiving_input(const struct parallel_child *pp_child) { return child_is_working(pp_child) && pp_child->process.in > 0; } +static int child_is_sending_output(const struct parallel_child *pp_child) +{ + /* + * all pp children which buffer output through run_command via ungroup=0 + * redirect stdout to stderr, so we just need to check process.err. + */ + return child_is_working(pp_child) && pp_child->process.err > 0; +} struct parallel_processes { size_t nr_processes; @@ -1562,7 +1570,7 @@ static void pp_init(struct parallel_processes *pp, CALLOC_ARRAY(pp->children, n); if (!opts->ungroup) - CALLOC_ARRAY(pp->pfd, n); + CALLOC_ARRAY(pp->pfd, n * 2); for (size_t i = 0; i < n; i++) { strbuf_init(&pp->children[i].err, 0); @@ -1707,21 +1715,63 @@ static void pp_buffer_stdin(struct parallel_processes *pp, } } -static void pp_buffer_stderr(struct parallel_processes *pp, - const struct run_process_parallel_opts *opts, - int output_timeout) +static void pp_buffer_io(struct parallel_processes *pp, + const struct run_process_parallel_opts *opts, + int timeout) { - while (poll(pp->pfd, opts->processes, output_timeout) < 0) { + /* for each potential child slot, prepare two pollfd entries */ + for (size_t i = 0; i < opts->processes; i++) { + if (child_is_sending_output(&pp->children[i])) { + pp->pfd[2*i].fd = pp->children[i].process.err; + pp->pfd[2*i].events = POLLIN | POLLHUP; + } else { + pp->pfd[2*i].fd = -1; + } + + if (child_is_receiving_input(&pp->children[i])) { + pp->pfd[2*i+1].fd = pp->children[i].process.in; + pp->pfd[2*i+1].events = POLLOUT; + } else { + pp->pfd[2*i+1].fd = -1; + } + } + + while (poll(pp->pfd, opts->processes * 2, timeout) < 0) { if (errno == EINTR) continue; pp_cleanup(pp, opts); die_errno("poll"); } - /* Buffer output from all pipes. */ for (size_t i = 0; i < opts->processes; i++) { + /* Handle input feeding (stdin) */ + if (pp->pfd[2*i+1].revents & (POLLOUT | POLLHUP | POLLERR)) { + if (opts->feed_pipe) { + int ret = opts->feed_pipe(pp->children[i].process.in, + opts->data, + pp->children[i].data); + if (ret < 0) + die_errno("feed_pipe"); + if (ret) { + /* done feeding */ + close(pp->children[i].process.in); + pp->children[i].process.in = 0; + } + } else { + /* + * No feed_pipe means there is nothing to do, so + * close the fd. Child input can be fed by other + * methods, such as opts->path_to_stdin which + * slurps a file via dup2, so clean up here. + */ + close(pp->children[i].process.in); + pp->children[i].process.in = 0; + } + } + + /* Handle output reading (stderr) */ if (child_is_working(&pp->children[i]) && - pp->pfd[i].revents & (POLLIN | POLLHUP)) { + pp->pfd[2*i].revents & (POLLIN | POLLHUP)) { int n = strbuf_read_once(&pp->children[i].err, pp->children[i].process.err, 0); if (n == 0) { @@ -1814,21 +1864,15 @@ static int pp_collect_finished(struct parallel_processes *pp, static void pp_handle_child_IO(struct parallel_processes *pp, const struct run_process_parallel_opts *opts, - int output_timeout) + int 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) { + pp_buffer_stdin(pp, opts); 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_buffer_io(pp, opts, timeout); pp_output(pp); } } @@ -1836,7 +1880,7 @@ static void pp_handle_child_IO(struct parallel_processes *pp, void run_processes_parallel(const struct run_process_parallel_opts *opts) { int i, code; - int output_timeout = 100; + int timeout = 100; int spawn_cap = 4; struct parallel_processes_for_signal pp_sig; struct parallel_processes pp = { @@ -1876,7 +1920,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) } if (!pp.nr_processes) break; - pp_handle_child_IO(&pp, opts, output_timeout); + pp_handle_child_IO(&pp, opts, timeout); code = pp_collect_finished(&pp, opts); if (code) { pp.shutdown = 1; From fc148b146ad41be71a7852c4867f0773cbfe1ff9 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Wed, 28 Jan 2026 23:39:26 +0200 Subject: [PATCH 11/12] receive-pack: convert update hooks to new API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hook API avoids creating a custom struct child_process and other internal hook plumbing (e.g. calling find_hook()) and prepares for the specification of hooks via configs or running parallel hooks. Execution is still sequential through the run_hooks_opt .jobs == 1, which is the unchanged default for all hooks. When use_sideband==1, the async thread redirects the hook outputs to sideband 2, otherwise it is not used and the hooks write directly to the fds inherited from the main parent process. When .jobs == 1, run-command's poll loop is avoided entirely via the ungroup=1 option like before (this was Jeff's suggestion), achieving the same real-time output performance. When running in parallel, run-command with ungroup=0 will capture and de-interleave the output of each hook, then write to the parent stderr which is redirected via dup2 to the sideband thread, so that each parallel hook output is presented clearly to the client. Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Helped-by: Jeff King Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 103 ++++++++++++++++++++++++++--------------- 1 file changed, 66 insertions(+), 37 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 9c49174616..f554dac1ef 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -561,6 +561,48 @@ static int copy_to_sideband(int in, int out UNUSED, void *arg UNUSED) return 0; } +/* + * Start an async thread which redirects hook stderr over the sideband. + * The original stderr fd is saved to `saved_stderr` and STDERR_FILENO is + * redirected to the async's input pipe. + */ +static void prepare_sideband_async(struct async *sideband_async, int *saved_stderr, int *started) +{ + *started = 0; + + if (!use_sideband) + return; + + memset(sideband_async, 0, sizeof(*sideband_async)); + sideband_async->proc = copy_to_sideband; + sideband_async->in = -1; + + if (!start_async(sideband_async)) { + *started = 1; + *saved_stderr = dup(STDERR_FILENO); + if (*saved_stderr >= 0) + dup2(sideband_async->in, STDERR_FILENO); + close(sideband_async->in); + } +} + +/* + * Restore the original stderr and wait for the async sideband thread to finish. + */ +static void finish_sideband_async(struct async *sideband_async, int saved_stderr, int started) +{ + if (!use_sideband) + return; + + if (saved_stderr >= 0) { + dup2(saved_stderr, STDERR_FILENO); + close(saved_stderr); + } + + if (started) + finish_async(sideband_async); +} + static void hmac_hash(unsigned char *out, const char *key_in, size_t key_len, const char *text, size_t text_len) @@ -941,29 +983,25 @@ static int run_receive_hook(struct command *commands, static int run_update_hook(struct command *cmd) { - struct child_process proc = CHILD_PROCESS_INIT; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct async sideband_async; + int sideband_async_started = 0; + int saved_stderr = -1; int code; - const char *hook_path = find_hook(the_repository, "update"); - if (!hook_path) - return 0; + strvec_pushl(&opt.args, + cmd->ref_name, + oid_to_hex(&cmd->old_oid), + oid_to_hex(&cmd->new_oid), + NULL); - strvec_push(&proc.args, hook_path); - strvec_push(&proc.args, cmd->ref_name); - strvec_push(&proc.args, oid_to_hex(&cmd->old_oid)); - strvec_push(&proc.args, oid_to_hex(&cmd->new_oid)); + prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); - proc.no_stdin = 1; - proc.stdout_to_stderr = 1; - proc.err = use_sideband ? -1 : 0; - proc.trace2_hook_name = "update"; + code = run_hooks_opt(the_repository, "update", &opt); - code = start_command(&proc); - if (code) - return code; - if (use_sideband) - copy_to_sideband(proc.err, -1, NULL); - return finish_command(&proc); + finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started); + + return code; } static struct command *find_command_by_refname(struct command *list, @@ -1639,34 +1677,25 @@ out: static void run_update_post_hook(struct command *commands) { + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct async sideband_async; struct command *cmd; - struct child_process proc = CHILD_PROCESS_INIT; - const char *hook; - - hook = find_hook(the_repository, "post-update"); - if (!hook) - return; + int sideband_async_started = 0; + int saved_stderr = -1; for (cmd = commands; cmd; cmd = cmd->next) { if (cmd->error_string || cmd->did_not_exist) continue; - if (!proc.args.nr) - strvec_push(&proc.args, hook); - strvec_push(&proc.args, cmd->ref_name); + strvec_push(&opt.args, cmd->ref_name); } - if (!proc.args.nr) + if (!opt.args.nr) return; - proc.no_stdin = 1; - proc.stdout_to_stderr = 1; - proc.err = use_sideband ? -1 : 0; - proc.trace2_hook_name = "post-update"; + prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); - if (!start_command(&proc)) { - if (use_sideband) - copy_to_sideband(proc.err, -1, NULL); - finish_command(&proc); - } + run_hooks_opt(the_repository, "post-update", &opt); + + finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started); } static void check_aliased_update_internal(struct command *cmd, From b5e9ad508c2f340bd2d2547d7370ae7ac6a0d65d Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Wed, 28 Jan 2026 23:39:27 +0200 Subject: [PATCH 12/12] receive-pack: convert receive hooks to hook API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This converts the last remaining hooks to the new hook API, for the same benefits as the previous conversions (no need to toggle signals, manage custom struct child_process, call find_hook(), prepares for specifying hooks via configs, etc.). See the previous three commits for a more in-depth explanation of how this all works. Signed-off-by: Emily Shaffer Signed-off-by: Ævar Arnfjörð Bjarmason Helped-by: Jeff King Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 178 +++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 103 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f554dac1ef..b5379a4895 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -791,7 +791,7 @@ static int check_cert_push_options(const struct string_list *push_options) return retval; } -static void prepare_push_cert_sha1(struct child_process *proc) +static void prepare_push_cert_sha1(struct run_hooks_opt *opt) { static int already_done; @@ -817,23 +817,23 @@ static void prepare_push_cert_sha1(struct child_process *proc) nonce_status = check_nonce(sigcheck.payload); } if (!is_null_oid(&push_cert_oid)) { - strvec_pushf(&proc->env, "GIT_PUSH_CERT=%s", + strvec_pushf(&opt->env, "GIT_PUSH_CERT=%s", oid_to_hex(&push_cert_oid)); - strvec_pushf(&proc->env, "GIT_PUSH_CERT_SIGNER=%s", + strvec_pushf(&opt->env, "GIT_PUSH_CERT_SIGNER=%s", sigcheck.signer ? sigcheck.signer : ""); - strvec_pushf(&proc->env, "GIT_PUSH_CERT_KEY=%s", + strvec_pushf(&opt->env, "GIT_PUSH_CERT_KEY=%s", sigcheck.key ? sigcheck.key : ""); - strvec_pushf(&proc->env, "GIT_PUSH_CERT_STATUS=%c", + strvec_pushf(&opt->env, "GIT_PUSH_CERT_STATUS=%c", sigcheck.result); if (push_cert_nonce) { - strvec_pushf(&proc->env, + strvec_pushf(&opt->env, "GIT_PUSH_CERT_NONCE=%s", push_cert_nonce); - strvec_pushf(&proc->env, + strvec_pushf(&opt->env, "GIT_PUSH_CERT_NONCE_STATUS=%s", nonce_status); if (nonce_status == NONCE_SLOP) - strvec_pushf(&proc->env, + strvec_pushf(&opt->env, "GIT_PUSH_CERT_NONCE_SLOP=%ld", nonce_stamp_slop); } @@ -845,94 +845,25 @@ struct receive_hook_feed_state { struct ref_push_report *report; int skip_broken; struct strbuf buf; - const struct string_list *push_options; }; -typedef int (*feed_fn)(void *, const char **, size_t *); -static int run_and_feed_hook(const char *hook_name, feed_fn feed, - struct receive_hook_feed_state *feed_state) +static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_task_cb) { - struct child_process proc = CHILD_PROCESS_INIT; - struct async muxer; - int code; - const char *hook_path = find_hook(the_repository, hook_name); - - if (!hook_path) - return 0; - - strvec_push(&proc.args, hook_path); - proc.in = -1; - proc.stdout_to_stderr = 1; - proc.trace2_hook_name = hook_name; - - if (feed_state->push_options) { - size_t i; - for (i = 0; i < feed_state->push_options->nr; i++) - strvec_pushf(&proc.env, - "GIT_PUSH_OPTION_%"PRIuMAX"=%s", - (uintmax_t)i, - feed_state->push_options->items[i].string); - strvec_pushf(&proc.env, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"", - (uintmax_t)feed_state->push_options->nr); - } else - strvec_pushf(&proc.env, "GIT_PUSH_OPTION_COUNT"); - - if (tmp_objdir) - strvec_pushv(&proc.env, tmp_objdir_env(tmp_objdir)); - - if (use_sideband) { - memset(&muxer, 0, sizeof(muxer)); - muxer.proc = copy_to_sideband; - muxer.in = -1; - code = start_async(&muxer); - if (code) - return code; - proc.err = muxer.in; - } - - prepare_push_cert_sha1(&proc); - - code = start_command(&proc); - if (code) { - if (use_sideband) - finish_async(&muxer); - return code; - } - - sigchain_push(SIGPIPE, SIG_IGN); - - while (1) { - const char *buf; - size_t n; - if (feed(feed_state, &buf, &n)) - break; - if (write_in_full(proc.in, buf, n) < 0) - break; - } - close(proc.in); - if (use_sideband) - finish_async(&muxer); - - sigchain_pop(SIGPIPE); - - return finish_command(&proc); -} - -static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep) -{ - struct receive_hook_feed_state *state = state_; + struct receive_hook_feed_state *state = pp_task_cb; struct command *cmd = state->cmd; + strbuf_reset(&state->buf); + while (cmd && state->skip_broken && (cmd->error_string || cmd->did_not_exist)) cmd = cmd->next; + if (!cmd) - return -1; /* EOF */ - if (!bufp) - return 0; /* OK, can feed something. */ - strbuf_reset(&state->buf); + return 1; /* no more commands left */ + if (!state->report) state->report = cmd->report; + if (state->report) { struct object_id *old_oid; struct object_id *new_oid; @@ -941,23 +872,33 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep) old_oid = state->report->old_oid ? state->report->old_oid : &cmd->old_oid; new_oid = state->report->new_oid ? state->report->new_oid : &cmd->new_oid; ref_name = state->report->ref_name ? state->report->ref_name : cmd->ref_name; + strbuf_addf(&state->buf, "%s %s %s\n", oid_to_hex(old_oid), oid_to_hex(new_oid), ref_name); + state->report = state->report->next; if (!state->report) - state->cmd = cmd->next; + cmd = cmd->next; } else { strbuf_addf(&state->buf, "%s %s %s\n", oid_to_hex(&cmd->old_oid), oid_to_hex(&cmd->new_oid), cmd->ref_name); - state->cmd = cmd->next; + cmd = cmd->next; } - if (bufp) { - *bufp = state->buf.buf; - *sizep = state->buf.len; + + state->cmd = cmd; + + if (state->buf.len > 0) { + int ret = write_in_full(hook_stdin_fd, state->buf.buf, state->buf.len); + if (ret < 0) { + if (errno == EPIPE) + return 1; /* child closed pipe */ + return ret; + } } - return 0; + + return state->cmd ? 0 : 1; /* 0 = more to come, 1 = EOF */ } static int run_receive_hook(struct command *commands, @@ -965,20 +906,51 @@ static int run_receive_hook(struct command *commands, int skip_broken, const struct string_list *push_options) { - struct receive_hook_feed_state state; - int status; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct command *iter = commands; + struct receive_hook_feed_state feed_state; + struct async sideband_async; + int sideband_async_started = 0; + int saved_stderr = -1; + int ret; - strbuf_init(&state.buf, 0); - state.cmd = commands; - state.skip_broken = skip_broken; - state.report = NULL; - if (feed_receive_hook(&state, NULL, NULL)) + /* if there are no valid commands, don't invoke the hook at all. */ + while (iter && skip_broken && (iter->error_string || iter->did_not_exist)) + iter = iter->next; + if (!iter) return 0; - state.cmd = commands; - state.push_options = push_options; - status = run_and_feed_hook(hook_name, feed_receive_hook, &state); - strbuf_release(&state.buf); - return status; + + if (push_options) { + for (int i = 0; i < push_options->nr; i++) + strvec_pushf(&opt.env, "GIT_PUSH_OPTION_%d=%s", i, + push_options->items[i].string); + strvec_pushf(&opt.env, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"", + (uintmax_t)push_options->nr); + } else { + strvec_push(&opt.env, "GIT_PUSH_OPTION_COUNT"); + } + + if (tmp_objdir) + strvec_pushv(&opt.env, tmp_objdir_env(tmp_objdir)); + + prepare_push_cert_sha1(&opt); + + prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); + + /* set up stdin callback */ + feed_state.cmd = commands; + feed_state.skip_broken = skip_broken; + feed_state.report = NULL; + strbuf_init(&feed_state.buf, 0); + opt.feed_pipe_cb_data = &feed_state; + opt.feed_pipe = feed_receive_hook_cb; + + ret = run_hooks_opt(the_repository, hook_name, &opt); + + strbuf_release(&feed_state.buf); + finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started); + + return ret; } static int run_update_hook(struct command *cmd)