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

[Emily Shaffer <emilyshaffer@google.com>]

On Sat, Jan 30, 2021 at 08:22:54PM -0800, Jonathan Tan wrote:
> 
> > 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 :-)

Updated.

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

I think it might be good enough to say "may be prepended" instead - the
quoting advice (wrap your paths) still holds.

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

Like in the previous patch, I opted to make it match the design doc
(UNKNOWN matches default, aka YES). I left the typo warning, though, as
it might be useful for someone trying to debug why "itneractive" isn't
working as they expect.

> 
> > +		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?

Hm. At the end of part II of this series "run_hook_ve()" is deleted
entirely; the implementation of "run_hooks" diverges significantly as
the series progresses (supporting stdin/stdout, etc) so I'd rather not
try to keep one of them based on the other, as I think it'll be more
complicated than it seems in this patch.

> 
> > +
> > +		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().

Sure.

> 
> [snip tests]
> 
> The tests look good.

Thanks for the detailed review.

 - Emily