| Message ID | 20210819033450.3382652-2-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks restarted | expand |
On Wed, Aug 18 2021, Emily Shaffer wrote: > @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix) > struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; > int ignore_missing = 0; > const char *hook_name; > - const char *hook_path; > + struct list_head *hooks; > + > struct option run_options[] = { > OPT_BOOL(0, "ignore-missing", &ignore_missing, > N_("exit quietly with a zero exit code if the requested hook cannot be found")), In general in this patch series there's a bunch of little whitespace changes like that along with other changes. I think it's probably best if I just absorb that in the "base" topic instead of doing them here. E.g. in this case we could also add a line between "struct option" and the rest. I don't mind either way, but the whitespace churn makes for distracting reading... > @@ -58,15 +59,16 @@ static int run(int argc, const char **argv, const char *prefix) > git_config(git_default_config, NULL); > > hook_name = argv[0]; > - if (ignore_missing) > - return run_hooks_oneshot(hook_name, &opt); > - hook_path = find_hook(hook_name); > - if (!hook_path) { > + hooks = list_hooks(hook_name); > + if (list_empty(hooks)) { > + /* ... act like run_hooks_oneshot() under --ignore-missing */ > + if (ignore_missing) > + return 0; > error("cannot find a hook named %s", hook_name); > return 1; > } > > - ret = run_hooks(hook_name, hook_path, &opt); > + ret = run_hooks(hook_name, hooks, &opt); > run_hooks_opt_clear(&opt); > return ret; This memory management is a bit inconsistent. So here we list_hooks(), pass it to run_hooks(), which clears it for us, OK... > -int run_hooks(const char *hook_name, const char *hook_path, > - struct run_hooks_opt *options) > +int run_hooks(const char *hook_name, struct list_head *hooks, > + struct run_hooks_opt *options) > { > - struct strbuf abs_path = STRBUF_INIT; > - struct hook my_hook = { > - .hook_path = hook_path, > - }; > struct hook_cb_data cb_data = { > .rc = 0, > .hook_name = hook_name, > @@ -197,11 +241,9 @@ int run_hooks(const char *hook_name, const char *hook_path, > if (!options) > BUG("a struct run_hooks_opt must be provided to run_hooks"); > > - if (options->absolute_path) { > - strbuf_add_absolute_path(&abs_path, hook_path); > - my_hook.hook_path = abs_path.buf; > - } > - cb_data.run_me = &my_hook; > + > + cb_data.head = hooks; > + cb_data.run_me = list_first_entry(hooks, struct hook, list); > > run_processes_parallel_tr2(jobs, > pick_next_hook, > @@ -213,18 +255,15 @@ int run_hooks(const char *hook_name, const char *hook_path, > "hook", > hook_name); > > - > - if (options->absolute_path) > - strbuf_release(&abs_path); > - free(my_hook.feed_pipe_cb_data); > + clear_hook_list(hooks); > > return cb_data.rc; > } Which we can see here will call clear_hook_list(), so far so good, but then... > int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) > { > - const char *hook_path; > - int ret; > + struct list_head *hooks; > + int ret = 0; > struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT; > > if (!options) > @@ -233,14 +272,19 @@ int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) > if (options->path_to_stdin && options->feed_pipe) > BUG("choose only one method to populate stdin"); > > - hook_path = find_hook(hook_name); > - if (!hook_path) { > - ret = 0; > + hooks = list_hooks(hook_name); > + > + /* > + * If you need to act on a missing hook, use run_found_hooks() > + * instead > + */ > + if (list_empty(hooks)) > goto cleanup; > - } > > - ret = run_hooks(hook_name, hook_path, options); > + ret = run_hooks(hook_name, hooks, options); > + > cleanup: > run_hooks_opt_clear(options); > + clear_hook_list(hooks); ...the oneshot command also does clear_hook_list(), after calling run_hooks() which cleared it already. That looks like a mistake, i.e. we should always trust run_hooks() to clear it, no?
On Tue, Aug 24, 2021 at 04:56:10PM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Wed, Aug 18 2021, Emily Shaffer wrote: > > > @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix) > > struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; > > int ignore_missing = 0; > > const char *hook_name; > > - const char *hook_path; > > + struct list_head *hooks; > > + > > struct option run_options[] = { > > OPT_BOOL(0, "ignore-missing", &ignore_missing, > > N_("exit quietly with a zero exit code if the requested hook cannot be found")), > > In general in this patch series there's a bunch of little whitespace > changes like that along with other changes. I think it's probably best > if I just absorb that in the "base" topic instead of doing them > here. E.g. in this case we could also add a line between "struct option" > and the rest. > > I don't mind either way, but the whitespace churn makes for distracting > reading... Ah, hm. I don't know if in this specific case it's necessary for me to even have this whitespace change, since 'run_options' is still a struct declaration. I'll just drop this one, but in general whichever whitespace bits you like from this topic, feel free to absorb. Will make a note to scan through the diff when I rebase onto your next reroll checking for spurious whitespace changes. > > > @@ -58,15 +59,16 @@ static int run(int argc, const char **argv, const char *prefix) > > git_config(git_default_config, NULL); > > > > hook_name = argv[0]; > > - if (ignore_missing) > > - return run_hooks_oneshot(hook_name, &opt); > > - hook_path = find_hook(hook_name); > > - if (!hook_path) { > > + hooks = list_hooks(hook_name); > > + if (list_empty(hooks)) { > > + /* ... act like run_hooks_oneshot() under --ignore-missing */ > > + if (ignore_missing) > > + return 0; > > error("cannot find a hook named %s", hook_name); > > return 1; > > } > > > > - ret = run_hooks(hook_name, hook_path, &opt); > > + ret = run_hooks(hook_name, hooks, &opt); > > run_hooks_opt_clear(&opt); > > return ret; > > This memory management is a bit inconsistent. So here we list_hooks(), > pass it to run_hooks(), which clears it for us, OK... > > > -int run_hooks(const char *hook_name, const char *hook_path, > > - struct run_hooks_opt *options) > > +int run_hooks(const char *hook_name, struct list_head *hooks, > > + struct run_hooks_opt *options) > > { > > - struct strbuf abs_path = STRBUF_INIT; > > - struct hook my_hook = { > > - .hook_path = hook_path, > > - }; > > struct hook_cb_data cb_data = { > > .rc = 0, > > .hook_name = hook_name, > > @@ -197,11 +241,9 @@ int run_hooks(const char *hook_name, const char *hook_path, > > if (!options) > > BUG("a struct run_hooks_opt must be provided to run_hooks"); > > > > - if (options->absolute_path) { > > - strbuf_add_absolute_path(&abs_path, hook_path); > > - my_hook.hook_path = abs_path.buf; > > - } > > - cb_data.run_me = &my_hook; > > + > > + cb_data.head = hooks; > > + cb_data.run_me = list_first_entry(hooks, struct hook, list); > > > > run_processes_parallel_tr2(jobs, > > pick_next_hook, > > @@ -213,18 +255,15 @@ int run_hooks(const char *hook_name, const char *hook_path, > > "hook", > > hook_name); > > > > - > > - if (options->absolute_path) > > - strbuf_release(&abs_path); > > - free(my_hook.feed_pipe_cb_data); > > + clear_hook_list(hooks); > > > > return cb_data.rc; > > } > > Which we can see here will call clear_hook_list(), so far so good, but then... > > > int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) > > { > > - const char *hook_path; > > - int ret; > > + struct list_head *hooks; > > + int ret = 0; > > struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT; > > > > if (!options) > > @@ -233,14 +272,19 @@ int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) > > if (options->path_to_stdin && options->feed_pipe) > > BUG("choose only one method to populate stdin"); > > > > - hook_path = find_hook(hook_name); > > - if (!hook_path) { > > - ret = 0; > > + hooks = list_hooks(hook_name); > > + > > + /* > > + * If you need to act on a missing hook, use run_found_hooks() > > + * instead > > + */ > > + if (list_empty(hooks)) > > goto cleanup; > > - } > > > > - ret = run_hooks(hook_name, hook_path, options); > > + ret = run_hooks(hook_name, hooks, options); > > + > > cleanup: > > run_hooks_opt_clear(options); > > + clear_hook_list(hooks); > > ...the oneshot command also does clear_hook_list(), after calling > run_hooks() which cleared it already. That looks like a mistake, > i.e. we should always trust run_hooks() to clear it, no? Ah, good catch. I will update the comment on run_hooks() and fix _oneshot() :) - Emily
On Thu, Aug 26 2021, Emily Shaffer wrote: > On Tue, Aug 24, 2021 at 04:56:10PM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> >> On Wed, Aug 18 2021, Emily Shaffer wrote: >> >> > @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix) >> > struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> > int ignore_missing = 0; >> > const char *hook_name; >> > - const char *hook_path; >> > + struct list_head *hooks; >> > + >> > struct option run_options[] = { >> > OPT_BOOL(0, "ignore-missing", &ignore_missing, >> > N_("exit quietly with a zero exit code if the requested hook cannot be found")), >> >> In general in this patch series there's a bunch of little whitespace >> changes like that along with other changes. I think it's probably best >> if I just absorb that in the "base" topic instead of doing them >> here. E.g. in this case we could also add a line between "struct option" >> and the rest. >> >> I don't mind either way, but the whitespace churn makes for distracting >> reading... > > Ah, hm. I don't know if in this specific case it's necessary for me to > even have this whitespace change, since 'run_options' is still a struct > declaration. I'll just drop this one, but in general whichever > whitespace bits you like from this topic, feel free to absorb. Will make > a note to scan through the diff when I rebase onto your next reroll > checking for spurious whitespace changes. > >> >> > @@ -58,15 +59,16 @@ static int run(int argc, const char **argv, const char *prefix) >> > git_config(git_default_config, NULL); >> > >> > hook_name = argv[0]; >> > - if (ignore_missing) >> > - return run_hooks_oneshot(hook_name, &opt); >> > - hook_path = find_hook(hook_name); >> > - if (!hook_path) { >> > + hooks = list_hooks(hook_name); >> > + if (list_empty(hooks)) { >> > + /* ... act like run_hooks_oneshot() under --ignore-missing */ >> > + if (ignore_missing) >> > + return 0; >> > error("cannot find a hook named %s", hook_name); >> > return 1; >> > } >> > >> > - ret = run_hooks(hook_name, hook_path, &opt); >> > + ret = run_hooks(hook_name, hooks, &opt); >> > run_hooks_opt_clear(&opt); >> > return ret; >> >> This memory management is a bit inconsistent. So here we list_hooks(), >> pass it to run_hooks(), which clears it for us, OK... >> >> > -int run_hooks(const char *hook_name, const char *hook_path, >> > - struct run_hooks_opt *options) >> > +int run_hooks(const char *hook_name, struct list_head *hooks, >> > + struct run_hooks_opt *options) >> > { >> > - struct strbuf abs_path = STRBUF_INIT; >> > - struct hook my_hook = { >> > - .hook_path = hook_path, >> > - }; >> > struct hook_cb_data cb_data = { >> > .rc = 0, >> > .hook_name = hook_name, >> > @@ -197,11 +241,9 @@ int run_hooks(const char *hook_name, const char *hook_path, >> > if (!options) >> > BUG("a struct run_hooks_opt must be provided to run_hooks"); >> > >> > - if (options->absolute_path) { >> > - strbuf_add_absolute_path(&abs_path, hook_path); >> > - my_hook.hook_path = abs_path.buf; >> > - } >> > - cb_data.run_me = &my_hook; >> > + >> > + cb_data.head = hooks; >> > + cb_data.run_me = list_first_entry(hooks, struct hook, list); >> > >> > run_processes_parallel_tr2(jobs, >> > pick_next_hook, >> > @@ -213,18 +255,15 @@ int run_hooks(const char *hook_name, const char *hook_path, >> > "hook", >> > hook_name); >> > >> > - >> > - if (options->absolute_path) >> > - strbuf_release(&abs_path); >> > - free(my_hook.feed_pipe_cb_data); >> > + clear_hook_list(hooks); >> > >> > return cb_data.rc; >> > } >> >> Which we can see here will call clear_hook_list(), so far so good, but then... >> >> > int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) >> > { >> > - const char *hook_path; >> > - int ret; >> > + struct list_head *hooks; >> > + int ret = 0; >> > struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT; >> > >> > if (!options) >> > @@ -233,14 +272,19 @@ int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) >> > if (options->path_to_stdin && options->feed_pipe) >> > BUG("choose only one method to populate stdin"); >> > >> > - hook_path = find_hook(hook_name); >> > - if (!hook_path) { >> > - ret = 0; >> > + hooks = list_hooks(hook_name); >> > + >> > + /* >> > + * If you need to act on a missing hook, use run_found_hooks() >> > + * instead >> > + */ >> > + if (list_empty(hooks)) >> > goto cleanup; >> > - } >> > >> > - ret = run_hooks(hook_name, hook_path, options); >> > + ret = run_hooks(hook_name, hooks, options); >> > + >> > cleanup: >> > run_hooks_opt_clear(options); >> > + clear_hook_list(hooks); >> >> ...the oneshot command also does clear_hook_list(), after calling >> run_hooks() which cleared it already. That looks like a mistake, >> i.e. we should always trust run_hooks() to clear it, no? > > Ah, good catch. I will update the comment on run_hooks() and fix > _oneshot() :) > > - Emily I found a further memory issue with this, on "seen" running e.g. t0001 when compiled with SANITIZE=leak is broken by this series. It's because in clear_hook_list() you clear the list of hooks, but forget to clear the malloc'd container, so a missing free() fixes it. As in the POC patch at the end of this mail. But e.g. "git hook list <name>" is still broken, easy enough to fix, just also needs fixing of the list_hooks_gently() callsites to e.g. this: diff --git a/builtin/hook.c b/builtin/hook.c index 50233366a8..2cd1831075 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -48,8 +48,10 @@ static int list(int argc, const char **argv, const char *prefix) head = list_hooks_gently(hookname); - if (list_empty(head)) + if (list_empty(head)) { + clear_hook_list(head); return 1; + } list_for_each(pos, head) { struct hook *item = list_entry(pos, struct hook, list); Although going to that length to make the SANITIZE=leak run clean is arguably overdoing it... diff --git a/hook.c b/hook.c index 23af86b9ea..e6e1e4173a 100644 --- a/hook.c +++ b/hook.c @@ -67,6 +67,7 @@ void clear_hook_list(struct list_head *head) struct list_head *pos, *tmp; list_for_each_safe(pos, tmp, head) remove_hook(pos); + free(head); } static int known_hook(const char *name) @@ -142,7 +143,16 @@ const char *find_hook_gently(const char *name) int hook_exists(const char *name) { - return !list_empty(list_hooks(name)); + struct list_head *hooks; + int exists; + + hooks = list_hooks(name); + + exists = !list_empty(hooks); + + clear_hook_list(hooks); + + return exists; } struct hook_config_cb
diff --git a/builtin/hook.c b/builtin/hook.c index 27dce6a2f0..e18357ba1f 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix) struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; int ignore_missing = 0; const char *hook_name; - const char *hook_path; + struct list_head *hooks; + struct option run_options[] = { OPT_BOOL(0, "ignore-missing", &ignore_missing, N_("exit quietly with a zero exit code if the requested hook cannot be found")), @@ -58,15 +59,16 @@ static int run(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); hook_name = argv[0]; - if (ignore_missing) - return run_hooks_oneshot(hook_name, &opt); - hook_path = find_hook(hook_name); - if (!hook_path) { + hooks = list_hooks(hook_name); + if (list_empty(hooks)) { + /* ... act like run_hooks_oneshot() under --ignore-missing */ + if (ignore_missing) + return 0; error("cannot find a hook named %s", hook_name); return 1; } - ret = run_hooks(hook_name, hook_path, &opt); + ret = run_hooks(hook_name, hooks, &opt); run_hooks_opt_clear(&opt); return ret; usage: diff --git a/hook.c b/hook.c index ee20b2e365..333cfd633a 100644 --- a/hook.c +++ b/hook.c @@ -4,6 +4,27 @@ #include "hook-list.h" #include "config.h" +static void free_hook(struct hook *ptr) +{ + if (ptr) + free(ptr->feed_pipe_cb_data); + free(ptr); +} + +static void remove_hook(struct list_head *to_remove) +{ + struct hook *hook_to_remove = list_entry(to_remove, struct hook, list); + list_del(to_remove); + free_hook(hook_to_remove); +} + +void clear_hook_list(struct list_head *head) +{ + struct list_head *pos, *tmp; + list_for_each_safe(pos, tmp, head) + remove_hook(pos); +} + static int known_hook(const char *name) { const char **p; @@ -68,7 +89,31 @@ const char *find_hook(const char *name) int hook_exists(const char *name) { - return !!find_hook(name); + return !list_empty(list_hooks(name)); +} + +struct list_head *list_hooks(const char *hookname) +{ + struct list_head *hook_head = xmalloc(sizeof(struct list_head)); + + INIT_LIST_HEAD(hook_head); + + if (!hookname) + BUG("null hookname was provided to hook_list()!"); + + if (have_git_dir()) { + const char *hook_path = find_hook(hookname); + + /* Add the hook from the hookdir */ + if (hook_path) { + struct hook *to_add = xmalloc(sizeof(*to_add)); + to_add->hook_path = hook_path; + to_add->feed_pipe_cb_data = NULL; + list_add_tail(&to_add->list, hook_head); + } + } + + return hook_head; } void run_hooks_opt_clear(struct run_hooks_opt *o) @@ -128,7 +173,10 @@ static int pick_next_hook(struct child_process *cp, cp->dir = hook_cb->options->dir; /* add command */ - strvec_push(&cp->args, run_me->hook_path); + if (hook_cb->options->absolute_path) + strvec_push(&cp->args, absolute_path(run_me->hook_path)); + else + strvec_push(&cp->args, run_me->hook_path); /* * add passed-in argv, without expanding - let the user get back @@ -139,12 +187,12 @@ static int pick_next_hook(struct child_process *cp, /* Provide context for errors if necessary */ *pp_task_cb = run_me; - /* - * 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->run_me = NULL; + /* Get the next entry ready */ + if (hook_cb->run_me->list.next == hook_cb->head) + hook_cb->run_me = NULL; + else + hook_cb->run_me = list_entry(hook_cb->run_me->list.next, + struct hook, list); return 1; } @@ -179,13 +227,9 @@ static int notify_hook_finished(int result, return 0; } -int run_hooks(const char *hook_name, const char *hook_path, - struct run_hooks_opt *options) +int run_hooks(const char *hook_name, struct list_head *hooks, + struct run_hooks_opt *options) { - struct strbuf abs_path = STRBUF_INIT; - struct hook my_hook = { - .hook_path = hook_path, - }; struct hook_cb_data cb_data = { .rc = 0, .hook_name = hook_name, @@ -197,11 +241,9 @@ int run_hooks(const char *hook_name, const char *hook_path, if (!options) BUG("a struct run_hooks_opt must be provided to run_hooks"); - if (options->absolute_path) { - strbuf_add_absolute_path(&abs_path, hook_path); - my_hook.hook_path = abs_path.buf; - } - cb_data.run_me = &my_hook; + + cb_data.head = hooks; + cb_data.run_me = list_first_entry(hooks, struct hook, list); run_processes_parallel_tr2(jobs, pick_next_hook, @@ -213,18 +255,15 @@ int run_hooks(const char *hook_name, const char *hook_path, "hook", hook_name); - - if (options->absolute_path) - strbuf_release(&abs_path); - free(my_hook.feed_pipe_cb_data); + clear_hook_list(hooks); return cb_data.rc; } int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) { - const char *hook_path; - int ret; + struct list_head *hooks; + int ret = 0; struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT; if (!options) @@ -233,14 +272,19 @@ int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) if (options->path_to_stdin && options->feed_pipe) BUG("choose only one method to populate stdin"); - hook_path = find_hook(hook_name); - if (!hook_path) { - ret = 0; + hooks = list_hooks(hook_name); + + /* + * If you need to act on a missing hook, use run_found_hooks() + * instead + */ + if (list_empty(hooks)) goto cleanup; - } - ret = run_hooks(hook_name, hook_path, options); + ret = run_hooks(hook_name, hooks, options); + cleanup: run_hooks_opt_clear(options); + clear_hook_list(hooks); return ret; } diff --git a/hook.h b/hook.h index 58dfbf474c..12b56616bb 100644 --- a/hook.h +++ b/hook.h @@ -3,6 +3,7 @@ #include "strbuf.h" #include "strvec.h" #include "run-command.h" +#include "list.h" /* * Returns the path to the hook file, or NULL if the hook is missing @@ -17,6 +18,7 @@ const char *find_hook(const char *name); int hook_exists(const char *hookname); struct hook { + struct list_head list; /* The path to the hook */ const char *hook_path; @@ -27,6 +29,12 @@ struct hook { void *feed_pipe_cb_data; }; +/* + * Provides a linked list of 'struct hook' detailing commands which should run + * in response to the 'hookname' event, in execution order. + */ +struct list_head *list_hooks(const char *hookname); + struct run_hooks_opt { /* Environment vars to be set for each hook */ @@ -97,6 +105,7 @@ struct hook_cb_data { /* rc reflects the cumulative failure state */ int rc; const char *hook_name; + struct list_head *head; struct hook *run_me; struct run_hooks_opt *options; int *invoked_hook; @@ -110,8 +119,8 @@ void run_hooks_opt_clear(struct run_hooks_opt *o); * * See run_hooks_oneshot() for the simpler one-shot API. */ -int run_hooks(const char *hookname, const char *hook_path, - struct run_hooks_opt *options); +int run_hooks(const char *hookname, struct list_head *hooks, + struct run_hooks_opt *options); /** * Calls find_hook() on your "hook_name" and runs the hooks (if any) @@ -123,4 +132,7 @@ int run_hooks(const char *hookname, const char *hook_path, */ int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options); +/* Empties the list at 'head', calling 'free_hook()' on each entry */ +void clear_hook_list(struct list_head *head); + #endif
To prepare for multihook support, teach hook.[hc] to take a list of hooks at run_hooks and run_found_hooks. Right now the list is always one entry, but in the future we will allow users to supply more than one executable for a single hook event. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- builtin/hook.c | 14 ++++--- hook.c | 104 +++++++++++++++++++++++++++++++++++-------------- hook.h | 16 +++++++- 3 files changed, 96 insertions(+), 38 deletions(-)