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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 5, 2020, 23:39 UTC
Message-ID: <20201005233903.GD1393696@google.com>
In-Reply-To: <20200909004939.1942347-7-emilyshaffer@google.com>

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.

[...]

Show 6 quoted lines

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

[...]

> --- a/builtin/hook.c
> +++ b/builtin/hook.c
> @@ -5,9 +5,11 @@

[...]

Show 20 quoted lines

> +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."), ...);

[...]

Show 29 quoted lines

> --- 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, Jonathan