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
