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?
