| Message ID | 20210311021037.3001235-20-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > @@ -1558,8 +1563,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; > + run_hooks_opt_init_async(&hook_opt); > > - if (run_hook_le(NULL, "pre-applypatch", NULL)) > + if (run_hooks("pre-applypatch", &hook_opt)) > exit(1); > > if (write_cache_as_tree(&tree, 0, NULL)) > @@ -1611,8 +1618,9 @@ static void do_commit(const struct am_state *state) > fclose(fp); > } > > - run_hook_le(NULL, "post-applypatch", NULL); > + run_hooks("post-applypatch", &hook_opt); > > + run_hooks_opt_clear(&hook_opt); > strbuf_release(&sb); > } This one does opt_init(), run_hooks(), and another run_hooks() and then opt_clear(). If run_hooks() is a read-only operation on the hook_opt, then that would be alright, but it just smells iffy that it is not done as two separate opt_init(), run_hooks(), opt_clear() sequences for two separate run_hooks() invocations. The same worry about future safety I meantioned elsewhere in the series also applies. Thanks.
On Fri, Mar 12, 2021 at 02:23:39AM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > @@ -1558,8 +1563,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; > > + run_hooks_opt_init_async(&hook_opt); > > > > - if (run_hook_le(NULL, "pre-applypatch", NULL)) > > + if (run_hooks("pre-applypatch", &hook_opt)) > > exit(1); > > > > if (write_cache_as_tree(&tree, 0, NULL)) > > @@ -1611,8 +1618,9 @@ static void do_commit(const struct am_state *state) > > fclose(fp); > > } > > > > - run_hook_le(NULL, "post-applypatch", NULL); > > + run_hooks("post-applypatch", &hook_opt); > > > > + run_hooks_opt_clear(&hook_opt); > > strbuf_release(&sb); > > } > > This one does opt_init(), run_hooks(), and another run_hooks() and > then opt_clear(). If run_hooks() is a read-only operation on the > hook_opt, then that would be alright, but it just smells iffy that > it is not done as two separate opt_init(), run_hooks(), opt_clear() > sequences for two separate run_hooks() invocations. The same worry > about future safety I meantioned elsewhere in the series also > applies. Interesting observation. I think the only thing that could be mutated in the run_hooks_opt struct today is the caller-provided callback data (run_hooks_opt.feed_pipe_ctx) - which presumably is being manipulated by the caller in a callback they wrote. But I don't think it hurts particularly to clear/init again between the two invocations, to be safe - so I will change the code here. - Emily
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 984fb998b2..0e7eb972ab 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -58,6 +58,9 @@ the message file. The default 'applypatch-msg' hook, when enabled, runs the 'commit-msg' hook, if the latter is enabled. +Hooks run during 'applypatch-msg' will not be parallelized, because hooks are +expected to edit the file holding the commit log message. + pre-applypatch ~~~~~~~~~~~~~~ @@ -73,6 +76,9 @@ make a commit if it does not pass certain test. The default 'pre-applypatch' hook, when enabled, runs the 'pre-commit' hook, if the latter is enabled. +Hooks run during 'pre-applypatch' will be run in parallel, unless hook.jobs is +configured to 1. + post-applypatch ~~~~~~~~~~~~~~~ @@ -82,6 +88,9 @@ and is invoked after the patch is applied and a commit is made. This hook is meant primarily for notification, and cannot affect the outcome of `git am`. +Hooks run during 'post-applypatch' will be run in parallel, unless hook.jobs is +configured to 1. + pre-commit ~~~~~~~~~~ diff --git a/builtin/am.c b/builtin/am.c index 8355e3566f..4467fd9e63 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -33,6 +33,7 @@ #include "string-list.h" #include "packfile.h" #include "repository.h" +#include "hook.h" /** * Returns the length of the first line of msg. @@ -426,9 +427,13 @@ 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_sync(&opt); assert(state->msg); - ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL); + strvec_push(&opt.args, am_path(state, "final-commit")); + ret = run_hooks("applypatch-msg", &opt); + run_hooks_opt_clear(&opt); if (!ret) { FREE_AND_NULL(state->msg); @@ -1558,8 +1563,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; + run_hooks_opt_init_async(&hook_opt); - if (run_hook_le(NULL, "pre-applypatch", NULL)) + if (run_hooks("pre-applypatch", &hook_opt)) exit(1); if (write_cache_as_tree(&tree, 0, NULL)) @@ -1611,8 +1618,9 @@ static void do_commit(const struct am_state *state) fclose(fp); } - run_hook_le(NULL, "post-applypatch", NULL); + run_hooks("post-applypatch", &hook_opt); + run_hooks_opt_clear(&hook_opt); strbuf_release(&sb); }
Teach pre-applypatch, post-applypatch, and applypatch-msg to use the hook.h library instead of the run-command.h library. This enables use of hooks specified in the config, in addition to those in the hookdir. These three hooks are called only by builtin/am.c. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/githooks.txt | 9 +++++++++ builtin/am.c | 14 +++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-)