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

On Mon, Oct 05, 2020 at 04:39:03PM -0700, Jonathan Nieder wrote:

Hi,

Emily Shaffer wrote:

quoted

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.

[...]

quoted

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

[...]

quoted

--- a/builtin/hook.c
+++ b/builtin/hook.c

@@ -5,9 +5,11 @@

[...]

quoted

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

[...]

quoted

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