| Message ID | 20210311021037.3001235-25-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > @@ -1435,12 +1436,19 @@ 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_sync(&opt); > + > strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); > - if (run_hook_le(env->v, push_to_checkout_hook, > - hash_to_hex(hash), NULL)) > + strvec_pushv(&opt.env, env->v); > + strvec_push(&opt.args, hash_to_hex(hash)); > + if (run_hooks(push_to_checkout_hook, &opt)) { > + run_hooks_opt_clear(&opt); > return "push-to-checkout hook declined"; > - else > + } else { > + run_hooks_opt_clear(&opt); > return NULL; > + } > } OK, we opt_init(), futz with opt, call run_hooks() and opt_clear() regardless of the outcome from run_hooks(). Narrow-sighted me wonders if it makes the use of the API easier if run_hooks() did the opt_clear() before it returns, but I haven't yet seen enough use at this point to judge. Thanks.
On Fri, Mar 12, 2021 at 02:24:41AM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > @@ -1435,12 +1436,19 @@ 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_sync(&opt); > > + > > strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); > > - if (run_hook_le(env->v, push_to_checkout_hook, > > - hash_to_hex(hash), NULL)) > > + strvec_pushv(&opt.env, env->v); > > + strvec_push(&opt.args, hash_to_hex(hash)); > > + if (run_hooks(push_to_checkout_hook, &opt)) { > > + run_hooks_opt_clear(&opt); > > return "push-to-checkout hook declined"; > > - else > > + } else { > > + run_hooks_opt_clear(&opt); > > return NULL; > > + } > > } > > OK, we opt_init(), futz with opt, call run_hooks() and opt_clear() > regardless of the outcome from run_hooks(). Narrow-sighted me > wonders if it makes the use of the API easier if run_hooks() did the > opt_clear() before it returns, but I haven't yet seen enough use at > this point to judge. Hrm, is that idiomatic? I guess it would be convenient, and as long as it doesn't touch explicitly caller-managed context pointer it should be safe, but wouldn't it be surprising? - Emily
Emily Shaffer <emilyshaffer@google.com> writes: >> OK, we opt_init(), futz with opt, call run_hooks() and opt_clear() >> regardless of the outcome from run_hooks(). Narrow-sighted me >> wonders if it makes the use of the API easier if run_hooks() did the >> opt_clear() before it returns, but I haven't yet seen enough use at >> this point to judge. > > Hrm, is that idiomatic? I guess it would be convenient, and as long as > it doesn't touch explicitly caller-managed context pointer it should be > safe, but wouldn't it be surprising? The precedent (at this point, I will not judge if it is a good pattern to emulate or an anti-pattern to stay away from) I had in mind was the run_command() which clears child_process structure as the side effect of internally calling finish_command(). Leaving them separate is of course more flexible, but depending on how small we can keep down the number of call patterns of this new API, always having to clear after run might become an unnecessary source of leaks. When I gave that comment, I didn't have enough input to decide, and now it has been so long since I gave my reviews, I do not quite remember what my impression after reading all the patches through was X-<.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index e5c2cef271..f2178dbc83 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -555,6 +555,7 @@ that switches branches while keeping the local changes in the working tree that do not interfere with the difference between the branches. +Hooks executed during 'push-to-checkout' will not be parallelized. pre-auto-gc ~~~~~~~~~~~ diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index d26040c477..234b70f0d1 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -29,6 +29,7 @@ #include "commit-reach.h" #include "worktree.h" #include "shallow.h" +#include "hook.h" static const char * const receive_pack_usage[] = { N_("git receive-pack <git-dir>"), @@ -1435,12 +1436,19 @@ 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_sync(&opt); + strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); - if (run_hook_le(env->v, push_to_checkout_hook, - hash_to_hex(hash), NULL)) + strvec_pushv(&opt.env, env->v); + strvec_push(&opt.args, hash_to_hex(hash)); + if (run_hooks(push_to_checkout_hook, &opt)) { + run_hooks_opt_clear(&opt); return "push-to-checkout hook declined"; - else + } else { + run_hooks_opt_clear(&opt); return NULL; + } } static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree) @@ -1464,7 +1472,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir)); - if (!find_hook(push_to_checkout_hook)) + if (!hook_exists(push_to_checkout_hook, HOOKDIR_USE_CONFIG)) retval = push_to_deploy(sha1, &env, work_tree); else retval = push_to_checkout(sha1, &env, work_tree);
By using hook.h instead of run-command.h to invoke push-to-checkout, hooks can now be specified in the config as well as in the hookdir. push-to-checkout is not called anywhere but in builtin/receive-pack.c. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/githooks.txt | 1 + builtin/receive-pack.c | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-)