diff mbox series

[v9,07/37] hook: add 'run' subcommand

Message ID 20210527000856.695702-8-emilyshaffer@google.com (mailing list archive)
State New, archived
Headers show
Series propose config-based hooks | expand

Commit Message

Emily Shaffer May 27, 2021, 12:08 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 run 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. They can be disabled, or made to print warnings, or to
prompt before running, with the 'hook.runHookDir' config.

Users may wish to provide hook commands like 'git config
hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this,
config-defined hooks are run in a shell. (Since hooks in $GITDIR/hooks
can't be specified with included arguments or paths which need expansion
like this, they are run without a shell instead.)

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---

Notes:
    Since v7, added support for "error" hook.runHookDir setting.
    
    Since v4, updated the docs, and did less local application of single
    quotes. In order for hookdir hooks to run successfully with a space in
    the path, though, they must not be run with 'sh -c'. So we can treat the
    hookdir hooks specially, and warn users via doc about special
    considerations for configured hooks with spaces in their path.

 Documentation/git-hook.txt    |  31 +++++++-
 builtin/hook.c                |  42 ++++++++++-
 hook.c                        | 137 +++++++++++++++++++++++++++++++++-
 hook.h                        |  26 ++++++-
 t/t1360-config-based-hooks.sh |  96 +++++++++++++++++++++++-
 5 files changed, 320 insertions(+), 12 deletions(-)

Comments

Ævar Arnfjörð Bjarmason June 3, 2021, 9:07 a.m. UTC | #1
On Wed, May 26 2021, Emily Shaffer wrote:

> +void run_hooks_opt_init(struct run_hooks_opt *o)
> +{
> +	strvec_init(&o->env);
> +	strvec_init(&o->args);
> +	o->run_hookdir = configured_hookdir_opt();
> +}

I suggested in
https://lore.kernel.org/git/87y2bs7gyc.fsf@evledraar.gmail.com/ that
this could and should be a RUN_HOOKS_OPT_INIT

After some digging I see that was the case in an earlier version of your
series, i.e. before:
https://lore.kernel.org/git/20210131042254.1032233-1-jonathantanmy@google.com/

You came up with this current pattern because of
configured_hookdir_opt().

But a better option here is to use a RUN_HOOKS_OPT_INIT still and just
defer the initialization of this "run_hookdir" member. I.e. set it to
"we have not asked the config yet" in the initializer. I.e. the diff on
top your series at the end of this E-Mail[1].

That along with doing the same for the "jobs" member means we can move
back to a RUN_HOOKS_OPT_INIT, and also that the final version of this
function in this series, i.e.:
	
	void run_hooks_opt_init_sync(struct run_hooks_opt *o)
	{
		strvec_init(&o->env);
		strvec_init(&o->args);
		o->path_to_stdin = NULL;
		o->run_hookdir = HOOKDIR_UNINITIALIZED;
		o->jobs = 1;
		o->dir = NULL;
		o->feed_pipe = NULL;
		o->feed_pipe_ctx = NULL;
		o->consume_sideband = NULL;
	}

Is now mostly redundant to a designated initializer. I.e. you don't need
to NULL any of these out anymore. Then either don't set "jobs" and have
"0" mean "ask config" or set it to "-1" or whatever for "uninitialized".

1.

diff --git a/hook.c b/hook.c
index ff80e52eddd..daf3ddcc188 100644
--- a/hook.c
+++ b/hook.c
@@ -154,7 +154,7 @@ int configured_hook_jobs(void)
 	return n;
 }
 
