From c9ef360403e0f64a5d4c70f66a9b25f02a98fda6 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 28 Jan 2026 23:39:16 +0200 Subject: [PATCH 01/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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/31] 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) From ee2fbfd6b28fba20bc936ad1c2cb2617ba251025 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Thu, 19 Feb 2026 00:23:45 +0200 Subject: [PATCH 13/31] hook: add internal state alloc/free callbacks Some hooks use opaque structs to keep internal state between callbacks. Because hooks ran sequentially (jobs == 1) with one command per hook, these internal states could be allocated on the stack for each hook run. Next commits add the ability to run multiple commands for each hook, so the states cannot be shared or stored on the stack anymore, especially since down the line we will also enable parallel execution (jobs > 1). Add alloc/free helpers for each hook, doing a "deep" alloc/init & free of their internal opaque struct. The alloc callback takes a context pointer, to initialize the struct at at the time of resource acquisition. These callbacks must always be provided together: no alloc without free and no free without alloc, otherwise a BUG() is triggered. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 33 ++++++++++++++++++++++++++------- hook.c | 13 +++++++++++++ hook.h | 25 ++++++++++++++++++++++++- refs.c | 24 +++++++++++++++++++----- transport.c | 27 ++++++++++++++++++++------- 5 files changed, 102 insertions(+), 20 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index b5379a4895..5259f09788 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -901,6 +901,26 @@ static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_ return state->cmd ? 0 : 1; /* 0 = more to come, 1 = EOF */ } +static void *receive_hook_feed_state_alloc(void *feed_pipe_ctx) +{ + struct receive_hook_feed_state *init_state = feed_pipe_ctx; + struct receive_hook_feed_state *data = xcalloc(1, sizeof(*data)); + data->report = init_state->report; + data->cmd = init_state->cmd; + data->skip_broken = init_state->skip_broken; + strbuf_init(&data->buf, 0); + return data; +} + +static void receive_hook_feed_state_free(void *data) +{ + struct receive_hook_feed_state *d = data; + if (!d) + return; + strbuf_release(&d->buf); + free(d); +} + static int run_receive_hook(struct command *commands, const char *hook_name, int skip_broken, @@ -908,7 +928,7 @@ static int run_receive_hook(struct command *commands, { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; struct command *iter = commands; - struct receive_hook_feed_state feed_state; + struct receive_hook_feed_state feed_init_state = { 0 }; struct async sideband_async; int sideband_async_started = 0; int saved_stderr = -1; @@ -938,16 +958,15 @@ static int run_receive_hook(struct command *commands, 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; + feed_init_state.cmd = commands; + feed_init_state.skip_broken = skip_broken; + opt.feed_pipe_ctx = &feed_init_state; opt.feed_pipe = feed_receive_hook_cb; + opt.feed_pipe_cb_data_alloc = receive_hook_feed_state_alloc; + opt.feed_pipe_cb_data_free = receive_hook_feed_state_free; 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; diff --git a/hook.c b/hook.c index cde7198412..83ff658866 100644 --- a/hook.c +++ b/hook.c @@ -133,6 +133,8 @@ static int notify_hook_finished(int result, static void run_hooks_opt_clear(struct run_hooks_opt *options) { + if (options->feed_pipe_cb_data_free) + options->feed_pipe_cb_data_free(options->feed_pipe_cb_data); strvec_clear(&options->env); strvec_clear(&options->args); } @@ -172,6 +174,17 @@ int run_hooks_opt(struct repository *r, const char *hook_name, if (!options->jobs) BUG("run_hooks_opt must be called with options.jobs >= 1"); + /* + * Ensure cb_data copy and free functions are either provided together, + * or neither one is provided. + */ + if ((options->feed_pipe_cb_data_alloc && !options->feed_pipe_cb_data_free) || + (!options->feed_pipe_cb_data_alloc && options->feed_pipe_cb_data_free)) + BUG("feed_pipe_cb_data_alloc and feed_pipe_cb_data_free must be set together"); + + if (options->feed_pipe_cb_data_alloc) + options->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx); + if (options->invoked_hook) *options->invoked_hook = 0; diff --git a/hook.h b/hook.h index 20eb56fd63..a6bdc6f90f 100644 --- a/hook.h +++ b/hook.h @@ -5,6 +5,9 @@ struct repository; +typedef void (*cb_data_free_fn)(void *data); +typedef void *(*cb_data_alloc_fn)(void *init_ctx); + struct run_hooks_opt { /* Environment vars to be set for each hook */ @@ -88,10 +91,30 @@ struct run_hooks_opt * 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. + * The caller is responsible for managing the memory for this data by + * providing alloc/free callbacks to `run_hooks_opt`. + * * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. */ void *feed_pipe_cb_data; + + /** + * Some hooks need to create a fresh `feed_pipe_cb_data` internal state, + * so they can keep track of progress without affecting one another. + * + * If provided, this function will be called to alloc & initialize the + * `feed_pipe_cb_data` for each hook. + * + * The `feed_pipe_ctx` pointer can be used to pass initialization data. + */ + cb_data_alloc_fn feed_pipe_cb_data_alloc; + + /** + * Called to free the memory initialized by `feed_pipe_cb_data_alloc`. + * + * Must always be provided when `feed_pipe_cb_data_alloc` is provided. + */ + cb_data_free_fn feed_pipe_cb_data_free; }; #define RUN_HOOKS_OPT_INIT { \ diff --git a/refs.c b/refs.c index 1e2ac90018..e06a137c29 100644 --- a/refs.c +++ b/refs.c @@ -2511,24 +2511,38 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_ return 0; /* no more input to feed */ } +static void *transaction_feed_cb_data_alloc(void *feed_pipe_ctx UNUSED) +{ + struct transaction_feed_cb_data *data = xmalloc(sizeof(*data)); + strbuf_init(&data->buf, 0); + data->index = 0; + return data; +} + +static void transaction_feed_cb_data_free(void *data) +{ + struct transaction_feed_cb_data *d = data; + if (!d) + return; + strbuf_release(&d->buf); + free(d); +} + static int run_transaction_hook(struct ref_transaction *transaction, const char *state) { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; - struct transaction_feed_cb_data feed_ctx = { 0 }; int ret = 0; strvec_push(&opt.args, state); opt.feed_pipe = transaction_hook_feed_stdin; opt.feed_pipe_ctx = transaction; - opt.feed_pipe_cb_data = &feed_ctx; - - strbuf_init(&feed_ctx.buf, 0); + opt.feed_pipe_cb_data_alloc = transaction_feed_cb_data_alloc; + opt.feed_pipe_cb_data_free = transaction_feed_cb_data_free; ret = run_hooks_opt(transaction->ref_store->repo, "reference-transaction", &opt); - strbuf_release(&feed_ctx.buf); return ret; } diff --git a/transport.c b/transport.c index e876cc9189..d25e9a4fb0 100644 --- a/transport.c +++ b/transport.c @@ -1357,21 +1357,36 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void return 0; } +static void *pre_push_hook_data_alloc(void *feed_pipe_ctx) +{ + struct feed_pre_push_hook_data *data = xmalloc(sizeof(*data)); + strbuf_init(&data->buf, 0); + data->refs = (struct ref *)feed_pipe_ctx; + return data; +} + +static void pre_push_hook_data_free(void *data) +{ + struct feed_pre_push_hook_data *d = data; + if (!d) + return; + strbuf_release(&d->buf); + free(d); +} + static int run_pre_push_hook(struct transport *transport, struct ref *remote_refs) { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; - struct feed_pre_push_hook_data data; int ret = 0; strvec_push(&opt.args, transport->remote->name); strvec_push(&opt.args, transport->url); - strbuf_init(&data.buf, 0); - data.refs = remote_refs; - opt.feed_pipe = pre_push_hook_feed_stdin; - opt.feed_pipe_cb_data = &data; + opt.feed_pipe_ctx = remote_refs; + opt.feed_pipe_cb_data_alloc = pre_push_hook_data_alloc; + opt.feed_pipe_cb_data_free = pre_push_hook_data_free; /* * pre-push hooks expect stdout & stderr to be separate, so don't merge @@ -1381,8 +1396,6 @@ static int run_pre_push_hook(struct transport *transport, ret = run_hooks_opt(the_repository, "pre-push", &opt); - strbuf_release(&data.buf); - return ret; } From 4a36cb4c9f0f508db2e5dda75673e0d4b1242007 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Thu, 19 Feb 2026 00:23:46 +0200 Subject: [PATCH 14/31] hook: run a list of hooks to prepare for multihook support Hooks are limited to run one command (the default from the hookdir) for each event. This limitation makes it impossible to run multiple commands via config files, which the next commits will add. Implement the ability to run a list of hooks in hook.[ch]. For now, the list contains only one entry representing the "default" hook from the hookdir, so there is no user-visible change in this commit. All hook commands still run sequentially like before. A separate patch series will enable running them in parallel. Signed-off-by: Emily Shaffer Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 139 ++++++++++++++++++++++++++++++++++++++++++++------------- hook.h | 59 ++++++++++++++++++------ 2 files changed, 153 insertions(+), 45 deletions(-) diff --git a/hook.c b/hook.c index 83ff658866..c008a7232d 100644 --- a/hook.c +++ b/hook.c @@ -47,9 +47,97 @@ const char *find_hook(struct repository *r, const char *name) return path.buf; } +static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) +{ + if (!h) + return; + + if (h->kind == HOOK_TRADITIONAL) + free((void *)h->u.traditional.path); + + if (cb_data_free) + cb_data_free(h->feed_pipe_cb_data); + + free(h); +} + +static void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free) +{ + struct string_list_item *item; + + for_each_string_list_item(item, hooks) + hook_clear(item->util, cb_data_free); + + string_list_clear(hooks, 0); +} + +/* Helper to detect and add default "traditional" hooks from the hookdir. */ +static void list_hooks_add_default(struct repository *r, const char *hookname, + struct string_list *hook_list, + struct run_hooks_opt *options) +{ + const char *hook_path = find_hook(r, hookname); + struct hook *h; + + if (!hook_path) + return; + + h = xcalloc(1, sizeof(struct hook)); + + /* + * If the hook is to run in a specific dir, a relative path can + * become invalid in that dir, so convert to an absolute path. + */ + if (options && options->dir) + hook_path = absolute_path(hook_path); + + /* Setup per-hook internal state cb data */ + if (options && options->feed_pipe_cb_data_alloc) + h->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx); + + h->kind = HOOK_TRADITIONAL; + h->u.traditional.path = xstrdup(hook_path); + + string_list_append(hook_list, hook_path)->util = h; +} + +/* + * Provides a list of hook commands to run for the 'hookname' event. + * + * This function consolidates hooks from two sources: + * 1. The config-based hooks (not yet implemented). + * 2. The "traditional" hook found in the repository hooks directory + * (e.g., .git/hooks/pre-commit). + * + * The list is ordered by execution priority. + * + * The caller is responsible for freeing the memory of the returned list + * using string_list_clear() and free(). + */ +static struct string_list *list_hooks(struct repository *r, const char *hookname, + struct run_hooks_opt *options) +{ + struct string_list *hook_head; + + if (!hookname) + BUG("null hookname was provided to hook_list()!"); + + hook_head = xmalloc(sizeof(struct string_list)); + string_list_init_dup(hook_head); + + /* Add the default "traditional" hooks from hookdir. */ + list_hooks_add_default(r, hookname, hook_head, options); + + return hook_head; +} + int hook_exists(struct repository *r, const char *name) { - return !!find_hook(r, name); + struct string_list *hooks = list_hooks(r, name, NULL); + int exists = hooks->nr > 0; + hook_list_clear(hooks, NULL); + free(hooks); + return exists; } static int pick_next_hook(struct child_process *cp, @@ -58,11 +146,14 @@ static int pick_next_hook(struct child_process *cp, void **pp_task_cb) { struct hook_cb_data *hook_cb = pp_cb; - const char *hook_path = hook_cb->hook_path; + struct string_list *hook_list = hook_cb->hook_command_list; + struct hook *h; - if (!hook_path) + if (hook_cb->hook_to_run_index >= hook_list->nr) return 0; + h = hook_list->items[hook_cb->hook_to_run_index++].util; + cp->no_stdin = 1; strvec_pushv(&cp->env, hook_cb->options->env.v); @@ -85,21 +176,20 @@ static int pick_next_hook(struct child_process *cp, cp->trace2_hook_name = hook_cb->hook_name; cp->dir = hook_cb->options->dir; - strvec_push(&cp->args, hook_path); + /* Add hook exec paths or commands */ + if (h->kind == HOOK_TRADITIONAL) + strvec_push(&cp->args, h->u.traditional.path); + + if (!cp->args.nr) + BUG("hook must have at least one command or exec 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 - * done. - */ - hook_cb->hook_path = NULL; + *pp_task_cb = h->feed_pipe_cb_data; return 1; } @@ -133,8 +223,6 @@ static int notify_hook_finished(int result, static void run_hooks_opt_clear(struct run_hooks_opt *options) { - if (options->feed_pipe_cb_data_free) - options->feed_pipe_cb_data_free(options->feed_pipe_cb_data); strvec_clear(&options->env); strvec_clear(&options->args); } @@ -142,13 +230,11 @@ static void run_hooks_opt_clear(struct run_hooks_opt *options) int run_hooks_opt(struct repository *r, const char *hook_name, struct run_hooks_opt *options) { - struct strbuf abs_path = STRBUF_INIT; struct hook_cb_data cb_data = { .rc = 0, .hook_name = hook_name, .options = options, }; - const char *const hook_path = find_hook(r, hook_name); int ret = 0; const struct run_process_parallel_opts opts = { .tr2_category = "hook", @@ -182,30 +268,21 @@ int run_hooks_opt(struct repository *r, const char *hook_name, (!options->feed_pipe_cb_data_alloc && options->feed_pipe_cb_data_free)) BUG("feed_pipe_cb_data_alloc and feed_pipe_cb_data_free must be set together"); - if (options->feed_pipe_cb_data_alloc) - options->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx); - if (options->invoked_hook) *options->invoked_hook = 0; - if (!hook_path && !options->error_if_missing) + cb_data.hook_command_list = list_hooks(r, hook_name, options); + if (!cb_data.hook_command_list->nr) { + if (options->error_if_missing) + ret = error("cannot find a hook named %s", hook_name); goto cleanup; - - if (!hook_path) { - ret = error("cannot find a hook named %s", hook_name); - goto cleanup; - } - - cb_data.hook_path = hook_path; - if (options->dir) { - strbuf_add_absolute_path(&abs_path, hook_path); - cb_data.hook_path = abs_path.buf; } run_processes_parallel(&opts); ret = cb_data.rc; cleanup: - strbuf_release(&abs_path); + hook_list_clear(cb_data.hook_command_list, options->feed_pipe_cb_data_free); + free(cb_data.hook_command_list); run_hooks_opt_clear(options); return ret; } diff --git a/hook.h b/hook.h index a6bdc6f90f..3256d2dddb 100644 --- a/hook.h +++ b/hook.h @@ -2,9 +2,41 @@ #define HOOK_H #include "strvec.h" #include "run-command.h" +#include "string-list.h" struct repository; +/** + * Represents a hook command to be run. + * Hooks can be: + * 1. "traditional" (found in the hooks directory) + * 2. "configured" (defined in Git's configuration, not yet implemented). + * The 'kind' field determines which part of the union 'u' is valid. + */ +struct hook { + enum { + HOOK_TRADITIONAL, + } kind; + union { + struct { + const char *path; + } traditional; + } u; + + /** + * Opaque data pointer used to keep internal state across callback calls. + * + * It can be accessed directly via the third hook callback arg: + * struct ... *state = pp_task_cb; + * + * The caller is responsible for managing the memory for this data by + * providing alloc/free callbacks to `run_hooks_opt`. + * + * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. + */ + void *feed_pipe_cb_data; +}; + typedef void (*cb_data_free_fn)(void *data); typedef void *(*cb_data_alloc_fn)(void *init_ctx); @@ -85,19 +117,6 @@ struct run_hooks_opt */ 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 by - * providing alloc/free callbacks to `run_hooks_opt`. - * - * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. - */ - void *feed_pipe_cb_data; - /** * Some hooks need to create a fresh `feed_pipe_cb_data` internal state, * so they can keep track of progress without affecting one another. @@ -128,7 +147,19 @@ struct hook_cb_data { /* rc reflects the cumulative failure state */ int rc; const char *hook_name; - const char *hook_path; + + /** + * A list of hook commands/paths to run for the 'hook_name' event. + * + * The 'string' member of each item holds the path (for traditional hooks) + * or the unique friendly-name for hooks specified in configs. + * The 'util' member of each item points to the corresponding struct hook. + */ + struct string_list *hook_command_list; + + /* Iterator/cursor for the above list, pointing to the next hook to run. */ + size_t hook_to_run_index; + struct run_hooks_opt *options; }; From 9fdaa6788924d4bb5ffc3a5908dae8a50e072f77 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Thu, 19 Feb 2026 00:23:47 +0200 Subject: [PATCH 15/31] hook: add "git hook list" command The previous commit introduced an ability to run multiple commands for hook events and next commit will introduce the ability to define hooks from configs, in addition to the "traditional" hooks from the hookdir. Introduce a new command "git hook list" to make inspecting hooks easier both for users and for the tests we will add. Further commits will expand on this, e.g. by adding a -z output mode. Signed-off-by: Emily Shaffer Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/git-hook.adoc | 5 ++++ builtin/hook.c | 60 +++++++++++++++++++++++++++++++++++++ hook.c | 17 ++--------- hook.h | 24 ++++++++++++++- t/t1800-hook.sh | 22 ++++++++++++++ 5 files changed, 112 insertions(+), 16 deletions(-) diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index f6cc72d2ca..eb0ffcb8a9 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -9,6 +9,7 @@ SYNOPSIS -------- [verse] 'git hook' run [--ignore-missing] [--to-stdin=] [-- ] +'git hook' list DESCRIPTION ----------- @@ -28,6 +29,10 @@ Any positional arguments to the hook should be passed after a mandatory `--` (or `--end-of-options`, see linkgit:gitcli[7]). See linkgit:githooks[5] for arguments hooks might expect (if any). +list:: + Print a list of hooks which will be run on `` event. If no + hooks are configured for that event, print a warning and return 1. + OPTIONS ------- diff --git a/builtin/hook.c b/builtin/hook.c index 7afec380d2..51660c4941 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -6,12 +6,16 @@ #include "hook.h" #include "parse-options.h" #include "strvec.h" +#include "abspath.h" #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") +#define BUILTIN_HOOK_LIST_USAGE \ + N_("git hook list ") static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, + BUILTIN_HOOK_LIST_USAGE, NULL }; @@ -20,6 +24,61 @@ static const char * const builtin_hook_run_usage[] = { NULL }; +static int list(int argc, const char **argv, const char *prefix, + struct repository *repo) +{ + static const char *const builtin_hook_list_usage[] = { + BUILTIN_HOOK_LIST_USAGE, + NULL + }; + struct string_list *head; + struct string_list_item *item; + const char *hookname = NULL; + int ret = 0; + + struct option list_options[] = { + OPT_END(), + }; + + argc = parse_options(argc, argv, prefix, list_options, + builtin_hook_list_usage, 0); + + /* + * The only unnamed argument provided should be the hook-name; if we add + * arguments later they probably should be caught by parse_options. + */ + if (argc != 1) + usage_msg_opt(_("You must specify a hook event name to list."), + builtin_hook_list_usage, list_options); + + hookname = argv[0]; + + head = list_hooks(repo, hookname, NULL); + + if (!head->nr) { + warning(_("No hooks found for event '%s'"), hookname); + ret = 1; /* no hooks found */ + goto cleanup; + } + + for_each_string_list_item(item, head) { + struct hook *h = item->util; + + switch (h->kind) { + case HOOK_TRADITIONAL: + printf("%s\n", _("hook from hookdir")); + break; + default: + BUG("unknown hook kind"); + } + } + +cleanup: + hook_list_clear(head, NULL); + free(head); + return ret; +} + static int run(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { @@ -77,6 +136,7 @@ int cmd_hook(int argc, parse_opt_subcommand_fn *fn = NULL; struct option builtin_hook_options[] = { OPT_SUBCOMMAND("run", &fn, run), + OPT_SUBCOMMAND("list", &fn, list), OPT_END(), }; diff --git a/hook.c b/hook.c index c008a7232d..979a97a538 100644 --- a/hook.c +++ b/hook.c @@ -61,7 +61,7 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) free(h); } -static void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free) +void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free) { struct string_list_item *item; @@ -101,20 +101,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, string_list_append(hook_list, hook_path)->util = h; } -/* - * Provides a list of hook commands to run for the 'hookname' event. - * - * This function consolidates hooks from two sources: - * 1. The config-based hooks (not yet implemented). - * 2. The "traditional" hook found in the repository hooks directory - * (e.g., .git/hooks/pre-commit). - * - * The list is ordered by execution priority. - * - * The caller is responsible for freeing the memory of the returned list - * using string_list_clear() and free(). - */ -static struct string_list *list_hooks(struct repository *r, const char *hookname, +struct string_list *list_hooks(struct repository *r, const char *hookname, struct run_hooks_opt *options) { struct string_list *hook_head; diff --git a/hook.h b/hook.h index 3256d2dddb..fea221f87d 100644 --- a/hook.h +++ b/hook.h @@ -163,7 +163,29 @@ struct hook_cb_data { struct run_hooks_opt *options; }; -/* +/** + * Provides a list of hook commands to run for the 'hookname' event. + * + * This function consolidates hooks from two sources: + * 1. The config-based hooks (not yet implemented). + * 2. The "traditional" hook found in the repository hooks directory + * (e.g., .git/hooks/pre-commit). + * + * The list is ordered by execution priority. + * + * The caller is responsible for freeing the memory of the returned list + * using string_list_clear() and free(). + */ +struct string_list *list_hooks(struct repository *r, const char *hookname, + struct run_hooks_opt *options); + +/** + * Frees the memory allocated for the hook list, including the `struct hook` + * items and their internal state. + */ +void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free); + +/** * Returns the path to the hook file, or NULL if the hook is missing * or disabled. Note that this points to static storage that will be * overwritten by further calls to find_hook and run_hook_*. diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index ed28a2fadb..3ec11f1249 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -10,9 +10,31 @@ test_expect_success 'git hook usage' ' test_expect_code 129 git hook run && test_expect_code 129 git hook run -h && test_expect_code 129 git hook run --unknown 2>err && + test_expect_code 129 git hook list && + test_expect_code 129 git hook list -h && grep "unknown option" err ' +test_expect_success 'git hook list: nonexistent hook' ' + cat >stderr.expect <<-\EOF && + warning: No hooks found for event '\''test-hook'\'' + EOF + test_expect_code 1 git hook list test-hook 2>stderr.actual && + test_cmp stderr.expect stderr.actual +' + +test_expect_success 'git hook list: traditional hook from hookdir' ' + test_hook test-hook <<-EOF && + echo Test hook + EOF + + cat >expect <<-\EOF && + hook from hookdir + EOF + git hook list test-hook >actual && + test_cmp expect actual +' + test_expect_success 'git hook run: nonexistent hook' ' cat >stderr.expect <<-\EOF && error: cannot find a hook named test-hook From 03b4043b9182bd3d36541371fa39f04d6d038286 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Thu, 19 Feb 2026 00:23:48 +0200 Subject: [PATCH 16/31] hook: include hooks from the config Teach the hook.[hc] library to parse configs to populate the list of hooks to run for a given event. Multiple commands can be specified for a given hook by providing "hook..command = " and "hook..event = " lines. Hooks will be started in config order of the "hook..event" lines and will be run sequentially (.jobs == 1) like before. Running the hooks in parallel will be enabled in a future patch. The "traditional" hook from the hookdir is run last, if present. A strmap cache is added to struct repository to avoid re-reading the configs on each rook run. This is useful for hooks like the ref-transaction which gets executed multiple times per process. Examples: $ git config --get-regexp "^hook\." hook.bar.command=~/bar.sh hook.bar.event=pre-commit # Will run ~/bar.sh, then .git/hooks/pre-commit $ git hook run pre-commit Signed-off-by: Emily Shaffer Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 15 +++ Documentation/git-hook.adoc | 128 ++++++++++++++++++++- builtin/hook.c | 3 + hook.c | 197 ++++++++++++++++++++++++++++++++- hook.h | 14 ++- repository.c | 6 + repository.h | 6 + t/t1800-hook.sh | 149 ++++++++++++++++++++++++- 8 files changed, 513 insertions(+), 5 deletions(-) create mode 100644 Documentation/config/hook.adoc diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc new file mode 100644 index 0000000000..9faafe3016 --- /dev/null +++ b/Documentation/config/hook.adoc @@ -0,0 +1,15 @@ +hook..command:: + The command to execute for `hook.`. `` is a unique + "friendly" name that identifies this hook. (The hook events that + trigger the command are configured with `hook..event`.) The + value can be an executable path or a shell oneliner. If more than + one value is specified for the same ``, only the last value + parsed is used. See linkgit:git-hook[1]. + +hook..event:: + The hook events that trigger `hook.`. The value is the name + of a hook event, like "pre-commit" or "update". (See + linkgit:githooks[5] for a complete list of hook events.) On the + specified event, the associated `hook..command` is executed. + This is a multi-valued key. To run `hook.` on multiple + events, specify the key more than once. See linkgit:git-hook[1]. diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index eb0ffcb8a9..7e4259e4f0 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -17,12 +17,96 @@ DESCRIPTION A command interface for running git hooks (see linkgit:githooks[5]), for use by other scripted git commands. +This command parses the default configuration files for sets of configs like +so: + + [hook "linter"] + event = pre-commit + command = ~/bin/linter --cpp20 + +In this example, `[hook "linter"]` represents one script - `~/bin/linter +--cpp20` - which can be shared by many repos, and even by many hook events, if +appropriate. + +To add an unrelated hook which runs on a different event, for example a +spell-checker for your commit messages, you would write a configuration like so: + + [hook "linter"] + event = pre-commit + command = ~/bin/linter --cpp20 + [hook "spellcheck"] + event = commit-msg + command = ~/bin/spellchecker + +With this config, when you run 'git commit', first `~/bin/linter --cpp20` will +have a chance to check your files to be committed (during the `pre-commit` hook +event`), and then `~/bin/spellchecker` will have a chance to check your commit +message (during the `commit-msg` hook event). + +Commands are run in the order Git encounters their associated +`hook..event` configs during the configuration parse (see +linkgit:git-config[1]). Although multiple `hook.linter.event` configs can be +added, only one `hook.linter.command` event is valid - Git uses "last-one-wins" +to determine which command to run. + +So if you wanted your linter to run when you commit as well as when you push, +you would configure it like so: + + [hook "linter"] + event = pre-commit + event = pre-push + command = ~/bin/linter --cpp20 + +With this config, `~/bin/linter --cpp20` would be run by Git before a commit is +generated (during `pre-commit`) as well as before a push is performed (during +`pre-push`). + +And if you wanted to run your linter as well as a secret-leak detector during +only the "pre-commit" hook event, you would configure it instead like so: + + [hook "linter"] + event = pre-commit + command = ~/bin/linter --cpp20 + [hook "no-leaks"] + event = pre-commit + command = ~/bin/leak-detector + +With this config, before a commit is generated (during `pre-commit`), Git would +first start `~/bin/linter --cpp20` and second start `~/bin/leak-detector`. It +would evaluate the output of each when deciding whether to proceed with the +commit. + +For a full list of hook events which you can set your `hook..event` to, +and how hooks are invoked during those events, see linkgit:githooks[5]. + +Git will ignore any `hook..event` that specifies an event it doesn't +recognize. This is intended so that tools which wrap Git can use the hook +infrastructure to run their own hooks; see "WRAPPERS" for more guidance. + +In general, when instructions suggest adding a script to +`.git/hooks/`, you can specify it in the config instead by running: + +---- +git config set hook..command +git config set --append hook..event +---- + +This way you can share the script between multiple repos. That is, `cp +~/my-script.sh ~/project/.git/hooks/pre-commit` would become: + +---- +git config set hook.my-script.command ~/my-script.sh +git config set --append hook.my-script.event pre-commit +---- + SUBCOMMANDS ----------- run:: - Run the `` hook. See linkgit:githooks[5] for - supported hook names. + Runs hooks configured for ``, in the order they are + discovered during the config parse. The default `` from + the hookdir is run last. See linkgit:githooks[5] for supported + hook names. + Any positional arguments to the hook should be passed after a @@ -46,6 +130,46 @@ OPTIONS tools that want to do a blind one-shot run of a hook that may or may not be present. +WRAPPERS +-------- + +`git hook run` has been designed to make it easy for tools which wrap Git to +configure and execute hooks using the Git hook infrastructure. It is possible to +provide arguments and stdin via the command line, as well as specifying parallel +or series execution if the user has provided multiple hooks. + +Assuming your wrapper wants to support a hook named "mywrapper-start-tests", you +can have your users specify their hooks like so: + + [hook "setup-test-dashboard"] + event = mywrapper-start-tests + command = ~/mywrapper/setup-dashboard.py --tap + +Then, in your 'mywrapper' tool, you can invoke any users' configured hooks by +running: + +---- +git hook run mywrapper-start-tests \ + # providing something to stdin + --stdin some-tempfile-123 \ + # execute hooks in serial + # plus some arguments of your own... + -- \ + --testname bar \ + baz +---- + +Take care to name your wrapper's hook events in a way which is unlikely to +overlap with Git's native hooks (see linkgit:githooks[5]) - a hook event named +`mywrappertool-validate-commit` is much less likely to be added to native Git +than a hook event named `validate-commit`. If Git begins to use a hook event +named the same thing as your wrapper hook, it may invoke your users' hooks in +unintended and unsupported ways. + +CONFIGURATION +------------- +include::config/hook.adoc[] + SEE ALSO -------- linkgit:githooks[5] diff --git a/builtin/hook.c b/builtin/hook.c index 51660c4941..e151bb2cd1 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -68,6 +68,9 @@ static int list(int argc, const char **argv, const char *prefix, case HOOK_TRADITIONAL: printf("%s\n", _("hook from hookdir")); break; + case HOOK_CONFIGURED: + printf("%s\n", h->u.configured.friendly_name); + break; default: BUG("unknown hook kind"); } diff --git a/hook.c b/hook.c index 979a97a538..8a9b405f76 100644 --- a/hook.c +++ b/hook.c @@ -4,9 +4,11 @@ #include "gettext.h" #include "hook.h" #include "path.h" +#include "parse.h" #include "run-command.h" #include "config.h" #include "strbuf.h" +#include "strmap.h" #include "environment.h" #include "setup.h" @@ -54,6 +56,10 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) if (h->kind == HOOK_TRADITIONAL) free((void *)h->u.traditional.path); + else if (h->kind == HOOK_CONFIGURED) { + free((void *)h->u.configured.friendly_name); + free((void *)h->u.configured.command); + } if (cb_data_free) cb_data_free(h->feed_pipe_cb_data); @@ -101,6 +107,187 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, string_list_append(hook_list, hook_path)->util = h; } +static void unsorted_string_list_remove(struct string_list *list, + const char *str) +{ + struct string_list_item *item = unsorted_string_list_lookup(list, str); + if (item) + unsorted_string_list_delete_item(list, item - list->items, 0); +} + +/* + * Callback struct to collect all hook.* keys in a single config pass. + * commands: friendly-name to command map. + * event_hooks: event-name to list of friendly-names map. + * disabled_hooks: set of friendly-names with hook.name.enabled = false. + */ +struct hook_all_config_cb { + struct strmap commands; + struct strmap event_hooks; + struct string_list disabled_hooks; +}; + +/* repo_config() callback that collects all hook.* configuration in one pass. */ +static int hook_config_lookup_all(const char *key, const char *value, + const struct config_context *ctx UNUSED, + void *cb_data) +{ + struct hook_all_config_cb *data = cb_data; + const char *name, *subkey; + char *hook_name; + size_t name_len = 0; + + if (parse_config_key(key, "hook", &name, &name_len, &subkey)) + return 0; + + if (!value) + return config_error_nonbool(key); + + /* Extract name, ensuring it is null-terminated. */ + hook_name = xmemdupz(name, name_len); + + if (!strcmp(subkey, "event")) { + struct string_list *hooks = + strmap_get(&data->event_hooks, value); + + if (!hooks) { + hooks = xcalloc(1, sizeof(*hooks)); + string_list_init_dup(hooks); + strmap_put(&data->event_hooks, value, hooks); + } + + /* Re-insert if necessary to preserve last-seen order. */ + unsorted_string_list_remove(hooks, hook_name); + string_list_append(hooks, hook_name); + } else if (!strcmp(subkey, "command")) { + /* Store command overwriting the old value */ + char *old = strmap_put(&data->commands, hook_name, + xstrdup(value)); + free(old); + } + + free(hook_name); + return 0; +} + +/* + * The hook config cache maps each hook event name to a string_list where + * every item's string is the hook's friendly-name and its util pointer is + * the corresponding command string. Both strings are owned by the map. + * + * Disabled hooks and hooks missing a command are already filtered out at + * parse time, so callers can iterate the list directly. + */ +void hook_cache_clear(struct strmap *cache) +{ + struct hashmap_iter iter; + struct strmap_entry *e; + + strmap_for_each_entry(cache, &iter, e) { + struct string_list *hooks = e->value; + string_list_clear(hooks, 1); /* free util (command) pointers */ + free(hooks); + } + strmap_clear(cache, 0); +} + +/* Populate `cache` with the complete hook configuration */ +static void build_hook_config_map(struct repository *r, struct strmap *cache) +{ + struct hook_all_config_cb cb_data; + struct hashmap_iter iter; + struct strmap_entry *e; + + strmap_init(&cb_data.commands); + strmap_init(&cb_data.event_hooks); + string_list_init_dup(&cb_data.disabled_hooks); + + /* Parse all configs in one run. */ + repo_config(r, hook_config_lookup_all, &cb_data); + + /* Construct the cache from parsed configs. */ + strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { + struct string_list *hook_names = e->value; + struct string_list *hooks = xcalloc(1, sizeof(*hooks)); + + string_list_init_dup(hooks); + + for (size_t i = 0; i < hook_names->nr; i++) { + const char *hname = hook_names->items[i].string; + char *command; + + command = strmap_get(&cb_data.commands, hname); + if (!command) + die(_("'hook.%s.command' must be configured or " + "'hook.%s.event' must be removed;" + " aborting."), hname, hname); + + /* util stores the command; owned by the cache. */ + string_list_append(hooks, hname)->util = + xstrdup(command); + } + + strmap_put(cache, e->key, hooks); + } + + strmap_clear(&cb_data.commands, 1); + string_list_clear(&cb_data.disabled_hooks, 0); + strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { + string_list_clear(e->value, 0); + free(e->value); + } + strmap_clear(&cb_data.event_hooks, 0); +} + +/* Return the hook config map for `r`, populating it first if needed. */ +static struct strmap *get_hook_config_cache(struct repository *r) +{ + struct strmap *cache = NULL; + + if (r) { + /* + * For in-repo calls, the map is stored in r->hook_config_cache, + * so repeated invocations don't parse the configs, so allocate + * it just once on the first call. + */ + if (!r->hook_config_cache) { + r->hook_config_cache = xcalloc(1, sizeof(*cache)); + strmap_init(r->hook_config_cache); + build_hook_config_map(r, r->hook_config_cache); + } + cache = r->hook_config_cache; + } + + return cache; +} + +static void list_hooks_add_configured(struct repository *r, + const char *hookname, + struct string_list *list, + struct run_hooks_opt *options) +{ + struct strmap *cache = get_hook_config_cache(r); + struct string_list *configured_hooks = strmap_get(cache, hookname); + + /* Iterate through configured hooks and initialize internal states */ + for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { + const char *friendly_name = configured_hooks->items[i].string; + const char *command = configured_hooks->items[i].util; + struct hook *hook = xcalloc(1, sizeof(struct hook)); + + if (options && options->feed_pipe_cb_data_alloc) + hook->feed_pipe_cb_data = + options->feed_pipe_cb_data_alloc( + options->feed_pipe_ctx); + + hook->kind = HOOK_CONFIGURED; + hook->u.configured.friendly_name = xstrdup(friendly_name); + hook->u.configured.command = xstrdup(command); + + string_list_append(list, friendly_name)->util = hook; + } +} + struct string_list *list_hooks(struct repository *r, const char *hookname, struct run_hooks_opt *options) { @@ -112,6 +299,9 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, hook_head = xmalloc(sizeof(struct string_list)); string_list_init_dup(hook_head); + /* Add hooks from the config, e.g. hook.myhook.event = pre-commit */ + list_hooks_add_configured(r, hookname, hook_head, options); + /* Add the default "traditional" hooks from hookdir. */ list_hooks_add_default(r, hookname, hook_head, options); @@ -164,8 +354,13 @@ static int pick_next_hook(struct child_process *cp, cp->dir = hook_cb->options->dir; /* Add hook exec paths or commands */ - if (h->kind == HOOK_TRADITIONAL) + if (h->kind == HOOK_TRADITIONAL) { strvec_push(&cp->args, h->u.traditional.path); + } else if (h->kind == HOOK_CONFIGURED) { + /* to enable oneliners, let config-specified hooks run in shell. */ + cp->use_shell = true; + strvec_push(&cp->args, h->u.configured.command); + } if (!cp->args.nr) BUG("hook must have at least one command or exec path"); diff --git a/hook.h b/hook.h index fea221f87d..e949f5d488 100644 --- a/hook.h +++ b/hook.h @@ -3,6 +3,7 @@ #include "strvec.h" #include "run-command.h" #include "string-list.h" +#include "strmap.h" struct repository; @@ -10,17 +11,22 @@ struct repository; * Represents a hook command to be run. * Hooks can be: * 1. "traditional" (found in the hooks directory) - * 2. "configured" (defined in Git's configuration, not yet implemented). + * 2. "configured" (defined in Git's configuration via hook..event). * The 'kind' field determines which part of the union 'u' is valid. */ struct hook { enum { HOOK_TRADITIONAL, + HOOK_CONFIGURED, } kind; union { struct { const char *path; } traditional; + struct { + const char *friendly_name; + const char *command; + } configured; } u; /** @@ -185,6 +191,12 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, */ void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free); +/** + * Frees the hook configuration cache stored in `struct repository`. + * Called by repo_clear(). + */ +void hook_cache_clear(struct strmap *cache); + /** * Returns the path to the hook file, or NULL if the hook is missing * or disabled. Note that this points to static storage that will be diff --git a/repository.c b/repository.c index c7e75215ac..2518d66975 100644 --- a/repository.c +++ b/repository.c @@ -1,6 +1,7 @@ #include "git-compat-util.h" #include "abspath.h" #include "repository.h" +#include "hook.h" #include "odb.h" #include "config.h" #include "object.h" @@ -393,6 +394,11 @@ void repo_clear(struct repository *repo) FREE_AND_NULL(repo->index); } + if (repo->hook_config_cache) { + hook_cache_clear(repo->hook_config_cache); + FREE_AND_NULL(repo->hook_config_cache); + } + if (repo->promisor_remote_config) { promisor_remote_clear(repo->promisor_remote_config); FREE_AND_NULL(repo->promisor_remote_config); diff --git a/repository.h b/repository.h index 6063c4b846..df91338259 100644 --- a/repository.h +++ b/repository.h @@ -157,6 +157,12 @@ struct repository { /* True if commit-graph has been disabled within this process. */ int commit_graph_disabled; + /* + * Lazily-populated cache mapping hook event names to configured hooks. + * NULL until first hook use. + */ + struct strmap *hook_config_cache; + /* Configurations related to promisor remotes. */ char *repository_format_partial_clone; struct promisor_remote_config *promisor_remote_config; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 3ec11f1249..f1048a5119 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -1,14 +1,31 @@ #!/bin/sh -test_description='git-hook command' +test_description='git-hook command and config-managed multihooks' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh +setup_hooks () { + test_config hook.ghi.command "/path/ghi" + test_config hook.ghi.event pre-commit --add + test_config hook.ghi.event test-hook --add + test_config_global hook.def.command "/path/def" + test_config_global hook.def.event pre-commit --add +} + +setup_hookdir () { + mkdir .git/hooks + write_script .git/hooks/pre-commit <<-EOF + echo \"Legacy Hook\" + EOF + test_when_finished rm -rf .git/hooks +} + test_expect_success 'git hook usage' ' test_expect_code 129 git hook && test_expect_code 129 git hook run && test_expect_code 129 git hook run -h && + test_expect_code 129 git hook list -h && test_expect_code 129 git hook run --unknown 2>err && test_expect_code 129 git hook list && test_expect_code 129 git hook list -h && @@ -35,6 +52,15 @@ test_expect_success 'git hook list: traditional hook from hookdir' ' test_cmp expect actual ' +test_expect_success 'git hook list: configured hook' ' + test_config hook.myhook.command "echo Hello" && + test_config hook.myhook.event test-hook --add && + + echo "myhook" >expect && + git hook list test-hook >actual && + test_cmp expect actual +' + test_expect_success 'git hook run: nonexistent hook' ' cat >stderr.expect <<-\EOF && error: cannot find a hook named test-hook @@ -172,6 +198,126 @@ test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' ' test_hook_tty commit -m"B.new" ' +test_expect_success 'git hook list orders by config order' ' + setup_hooks && + + cat >expected <<-\EOF && + def + ghi + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'git hook list reorders on duplicate event declarations' ' + setup_hooks && + + # 'def' is usually configured globally; move it to the end by + # configuring it locally. + test_config hook.def.event "pre-commit" --add && + + cat >expected <<-\EOF && + ghi + def + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'hook can be configured for multiple events' ' + setup_hooks && + + # 'ghi' should be included in both 'pre-commit' and 'test-hook' + git hook list pre-commit >actual && + grep "ghi" actual && + git hook list test-hook >actual && + grep "ghi" actual +' + +test_expect_success 'git hook list shows hooks from the hookdir' ' + setup_hookdir && + + cat >expected <<-\EOF && + hook from hookdir + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'inline hook definitions execute oneliners' ' + test_config hook.oneliner.event "pre-commit" && + test_config hook.oneliner.command "echo \"Hello World\"" && + + echo "Hello World" >expected && + + # hooks are run with stdout_to_stderr = 1 + git hook run pre-commit 2>actual && + test_cmp expected actual +' + +test_expect_success 'inline hook definitions resolve paths' ' + write_script sample-hook.sh <<-\EOF && + echo \"Sample Hook\" + EOF + + test_when_finished "rm sample-hook.sh" && + + test_config hook.sample-hook.event pre-commit && + test_config hook.sample-hook.command "\"$(pwd)/sample-hook.sh\"" && + + echo \"Sample Hook\" >expected && + + # hooks are run with stdout_to_stderr = 1 + git hook run pre-commit 2>actual && + test_cmp expected actual +' + +test_expect_success 'hookdir hook included in git hook run' ' + setup_hookdir && + + echo \"Legacy Hook\" >expected && + + # hooks are run with stdout_to_stderr = 1 + git hook run pre-commit 2>actual && + test_cmp expected actual +' + +test_expect_success 'stdin to multiple hooks' ' + test_config hook.stdin-a.event "test-hook" && + test_config hook.stdin-a.command "xargs -P1 -I% echo a%" && + test_config hook.stdin-b.event "test-hook" && + test_config hook.stdin-b.command "xargs -P1 -I% echo b%" && + + cat >input <<-\EOF && + 1 + 2 + 3 + EOF + + cat >expected <<-\EOF && + a1 + a2 + a3 + b1 + b2 + b3 + EOF + + git hook run --to-stdin=input test-hook 2>actual && + test_cmp expected actual +' + +test_expect_success 'rejects hooks with no commands configured' ' + test_config hook.broken.event "test-hook" && + test_must_fail git hook list test-hook 2>actual && + test_grep "hook.broken.command" actual && + test_must_fail git hook run test-hook 2>actual && + test_grep "hook.broken.command" actual +' + test_expect_success 'git hook run a hook with a bad shebang' ' test_when_finished "rm -rf bad-hooks" && mkdir bad-hooks && @@ -189,6 +335,7 @@ test_expect_success 'git hook run a hook with a bad shebang' ' ' test_expect_success 'stdin to hooks' ' + mkdir -p .git/hooks && write_script .git/hooks/test-hook <<-\EOF && echo BEGIN stdin cat From 1ecce722cdb9c42dd4c69e45e02cb850cd558ef2 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Thu, 19 Feb 2026 00:23:49 +0200 Subject: [PATCH 17/31] hook: allow disabling config hooks Hooks specified via configs are always enabled, however users might want to disable them without removing from the config, like locally disabling a global hook. Add a hook..enabled config which defaults to true and can be optionally set for each configured hook. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 7 +++++++ hook.c | 20 ++++++++++++++++++++ t/t1800-hook.sh | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 9faafe3016..0cda4745a6 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -13,3 +13,10 @@ hook..event:: specified event, the associated `hook..command` is executed. This is a multi-valued key. To run `hook.` on multiple events, specify the key more than once. See linkgit:git-hook[1]. + +hook..enabled:: + Whether the hook `hook.` is enabled. Defaults to `true`. + Set to `false` to disable the hook without removing its + configuration. This is particularly useful when a hook is defined + in a system or global config file and needs to be disabled for a + specific repository. See linkgit:git-hook[1]. diff --git a/hook.c b/hook.c index 8a9b405f76..35c24bf33d 100644 --- a/hook.c +++ b/hook.c @@ -164,6 +164,21 @@ static int hook_config_lookup_all(const char *key, const char *value, char *old = strmap_put(&data->commands, hook_name, xstrdup(value)); free(old); + } else if (!strcmp(subkey, "enabled")) { + switch (git_parse_maybe_bool(value)) { + case 0: /* disabled */ + if (!unsorted_string_list_lookup(&data->disabled_hooks, + hook_name)) + string_list_append(&data->disabled_hooks, + hook_name); + break; + case 1: /* enabled: undo a prior disabled entry */ + unsorted_string_list_remove(&data->disabled_hooks, + hook_name); + break; + default: + break; /* ignore unrecognised values */ + } } free(hook_name); @@ -216,6 +231,11 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) const char *hname = hook_names->items[i].string; char *command; + /* filter out disabled hooks */ + if (unsorted_string_list_lookup(&cb_data.disabled_hooks, + hname)) + continue; + command = strmap_get(&cb_data.commands, hname); if (!command) die(_("'hook.%s.command' must be configured or " diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index f1048a5119..9797802735 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -318,6 +318,38 @@ test_expect_success 'rejects hooks with no commands configured' ' test_grep "hook.broken.command" actual ' +test_expect_success 'disabled hook is not run' ' + test_config hook.skipped.event "test-hook" && + test_config hook.skipped.command "echo \"Should not run\"" && + test_config hook.skipped.enabled false && + + git hook run --ignore-missing test-hook 2>actual && + test_must_be_empty actual +' + +test_expect_success 'disabled hook does not appear in git hook list' ' + test_config hook.active.event "pre-commit" && + test_config hook.active.command "echo active" && + test_config hook.inactive.event "pre-commit" && + test_config hook.inactive.command "echo inactive" && + test_config hook.inactive.enabled false && + + git hook list pre-commit >actual && + test_grep "active" actual && + test_grep ! "inactive" actual +' + +test_expect_success 'globally disabled hook can be re-enabled locally' ' + test_config_global hook.global-hook.event "test-hook" && + test_config_global hook.global-hook.command "echo \"global-hook ran\"" && + test_config_global hook.global-hook.enabled false && + test_config hook.global-hook.enabled true && + + echo "global-hook ran" >expected && + git hook run test-hook 2>actual && + test_cmp expected actual +' + test_expect_success 'git hook run a hook with a bad shebang' ' test_when_finished "rm -rf bad-hooks" && mkdir bad-hooks && From d084fa2a915784d65257fbaff43f00b3ea5c8a44 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Thu, 19 Feb 2026 00:23:50 +0200 Subject: [PATCH 18/31] hook: allow event = "" to overwrite previous values Add the ability for empty events to clear previously set multivalue variables, so the newly added "hook.*.event" behave like the other multivalued keys. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 4 +++- hook.c | 29 +++++++++++++++++++---------- t/t1800-hook.sh | 12 ++++++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 0cda4745a6..64e845a260 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -12,7 +12,9 @@ hook..event:: linkgit:githooks[5] for a complete list of hook events.) On the specified event, the associated `hook..command` is executed. This is a multi-valued key. To run `hook.` on multiple - events, specify the key more than once. See linkgit:git-hook[1]. + events, specify the key more than once. An empty value resets + the list of events, clearing any previously defined events for + `hook.`. See linkgit:git-hook[1]. hook..enabled:: Whether the hook `hook.` is enabled. Defaults to `true`. diff --git a/hook.c b/hook.c index 35c24bf33d..fee0a7ab4f 100644 --- a/hook.c +++ b/hook.c @@ -147,18 +147,27 @@ static int hook_config_lookup_all(const char *key, const char *value, hook_name = xmemdupz(name, name_len); if (!strcmp(subkey, "event")) { - struct string_list *hooks = - strmap_get(&data->event_hooks, value); + if (!*value) { + /* Empty values reset previous events for this hook. */ + struct hashmap_iter iter; + struct strmap_entry *e; - if (!hooks) { - hooks = xcalloc(1, sizeof(*hooks)); - string_list_init_dup(hooks); - strmap_put(&data->event_hooks, value, hooks); + strmap_for_each_entry(&data->event_hooks, &iter, e) + unsorted_string_list_remove(e->value, hook_name); + } else { + struct string_list *hooks = + strmap_get(&data->event_hooks, value); + + if (!hooks) { + hooks = xcalloc(1, sizeof(*hooks)); + string_list_init_dup(hooks); + strmap_put(&data->event_hooks, value, hooks); + } + + /* Re-insert if necessary to preserve last-seen order. */ + unsorted_string_list_remove(hooks, hook_name); + string_list_append(hooks, hook_name); } - - /* Re-insert if necessary to preserve last-seen order. */ - unsorted_string_list_remove(hooks, hook_name); - string_list_append(hooks, hook_name); } else if (!strcmp(subkey, "command")) { /* Store command overwriting the old value */ char *old = strmap_put(&data->commands, hook_name, diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 9797802735..fb6bc554b9 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -226,6 +226,18 @@ test_expect_success 'git hook list reorders on duplicate event declarations' ' test_cmp expected actual ' +test_expect_success 'git hook list: empty event value resets events' ' + setup_hooks && + + # ghi is configured for pre-commit; reset it with an empty value + test_config hook.ghi.event "" --add && + + # only def should remain for pre-commit + echo "def" >expected && + git hook list pre-commit >actual && + test_cmp expected actual +' + test_expect_success 'hook can be configured for multiple events' ' setup_hooks && From b51e238ddf896439d2746b883d892f8f9bba649f Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Thu, 19 Feb 2026 00:23:51 +0200 Subject: [PATCH 19/31] hook: allow out-of-repo 'git hook' invocations Since hooks can now be supplied via the config, and a config can be present without a gitdir via the global and system configs, we can start to allow 'git hook run' to occur without a gitdir. This enables us to do things like run sendemail-validate hooks when running 'git send-email' from a nongit directory. It still doesn't make sense to look for hooks in the hookdir in nongit repos, though, as there is no hookdir. Signed-off-by: Emily Shaffer Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- git.c | 2 +- hook.c | 30 ++++++++++++++++++++++++++++-- t/t1800-hook.sh | 16 +++++++++++----- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/git.c b/git.c index c5fad56813..a9e462ee32 100644 --- a/git.c +++ b/git.c @@ -586,7 +586,7 @@ static struct cmd_struct commands[] = { { "grep", cmd_grep, RUN_SETUP_GENTLY }, { "hash-object", cmd_hash_object }, { "help", cmd_help }, - { "hook", cmd_hook, RUN_SETUP }, + { "hook", cmd_hook, RUN_SETUP_GENTLY }, { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT }, { "init", cmd_init_db }, { "init-db", cmd_init_db }, diff --git a/hook.c b/hook.c index fee0a7ab4f..2c8252b2c4 100644 --- a/hook.c +++ b/hook.c @@ -18,6 +18,9 @@ const char *find_hook(struct repository *r, const char *name) int found_hook; + if (!r || !r->gitdir) + return NULL; + repo_git_path_replace(r, &path, "hooks/%s", name); found_hook = access(path.buf, X_OK) >= 0; #ifdef STRIP_EXTENSION @@ -268,12 +271,18 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) strmap_clear(&cb_data.event_hooks, 0); } -/* Return the hook config map for `r`, populating it first if needed. */ +/* + * Return the hook config map for `r`, populating it first if needed. + * + * Out-of-repo calls (r->gitdir == NULL) allocate and return a temporary + * cache map; the caller is responsible for freeing it with + * hook_cache_clear() + free(). + */ static struct strmap *get_hook_config_cache(struct repository *r) { struct strmap *cache = NULL; - if (r) { + if (r && r->gitdir) { /* * For in-repo calls, the map is stored in r->hook_config_cache, * so repeated invocations don't parse the configs, so allocate @@ -285,6 +294,14 @@ static struct strmap *get_hook_config_cache(struct repository *r) build_hook_config_map(r, r->hook_config_cache); } cache = r->hook_config_cache; + } else { + /* + * Out-of-repo calls (no gitdir) allocate and return a temporary + * map cache which gets free'd immediately by the caller. + */ + cache = xcalloc(1, sizeof(*cache)); + strmap_init(cache); + build_hook_config_map(r, cache); } return cache; @@ -315,6 +332,15 @@ static void list_hooks_add_configured(struct repository *r, string_list_append(list, friendly_name)->util = hook; } + + /* + * Cleanup temporary cache for out-of-repo calls since they can't be + * stored persistently. Next out-of-repo calls will have to re-parse. + */ + if (!r || !r->gitdir) { + hook_cache_clear(cache); + free(cache); + } } struct string_list *list_hooks(struct repository *r, const char *hookname, diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index fb6bc554b9..e58151e8f8 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -131,12 +131,18 @@ test_expect_success 'git hook run -- pass arguments' ' test_cmp expect actual ' -test_expect_success 'git hook run -- out-of-repo runs excluded' ' - test_hook test-hook <<-EOF && - echo Test hook - EOF +test_expect_success 'git hook run: out-of-repo runs execute global hooks' ' + test_config_global hook.global-hook.event test-hook --add && + test_config_global hook.global-hook.command "echo no repo no problems" --add && - nongit test_must_fail git hook run test-hook + echo "global-hook" >expect && + nongit git hook list test-hook >actual && + test_cmp expect actual && + + echo "no repo no problems" >expect && + + nongit git hook run test-hook 2>actual && + test_cmp expect actual ' test_expect_success 'git -c core.hooksPath= hook run' ' From 4b12cd3ae3acbc819189758d09f9c983bde16040 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Thu, 19 Feb 2026 00:23:52 +0200 Subject: [PATCH 20/31] hook: add -z option to "git hook list" Add a NUL-terminate mode to git hook list, just in case hooks are configured with weird characters like newlines in their names. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/git-hook.adoc | 8 ++++++-- builtin/hook.c | 9 ++++++--- t/t1800-hook.sh | 13 +++++++++++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 7e4259e4f0..12d2701b52 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git hook' run [--ignore-missing] [--to-stdin=] [-- ] -'git hook' list +'git hook' list [-z] DESCRIPTION ----------- @@ -113,9 +113,10 @@ Any positional arguments to the hook should be passed after a mandatory `--` (or `--end-of-options`, see linkgit:gitcli[7]). See linkgit:githooks[5] for arguments hooks might expect (if any). -list:: +list [-z]:: Print a list of hooks which will be run on `` event. If no hooks are configured for that event, print a warning and return 1. + Use `-z` to terminate output lines with NUL instead of newlines. OPTIONS ------- @@ -130,6 +131,9 @@ OPTIONS tools that want to do a blind one-shot run of a hook that may or may not be present. +-z:: + Terminate "list" output lines with NUL instead of newlines. + WRAPPERS -------- diff --git a/builtin/hook.c b/builtin/hook.c index e151bb2cd1..83020dfb4f 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -11,7 +11,7 @@ #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") #define BUILTIN_HOOK_LIST_USAGE \ - N_("git hook list ") + N_("git hook list [-z] ") static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, @@ -34,9 +34,12 @@ static int list(int argc, const char **argv, const char *prefix, struct string_list *head; struct string_list_item *item; const char *hookname = NULL; + int line_terminator = '\n'; int ret = 0; struct option list_options[] = { + OPT_SET_INT('z', NULL, &line_terminator, + N_("use NUL as line terminator"), '\0'), OPT_END(), }; @@ -66,10 +69,10 @@ static int list(int argc, const char **argv, const char *prefix, switch (h->kind) { case HOOK_TRADITIONAL: - printf("%s\n", _("hook from hookdir")); + printf("%s%c", _("hook from hookdir"), line_terminator); break; case HOOK_CONFIGURED: - printf("%s\n", h->u.configured.friendly_name); + printf("%s%c", h->u.configured.friendly_name, line_terminator); break; default: BUG("unknown hook kind"); diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index e58151e8f8..b1583e9ef9 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -61,6 +61,19 @@ test_expect_success 'git hook list: configured hook' ' test_cmp expect actual ' +test_expect_success 'git hook list: -z shows NUL-terminated output' ' + test_hook test-hook <<-EOF && + echo Test hook + EOF + test_config hook.myhook.command "echo Hello" && + test_config hook.myhook.event test-hook --add && + + printf "myhookQhook from hookdirQ" >expect && + git hook list -z test-hook >actual.raw && + nul_to_q actual && + test_cmp expect actual +' + test_expect_success 'git hook run: nonexistent hook' ' cat >stderr.expect <<-\EOF && error: cannot find a hook named test-hook From 005f3fbe07a20dd5f7dea57f6f46cd797387e56a Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 2 Mar 2026 21:17:04 +0200 Subject: [PATCH 21/31] builtin/receive-pack: avoid spinning no-op sideband async threads Exit early if the hooks do not exist, to avoid spinning up/down sideband async threads which no-op. It is important to call the hook_exists() API provided by hook.[ch] because it covers both config-defined hooks and the "traditional" hooks from the hookdir. find_hook() only covers the hookdir hooks. The regression happened because the no-op async threads add some additional overhead which can be measured with the receive-refs test of the benchmarks suite [1]. Reproduced using: cd benchmarks/receive-refs && \ ./run --revisions /path/to/git \ fc148b146ad41be71a7852c4867f0773cbfe1ff9~,fc148b146ad41be71a7852c4867f0773cbfe1ff9 \ --parameter-list refformat reftable --parameter-list refcount 10000 1: https://gitlab.com/gitlab-org/data-access/git/benchmarks Fixes: fc148b146ad4 ("receive-pack: convert update hooks to new API") Reported-by: Patrick Steinhardt Helped-by: Jeff King Signed-off-by: Adrian Ratiu [jc: avoid duplicated hardcoded hook names] Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index b5379a4895..e53aaf5104 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -914,6 +914,9 @@ static int run_receive_hook(struct command *commands, int saved_stderr = -1; int ret; + if (!hook_exists(the_repository, hook_name)) + return 0; + /* 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; @@ -955,12 +958,16 @@ static int run_receive_hook(struct command *commands, static int run_update_hook(struct command *cmd) { + static const char hook_name[] = "update"; struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; struct async sideband_async; int sideband_async_started = 0; int saved_stderr = -1; int code; + if (!hook_exists(the_repository, hook_name)) + return 0; + strvec_pushl(&opt.args, cmd->ref_name, oid_to_hex(&cmd->old_oid), @@ -969,7 +976,7 @@ static int run_update_hook(struct command *cmd) prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); - code = run_hooks_opt(the_repository, "update", &opt); + code = run_hooks_opt(the_repository, hook_name, &opt); finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started); @@ -1649,12 +1656,16 @@ out: static void run_update_post_hook(struct command *commands) { + static const char hook_name[] = "post-update"; struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; struct async sideband_async; struct command *cmd; int sideband_async_started = 0; int saved_stderr = -1; + if (!hook_exists(the_repository, hook_name)) + return; + for (cmd = commands; cmd; cmd = cmd->next) { if (cmd->error_string || cmd->did_not_exist) continue; @@ -1665,7 +1676,7 @@ static void run_update_post_hook(struct command *commands) prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); - run_hooks_opt(the_repository, "post-update", &opt); + run_hooks_opt(the_repository, hook_name, &opt); finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started); } From fd9e8f102f1342814307a437d936d4e7dc3b005a Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:07 +0200 Subject: [PATCH 22/31] hook: move unsorted_string_list_remove() to string-list.[ch] Move the convenience wrapper from hook to string-list since it's a more suitable place. Add a doc comment to the header. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 8 -------- string-list.c | 9 +++++++++ string-list.h | 8 ++++++++ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/hook.c b/hook.c index 2c8252b2c4..313a6b9937 100644 --- a/hook.c +++ b/hook.c @@ -110,14 +110,6 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, string_list_append(hook_list, hook_path)->util = h; } -static void unsorted_string_list_remove(struct string_list *list, - const char *str) -{ - struct string_list_item *item = unsorted_string_list_lookup(list, str); - if (item) - unsorted_string_list_delete_item(list, item - list->items, 0); -} - /* * Callback struct to collect all hook.* keys in a single config pass. * commands: friendly-name to command map. diff --git a/string-list.c b/string-list.c index fffa2ad4b6..d260b873c8 100644 --- a/string-list.c +++ b/string-list.c @@ -281,6 +281,15 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_ list->nr--; } +void unsorted_string_list_remove(struct string_list *list, const char *str, + int free_util) +{ + struct string_list_item *item = unsorted_string_list_lookup(list, str); + if (item) + unsorted_string_list_delete_item(list, item - list->items, + free_util); +} + /* * append a substring [p..end] to list; return number of things it * appended to the list. diff --git a/string-list.h b/string-list.h index 3ad862a187..b86ee7c099 100644 --- a/string-list.h +++ b/string-list.h @@ -265,6 +265,14 @@ struct string_list_item *unsorted_string_list_lookup(struct string_list *list, */ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util); +/** + * Remove the first item matching `str` from an unsorted string_list. + * No-op if `str` is not found. If `free_util` is non-zero, the `util` + * pointer of the removed item is freed before deletion. + */ +void unsorted_string_list_remove(struct string_list *list, const char *str, + int free_util); + /** * Split string into substrings on characters in `delim` and append the * substrings to `list`. The input string is not modified. From 3f80ac69cfd83bf787fb7565379336ffadf154b4 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:08 +0200 Subject: [PATCH 23/31] hook: fix minor style issues Fix some minor style nits pointed by Patrick and Junio: * Use CALLOC_ARRAY instead of xcalloc. * Init struct members during declaration. * Simplify if condition boolean logic. * Missing curly braces in if/else stmts. * Unnecessary header includes. * Capitalization in error/warn messages. * Comment spelling: free'd -> freed. These contain no logic changes, the code behaves the same as before. Suggested-by: Junio C Hamano Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 6 ++---- builtin/receive-pack.c | 11 +++++++---- hook.c | 25 +++++++++++++------------ refs.c | 3 ++- t/t1800-hook.sh | 2 +- transport.c | 3 ++- 6 files changed, 27 insertions(+), 23 deletions(-) diff --git a/builtin/hook.c b/builtin/hook.c index 83020dfb4f..c622a7399c 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -5,8 +5,6 @@ #include "gettext.h" #include "hook.h" #include "parse-options.h" -#include "strvec.h" -#include "abspath.h" #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") @@ -51,7 +49,7 @@ static int list(int argc, const char **argv, const char *prefix, * arguments later they probably should be caught by parse_options. */ if (argc != 1) - usage_msg_opt(_("You must specify a hook event name to list."), + usage_msg_opt(_("you must specify a hook event name to list."), builtin_hook_list_usage, list_options); hookname = argv[0]; @@ -59,7 +57,7 @@ static int list(int argc, const char **argv, const char *prefix, head = list_hooks(repo, hookname, NULL); if (!head->nr) { - warning(_("No hooks found for event '%s'"), hookname); + warning(_("no hooks found for event '%s'"), hookname); ret = 1; /* no hooks found */ goto cleanup; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 415bb57362..6066cbae8e 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -904,7 +904,8 @@ static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_ static void *receive_hook_feed_state_alloc(void *feed_pipe_ctx) { struct receive_hook_feed_state *init_state = feed_pipe_ctx; - struct receive_hook_feed_state *data = xcalloc(1, sizeof(*data)); + struct receive_hook_feed_state *data; + CALLOC_ARRAY(data, 1); data->report = init_state->report; data->cmd = init_state->cmd; data->skip_broken = init_state->skip_broken; @@ -928,7 +929,11 @@ static int run_receive_hook(struct command *commands, { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; struct command *iter = commands; - struct receive_hook_feed_state feed_init_state = { 0 }; + struct receive_hook_feed_state feed_init_state = { + .cmd = commands, + .skip_broken = skip_broken, + .buf = STRBUF_INIT, + }; struct async sideband_async; int sideband_async_started = 0; int saved_stderr = -1; @@ -961,8 +966,6 @@ static int run_receive_hook(struct command *commands, prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); /* set up stdin callback */ - feed_init_state.cmd = commands; - feed_init_state.skip_broken = skip_broken; opt.feed_pipe_ctx = &feed_init_state; opt.feed_pipe = feed_receive_hook_cb; opt.feed_pipe_cb_data_alloc = receive_hook_feed_state_alloc; diff --git a/hook.c b/hook.c index 313a6b9937..c5a6788dd5 100644 --- a/hook.c +++ b/hook.c @@ -57,9 +57,9 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) if (!h) return; - if (h->kind == HOOK_TRADITIONAL) + if (h->kind == HOOK_TRADITIONAL) { free((void *)h->u.traditional.path); - else if (h->kind == HOOK_CONFIGURED) { + } else if (h->kind == HOOK_CONFIGURED) { free((void *)h->u.configured.friendly_name); free((void *)h->u.configured.command); } @@ -91,7 +91,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, if (!hook_path) return; - h = xcalloc(1, sizeof(struct hook)); + CALLOC_ARRAY(h, 1); /* * If the hook is to run in a specific dir, a relative path can @@ -154,7 +154,7 @@ static int hook_config_lookup_all(const char *key, const char *value, strmap_get(&data->event_hooks, value); if (!hooks) { - hooks = xcalloc(1, sizeof(*hooks)); + CALLOC_ARRAY(hooks, 1); string_list_init_dup(hooks); strmap_put(&data->event_hooks, value, hooks); } @@ -227,7 +227,8 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) /* Construct the cache from parsed configs. */ strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { struct string_list *hook_names = e->value; - struct string_list *hooks = xcalloc(1, sizeof(*hooks)); + struct string_list *hooks; + CALLOC_ARRAY(hooks, 1); string_list_init_dup(hooks); @@ -281,7 +282,7 @@ static struct strmap *get_hook_config_cache(struct repository *r) * it just once on the first call. */ if (!r->hook_config_cache) { - r->hook_config_cache = xcalloc(1, sizeof(*cache)); + CALLOC_ARRAY(r->hook_config_cache, 1); strmap_init(r->hook_config_cache); build_hook_config_map(r, r->hook_config_cache); } @@ -289,9 +290,9 @@ static struct strmap *get_hook_config_cache(struct repository *r) } else { /* * Out-of-repo calls (no gitdir) allocate and return a temporary - * map cache which gets free'd immediately by the caller. + * cache which gets freed immediately by the caller. */ - cache = xcalloc(1, sizeof(*cache)); + CALLOC_ARRAY(cache, 1); strmap_init(cache); build_hook_config_map(r, cache); } @@ -311,7 +312,8 @@ static void list_hooks_add_configured(struct repository *r, for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { const char *friendly_name = configured_hooks->items[i].string; const char *command = configured_hooks->items[i].util; - struct hook *hook = xcalloc(1, sizeof(struct hook)); + struct hook *hook; + CALLOC_ARRAY(hook, 1); if (options && options->feed_pipe_cb_data_alloc) hook->feed_pipe_cb_data = @@ -343,7 +345,7 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, if (!hookname) BUG("null hookname was provided to hook_list()!"); - hook_head = xmalloc(sizeof(struct string_list)); + CALLOC_ARRAY(hook_head, 1); string_list_init_dup(hook_head); /* Add hooks from the config, e.g. hook.myhook.event = pre-commit */ @@ -493,8 +495,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name, * Ensure cb_data copy and free functions are either provided together, * or neither one is provided. */ - if ((options->feed_pipe_cb_data_alloc && !options->feed_pipe_cb_data_free) || - (!options->feed_pipe_cb_data_alloc && options->feed_pipe_cb_data_free)) + if (!options->feed_pipe_cb_data_alloc != !options->feed_pipe_cb_data_free) BUG("feed_pipe_cb_data_alloc and feed_pipe_cb_data_free must be set together"); if (options->invoked_hook) diff --git a/refs.c b/refs.c index a3363518e8..bd91c5c882 100644 --- a/refs.c +++ b/refs.c @@ -2599,7 +2599,8 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_ static void *transaction_feed_cb_data_alloc(void *feed_pipe_ctx UNUSED) { - struct transaction_feed_cb_data *data = xmalloc(sizeof(*data)); + struct transaction_feed_cb_data *data; + CALLOC_ARRAY(data, 1); strbuf_init(&data->buf, 0); data->index = 0; return data; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index b1583e9ef9..952bf97b86 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -34,7 +34,7 @@ test_expect_success 'git hook usage' ' test_expect_success 'git hook list: nonexistent hook' ' cat >stderr.expect <<-\EOF && - warning: No hooks found for event '\''test-hook'\'' + warning: no hooks found for event '\''test-hook'\'' EOF test_expect_code 1 git hook list test-hook 2>stderr.actual && test_cmp stderr.expect stderr.actual diff --git a/transport.c b/transport.c index 107f4fa5dc..56a4015389 100644 --- a/transport.c +++ b/transport.c @@ -1360,7 +1360,8 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void static void *pre_push_hook_data_alloc(void *feed_pipe_ctx) { - struct feed_pre_push_hook_data *data = xmalloc(sizeof(*data)); + struct feed_pre_push_hook_data *data; + CALLOC_ARRAY(data, 1); strbuf_init(&data->buf, 0); data->refs = (struct ref *)feed_pipe_ctx; return data; From 90f98a0d4770bb8311e87b694fc78818dd63d9d3 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:09 +0200 Subject: [PATCH 24/31] hook: rename cb_data_free/alloc -> hook_data_free/alloc Rename the hook callback function types to use the hook prefix. This is a style fix with no logic changes. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 4 ++-- hook.h | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hook.c b/hook.c index c5a6788dd5..bc1c45b16d 100644 --- a/hook.c +++ b/hook.c @@ -52,7 +52,7 @@ const char *find_hook(struct repository *r, const char *name) return path.buf; } -static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) +static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free) { if (!h) return; @@ -70,7 +70,7 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) free(h); } -void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free) +void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free) { struct string_list_item *item; diff --git a/hook.h b/hook.h index e949f5d488..e514c1b45b 100644 --- a/hook.h +++ b/hook.h @@ -43,8 +43,8 @@ struct hook { void *feed_pipe_cb_data; }; -typedef void (*cb_data_free_fn)(void *data); -typedef void *(*cb_data_alloc_fn)(void *init_ctx); +typedef void (*hook_data_free_fn)(void *data); +typedef void *(*hook_data_alloc_fn)(void *init_ctx); struct run_hooks_opt { @@ -132,14 +132,14 @@ struct run_hooks_opt * * The `feed_pipe_ctx` pointer can be used to pass initialization data. */ - cb_data_alloc_fn feed_pipe_cb_data_alloc; + hook_data_alloc_fn feed_pipe_cb_data_alloc; /** * Called to free the memory initialized by `feed_pipe_cb_data_alloc`. * * Must always be provided when `feed_pipe_cb_data_alloc` is provided. */ - cb_data_free_fn feed_pipe_cb_data_free; + hook_data_free_fn feed_pipe_cb_data_free; }; #define RUN_HOOKS_OPT_INIT { \ @@ -189,7 +189,7 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, * Frees the memory allocated for the hook list, including the `struct hook` * items and their internal state. */ -void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free); +void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free); /** * Frees the hook configuration cache stored in `struct repository`. From c694016e6116cd60905b3d79fd7a708c27ec6d8c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:10 +0200 Subject: [PATCH 25/31] hook: detect & emit two more bugs Trigger a bug when an unknown hook type is encountered while setting up hook execution. Also issue a bug if a configured hook is enabled without a cmd. Mostly useful for defensive coding. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hook.c b/hook.c index bc1c45b16d..b8ed4d79e2 100644 --- a/hook.c +++ b/hook.c @@ -408,7 +408,11 @@ static int pick_next_hook(struct child_process *cp, } else if (h->kind == HOOK_CONFIGURED) { /* to enable oneliners, let config-specified hooks run in shell. */ cp->use_shell = true; + if (!h->u.configured.command) + BUG("non-disabled HOOK_CONFIGURED hook has no command"); strvec_push(&cp->args, h->u.configured.command); + } else { + BUG("unknown hook kind"); } if (!cp->args.nr) From 468e466a78f5aa514f401082094a2ca38db100b8 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:11 +0200 Subject: [PATCH 26/31] hook: replace hook_list_clear() -> string_list_clear_func() Replace the custom function with string_list_clear_func() which is a more common pattern for clearing a string_list. To be able to do this, rework hook_clear() into hook_free(), so it can be passed to string_list_clear_func(). A slight complication is the need to keep a copy of the internal cb data free() pointer, however I think it's worth it since the API becomes cleaner, e.g. no more calls with NULL function args like hook_list_clear(hooks, NULL). Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 2 +- hook.c | 50 +++++++++++++++++++++++++++++--------------------- hook.h | 20 ++++++++++++++------ 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/builtin/hook.c b/builtin/hook.c index c622a7399c..8fc647a4de 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -78,7 +78,7 @@ static int list(int argc, const char **argv, const char *prefix, } cleanup: - hook_list_clear(head, NULL); + string_list_clear_func(head, hook_free); free(head); return ret; } diff --git a/hook.c b/hook.c index b8ed4d79e2..f6bb1999ae 100644 --- a/hook.c +++ b/hook.c @@ -52,8 +52,14 @@ const char *find_hook(struct repository *r, const char *name) return path.buf; } -static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free) +/* + * Frees a struct hook stored as the util pointer of a string_list_item. + * Suitable for use as a string_list_clear_func_t callback. + */ +void hook_free(void *p, const char *str UNUSED) { + struct hook *h = p; + if (!h) return; @@ -64,22 +70,12 @@ static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free) free((void *)h->u.configured.command); } - if (cb_data_free) - cb_data_free(h->feed_pipe_cb_data); + if (h->data_free && h->feed_pipe_cb_data) + h->data_free(h->feed_pipe_cb_data); free(h); } -void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free) -{ - struct string_list_item *item; - - for_each_string_list_item(item, hooks) - hook_clear(item->util, cb_data_free); - - string_list_clear(hooks, 0); -} - /* Helper to detect and add default "traditional" hooks from the hookdir. */ static void list_hooks_add_default(struct repository *r, const char *hookname, struct string_list *hook_list, @@ -100,9 +96,15 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, if (options && options->dir) hook_path = absolute_path(hook_path); - /* Setup per-hook internal state cb data */ - if (options && options->feed_pipe_cb_data_alloc) + /* + * Setup per-hook internal state callback data. + * When provided, the alloc/free callbacks are always provided + * together, so use them to alloc/free the internal hook state. + */ + if (options && options->feed_pipe_cb_data_alloc) { h->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx); + h->data_free = options->feed_pipe_cb_data_free; + } h->kind = HOOK_TRADITIONAL; h->u.traditional.path = xstrdup(hook_path); @@ -148,7 +150,7 @@ static int hook_config_lookup_all(const char *key, const char *value, struct strmap_entry *e; strmap_for_each_entry(&data->event_hooks, &iter, e) - unsorted_string_list_remove(e->value, hook_name); + unsorted_string_list_remove(e->value, hook_name, 0); } else { struct string_list *hooks = strmap_get(&data->event_hooks, value); @@ -160,7 +162,7 @@ static int hook_config_lookup_all(const char *key, const char *value, } /* Re-insert if necessary to preserve last-seen order. */ - unsorted_string_list_remove(hooks, hook_name); + unsorted_string_list_remove(hooks, hook_name, 0); string_list_append(hooks, hook_name); } } else if (!strcmp(subkey, "command")) { @@ -178,7 +180,7 @@ static int hook_config_lookup_all(const char *key, const char *value, break; case 1: /* enabled: undo a prior disabled entry */ unsorted_string_list_remove(&data->disabled_hooks, - hook_name); + hook_name, 0); break; default: break; /* ignore unrecognised values */ @@ -315,10 +317,16 @@ static void list_hooks_add_configured(struct repository *r, struct hook *hook; CALLOC_ARRAY(hook, 1); - if (options && options->feed_pipe_cb_data_alloc) + /* + * When provided, the alloc/free callbacks are always provided + * together, so use them to alloc/free the internal hook state. + */ + if (options && options->feed_pipe_cb_data_alloc) { hook->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc( options->feed_pipe_ctx); + hook->data_free = options->feed_pipe_cb_data_free; + } hook->kind = HOOK_CONFIGURED; hook->u.configured.friendly_name = xstrdup(friendly_name); @@ -361,7 +369,7 @@ int hook_exists(struct repository *r, const char *name) { struct string_list *hooks = list_hooks(r, name, NULL); int exists = hooks->nr > 0; - hook_list_clear(hooks, NULL); + string_list_clear_func(hooks, hook_free); free(hooks); return exists; } @@ -515,7 +523,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name, run_processes_parallel(&opts); ret = cb_data.rc; cleanup: - hook_list_clear(cb_data.hook_command_list, options->feed_pipe_cb_data_free); + string_list_clear_func(cb_data.hook_command_list, hook_free); free(cb_data.hook_command_list); run_hooks_opt_clear(options); return ret; diff --git a/hook.h b/hook.h index e514c1b45b..168c6495a4 100644 --- a/hook.h +++ b/hook.h @@ -7,6 +7,9 @@ struct repository; +typedef void (*hook_data_free_fn)(void *data); +typedef void *(*hook_data_alloc_fn)(void *init_ctx); + /** * Represents a hook command to be run. * Hooks can be: @@ -41,10 +44,15 @@ struct hook { * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. */ void *feed_pipe_cb_data; -}; -typedef void (*hook_data_free_fn)(void *data); -typedef void *(*hook_data_alloc_fn)(void *init_ctx); + /** + * Callback to free `feed_pipe_cb_data`. + * + * It is called automatically and points to the `feed_pipe_cb_data_free` + * provided via the `run_hook_opt` parameter. + */ + hook_data_free_fn data_free; +}; struct run_hooks_opt { @@ -186,10 +194,10 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, struct run_hooks_opt *options); /** - * Frees the memory allocated for the hook list, including the `struct hook` - * items and their internal state. + * Frees a struct hook stored as the util pointer of a string_list_item. + * Suitable for use as a string_list_clear_func_t callback. */ -void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free); +void hook_free(void *p, const char *str UNUSED); /** * Frees the hook configuration cache stored in `struct repository`. From ee360a026801d7cd5b18015cb2a9687100c196c7 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:12 +0200 Subject: [PATCH 27/31] hook: make consistent use of friendly-name in docs Both `name` and `friendly-name` is being used. Standardize on `friendly-name` for consistency since name is rather generic, even when used in the hooks namespace. Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 30 +++++++++++++++--------------- Documentation/git-hook.adoc | 6 +++--- hook.c | 2 +- hook.h | 2 +- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 64e845a260..9e78f26439 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -1,23 +1,23 @@ -hook..command:: - The command to execute for `hook.`. `` is a unique - "friendly" name that identifies this hook. (The hook events that - trigger the command are configured with `hook..event`.) The - value can be an executable path or a shell oneliner. If more than - one value is specified for the same ``, only the last value - parsed is used. See linkgit:git-hook[1]. +hook..command:: + The command to execute for `hook.`. `` + is a unique name that identifies this hook. The hook events that + trigger the command are configured with `hook..event`. + The value can be an executable path or a shell oneliner. If more than + one value is specified for the same ``, only the last + value parsed is used. See linkgit:git-hook[1]. -hook..event:: - The hook events that trigger `hook.`. The value is the name - of a hook event, like "pre-commit" or "update". (See +hook..event:: + The hook events that trigger `hook.`. The value is the + name of a hook event, like "pre-commit" or "update". (See linkgit:githooks[5] for a complete list of hook events.) On the - specified event, the associated `hook..command` is executed. - This is a multi-valued key. To run `hook.` on multiple + specified event, the associated `hook..command` is executed. + This is a multi-valued key. To run `hook.` on multiple events, specify the key more than once. An empty value resets the list of events, clearing any previously defined events for - `hook.`. See linkgit:git-hook[1]. + `hook.`. See linkgit:git-hook[1]. -hook..enabled:: - Whether the hook `hook.` is enabled. Defaults to `true`. +hook..enabled:: + Whether the hook `hook.` is enabled. Defaults to `true`. Set to `false` to disable the hook without removing its configuration. This is particularly useful when a hook is defined in a system or global config file and needs to be disabled for a diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 12d2701b52..966388660a 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -44,7 +44,7 @@ event`), and then `~/bin/spellchecker` will have a chance to check your commit message (during the `commit-msg` hook event). Commands are run in the order Git encounters their associated -`hook..event` configs during the configuration parse (see +`hook..event` configs during the configuration parse (see linkgit:git-config[1]). Although multiple `hook.linter.event` configs can be added, only one `hook.linter.command` event is valid - Git uses "last-one-wins" to determine which command to run. @@ -76,10 +76,10 @@ first start `~/bin/linter --cpp20` and second start `~/bin/leak-detector`. It would evaluate the output of each when deciding whether to proceed with the commit. -For a full list of hook events which you can set your `hook..event` to, +For a full list of hook events which you can set your `hook..event` to, and how hooks are invoked during those events, see linkgit:githooks[5]. -Git will ignore any `hook..event` that specifies an event it doesn't +Git will ignore any `hook..event` that specifies an event it doesn't recognize. This is intended so that tools which wrap Git can use the hook infrastructure to run their own hooks; see "WRAPPERS" for more guidance. diff --git a/hook.c b/hook.c index f6bb1999ae..7f89ae9cc2 100644 --- a/hook.c +++ b/hook.c @@ -116,7 +116,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, * Callback struct to collect all hook.* keys in a single config pass. * commands: friendly-name to command map. * event_hooks: event-name to list of friendly-names map. - * disabled_hooks: set of friendly-names with hook.name.enabled = false. + * disabled_hooks: set of friendly-names with hook..enabled = false. */ struct hook_all_config_cb { struct strmap commands; diff --git a/hook.h b/hook.h index 168c6495a4..49b40d949b 100644 --- a/hook.h +++ b/hook.h @@ -14,7 +14,7 @@ typedef void *(*hook_data_alloc_fn)(void *init_ctx); * Represents a hook command to be run. * Hooks can be: * 1. "traditional" (found in the hooks directory) - * 2. "configured" (defined in Git's configuration via hook..event). + * 2. "configured" (defined in Git's configuration via hook..event). * The 'kind' field determines which part of the union 'u' is valid. */ struct hook { From 5c887b271287107bac1f841b590ba137a268c74c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:13 +0200 Subject: [PATCH 28/31] t1800: add test to verify hook execution ordering There is a documented expectation that configured hooks are run before the hook from the hookdir. Add a test for it. While at it, I noticed that `git hook list -h` runs twice in the `git hook usage` test, so remove one invocation. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- t/t1800-hook.sh | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 952bf97b86..7eee84fc39 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -25,7 +25,6 @@ test_expect_success 'git hook usage' ' test_expect_code 129 git hook && test_expect_code 129 git hook run && test_expect_code 129 git hook run -h && - test_expect_code 129 git hook list -h && test_expect_code 129 git hook run --unknown 2>err && test_expect_code 129 git hook list && test_expect_code 129 git hook list -h && @@ -381,6 +380,34 @@ test_expect_success 'globally disabled hook can be re-enabled locally' ' test_cmp expected actual ' +test_expect_success 'configured hooks run before hookdir hook' ' + setup_hookdir && + test_config hook.first.event "pre-commit" && + test_config hook.first.command "echo first" && + test_config hook.second.event "pre-commit" && + test_config hook.second.command "echo second" && + + cat >expected <<-\EOF && + first + second + hook from hookdir + EOF + + git hook list pre-commit >actual && + test_cmp expected actual && + + # "Legacy Hook" is the output of the hookdir pre-commit script + # written by setup_hookdir() above. + cat >expected <<-\EOF && + first + second + "Legacy Hook" + EOF + + git hook run pre-commit 2>actual && + test_cmp expected actual +' + test_expect_success 'git hook run a hook with a bad shebang' ' test_when_finished "rm -rf bad-hooks" && mkdir bad-hooks && From 2a99233083a1f03e25d457214d3897b9a9d44363 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:14 +0200 Subject: [PATCH 29/31] hook: refactor hook_config_cache from strmap to named struct Replace the raw `struct strmap *hook_config_cache` in `struct repository` with a `struct hook_config_cache` which wraps the strmap in a named field. Replace the bare `char *command` util pointer stored in each string_list item with a heap-allocated `struct hook_config_cache_entry` that carries that command string. This is just a refactoring with no behavior changes, to give the cache struct room to grow so it can carry the additional hook metadata we'll be adding in the following commits. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 61 +++++++++++++++++++++++++++++++++------------------- hook.h | 10 ++++++++- repository.h | 3 ++- 3 files changed, 50 insertions(+), 24 deletions(-) diff --git a/hook.c b/hook.c index 7f89ae9cc2..4fe50aa38c 100644 --- a/hook.c +++ b/hook.c @@ -112,6 +112,15 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, string_list_append(hook_list, hook_path)->util = h; } +/* + * Cache entry stored as the .util pointer of string_list items inside the + * hook config cache. For now carries only the command for the hook. Next + * commits will add more data. + */ +struct hook_config_cache_entry { + char *command; +}; + /* * Callback struct to collect all hook.* keys in a single config pass. * commands: friendly-name to command map. @@ -194,26 +203,32 @@ static int hook_config_lookup_all(const char *key, const char *value, /* * The hook config cache maps each hook event name to a string_list where * every item's string is the hook's friendly-name and its util pointer is - * the corresponding command string. Both strings are owned by the map. + * a hook_config_cache_entry. All strings are owned by the map. * * Disabled hooks and hooks missing a command are already filtered out at * parse time, so callers can iterate the list directly. */ -void hook_cache_clear(struct strmap *cache) +void hook_cache_clear(struct hook_config_cache *cache) { struct hashmap_iter iter; struct strmap_entry *e; - strmap_for_each_entry(cache, &iter, e) { + strmap_for_each_entry(&cache->hooks, &iter, e) { struct string_list *hooks = e->value; - string_list_clear(hooks, 1); /* free util (command) pointers */ + for (size_t i = 0; i < hooks->nr; i++) { + struct hook_config_cache_entry *entry = hooks->items[i].util; + free(entry->command); + free(entry); + } + string_list_clear(hooks, 0); free(hooks); } - strmap_clear(cache, 0); + strmap_clear(&cache->hooks, 0); } /* Populate `cache` with the complete hook configuration */ -static void build_hook_config_map(struct repository *r, struct strmap *cache) +static void build_hook_config_map(struct repository *r, + struct hook_config_cache *cache) { struct hook_all_config_cb cb_data; struct hashmap_iter iter; @@ -236,6 +251,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) for (size_t i = 0; i < hook_names->nr; i++) { const char *hname = hook_names->items[i].string; + struct hook_config_cache_entry *entry; char *command; /* filter out disabled hooks */ @@ -249,12 +265,13 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) "'hook.%s.event' must be removed;" " aborting."), hname, hname); - /* util stores the command; owned by the cache. */ - string_list_append(hooks, hname)->util = - xstrdup(command); + /* util stores a cache entry; owned by the cache. */ + CALLOC_ARRAY(entry, 1); + entry->command = xstrdup(command); + string_list_append(hooks, hname)->util = entry; } - strmap_put(cache, e->key, hooks); + strmap_put(&cache->hooks, e->key, hooks); } strmap_clear(&cb_data.commands, 1); @@ -267,25 +284,25 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) } /* - * Return the hook config map for `r`, populating it first if needed. + * Return the hook config cache for `r`, populating it first if needed. * * Out-of-repo calls (r->gitdir == NULL) allocate and return a temporary - * cache map; the caller is responsible for freeing it with + * cache; the caller is responsible for freeing it with * hook_cache_clear() + free(). */ -static struct strmap *get_hook_config_cache(struct repository *r) +static struct hook_config_cache *get_hook_config_cache(struct repository *r) { - struct strmap *cache = NULL; + struct hook_config_cache *cache = NULL; if (r && r->gitdir) { /* - * For in-repo calls, the map is stored in r->hook_config_cache, - * so repeated invocations don't parse the configs, so allocate + * For in-repo calls, the cache is stored in r->hook_config_cache, + * so repeated invocations don't parse the configs; allocate * it just once on the first call. */ if (!r->hook_config_cache) { CALLOC_ARRAY(r->hook_config_cache, 1); - strmap_init(r->hook_config_cache); + strmap_init(&r->hook_config_cache->hooks); build_hook_config_map(r, r->hook_config_cache); } cache = r->hook_config_cache; @@ -295,7 +312,7 @@ static struct strmap *get_hook_config_cache(struct repository *r) * cache which gets freed immediately by the caller. */ CALLOC_ARRAY(cache, 1); - strmap_init(cache); + strmap_init(&cache->hooks); build_hook_config_map(r, cache); } @@ -307,13 +324,13 @@ static void list_hooks_add_configured(struct repository *r, struct string_list *list, struct run_hooks_opt *options) { - struct strmap *cache = get_hook_config_cache(r); - struct string_list *configured_hooks = strmap_get(cache, hookname); + struct hook_config_cache *cache = get_hook_config_cache(r); + struct string_list *configured_hooks = strmap_get(&cache->hooks, hookname); /* Iterate through configured hooks and initialize internal states */ for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { const char *friendly_name = configured_hooks->items[i].string; - const char *command = configured_hooks->items[i].util; + struct hook_config_cache_entry *entry = configured_hooks->items[i].util; struct hook *hook; CALLOC_ARRAY(hook, 1); @@ -330,7 +347,7 @@ static void list_hooks_add_configured(struct repository *r, hook->kind = HOOK_CONFIGURED; hook->u.configured.friendly_name = xstrdup(friendly_name); - hook->u.configured.command = xstrdup(command); + hook->u.configured.command = xstrdup(entry->command); string_list_append(list, friendly_name)->util = hook; } diff --git a/hook.h b/hook.h index 49b40d949b..4d0c22f1dc 100644 --- a/hook.h +++ b/hook.h @@ -199,11 +199,19 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, */ void hook_free(void *p, const char *str UNUSED); +/** + * Persistent cache for hook configuration, stored on `struct repository`. + * Populated lazily on first hook use and freed by repo_clear(). + */ +struct hook_config_cache { + struct strmap hooks; /* maps event name -> string_list of hooks */ +}; + /** * Frees the hook configuration cache stored in `struct repository`. * Called by repo_clear(). */ -void hook_cache_clear(struct strmap *cache); +void hook_cache_clear(struct hook_config_cache *cache); /** * Returns the path to the hook file, or NULL if the hook is missing diff --git a/repository.h b/repository.h index 7830eb7d4b..8d6e0b3dd2 100644 --- a/repository.h +++ b/repository.h @@ -12,6 +12,7 @@ struct lock_file; struct pathspec; struct object_database; struct submodule_cache; +struct hook_config_cache; struct promisor_remote_config; struct remote_state; @@ -170,7 +171,7 @@ struct repository { * Lazily-populated cache mapping hook event names to configured hooks. * NULL until first hook use. */ - struct strmap *hook_config_cache; + struct hook_config_cache *hook_config_cache; /* Configurations related to promisor remotes. */ char *repository_format_partial_clone; From dcd7ab7d2ef092ef9350cdfcc57c9bf92d942c20 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:15 +0200 Subject: [PATCH 30/31] hook: show config scope in git hook list Users running "git hook list" can see which hooks are configured but have no way to tell at which config scope (local, global, system...) each hook was defined. Store the scope from ctx->kvi->scope in the single-pass config callback, then carry it through the cache to the hook structs, so we can expose it to users via the "git hook list --show-scope" flag, which mirrors the existing git config --show-scope convention. Without the flag the output is unchanged. Example usage: $ git hook list --show-scope pre-commit linter (global) no-leaks (local) hook from hookdir Traditional hooks from the hookdir are unaffected by --show-scope since the config scope concept does not apply to them. Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/git-hook.adoc | 9 +++++++-- builtin/hook.c | 14 ++++++++++++-- hook.c | 24 ++++++++++++++++++++---- hook.h | 2 ++ t/t1800-hook.sh | 19 +++++++++++++++++++ 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 966388660a..4d4e728327 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git hook' run [--ignore-missing] [--to-stdin=] [-- ] -'git hook' list [-z] +'git hook' list [-z] [--show-scope] DESCRIPTION ----------- @@ -113,7 +113,7 @@ Any positional arguments to the hook should be passed after a mandatory `--` (or `--end-of-options`, see linkgit:gitcli[7]). See linkgit:githooks[5] for arguments hooks might expect (if any). -list [-z]:: +list [-z] [--show-scope]:: Print a list of hooks which will be run on `` event. If no hooks are configured for that event, print a warning and return 1. Use `-z` to terminate output lines with NUL instead of newlines. @@ -134,6 +134,11 @@ OPTIONS -z:: Terminate "list" output lines with NUL instead of newlines. +--show-scope:: + For "list"; print the config scope (e.g. `local`, `global`, `system`) + in parentheses after the friendly name of each configured hook, to show + where it was defined. Traditional hooks from the hookdir are unaffected. + WRAPPERS -------- diff --git a/builtin/hook.c b/builtin/hook.c index 8fc647a4de..c806640361 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -9,7 +9,7 @@ #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") #define BUILTIN_HOOK_LIST_USAGE \ - N_("git hook list [-z] ") + N_("git hook list [-z] [--show-scope] ") static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, @@ -33,11 +33,14 @@ static int list(int argc, const char **argv, const char *prefix, struct string_list_item *item; const char *hookname = NULL; int line_terminator = '\n'; + int show_scope = 0; int ret = 0; struct option list_options[] = { OPT_SET_INT('z', NULL, &line_terminator, N_("use NUL as line terminator"), '\0'), + OPT_BOOL(0, "show-scope", &show_scope, + N_("show the config scope that defined each hook")), OPT_END(), }; @@ -70,7 +73,14 @@ static int list(int argc, const char **argv, const char *prefix, printf("%s%c", _("hook from hookdir"), line_terminator); break; case HOOK_CONFIGURED: - printf("%s%c", h->u.configured.friendly_name, line_terminator); + if (show_scope) + printf("%s (%s)%c", + h->u.configured.friendly_name, + config_scope_name(h->u.configured.scope), + line_terminator); + else + printf("%s%c", h->u.configured.friendly_name, + line_terminator); break; default: BUG("unknown hook kind"); diff --git a/hook.c b/hook.c index 4fe50aa38c..2c03baeaac 100644 --- a/hook.c +++ b/hook.c @@ -114,11 +114,11 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, /* * Cache entry stored as the .util pointer of string_list items inside the - * hook config cache. For now carries only the command for the hook. Next - * commits will add more data. + * hook config cache. */ struct hook_config_cache_entry { char *command; + enum config_scope scope; }; /* @@ -135,7 +135,7 @@ struct hook_all_config_cb { /* repo_config() callback that collects all hook.* configuration in one pass. */ static int hook_config_lookup_all(const char *key, const char *value, - const struct config_context *ctx UNUSED, + const struct config_context *ctx, void *cb_data) { struct hook_all_config_cb *data = cb_data; @@ -172,7 +172,19 @@ static int hook_config_lookup_all(const char *key, const char *value, /* Re-insert if necessary to preserve last-seen order. */ unsorted_string_list_remove(hooks, hook_name, 0); - string_list_append(hooks, hook_name); + + if (!ctx->kvi) + BUG("hook config callback called without key-value info"); + + /* + * Stash the config scope in the util pointer for + * later retrieval in build_hook_config_map(). This + * intermediate struct is transient and never leaves + * that function, so we pack the enum value into the + * pointer rather than heap-allocating a wrapper. + */ + string_list_append(hooks, hook_name)->util = + (void *)(uintptr_t)ctx->kvi->scope; } } else if (!strcmp(subkey, "command")) { /* Store command overwriting the old value */ @@ -251,6 +263,8 @@ static void build_hook_config_map(struct repository *r, for (size_t i = 0; i < hook_names->nr; i++) { const char *hname = hook_names->items[i].string; + enum config_scope scope = + (enum config_scope)(uintptr_t)hook_names->items[i].util; struct hook_config_cache_entry *entry; char *command; @@ -268,6 +282,7 @@ static void build_hook_config_map(struct repository *r, /* util stores a cache entry; owned by the cache. */ CALLOC_ARRAY(entry, 1); entry->command = xstrdup(command); + entry->scope = scope; string_list_append(hooks, hname)->util = entry; } @@ -348,6 +363,7 @@ static void list_hooks_add_configured(struct repository *r, hook->kind = HOOK_CONFIGURED; hook->u.configured.friendly_name = xstrdup(friendly_name); hook->u.configured.command = xstrdup(entry->command); + hook->u.configured.scope = entry->scope; string_list_append(list, friendly_name)->util = hook; } diff --git a/hook.h b/hook.h index 4d0c22f1dc..0d711ed21a 100644 --- a/hook.h +++ b/hook.h @@ -1,5 +1,6 @@ #ifndef HOOK_H #define HOOK_H +#include "config.h" #include "strvec.h" #include "run-command.h" #include "string-list.h" @@ -29,6 +30,7 @@ struct hook { struct { const char *friendly_name; const char *command; + enum config_scope scope; } configured; } u; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 7eee84fc39..aed07575e3 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -408,6 +408,25 @@ test_expect_success 'configured hooks run before hookdir hook' ' test_cmp expected actual ' +test_expect_success 'git hook list --show-scope shows config scope' ' + test_config_global hook.global-hook.command "echo global" && + test_config_global hook.global-hook.event test-hook --add && + test_config hook.local-hook.command "echo local" && + test_config hook.local-hook.event test-hook --add && + + cat >expected <<-\EOF && + global-hook (global) + local-hook (local) + EOF + git hook list --show-scope test-hook >actual && + test_cmp expected actual && + + # without --show-scope the scope must not appear + git hook list test-hook >actual && + test_grep ! "(global)" actual && + test_grep ! "(local)" actual +' + test_expect_success 'git hook run a hook with a bad shebang' ' test_when_finished "rm -rf bad-hooks" && mkdir bad-hooks && From 86a30747aeef05c03e3ab9da85157bdc815357b5 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:16 +0200 Subject: [PATCH 31/31] hook: show disabled hooks in "git hook list" Disabled hooks were filtered out of the cache entirely, making them invisible to "git hook list". Keep them in the cache with a new "disabled" flag which is propagated to the respective struct hook. "git hook list" now shows disabled hooks annotated with "(disabled)" in the config order. With --show-scope, it looks like: $ git hook list --show-scope pre-commit linter (global) no-leaks (local, disabled) hook from hookdir A disabled hook without a command issues a warning instead of the fatal "hook.X.command must be configured" error. We could also throw an error, however it seemd a bit excessive to me in this case. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 18 ++++++++++------- hook.c | 54 +++++++++++++++++++++++++++++++++---------------- hook.h | 1 + t/t1800-hook.sh | 33 +++++++++++++++++++++++++++--- 4 files changed, 79 insertions(+), 27 deletions(-) diff --git a/builtin/hook.c b/builtin/hook.c index c806640361..ff446948fa 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -72,16 +72,20 @@ static int list(int argc, const char **argv, const char *prefix, case HOOK_TRADITIONAL: printf("%s%c", _("hook from hookdir"), line_terminator); break; - case HOOK_CONFIGURED: - if (show_scope) - printf("%s (%s)%c", - h->u.configured.friendly_name, - config_scope_name(h->u.configured.scope), + case HOOK_CONFIGURED: { + const char *name = h->u.configured.friendly_name; + const char *scope = show_scope ? + config_scope_name(h->u.configured.scope) : NULL; + if (scope) + printf("%s (%s%s)%c", name, scope, + h->u.configured.disabled ? ", disabled" : "", line_terminator); + else if (h->u.configured.disabled) + printf("%s (disabled)%c", name, line_terminator); else - printf("%s%c", h->u.configured.friendly_name, - line_terminator); + printf("%s%c", name, line_terminator); break; + } default: BUG("unknown hook kind"); } diff --git a/hook.c b/hook.c index 2c03baeaac..4f4f060156 100644 --- a/hook.c +++ b/hook.c @@ -119,6 +119,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, struct hook_config_cache_entry { char *command; enum config_scope scope; + int disabled; }; /* @@ -217,8 +218,10 @@ static int hook_config_lookup_all(const char *key, const char *value, * every item's string is the hook's friendly-name and its util pointer is * a hook_config_cache_entry. All strings are owned by the map. * - * Disabled hooks and hooks missing a command are already filtered out at - * parse time, so callers can iterate the list directly. + * Disabled hooks are kept in the cache with entry->disabled set, so that + * "git hook list" can display them. Hooks missing a command are filtered + * out at build time; if a disabled hook has no command it is silently + * skipped rather than triggering a fatal error. */ void hook_cache_clear(struct hook_config_cache *cache) { @@ -268,21 +271,26 @@ static void build_hook_config_map(struct repository *r, struct hook_config_cache_entry *entry; char *command; - /* filter out disabled hooks */ - if (unsorted_string_list_lookup(&cb_data.disabled_hooks, - hname)) - continue; + int is_disabled = + !!unsorted_string_list_lookup( + &cb_data.disabled_hooks, hname); command = strmap_get(&cb_data.commands, hname); - if (!command) - die(_("'hook.%s.command' must be configured or " - "'hook.%s.event' must be removed;" - " aborting."), hname, hname); + if (!command) { + if (is_disabled) + warning(_("disabled hook '%s' has no " + "command configured"), hname); + else + die(_("'hook.%s.command' must be configured or " + "'hook.%s.event' must be removed;" + " aborting."), hname, hname); + } /* util stores a cache entry; owned by the cache. */ CALLOC_ARRAY(entry, 1); - entry->command = xstrdup(command); + entry->command = command ? xstrdup(command) : NULL; entry->scope = scope; + entry->disabled = is_disabled; string_list_append(hooks, hname)->util = entry; } @@ -362,8 +370,10 @@ static void list_hooks_add_configured(struct repository *r, hook->kind = HOOK_CONFIGURED; hook->u.configured.friendly_name = xstrdup(friendly_name); - hook->u.configured.command = xstrdup(entry->command); + hook->u.configured.command = + entry->command ? xstrdup(entry->command) : NULL; hook->u.configured.scope = entry->scope; + hook->u.configured.disabled = entry->disabled; string_list_append(list, friendly_name)->util = hook; } @@ -401,7 +411,16 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, int hook_exists(struct repository *r, const char *name) { struct string_list *hooks = list_hooks(r, name, NULL); - int exists = hooks->nr > 0; + int exists = 0; + + for (size_t i = 0; i < hooks->nr; i++) { + struct hook *h = hooks->items[i].util; + if (h->kind == HOOK_TRADITIONAL || + !h->u.configured.disabled) { + exists = 1; + break; + } + } string_list_clear_func(hooks, hook_free); free(hooks); return exists; @@ -416,10 +435,11 @@ static int pick_next_hook(struct child_process *cp, struct string_list *hook_list = hook_cb->hook_command_list; struct hook *h; - if (hook_cb->hook_to_run_index >= hook_list->nr) - return 0; - - h = hook_list->items[hook_cb->hook_to_run_index++].util; + do { + if (hook_cb->hook_to_run_index >= hook_list->nr) + return 0; + h = hook_list->items[hook_cb->hook_to_run_index++].util; + } while (h->kind == HOOK_CONFIGURED && h->u.configured.disabled); cp->no_stdin = 1; strvec_pushv(&cp->env, hook_cb->options->env.v); diff --git a/hook.h b/hook.h index 0d711ed21a..0432df963f 100644 --- a/hook.h +++ b/hook.h @@ -31,6 +31,7 @@ struct hook { const char *friendly_name; const char *command; enum config_scope scope; + int disabled; } configured; } u; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index aed07575e3..cbf7d2bf80 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -357,7 +357,15 @@ test_expect_success 'disabled hook is not run' ' test_must_be_empty actual ' -test_expect_success 'disabled hook does not appear in git hook list' ' +test_expect_success 'disabled hook with no command warns' ' + test_config hook.nocommand.event "pre-commit" && + test_config hook.nocommand.enabled false && + + git hook list pre-commit 2>actual && + test_grep "disabled hook.*nocommand.*no command configured" actual +' + +test_expect_success 'disabled hook appears as disabled in git hook list' ' test_config hook.active.event "pre-commit" && test_config hook.active.command "echo active" && test_config hook.inactive.event "pre-commit" && @@ -365,8 +373,27 @@ test_expect_success 'disabled hook does not appear in git hook list' ' test_config hook.inactive.enabled false && git hook list pre-commit >actual && - test_grep "active" actual && - test_grep ! "inactive" actual + test_grep "^active$" actual && + test_grep "^inactive (disabled)$" actual +' + +test_expect_success 'disabled hook shows scope with --show-scope' ' + test_config hook.myhook.event "pre-commit" && + test_config hook.myhook.command "echo hi" && + test_config hook.myhook.enabled false && + + git hook list --show-scope pre-commit >actual && + test_grep "myhook (local, disabled)" actual +' + +test_expect_success 'disabled configured hook is not reported as existing by hook_exists' ' + test_when_finished "rm -f git-bugreport-hook-exists-test.txt" && + test_config hook.linter.event "pre-commit" && + test_config hook.linter.command "echo lint" && + test_config hook.linter.enabled false && + + git bugreport -s hook-exists-test && + test_grep ! "pre-commit" git-bugreport-hook-exists-test.txt ' test_expect_success 'globally disabled hook can be re-enabled locally' '