diff mbox series

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

Message ID 20200909004939.1942347-7-emilyshaffer@google.com
State New
Headers show
Series propose config-based hooks | expand

Commit Message

Emily Shaffer Sept. 9, 2020, 12:49 a.m. UTC
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 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.

Users may wish to provide hook commands like 'git config
hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this, the
contents of the 'hook.*.command' and 'hookcmd.*.command' strings are
first split by space or quotes into an argv_array, then expanded with
'expand_user_path()'.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 builtin/hook.c                | 30 ++++++++++++++++++++
 hook.c                        | 52 ++++++++++++++++++++++++++++++++---
 hook.h                        |  3 ++
 t/t1360-config-based-hooks.sh | 28 +++++++++++++++++++
 4 files changed, 109 insertions(+), 4 deletions(-)

Comments

Phillip Wood Sept. 11, 2020, 1:30 p.m. UTC | #1
Hi Emily

On 09/09/2020 01:49, 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.
> 
> For now, the hook commands will 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.
> 
> Users may wish to provide hook commands like 'git config
> hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this, the
> contents of the 'hook.*.command' and 'hookcmd.*.command' strings are
> first split by space or quotes into an argv_array, then expanded with
> 'expand_user_path()'.
> 
 > [...]
> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> index ebf8f38d68..ee8114250d 100755
> --- a/t/t1360-config-based-hooks.sh
> +++ b/t/t1360-config-based-hooks.sh
> @@ -84,4 +84,32 @@ test_expect_success 'git hook list --porcelain prints just the command' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success 'inline hook definitions execute oneliners' '
> +	test_config hook.pre-commit.command "echo \"Hello World\"" &&
> +
> +	echo "Hello World" >expected &&
> +
> +	# hooks are run with stdout_to_stderr = 1
> +	git hook run pre-commit 2>actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'inline hook definitions resolve paths' '
> +	cat >~/sample-hook.sh <<-EOF &&
> +	echo \"Sample Hook\"
> +	EOF

I think this could use `write_script`. I'm rather scared of the '~' in 
the script path, can we write it to the test directory please.

Best Wishes

Phillip

> +	test_when_finished "rm ~/sample-hook.sh" &&
> +
> +	chmod +x ~/sample-hook.sh &&
> +
> +	test_config hook.pre-commit.command "~/sample-hook.sh" &&
> +
> +	echo \"Sample Hook\" >expected &&
> +
> +	# hooks are run with stdout_to_stderr = 1
> +	git hook run pre-commit 2>actual &&
> +	test_cmp expected actual
> +'
> +
>   test_done
>
Josh Steadmon Sept. 28, 2020, 7:29 p.m. UTC | #2
On 2020.09.08 17:49, 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.
> 
> For now, the hook commands will in config order, in series. As alternate

Looks like a small typo here:
s/will in config order/will run in config order/
Jonathan Nieder Oct. 5, 2020, 11:39 p.m. UTC | #3
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.

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

[...]
> --- 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
Emily Shaffer Oct. 6, 2020, 10:57 p.m. UTC | #4
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
diff mbox series

Patch

diff --git a/builtin/hook.c b/builtin/hook.c
index 0d92124ca6..a8f8b03699 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -5,9 +5,11 @@ 
 #include "hook.h"
 #include "parse-options.h"
 #include "strbuf.h"
+#include "strvec.h"
 
 static const char * const builtin_hook_usage[] = {
 	N_("git hook list <hookname>"),
+	N_("git hook run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] <hookname>"),
 	NULL
 };
 
@@ -62,6 +64,32 @@  static int list(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+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);
+
+	strbuf_addstr(&hookname, argv[0]);
+
+	return run_hooks(envs.v, &hookname, &args);
+}
+
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
 	struct option builtin_hook_options[] = {
@@ -72,6 +100,8 @@  int cmd_hook(int argc, const char **argv, const char *prefix)
 
 	if (!strcmp(argv[1], "list"))
 		return list(argc - 1, argv + 1, prefix);
+	if (!strcmp(argv[1], "run"))
+		return run(argc - 1, argv + 1, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index b006950eb8..0dab981681 100644
--- 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);
 }
@@ -78,7 +81,7 @@  static int hook_config_lookup(const char *key, const char *value, void *hook_key
 			if (0 == strcmp(hook->command.buf, command))
 				remove_hook(pos);
 		}
-		emplace_hook(pos, command);
+		emplace_hook(pos, command, 0);
 	}
 
 	return 0;
@@ -87,6 +90,7 @@  static int hook_config_lookup(const char *key, const char *value, void *hook_key
 struct list_head* hook_list(const struct strbuf* hookname)
 {
 	struct strbuf hook_key = STRBUF_INIT;
+	const char *legacy_hook_path = NULL;
 
 	if (!hookname)
 		return NULL;
@@ -98,5 +102,45 @@  struct list_head* hook_list(const struct strbuf* hookname)
 
 	git_config(hook_config_lookup, (void*)hook_key.buf);
 
+	legacy_hook_path = find_hook(hookname->buf);
+
+	/* TODO: check hook.runHookDir */
+	if (legacy_hook_path)
+		emplace_hook(&hook_head, legacy_hook_path, 1);
+
 	return &hook_head;
 }
+
+int run_hooks(const char *const *env, const struct strbuf *hookname,
+	      const struct strvec *args)
+{
+	struct list_head *to_run, *pos = NULL, *tmp = NULL;
+	int rc = 0;
+
+	to_run = hook_list(hookname);
+
+	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);
+
+		/* 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
+		 */
+		if (args)
+			strvec_pushv(&hook_proc.args, args->v);
+
+		hook_proc.env = env;
+		hook_proc.no_stdin = 1;
+		hook_proc.stdout_to_stderr = 1;
+		hook_proc.trace2_hook_name = hook->command.buf;
+		hook_proc.use_shell = 1;
+
+		rc |= run_command(&hook_proc);
+	}
+
+	return rc;
+}
diff --git a/hook.h b/hook.h
index aaf6511cff..d020788a6b 100644
--- a/hook.h
+++ b/hook.h
@@ -1,6 +1,7 @@ 
 #include "config.h"
 #include "list.h"
 #include "strbuf.h"
+#include "strvec.h"
 
 struct hook
 {
@@ -10,6 +11,8 @@  struct hook
 };
 
 struct list_head* hook_list(const struct strbuf *hookname);
+int run_hooks(const char *const *env, const struct strbuf *hookname,
+	      const struct strvec *args);
 
 void free_hook(struct hook *ptr);
 void clear_hook_list(void);
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index ebf8f38d68..ee8114250d 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -84,4 +84,32 @@  test_expect_success 'git hook list --porcelain prints just the command' '
 	test_cmp expected actual
 '
 
+test_expect_success 'inline hook definitions execute oneliners' '
+	test_config hook.pre-commit.command "echo \"Hello World\"" &&
+
+	echo "Hello World" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'inline hook definitions resolve paths' '
+	cat >~/sample-hook.sh <<-EOF &&
+	echo \"Sample Hook\"
+	EOF
+
+	test_when_finished "rm ~/sample-hook.sh" &&
+
+	chmod +x ~/sample-hook.sh &&
+
+	test_config hook.pre-commit.command "~/sample-hook.sh" &&
+
+	echo \"Sample Hook\" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
 test_done