Re: [PATCH v4 6/9] hook: add 'run' subcommand

[Emily Shaffer <emilyshaffer@google.com>]

On Mon, Oct 05, 2020 at 04:39:03PM -0700, Jonathan Nieder wrote:
> 
> Hi,
> 
> Emily Shaffer 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.
> 
> Exciting!
> 
> I would even be tempted to put this earlier in the series: providing a
> "git hook run" command that only supports legacy hooks and then
> improving it from there to support config-based hooks.  This ordering is
> also fine, though.

Oh, interesting! I sort of wish I had started with that ordering... but
now it seems a little unwieldy to switch. I'd probably want to do 100%
of the run_hook_(ve|le) conversions first, in that case, and delete the
old hook API. But at this point I think it would be a pretty large
amount of overhead to switch.

> 
> [...]
> > ---
> >  builtin/hook.c                | 30 ++++++++++++++++++++
> >  hook.c                        | 52 ++++++++++++++++++++++++++++++++---
> >  hook.h                        |  3 ++
> >  t/t1360-config-based-hooks.sh | 28 +++++++++++++++++++
> >  4 files changed, 109 insertions(+), 4 deletions(-)
> 
> Needs docs.

Done

> 
> [...]
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -5,9 +5,11 @@
> [...]
> > +static int run(int argc, const char **argv, const char *prefix)
> > +{
> > +	struct strbuf hookname = STRBUF_INIT;
> > +	struct strvec envs = STRVEC_INIT;
> > +	struct strvec args = STRVEC_INIT;
> > +
> > +	struct option run_options[] = {
> > +		OPT_STRVEC('e', "env", &envs, N_("var"),
> > +			   N_("environment variables for hook to use")),
> > +		OPT_STRVEC('a', "arg", &args, N_("args"),
> > +			   N_("argument to pass to hook")),
> > +		OPT_END(),
> > +	};
> > +
> > +	argc = parse_options(argc, argv, prefix, run_options,
> > +			     builtin_hook_usage, 0);
> > +
> > +	if (argc < 1)
> > +		usage_msg_opt(_("a hookname must be provided to operate on."),
> > +			      builtin_hook_usage, run_options);
> 
> Error message nit: what does it mean to operate on a hookname?
> 
> Perhaps this should allude to the usage string?
> 
> 	usage_msg_opt(_("missing <hookname> parameter"), ...);
> 
> Or to match the conversational approach of commands like "clone":
> 
> 	usage_msg_opt(_("You must specify a hook to run."), ...);
> 

Yeah, I like this one. I noticed the same error (untranslated, even!) is
used for list, so I'll fix that too.

> [...]
> > --- a/hook.c
> > +++ b/hook.c
> > @@ -2,6 +2,7 @@
> >  
> >  #include "hook.h"
> >  #include "config.h"
> > +#include "run-command.h"
> >  
> >  /*
> >   * NEEDSWORK: a stateful hook_head means we can't run two hook events in the
> > @@ -21,13 +22,15 @@ void free_hook(struct hook *ptr)
> >  	}
> >  }
> >  
> > -static void emplace_hook(struct list_head *pos, const char *command)
> > +static void emplace_hook(struct list_head *pos, const char *command, int quoted)
> >  {
> >  	struct hook *to_add = malloc(sizeof(struct hook));
> >  	to_add->origin = current_config_scope();
> >  	strbuf_init(&to_add->command, 0);
> > -	/* even with use_shell, run_command() needs quotes */
> > -	strbuf_addf(&to_add->command, "'%s'", command);
> > +	if (quoted)
> > +		strbuf_addf(&to_add->command, "'%s'", command);
> > +	else
> > +		strbuf_addstr(&to_add->command, command);
> >  
> >  	list_add_tail(&to_add->list, pos);
> >  }
> 
> This would need to use sq_quote_* to be safe, but we can do something
> simpler: if we accumulate parameters in an argv_array passed to
> run_command, then they will be safely passed to the shell without
> triggering expansion.

Thanks. I'll do that - no point in duplicating the work :)

 - Emily