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

[Jonathan Tan <jonathantanmy@google.com>]

> 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 :-)

> @@ -64,6 +65,32 @@ in the order they should be run, and print the config scope where the relevant
>  `hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
>  This output is human-readable and the format is subject to change over time.
>  
> +run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] `<hook-name>`::
> +
> +Runs hooks configured for `<hook-name>`, in the same order displayed by `git
> +hook list`. Hooks configured this way are run prepended with `sh -c`, so paths
> +containing special characters or spaces should be wrapped in single quotes:
> +`command = '/my/path with spaces/script.sh' some args`.

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/

> @@ -135,6 +136,56 @@ enum hookdir_opt configured_hookdir_opt(void)
>  	return HOOKDIR_UNKNOWN;
>  }
>  
> +static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
> +{
> +	struct strbuf prompt = STRBUF_INIT;
> +	/*
> +	 * If the path doesn't exist, don't bother adding the empty hook and
> +	 * don't bother checking the config or prompting the user.
> +	 */
> +	if (!path)
> +		return 0;
> +
> +	switch (cfg)
> +	{
> +		case HOOKDIR_NO:
> +			return 0;
> +		case HOOKDIR_UNKNOWN:
> +			fprintf(stderr,
> +				_("Unrecognized value for 'hook.runHookDir'. "
> +				  "Is there a typo? "));
> +			/* FALLTHROUGH */

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.