| Message ID | 20210311021037.3001235-24-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > @@ -3070,6 +3071,8 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l > unsigned flags) > { > int ret; > + struct run_hooks_opt hook_opt; > + run_hooks_opt_init_async(&hook_opt); > Nit. blank line between the last of decls and the first stmt (many identical nits exist everywhere in this series). > /* > * TODO trace2: replace "the_repository" with the actual repo instance > @@ -3088,9 +3091,13 @@ static int do_write_locked_index(s > else > ret = close_lock_file_gently(lock); > > - run_hook_le(NULL, "post-index-change", > - istate->updated_workdir ? "1" : "0", > - istate->updated_skipworktree ? "1" : "0", NULL); > + strvec_pushl(&hook_opt.args, > + istate->updated_workdir ? "1" : "0", > + istate->updated_skipworktree ? "1" : "0", > + NULL); > + run_hooks("post-index-change", &hook_opt); > + run_hooks_opt_clear(&hook_opt); There is one early return before the precontext of this hunk that bypasses this opt_clear() call. It is before any member of hook_opt structure that was opt_init()'ed gets touched, so with the current code, there is no leak, but it probably is laying a landmine for the future, where opt_init() may allocate some resource to its member, with the expectation that all users of the API would call opt_clear() to release. Or the caller of the API (like this one) may start mucking with the opt structure before the existing early return, at which point the current assumption that it is safe to return from that point without opt_clear() would be broken. I saw that there are other early returns in the series that are safe right now but may become unsafe when the API implementation gets extended that way. If it does not involve too much code churning, we may want to restructure the code to make these early returns into "goto"s that jump to a single exit point, so that we can always match opt_init() with opt_clear(), like the structure of the existing code allowed cmd_rebase() to use the hooks API cleanly in [v8 22/37]. Thanks.
On Fri, Mar 12, 2021 at 02:22:08AM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > @@ -3070,6 +3071,8 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l > > unsigned flags) > > { > > int ret; > > + struct run_hooks_opt hook_opt; > > + run_hooks_opt_init_async(&hook_opt); > > > > Nit. blank line between the last of decls and the first stmt (many > identical nits exist everywhere in this series). > > > /* > > * TODO trace2: replace "the_repository" with the actual repo instance > > @@ -3088,9 +3091,13 @@ static int do_write_locked_index(s > > else > > ret = close_lock_file_gently(lock); > > > > - run_hook_le(NULL, "post-index-change", > > - istate->updated_workdir ? "1" : "0", > > - istate->updated_skipworktree ? "1" : "0", NULL); > > + strvec_pushl(&hook_opt.args, > > + istate->updated_workdir ? "1" : "0", > > + istate->updated_skipworktree ? "1" : "0", > > + NULL); > > + run_hooks("post-index-change", &hook_opt); > > + run_hooks_opt_clear(&hook_opt); > > There is one early return before the precontext of this hunk that > bypasses this opt_clear() call. It is before any member of hook_opt > structure that was opt_init()'ed gets touched, so with the current > code, there is no leak, but it probably is laying a landmine for the > future, where opt_init() may allocate some resource to its member, > with the expectation that all users of the API would call > opt_clear() to release. Or the caller of the API (like this one) may > start mucking with the opt structure before the existing early return, > at which point the current assumption that it is safe to return from > that point without opt_clear() would be broken. > > I saw that there are other early returns in the series that are safe > right now but may become unsafe when the API implementation gets > extended that way. If it does not involve too much code churning, > we may want to restructure the code to make these early returns into > "goto"s that jump to a single exit point, so that we can always > match opt_init() with opt_clear(), like the structure of the > existing code allowed cmd_rebase() to use the hooks API cleanly in > [v8 22/37]. OK. I'll audit this second half of the series looking for this type of thing and try to clean up/use gotos if appropriate/etc. Thanks for pointing it out. - Emily
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index e3a0375827..e5c2cef271 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -720,6 +720,9 @@ and "0" meaning they were not. Only one parameter should be set to "1" when the hook runs. The hook running passing "1", "1" should not be possible. +Hooks run during 'post-index-change' will be run in parallel, unless hook.jobs +is configured to 1. + GIT --- Part of the linkgit:git[1] suite diff --git a/read-cache.c b/read-cache.c index 1e9a50c6c7..fd6c111372 100644 --- a/read-cache.c +++ b/read-cache.c @@ -25,6 +25,7 @@ #include "fsmonitor.h" #include "thread-utils.h" #include "progress.h" +#include "hook.h" /* Mask for the name length in ce_flags in the on-disk index */ @@ -3070,6 +3071,8 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l unsigned flags) { int ret; + struct run_hooks_opt hook_opt; + run_hooks_opt_init_async(&hook_opt); /* * TODO trace2: replace "the_repository" with the actual repo instance @@ -3088,9 +3091,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l else ret = close_lock_file_gently(lock); - run_hook_le(NULL, "post-index-change", - istate->updated_workdir ? "1" : "0", - istate->updated_skipworktree ? "1" : "0", NULL); + strvec_pushl(&hook_opt.args, + istate->updated_workdir ? "1" : "0", + istate->updated_skipworktree ? "1" : "0", + NULL); + run_hooks("post-index-change", &hook_opt); + run_hooks_opt_clear(&hook_opt); + istate->updated_workdir = 0; istate->updated_skipworktree = 0;
By using hook.h instead of run-command.h to run, post-index-change hooks can now be specified in the config in addition to the hookdir. post-index-change is not run anywhere besides in read-cache.c. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/githooks.txt | 3 +++ read-cache.c | 13 ++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-)