| Message ID | 20210715232603.3415111-3-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config-based hooks restarted | expand |
On Thu, Jul 15 2021, Emily Shaffer wrote: > In many cases, there's no reason not to allow hooks to execute in > parallel. run_processes_parallel() is well-suited - it's a task queue > that runs its housekeeping in series, which means users don't > need to worry about thread safety on their callback data. True > multithreaded execution with the async_* functions isn't necessary here. > Synchronous hook execution can be achieved by only allowing 1 job to run > at a time. > > Teach run_hooks() to use that function for simple hooks which don't > require stdin or capture of stderr. This doesn't mention... > int ret; > - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; > + struct run_hooks_opt opt; > > + run_hooks_opt_init_sync(&opt); ...why we need to bring the s/macro/func/ init pattern, back, but looking ahead... > +int configured_hook_jobs(void)a > +{ > + int n = online_cpus(); > + git_config_get_int("hook.jobs", &n); > + > + return n; > +} > + > int hook_exists(const char *name) > { > return !!find_hook(name); > @@ -117,6 +125,26 @@ struct list_head* hook_list(const char* hookname) > return hook_head; > } > > +void run_hooks_opt_init_sync(struct run_hooks_opt *o) > +{ > + strvec_init(&o->env); > + strvec_init(&o->args); > + o->path_to_stdin = NULL; > + o->jobs = 1; > + o->dir = NULL; > + o->feed_pipe = NULL; > + o->feed_pipe_ctx = NULL; > + o->consume_sideband = NULL; > + o->invoked_hook = NULL; > + o->absolute_path = 0; > +} > + > +void run_hooks_opt_init_async(struct run_hooks_opt *o) > +{ > + run_hooks_opt_init_sync(o); > + o->jobs = configured_hook_jobs(); > +} ...okey, so it's because you brought back the "call jobs function" in one of the init functions. I had a comment in a previous round, I found https://lore.kernel.org/git/87lf7bzbrk.fsf@evledraar.gmail.com/, but I think there was a later one where I commented on the "jobs" field specifically. Anyway, it seems much easier to me to just keep the simpler macro init and then: > - if (options->jobs != 1) > - BUG("we do not handle %d or any other != 1 job number yet", options->jobs); > - > run_processes_parallel_tr2(options->jobs, > pick_next_hook, > notify_start_failure, There's this one place where we use the "jobs" parameter, just do something like this there: int configured_hook_jobs(void) { static int jobs; if (!jobs) return jobs; if (git_config_get_int("hook.jobs", &jobs)) jobs = online_cpus(); return jobs; } I.e. you also needlessly call online_cpus() when we're about to override it in the config. The git_config_get_int()'s return value indicates whether you need to do that. Then just: int jobs = options->jobs ? options->jobs : configured_hook_jobs(); run_processes_parallel_tr2(jobs, [...]); Or some such, i.e. we can defer getting the job number away from startup to when we actually need to start those jobs, and your whole use of a function init pattern came down to doing that really early. As an aside if you /do/ need to do init-via-function my 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01) in "next" shows a much nicer way to do that. I.e. you'd just do: void run_hooks_opt_init_sync(struct run_hooks_opt *o) { struct run_hooks_opt blank = RUN_HOOKS_OPT_INIT; memcpy(o, &blank, sizeof(*o)); } void run_hooks_opt_init_async(struct run_hooks_opt *o) { run_hooks_opt_init_sync(o); o->jobs = configured_hook_jobs(); } In some cases we do actually need to do init via functions, but can init a large option via the macro, which IMO is nicer to read, but here I think we don't need the functions at all per the above.
On Fri, Jul 16, 2021 at 10:36:10AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Jul 15 2021, Emily Shaffer wrote: > > > In many cases, there's no reason not to allow hooks to execute in > > parallel. run_processes_parallel() is well-suited - it's a task queue > > that runs its housekeeping in series, which means users don't > > need to worry about thread safety on their callback data. True > > multithreaded execution with the async_* functions isn't necessary here. > > Synchronous hook execution can be achieved by only allowing 1 job to run > > at a time. > > > > Teach run_hooks() to use that function for simple hooks which don't > > require stdin or capture of stderr. > > This doesn't mention... > > > int ret; > > - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; > > + struct run_hooks_opt opt; > > > > + run_hooks_opt_init_sync(&opt); > > > ...why we need to bring the s/macro/func/ init pattern, back, but looking ahead... > > > +int configured_hook_jobs(void)a > > +{ > > + int n = online_cpus(); > > + git_config_get_int("hook.jobs", &n); > > + > > + return n; > > +} > > + > > int hook_exists(const char *name) > > { > > return !!find_hook(name); > > @@ -117,6 +125,26 @@ struct list_head* hook_list(const char* hookname) > > return hook_head; > > } > > > > +void run_hooks_opt_init_sync(struct run_hooks_opt *o) > > +{ > > + strvec_init(&o->env); > > + strvec_init(&o->args); > > + o->path_to_stdin = NULL; > > + o->jobs = 1; > > + o->dir = NULL; > > + o->feed_pipe = NULL; > > + o->feed_pipe_ctx = NULL; > > + o->consume_sideband = NULL; > > + o->invoked_hook = NULL; > > + o->absolute_path = 0; > > +} > > + > > +void run_hooks_opt_init_async(struct run_hooks_opt *o) > > +{ > > + run_hooks_opt_init_sync(o); > > + o->jobs = configured_hook_jobs(); > > +} > > ...okey, so it's because you brought back the "call jobs function" in > one of the init functions. > > I had a comment in a previous round, I found > https://lore.kernel.org/git/87lf7bzbrk.fsf@evledraar.gmail.com/, but I > think there was a later one where I commented on the "jobs" field > specifically. > > Anyway, it seems much easier to me to just keep the simpler macro init > and then: > > > - if (options->jobs != 1) > > - BUG("we do not handle %d or any other != 1 job number yet", options->jobs); > > - > > run_processes_parallel_tr2(options->jobs, > > pick_next_hook, > > notify_start_failure, > > There's this one place where we use the "jobs" parameter, just do > something like this there: > > int configured_hook_jobs(void) > { > static int jobs; > if (!jobs) > return jobs; > if (git_config_get_int("hook.jobs", &jobs)) > jobs = online_cpus(); > return jobs; > } > > I.e. you also needlessly call online_cpus() when we're about to override > it in the config. The git_config_get_int()'s return value indicates > whether you need to do that. Then just: > > int jobs = options->jobs ? options->jobs : configured_hook_jobs(); > run_processes_parallel_tr2(jobs, [...]); Ahh, and then let RUN_HOOKS_OPT_INIT_ASYNC set jobs to 0 ("go look it up"). Yeah, that makes sense. Shout if somehow you meant to leave just one initializer macro; otherwise, I'll do it this way - with RUN_HOOKS_OPT_INIT_ASYNC and RUN_HOOKS_OPT_INIT_SYNC. I think it's valuable for hook callers to make it very plain at the callsite whether they're parallelizable or not, and I think struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; opt.jobs = 0; doesn't make that as obvious. > > Or some such, i.e. we can defer getting the job number away from startup > to when we actually need to start those jobs, and your whole use of a > function init pattern came down to doing that really early. > > As an aside if you /do/ need to do init-via-function my 5726a6b4012 (*.c > *_init(): define in terms of corresponding *_INIT macro, 2021-07-01) in > "next" shows a much nicer way to do that. I.e. you'd just do: > > void run_hooks_opt_init_sync(struct run_hooks_opt *o) > { > struct run_hooks_opt blank = RUN_HOOKS_OPT_INIT; > memcpy(o, &blank, sizeof(*o)); > } > > void run_hooks_opt_init_async(struct run_hooks_opt *o) > { > run_hooks_opt_init_sync(o); > o->jobs = configured_hook_jobs(); > } > > In some cases we do actually need to do init via functions, but can init > a large option via the macro, which IMO is nicer to read, but here I > think we don't need the functions at all per the above.
On Thu, Jul 22 2021, Emily Shaffer wrote: > On Fri, Jul 16, 2021 at 10:36:10AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> >> On Thu, Jul 15 2021, Emily Shaffer wrote: >> >> > In many cases, there's no reason not to allow hooks to execute in >> > parallel. run_processes_parallel() is well-suited - it's a task queue >> > that runs its housekeeping in series, which means users don't >> > need to worry about thread safety on their callback data. True >> > multithreaded execution with the async_* functions isn't necessary here. >> > Synchronous hook execution can be achieved by only allowing 1 job to run >> > at a time. >> > >> > Teach run_hooks() to use that function for simple hooks which don't >> > require stdin or capture of stderr. >> >> This doesn't mention... >> >> > int ret; >> > - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> > + struct run_hooks_opt opt; >> > >> > + run_hooks_opt_init_sync(&opt); >> >> >> ...why we need to bring the s/macro/func/ init pattern, back, but looking ahead... >> >> > +int configured_hook_jobs(void)a >> > +{ >> > + int n = online_cpus(); >> > + git_config_get_int("hook.jobs", &n); >> > + >> > + return n; >> > +} >> > + >> > int hook_exists(const char *name) >> > { >> > return !!find_hook(name); >> > @@ -117,6 +125,26 @@ struct list_head* hook_list(const char* hookname) >> > return hook_head; >> > } >> > >> > +void run_hooks_opt_init_sync(struct run_hooks_opt *o) >> > +{ >> > + strvec_init(&o->env); >> > + strvec_init(&o->args); >> > + o->path_to_stdin = NULL; >> > + o->jobs = 1; >> > + o->dir = NULL; >> > + o->feed_pipe = NULL; >> > + o->feed_pipe_ctx = NULL; >> > + o->consume_sideband = NULL; >> > + o->invoked_hook = NULL; >> > + o->absolute_path = 0; >> > +} >> > + >> > +void run_hooks_opt_init_async(struct run_hooks_opt *o) >> > +{ >> > + run_hooks_opt_init_sync(o); >> > + o->jobs = configured_hook_jobs(); >> > +} >> >> ...okey, so it's because you brought back the "call jobs function" in >> one of the init functions. >> >> I had a comment in a previous round, I found >> https://lore.kernel.org/git/87lf7bzbrk.fsf@evledraar.gmail.com/, but I >> think there was a later one where I commented on the "jobs" field >> specifically. >> >> Anyway, it seems much easier to me to just keep the simpler macro init >> and then: >> >> > - if (options->jobs != 1) >> > - BUG("we do not handle %d or any other != 1 job number yet", options->jobs); >> > - >> > run_processes_parallel_tr2(options->jobs, >> > pick_next_hook, >> > notify_start_failure, >> >> There's this one place where we use the "jobs" parameter, just do >> something like this there: >> >> int configured_hook_jobs(void) >> { >> static int jobs; >> if (!jobs) >> return jobs; >> if (git_config_get_int("hook.jobs", &jobs)) >> jobs = online_cpus(); >> return jobs; >> } >> >> I.e. you also needlessly call online_cpus() when we're about to override >> it in the config. The git_config_get_int()'s return value indicates >> whether you need to do that. Then just: >> >> int jobs = options->jobs ? options->jobs : configured_hook_jobs(); >> run_processes_parallel_tr2(jobs, [...]); > > Ahh, and then let RUN_HOOKS_OPT_INIT_ASYNC set jobs to 0 ("go look it > up"). Yeah, that makes sense. > > Shout if somehow you meant to leave just one initializer macro; > otherwise, I'll do it this way - with RUN_HOOKS_OPT_INIT_ASYNC and > RUN_HOOKS_OPT_INIT_SYNC. I think it's valuable for hook callers to make > it very plain at the callsite whether they're parallelizable or not, and > I think > > struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; > opt.jobs = 0; > > doesn't make that as obvious. Yes agreed, sorry about the ambiguity, I meant we should have two init macros, just like e.g. STRING_LIST_INIT_NODUP and STRING_LIST_INIT_DUP.
diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt new file mode 100644 index 0000000000..96d3d6572c --- /dev/null +++ b/Documentation/config/hook.txt @@ -0,0 +1,4 @@ +hook.jobs:: + Specifies how many hooks can be run simultaneously during parallelized + hook execution. If unspecified, defaults to the number of processors on + the current system. diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index fa68c1f391..8bf82b5dd4 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -8,7 +8,8 @@ git-hook - run git hooks SYNOPSIS -------- [verse] -'git hook' run [--to-stdin=<path>] [--ignore-missing] <hook-name> [-- <hook-args>] +'git hook' run [--to-stdin=<path>] [--ignore-missing] [(-j|--jobs) <n>] + <hook-name> [-- <hook-args>] DESCRIPTION ----------- @@ -42,6 +43,20 @@ OPTIONS tools that want to do a blind one-shot run of a hook that may or may not be present. +-j:: +--jobs:: + Only valid for `run`. ++ +Specify how many hooks to run simultaneously. If this flag is not specified, use +the value of the `hook.jobs` config. If the config is not specified, use the +number of CPUs on the current system. Some hooks may be ineligible for +parallelization: for example, 'commit-msg' intends hooks modify the commit +message body and cannot be parallelized. + +CONFIGURATION +------------- +include::config/hook.txt[] + SEE ALSO -------- linkgit:githooks[5] diff --git a/builtin/am.c b/builtin/am.c index 6e4f9c8036..bdad38142a 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -445,8 +445,9 @@ static void am_destroy(const struct am_state *state) static int run_applypatch_msg_hook(struct am_state *state) { int ret; - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; + run_hooks_opt_init_sync(&opt); assert(state->msg); strvec_push(&opt.args, am_path(state, "final-commit")); ret = run_hooks("applypatch-msg", &opt); @@ -467,9 +468,10 @@ static int run_applypatch_msg_hook(struct am_state *state) */ static int run_post_rewrite_hook(const struct am_state *state) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; int ret; + run_hooks_opt_init_async(&opt); strvec_push(&opt.args, "rebase"); opt.path_to_stdin = am_path(state, "rewritten"); @@ -1602,9 +1604,10 @@ static void do_commit(const struct am_state *state) struct commit_list *parents = NULL; const char *reflog_msg, *author, *committer = NULL; struct strbuf sb = STRBUF_INIT; - struct run_hooks_opt hook_opt_pre = RUN_HOOKS_OPT_INIT; - struct run_hooks_opt hook_opt_post = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt hook_opt_pre; + struct run_hooks_opt hook_opt_post; + run_hooks_opt_init_async(&hook_opt_pre); if (run_hooks("pre-applypatch", &hook_opt_pre)) { run_hooks_opt_clear(&hook_opt_pre); exit(1); @@ -1659,6 +1662,7 @@ static void do_commit(const struct am_state *state) fclose(fp); } + run_hooks_opt_init_async(&hook_opt_post); run_hooks("post-applypatch", &hook_opt_post); run_hooks_opt_clear(&hook_opt_pre); diff --git a/builtin/checkout.c b/builtin/checkout.c index 6205ace09f..be4450a433 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -107,9 +107,10 @@ struct branch_info { static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit, int changed) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; int rc; + run_hooks_opt_init_sync(&opt); /* "new_commit" can be NULL when checking out from the index before a commit exists. */ strvec_pushl(&opt.args, diff --git a/builtin/clone.c b/builtin/clone.c index de57a3119b..87cfbf60e5 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -776,7 +776,7 @@ static int checkout(int submodule_progress) struct tree *tree; struct tree_desc t; int err = 0; - struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt hook_opt; if (option_no_checkout) return 0; @@ -822,6 +822,7 @@ static int checkout(int submodule_progress) if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); + run_hooks_opt_init_sync(&hook_opt); strvec_pushl(&hook_opt.args, oid_to_hex(null_oid()), oid_to_hex(&oid), "1", NULL); err |= run_hooks("post-checkout", &hook_opt); run_hooks_opt_clear(&hook_opt); diff --git a/builtin/gc.c b/builtin/gc.c index a12641a691..16890b097c 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -349,7 +349,7 @@ static void add_repack_incremental_option(void) static int need_to_gc(void) { - struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt hook_opt; /* * Setting gc.auto to 0 or negative can disable the @@ -397,6 +397,7 @@ static int need_to_gc(void) else return 0; + run_hooks_opt_init_async(&hook_opt); if (run_hooks("pre-auto-gc", &hook_opt)) { run_hooks_opt_clear(&hook_opt); return 0; diff --git a/builtin/hook.c b/builtin/hook.c index a41ff36da9..d196d8498c 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -21,7 +21,7 @@ static const char * const builtin_hook_run_usage[] = { static int run(int argc, const char **argv, const char *prefix) { int i; - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; int rc = 0; int ignore_missing = 0; const char *hook_name; @@ -32,9 +32,13 @@ static int run(int argc, const char **argv, const char *prefix) N_("exit quietly with a zero exit code if the requested hook cannot be found")), OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"), N_("file to read into hooks' stdin")), + OPT_INTEGER('j', "jobs", &opt.jobs, + N_("run up to <n> hooks simultaneously")), OPT_END(), }; + run_hooks_opt_init_async(&opt); + argc = parse_options(argc, argv, prefix, run_options, builtin_hook_run_usage, PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH); diff --git a/builtin/merge.c b/builtin/merge.c index 0425c9bf2b..67c2eba053 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -448,7 +448,7 @@ static void finish(struct commit *head_commit, const struct object_id *new_head, const char *msg) { struct strbuf reflog_message = STRBUF_INIT; - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; const struct object_id *head = &head_commit->object.oid; if (!msg) @@ -490,6 +490,7 @@ static void finish(struct commit *head_commit, } /* Run a post-merge hook */ + run_hooks_opt_init_async(&opt); strvec_push(&opt.args, squash ? "1" : "0"); run_hooks("post-merge", &opt); run_hooks_opt_clear(&opt); diff --git a/builtin/rebase.c b/builtin/rebase.c index 2081f6fa8d..fe9f144cad 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1314,7 +1314,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) char *squash_onto_name = NULL; int reschedule_failed_exec = -1; int allow_preemptive_ff = 1; - struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt hook_opt; struct option builtin_rebase_options[] = { OPT_STRING(0, "onto", &options.onto_name, N_("revision"), @@ -2024,6 +2024,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } /* If a hook exists, give it a chance to interrupt*/ + run_hooks_opt_init_async(&hook_opt); strvec_pushl(&hook_opt.args, options.upstream_arg, argc ? argv[0] : NULL, NULL); if (!ok_to_skip_pre_rebase && run_hooks("pre-rebase", &hook_opt)) { diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index cd658f41d5..e4726eb211 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -910,7 +910,7 @@ static int run_receive_hook(struct command *commands, int skip_broken, const struct string_list *push_options) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; struct receive_hook_feed_context ctx; int rc; struct command *iter = commands; @@ -922,6 +922,7 @@ static int run_receive_hook(struct command *commands, return 0; /* pre-receive hooks should run in series as the hook updates refs */ + run_hooks_opt_init_async(&opt); if (!strcmp(hook_name, "pre-receive")) opt.jobs = 1; @@ -956,9 +957,10 @@ static int run_receive_hook(struct command *commands, static int run_update_hook(struct command *cmd) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; int code; + run_hooks_opt_init_async(&opt); strvec_pushl(&opt.args, cmd->ref_name, oid_to_hex(&cmd->old_oid), @@ -1443,7 +1445,10 @@ static const char *push_to_checkout(unsigned char *hash, struct strvec *env, const char *work_tree) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; + + + run_hooks_opt_init_sync(&opt); opt.invoked_hook = invoked_hook; strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); @@ -1642,8 +1647,9 @@ static const char *update(struct command *cmd, struct shallow_info *si) static void run_update_post_hook(struct command *commands) { struct command *cmd; - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; + run_hooks_opt_init_async(&opt); for (cmd = commands; cmd; cmd = cmd->next) { if (cmd->error_string || cmd->did_not_exist) continue; diff --git a/builtin/worktree.c b/builtin/worktree.c index 2ad26a76f4..5a2c9d1039 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -382,8 +382,9 @@ static int add_worktree(const char *path, const char *refname, * is_junk is cleared, but do return appropriate code when hook fails. */ if (!ret && opts->checkout) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; + run_hooks_opt_init_sync(&opt); strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL); strvec_pushl(&opt.args, oid_to_hex(null_oid()), diff --git a/commit.c b/commit.c index cf62ebceae..58ee2c81bb 100644 --- a/commit.c +++ b/commit.c @@ -1700,10 +1700,12 @@ int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; va_list args; const char *arg; int ret; + + run_hooks_opt_init_sync(&opt); strvec_pushf(&opt.env, "GIT_INDEX_FILE=%s", index_file); /* diff --git a/hook.c b/hook.c index c1dac6982f..935751fa6c 100644 --- a/hook.c +++ b/hook.c @@ -88,6 +88,14 @@ const char *find_hook(const char *name) return path.buf; } +int configured_hook_jobs(void) +{ + int n = online_cpus(); + git_config_get_int("hook.jobs", &n); + + return n; +} + int hook_exists(const char *name) { return !!find_hook(name); @@ -117,6 +125,26 @@ struct list_head* hook_list(const char* hookname) return hook_head; } +void run_hooks_opt_init_sync(struct run_hooks_opt *o) +{ + strvec_init(&o->env); + strvec_init(&o->args); + o->path_to_stdin = NULL; + o->jobs = 1; + o->dir = NULL; + o->feed_pipe = NULL; + o->feed_pipe_ctx = NULL; + o->consume_sideband = NULL; + o->invoked_hook = NULL; + o->absolute_path = 0; +} + +void run_hooks_opt_init_async(struct run_hooks_opt *o) +{ + run_hooks_opt_init_sync(o); + o->jobs = configured_hook_jobs(); +} + void run_hooks_opt_clear(struct run_hooks_opt *o) { strvec_clear(&o->env); @@ -238,11 +266,9 @@ int run_found_hooks(const char *hook_name, struct list_head *hooks, .invoked_hook = options->invoked_hook, }; + cb_data.head = hooks; cb_data.run_me = list_first_entry(hooks, struct hook, list); - if (options->jobs != 1) - BUG("we do not handle %d or any other != 1 job number yet", options->jobs); - run_processes_parallel_tr2(options->jobs, pick_next_hook, notify_start_failure, diff --git a/hook.h b/hook.h index b97237931b..586ddf40bb 100644 --- a/hook.h +++ b/hook.h @@ -35,6 +35,9 @@ struct hook { */ struct list_head* hook_list(const char *hookname); +/* Provides the number of threads to use for parallel hook execution. */ +int configured_hook_jobs(void); + struct run_hooks_opt { /* Environment vars to be set for each hook */ @@ -93,12 +96,6 @@ struct run_hooks_opt int *invoked_hook; }; -#define RUN_HOOKS_OPT_INIT { \ - .jobs = 1, \ - .env = STRVEC_INIT, \ - .args = STRVEC_INIT, \ -} - /* * To specify a 'struct string_list', set 'run_hooks_opt.feed_pipe_ctx' to the * string_list and set 'run_hooks_opt.feed_pipe' to 'pipe_from_string_list()'. @@ -117,6 +114,8 @@ struct hook_cb_data { int *invoked_hook; }; +void run_hooks_opt_init_sync(struct run_hooks_opt *o); +void run_hooks_opt_init_async(struct run_hooks_opt *o); void run_hooks_opt_clear(struct run_hooks_opt *o); /* diff --git a/read-cache.c b/read-cache.c index f801313cc9..e8cbbc6ef2 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3063,7 +3063,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l { int ret; int was_full = !istate->sparse_index; - struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt hook_opt; ret = convert_to_sparse(istate); @@ -3092,6 +3092,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l else ret = close_lock_file_gently(lock); + run_hooks_opt_init_async(&hook_opt); strvec_pushl(&hook_opt.args, istate->updated_workdir ? "1" : "0", istate->updated_skipworktree ? "1" : "0", diff --git a/refs.c b/refs.c index 1149e7e7dc..61d0bb2579 100644 --- a/refs.c +++ b/refs.c @@ -2063,7 +2063,7 @@ static int run_transaction_hook(struct ref_transaction *transaction, const char *state) { struct strbuf buf = STRBUF_INIT; - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; struct string_list to_stdin = STRING_LIST_INIT_DUP; int ret = 0, i; char o[GIT_MAX_HEXSZ + 1], n[GIT_MAX_HEXSZ + 1]; @@ -2071,6 +2071,7 @@ static int run_transaction_hook(struct ref_transaction *transaction, if (!hook_exists("reference-transaction")) return ret; + run_hooks_opt_init_async(&opt); strvec_push(&opt.args, state); for (i = 0; i < transaction->nr; i++) { diff --git a/reset.c b/reset.c index e6af33b901..48d45f5b79 100644 --- a/reset.c +++ b/reset.c @@ -128,7 +128,9 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, reflog_head); } if (run_hook) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; + + run_hooks_opt_init_sync(&opt); strvec_pushl(&opt.args, oid_to_hex(orig ? orig : null_oid()), oid_to_hex(oid), diff --git a/sequencer.c b/sequencer.c index 2440b9dccd..17b93242a7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1148,11 +1148,13 @@ int update_head_with_reflog(const struct commit *old_head, static int run_rewrite_hook(const struct object_id *oldoid, const struct object_id *newoid) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; struct strbuf tmp = STRBUF_INIT; struct string_list to_stdin = STRING_LIST_INIT_DUP; int code; + run_hooks_opt_init_async(&opt); + strvec_push(&opt.args, "amend"); strbuf_addf(&tmp, @@ -4524,7 +4526,7 @@ static int pick_commits(struct repository *r, if (!stat(rebase_path_rewritten_list(), &st) && st.st_size > 0) { struct child_process notes_cp = CHILD_PROCESS_INIT; - struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt hook_opt; notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY); notes_cp.git_cmd = 1; @@ -4534,6 +4536,7 @@ static int pick_commits(struct repository *r, /* we don't care if this copying failed */ run_command(¬es_cp); + run_hooks_opt_init_async(&hook_opt); hook_opt.path_to_stdin = rebase_path_rewritten_list(); strvec_push(&hook_opt.args, "rebase"); run_hooks("post-rewrite", &hook_opt); diff --git a/transport.c b/transport.c index 9969ed2cdd..3381d24225 100644 --- a/transport.c +++ b/transport.c @@ -1200,11 +1200,13 @@ static int run_pre_push_hook(struct transport *transport, struct ref *remote_refs) { int ret = 0; - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt; struct strbuf tmp = STRBUF_INIT; struct ref *r; struct string_list to_stdin = STRING_LIST_INIT_DUP; + run_hooks_opt_init_async(&opt); + strvec_push(&opt.args, transport->remote->name); strvec_push(&opt.args, transport->url);
In many cases, there's no reason not to allow hooks to execute in parallel. run_processes_parallel() is well-suited - it's a task queue that runs its housekeeping in series, which means users don't need to worry about thread safety on their callback data. True multithreaded execution with the async_* functions isn't necessary here. Synchronous hook execution can be achieved by only allowing 1 job to run at a time. Teach run_hooks() to use that function for simple hooks which don't require stdin or capture of stderr. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/config/hook.txt | 4 ++++ Documentation/git-hook.txt | 17 ++++++++++++++++- builtin/am.c | 12 ++++++++---- builtin/checkout.c | 3 ++- builtin/clone.c | 3 ++- builtin/gc.c | 3 ++- builtin/hook.c | 6 +++++- builtin/merge.c | 3 ++- builtin/rebase.c | 3 ++- builtin/receive-pack.c | 14 ++++++++++---- builtin/worktree.c | 3 ++- commit.c | 4 +++- hook.c | 32 +++++++++++++++++++++++++++++--- hook.h | 11 +++++------ read-cache.c | 3 ++- refs.c | 3 ++- reset.c | 4 +++- sequencer.c | 7 +++++-- transport.c | 4 +++- 19 files changed, 107 insertions(+), 32 deletions(-) create mode 100644 Documentation/config/hook.txt