Re: [PATCH v4 6/9] hook: add 'run' subcommand
From: Jonathan Nieder <hidden>
Date: 2020-10-05 23:39:08
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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- a/builtin/hook.c +++ b/builtin/hook.c@@ -5,9 +5,11 @@
[...]
quoted hunk ↗ jump to hunk
+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."), ...);
[...]quoted hunk ↗ jump to hunk
--- 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