Re: [PATCH v3 1/6] hook: run a list of hooks instead
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-08-24 15:01:09
On Wed, Aug 18 2021, Emily Shaffer wrote:
quoted hunk ↗ jump to hunk
@@ -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...
quoted hunk ↗ jump to hunk
@@ -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...
quoted hunk ↗ jump to hunk
-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...
quoted hunk ↗ jump to hunk
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?