-static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
+static int should_include_hookdir(struct run_hooks_opt *options, const char *path)
 {
 	struct strbuf prompt = STRBUF_INIT;
 	/*
@@ -164,7 +164,10 @@ static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
 	if (!path)
 		return 0;
 
-	switch (cfg)
+	if (options->run_hookdir == HOOKDIR_UNINITIALIZED)
+		options->run_hookdir = configured_hookdir_opt();
+
+	switch (options->run_hookdir)
 	{
 	case HOOKDIR_ERROR:
 		fprintf(stderr, _("Skipping legacy hook at '%s'\n"),
@@ -292,7 +295,7 @@ void run_hooks_opt_init_sync(struct run_hooks_opt *o)
 	strvec_init(&o->env);
 	strvec_init(&o->args);
 	o->path_to_stdin = NULL;
-	o->run_hookdir = configured_hookdir_opt();
+	o->run_hookdir = HOOKDIR_UNINITIALIZED;
 	o->jobs = 1;
 	o->dir = NULL;
 	o->feed_pipe = NULL;
@@ -312,6 +315,9 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
 	struct strbuf hook_key = STRBUF_INIT;
 	int could_run_hookdir;
 
+	if (should_run_hookdir != HOOKDIR_USE_CONFIG)
+		BUG("no callers want !HOOKDIR_USE_CONFIG?");
+
 	if (should_run_hookdir == HOOKDIR_USE_CONFIG)
 		should_run_hookdir = configured_hookdir_opt();
 
@@ -459,7 +465,7 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
 		struct hook *hook = list_entry(pos, struct hook, list);
 
 		if (hook->from_hookdir &&
-		    !should_include_hookdir(hook->command.buf, options->run_hookdir))
+		    !should_include_hookdir(options, hook->command.buf))
 			    list_del(pos);
 	}
 
diff --git a/hook.h b/hook.h
index f32189380a8..3c4491a74e7 100644
--- a/hook.h
+++ b/hook.h
@@ -30,6 +30,7 @@ struct list_head* hook_list(const char *hookname);
 
 enum hookdir_opt
 {
+	HOOKDIR_UNINITIALIZED,
 	HOOKDIR_USE_CONFIG,
 	HOOKDIR_NO,
 	HOOKDIR_ERROR,
Junio C Hamano June 3, 2021, 10:29 p.m. UTC | #2
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, May 26 2021, Emily Shaffer wrote:
>
>> +void run_hooks_opt_init(struct run_hooks_opt *o)
>> +{
>> +	strvec_init(&o->env);
>> +	strvec_init(&o->args);
>> +	o->run_hookdir = configured_hookdir_opt();
>> +}
>
> I suggested in
> https://lore.kernel.org/git/87y2bs7gyc.fsf@evledraar.gmail.com/ that
> this could and should be a RUN_HOOKS_OPT_INIT
>
> After some digging I see that was the case in an earlier version of your
> series, i.e. before:
> https://lore.kernel.org/git/20210131042254.1032233-1-jonathantanmy@google.com/
>
> You came up with this current pattern because of
> configured_hookdir_opt().
>
> But a better option here is to use a RUN_HOOKS_OPT_INIT still and just
> defer the initialization of this "run_hookdir" member. I.e. set it to
> "we have not asked the config yet" in the initializer. I.e. the diff on
> top your series at the end of this E-Mail[1].

When I compared the result of applying your 31-patch series to
2.32-rc2 and the result of rebasing this series on the same base,
before sending out a responce to Emily's reaction, I found that the
31-patch series did a good job of not stepping on the "hook defined
with configuration" part and concentrated on providing a clean base
to build on with a better structure in the series, and there weren't
many changes that overlapped with Emily's series in a significant
way.  The above was one of the "overlapping" differences that stood
out.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index c84520cb38..8f96c347ea 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -9,11 +9,12 @@  SYNOPSIS
 --------
 [verse]
 'git hook' list <hook-name>
+'git hook' run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] <hook-name>
 
 DESCRIPTION
 -----------
-You can list configured hooks with this command. Later, you will be able to run,
-add, and modify hooks with this command.
+You can list and run configured hooks with this command. Later, you will be able
+to add and modify hooks with this command.
 
 This command parses the default configuration files for sections `hook` and
 `hookcmd`. `hook` is used to describe the commands which will be run during a
@@ -97,6 +98,32 @@  in the order they should be run, and print the config scope where the relevant
 `hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
 This output is human-readable and the format is subject to change over time.
 
+run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] `<hook-name>`::
+
+Runs hooks configured for `<hook-name>`, in the same order displayed by `git
+hook list`. Hooks configured this way may be run prepended with `sh -c`, so
+paths containing special characters or spaces should be wrapped in single
+quotes: `command = '/my/path with spaces/script.sh' some args`.
+
+OPTIONS
+-------
+--run-hookdir::
+	Overrides the hook.runHookDir config. Must be 'yes', 'warn',
+	'interactive', or 'no'. Specifies how to handle hooks located in the Git
+	hook directory (core.hooksPath).
+
+-a::
+--arg::
+	Only valid for `run`.
++
+Specify arguments to pass to every hook that is run.
+
+-e::
+--env::
+	Only valid for `run`.
++
+Specify environment variables to set for every hook that is run.
+
 CONFIGURATION
 -------------
 include::config/hook.txt[]
diff --git a/builtin/hook.c b/builtin/hook.c
index b1e63a9576..4673c9091c 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -4,9 +4,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
 };
 
@@ -98,6 +100,40 @@  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 run_hooks_opt opt;
+	int rc = 0;
+
+	struct option run_options[] = {
+		OPT_STRVEC('e', "env", &opt.env, N_("var"),
+			   N_("environment variables for hook to use")),
+		OPT_STRVEC('a', "arg", &opt.args, N_("args"),
+			   N_("argument to pass to hook")),
+		OPT_END(),
+	};
+
+	run_hooks_opt_init(&opt);
+
+	argc = parse_options(argc, argv, prefix, run_options,
+			     builtin_hook_usage, 0);
+
+	if (argc < 1)
+		usage_msg_opt(_("You must specify a hook event to run."),
+			      builtin_hook_usage, run_options);
+
+	strbuf_addstr(&hookname, argv[0]);
+	opt.run_hookdir = should_run_hookdir;
+
+	rc = run_hooks(hookname.buf, &opt);
+
+	strbuf_release(&hookname);
+	run_hooks_opt_clear(&opt);
+
+	return rc;
+}
+
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
 	const char *run_hookdir = NULL;
@@ -109,10 +145,10 @@  int cmd_hook(int argc, const char **argv, const char *prefix)
 	};
 
 	argc = parse_options(argc, argv, prefix, builtin_hook_options,
-			     builtin_hook_usage, 0);
+			     builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);
 
 	/* after the parse, we should have "<command> <hookname> <args...>" */
-	if (argc < 1)
+	if (argc < 2)
 		usage_with_options(builtin_hook_usage, builtin_hook_options);
 
 	git_config(git_default_config, NULL);
@@ -142,6 +178,8 @@  int cmd_hook(int argc, const char **argv, const char *prefix)
 
 	if (!strcmp(argv[0], "list"))
 		return list(argc, argv, prefix);
+	if (!strcmp(argv[0], "run"))
+		return run(argc, argv, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index 65cbad8dba..b631da659b 100644
--- a/hook.c
+++ b/hook.c
@@ -3,13 +3,13 @@ 
 #include "hook.h"
 #include "config.h"
 #include "run-command.h"
+#include "prompt.h"
 
 void free_hook(struct hook *ptr)
 {
-	if (ptr) {
+	if (ptr)
 		strbuf_release(&ptr->command);
-		free(ptr);
-	}
+	free(ptr);
 }
 
 static struct hook * find_hook_by_command(struct list_head *head, const char *command)
@@ -143,6 +143,70 @@  enum hookdir_opt configured_hookdir_opt(void)
 	return HOOKDIR_UNKNOWN;
 }
 
+static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
+{
+	struct strbuf prompt = STRBUF_INIT;
+	/*
+	 * If the path doesn't exist, don't bother adding the empty hook and
+	 * don't bother checking the config or prompting the user.
+	 */
+	if (!path)
+		return 0;
+
+	switch (cfg)
+	{
+	case HOOKDIR_ERROR:
+		fprintf(stderr, _("Skipping legacy hook at '%s'\n"),
+			path);
+		/* FALLTHROUGH */
+	case HOOKDIR_NO:
+		return 0;
+	case HOOKDIR_WARN:
+		fprintf(stderr, _("Running legacy hook at '%s'\n"),
+			path);
+		return 1;
+	case HOOKDIR_INTERACTIVE:
+		do {
+			/*
+			 * TRANSLATORS: Make sure to include [Y] and [n]
+			 * in your translation. Only English input is
+			 * accepted. Default option is "yes".
+			 */
+			fprintf(stderr, _("Run '%s'? [Y/n] "), path);
+			git_read_line_interactively(&prompt);
+			/*
+			 * In case of prompt = '' - that is, user hit enter,
+			 * saying "yes I want the default" - strncasecmp will
+			 * return 0 regardless. So list the default first.
+			 *
+			 * Case insensitively, accept "y", "ye", or "yes" as
+			 * "yes"; accept "n" or "no" as "no".
+			 */
+			if (!strncasecmp(prompt.buf, "yes", prompt.len)) {
+				strbuf_release(&prompt);
+				return 1;
+			} else if (!strncasecmp(prompt.buf, "no", prompt.len)) {
+				strbuf_release(&prompt);
+				return 0;
+			}
+			/* otherwise, we didn't understand the input */
+		} while (prompt.len); /* an empty reply means default (yes) */
+		return 1;
+	/*
+	 * HOOKDIR_UNKNOWN should match the default behavior, but let's
+	 * give a heads up to the user.
+	 */
+	case HOOKDIR_UNKNOWN:
+		fprintf(stderr,
+			_("Unrecognized value for 'hook.runHookDir'. "
+			  "Is there a typo? "));
+		/* FALLTHROUGH */
+	case HOOKDIR_YES:
+	default:
+		return 1;
+	}
+}
+
 struct list_head* hook_list(const char* hookname)
 {
 	struct strbuf hook_key = STRBUF_INIT;
@@ -176,3 +240,70 @@  struct list_head* hook_list(const char* hookname)
 	strbuf_release(&hook_key);
 	return hook_head;
 }
+
+void run_hooks_opt_init(struct run_hooks_opt *o)
+{
+	strvec_init(&o->env);
+	strvec_init(&o->args);
+	o->run_hookdir = configured_hookdir_opt();
+}
+
+void run_hooks_opt_clear(struct run_hooks_opt *o)
+{
+	strvec_clear(&o->env);
+	strvec_clear(&o->args);
+}
+
+static void prepare_hook_cp(const char *hookname, struct hook *hook,
+			    struct run_hooks_opt *options,
+			    struct child_process *cp)
+{
+	if (!hook)
+		return;
+
+	cp->no_stdin = 1;
+	cp->env = options->env.v;
+	cp->stdout_to_stderr = 1;
+	cp->trace2_hook_name = hookname;
+
+	/*
+	 * Commands from the config could be oneliners, but we know
+	 * for certain that hookdir commands are not.
+	 */
+	cp->use_shell = !hook->from_hookdir;
+
+	/* add command */
+	strvec_push(&cp->args, hook->command.buf);
+
+	/*
+	 * add passed-in argv, without expanding - let the user get back
+	 * exactly what they put in
+	 */
+	strvec_pushv(&cp->args, options->args.v);
+}
+
+int run_hooks(const char *hookname, struct run_hooks_opt *options)
+{
+	struct list_head *to_run, *pos = NULL, *tmp = NULL;
+	int rc = 0;
+
+	if (!options)
+		BUG("a struct run_hooks_opt must be provided to run_hooks");
+
+	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);
+
+		if (hook->from_hookdir &&
+		    !should_include_hookdir(hook->command.buf, options->run_hookdir))
+			continue;
+
+		prepare_hook_cp(hookname, hook, options, &hook_proc);
+
+		rc |= run_command(&hook_proc);
+	}
+
+	return rc;
+}
diff --git a/hook.h b/hook.h
index 7f2b2ee8f2..fb5132305f 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 {
 	struct list_head list;
@@ -36,11 +37,30 @@  enum hookdir_opt
  */
 enum hookdir_opt configured_hookdir_opt(void);
 
+struct run_hooks_opt
+{
+	/* Environment vars to be set for each hook */
+	struct strvec env;
+
+	/* Args to be passed to each hook */
+	struct strvec args;
+
+	/*
+	 * How should the hookdir be handled?
+	 * Leave the RUN_HOOKS_OPT_INIT default in most cases; this only needs
+	 * to be overridden if the user can override it at the command line.
+	 */
+	enum hookdir_opt run_hookdir;
+};
+
+void run_hooks_opt_init(struct run_hooks_opt *o);
+void run_hooks_opt_clear(struct run_hooks_opt *o);
+
 /*
- * Provides the hookdir_opt specified in the config without consulting any
- * command line arguments.
+ * Runs all hooks associated to the 'hookname' event in order. Each hook will be
+ * passed 'env' and 'args'.
  */
-enum hookdir_opt configured_hookdir_opt(void);
+int run_hooks(const char *hookname, struct run_hooks_opt *options);
 
 /* Free memory associated with a 'struct hook' */
 void free_hook(struct hook *ptr);
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 33ac27aa97..3dddd41e4f 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -115,7 +115,10 @@  test_expect_success 'hook.runHookDir = no is respected by list' '
 
 	git hook list pre-commit >actual &&
 	# the hookdir annotation is translated
-	test_cmp expected actual
+	test_cmp expected actual &&
+
+	git hook run pre-commit 2>actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'hook.runHookDir = error is respected by list' '
@@ -129,6 +132,13 @@  test_expect_success 'hook.runHookDir = error is respected by list' '
 
 	git hook list pre-commit >actual &&
 	# the hookdir annotation is translated
+	test_cmp expected actual &&
+
+	cat >expected <<-EOF &&
+	Skipping legacy hook at '\''$(pwd)/.git/hooks/pre-commit'\''
+	EOF
+
+	git hook run pre-commit 2>actual &&
 	test_cmp expected actual
 '
 
@@ -143,6 +153,14 @@  test_expect_success 'hook.runHookDir = warn is respected by list' '
 
 	git hook list pre-commit >actual &&
 	# the hookdir annotation is translated
+	test_cmp expected actual &&
+
+	cat >expected <<-EOF &&
+	Running legacy hook at '\''$(pwd)/.git/hooks/pre-commit'\''
+	"Legacy Hook"
+	EOF
+
+	git hook run pre-commit 2>actual &&
 	test_cmp expected actual
 '
 
@@ -182,7 +200,7 @@  test_expect_success 'git hook list removes skipped inlined hook' '
 	test_cmp expected actual
 '
 
-test_expect_success 'hook.runHookDir = interactive is respected by list' '
+test_expect_success 'hook.runHookDir = interactive is respected by list and run' '
 	setup_hookdir &&
 
 	test_config hook.runHookDir "interactive" &&
@@ -193,9 +211,83 @@  test_expect_success 'hook.runHookDir = interactive is respected by list' '
 
 	git hook list pre-commit >actual &&
 	# the hookdir annotation is translated
+	test_cmp expected actual &&
+
+	test_write_lines n | git hook run pre-commit 2>actual &&
+	! grep "Legacy Hook" actual &&
+
+	test_write_lines y | git hook run pre-commit 2>actual &&
+	grep "Legacy Hook" 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' '
+	write_script sample-hook.sh <<-EOF &&
+	echo \"Sample Hook\"
+	EOF
+
+	test_when_finished "rm sample-hook.sh" &&
+
+	test_config hook.pre-commit.command "\"$(pwd)/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_expect_success 'git hook run can pass args and env vars' '
+	write_script sample-hook.sh <<-\EOF &&
+	echo $1
+	echo $2
+	echo $TEST_ENV_1
+	echo $TEST_ENV_2
+	EOF
+
+	test_config hook.pre-commit.command "\"$(pwd)/sample-hook.sh\"" &&
+
+	cat >expected <<-EOF &&
+	arg1
+	arg2
+	env1
+	env2
+	EOF
+
+	git hook run --arg arg1 \
+		--env TEST_ENV_1=env1 \
+		-a arg2 \
+		-e TEST_ENV_2=env2 \
+		pre-commit 2>actual &&
+
 	test_cmp expected actual
 '
 
+test_expect_success 'hookdir hook included in git hook run' '
+	setup_hookdir &&
+
+	echo \"Legacy Hook\" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'out-of-repo runs excluded' '
+	setup_hooks &&
+
+	nongit test_must_fail git hook run pre-commit
+'
+
 test_expect_success 'hook.runHookDir is tolerant to unknown values' '
 	setup_hookdir &&