Re: [PATCH v7 08/17] hook: add 'run' subcommand

In order to enable hooks to be run as an external process, by a
standalone Git command, or by tools which wrap Git, provide an external
means to run all configured hook commands for a given hook event.

For now, the hook commands will run in config order, in series. As
alternate ordering or parallelism is supported in the future, we should
add knobs to use those to the command line as well.

As with the legacy hook implementation, all stdout generated by hook
commands is redirected to stderr. Piping from stdin is not yet
supported.

Legacy hooks (those present in $GITDIR/hooks) are run at the end of the
execution list. For now, there is no way to disable them.

Not true anymore now that we have hook.runHookDir :-)

I learned recently that this may not work the way I expect [1], so you
might want to specifically call this out for someone who knows how
run-command and running-with-shell works.

[1] https://lore.kernel.org/git/YAs9pTBsdskC8CPN@coredump.intra.peff.net/ (local)

Same comment (about UNKNOWN and defaulting to WARN instead of YES) as in
one of the previous patches.

+		case HOOKDIR_WARN:
+			fprintf(stderr, _("Running legacy hook at '%s'\n"),
+				path);
+			return 1;
+		case HOOKDIR_INTERACTIVE:
+			do {
+				/*
+				 * TRANSLATORS: Make sure to include [Y] and [n]
+				 * in your translation. Only English input is
+				 * accepted. Default option is "yes".
+				 */
+				fprintf(stderr, _("Run '%s'? [Yn] "), path);
+				git_read_line_interactively(&prompt);
+				strbuf_tolower(&prompt);
+				if (starts_with(prompt.buf, "n")) {
+					strbuf_release(&prompt);
+					return 0;
+				} else if (starts_with(prompt.buf, "y")) {
+					strbuf_release(&prompt);
+					return 1;
+				}
+				/* otherwise, we didn't understand the input */
+			} while (prompt.len); /* an empty reply means "Yes" */
+			strbuf_release(&prompt);
+			return 1;
+		case HOOKDIR_YES:
+		default:
+			return 1;
+	}
+}

[snip]

+int run_hooks(const char *hookname, struct run_hooks_opt *options)
+{
+	struct strbuf hookname_str = STRBUF_INIT;
+	struct list_head *to_run, *pos = NULL, *tmp = NULL;
+	int rc = 0;
+
+	if (!options)
+		BUG("a struct run_hooks_opt must be provided to run_hooks");
+
+	strbuf_addstr(&hookname_str, hookname);
+
+	to_run = hook_list(&hookname_str);
+
+	list_for_each_safe(pos, tmp, to_run) {
+		struct child_process hook_proc = CHILD_PROCESS_INIT;
+		struct hook *hook = list_entry(pos, struct hook, list);
+
+		hook_proc.env = options->env.v;
+		hook_proc.no_stdin = 1;
+		hook_proc.stdout_to_stderr = 1;
+		hook_proc.trace2_hook_name = hook->command.buf;
+		hook_proc.use_shell = 1;

I think this is based on run_hook_ve() in run-command.c - could we
refactor that to avoid duplication of code?

+
+		if (hook->from_hookdir) {
+		    if (!should_include_hookdir(hook->command.buf, options->run_hookdir))
+			continue;
+		    /*
+		     * Commands from the config could be oneliners, but we know
+		     * for certain that hookdir commands are not.
+		     */
+		    hook_proc.use_shell = 0;
+		}
+
+		/* add command */
+		strvec_push(&hook_proc.args, hook->command.buf);
+
+		/*
+		 * add passed-in argv, without expanding - let the user get back
+		 * exactly what they put in
+		 */
+		strvec_pushv(&hook_proc.args, options->args.v);
+
+		rc |= run_command(&hook_proc);
+	}
+
+	return rc;
+}

[snip]

+struct run_hooks_opt
+{
+	/* Environment vars to be set for each hook */
+	struct strvec env;
+
+	/* Args to be passed to each hook */
+	struct strvec args;
+
+	/*
+	 * How should the hookdir be handled?
+	 * Leave the RUN_HOOKS_OPT_INIT default in most cases; this only needs
+	 * to be overridden if the user can override it at the command line.
+	 */
+	enum hookdir_opt run_hookdir;
+};
+
+#define RUN_HOOKS_OPT_INIT  {   		\
+	.env = STRVEC_INIT, 				\
+	.args = STRVEC_INIT, 			\
+	.run_hookdir = configured_hookdir_opt()	\
+}

I don't think we have function invocations in our declarations like
this. Maybe stick to just using run_hooks_opt_init().

[snip tests]

The tests look good.