Re: [PATCH v3 1/6] hook: run a list of hooks instead

On Tue, Aug 24, 2021 at 04:56:10PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Wed, Aug 18 2021, Emily Shaffer wrote:

quoted

@@ -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.

quoted

@@ -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

-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

 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