diff mbox series

[v7,05/17] hook: respect hook.runHookDir

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

Commit Message

Emily Shaffer Dec. 22, 2020, 12:02 a.m. UTC
Include hooks specified in the hook directory in the list of hooks to
run. These hooks do need to be treated differently from config-specified
ones - they do not need to run in a shell, and later on may be disabled
or warned about based on a config setting.

Because they are at least as local as the local config, we'll run them
last - to keep the hook execution order from global to local.

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

Notes:
    Newly split into its own commit since v4, and taking place much sooner.
    
    An unfortunate side effect of adding this support *before* the
    hook.runHookDir support is that the labels on the list are not clear -
    because we aren't yet flagging which hooks are from the hookdir versus
    the config. I suppose we could move the addition of that field to the
    struct hook up to this patch, but it didn't make a lot of sense to me to
    do it just for cosmetic purposes.

 Documentation/config/hook.txt |  5 ++++
 builtin/hook.c                | 54 +++++++++++++++++++++++++++++++++--
 hook.c                        | 21 ++++++++++++++
 hook.h                        | 15 ++++++++++
 t/t1360-config-based-hooks.sh | 43 ++++++++++++++++++++++++++++
 5 files changed, 135 insertions(+), 3 deletions(-)

Comments

Jonathan Tan Jan. 31, 2021, 3:35 a.m. UTC | #1
> Include hooks specified in the hook directory in the list of hooks to
> run. These hooks do need to be treated differently from config-specified
> ones - they do not need to run in a shell, and later on may be disabled
> or warned about based on a config setting.
> 
> Because they are at least as local as the local config, we'll run them
> last - to keep the hook execution order from global to local.

This commit message doesn't seem to match the code change. Firstly,
we're teaching hook.runHookDir, not respecting it (since it did not
exist before this commit), and it's about showing it in "list" and not
about running it at all.

Perhaps just "hook: teach hook.runHookDir" as the subject and as the
body:

  For now, this just affects the output of "git hook list". In the
  future, this will affect the behavior of "git hook run" and when Git
  runs hooks before or after its operations.

> +	switch (should_run_hookdir) {
> +		case HOOKDIR_NO:
> +			strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
> +			break;
> +		case HOOKDIR_INTERACTIVE:
> +			strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
> +			break;
> +		case HOOKDIR_WARN:
> +		case HOOKDIR_UNKNOWN:

Hmm...UNKNOWN is the same as WARN? This doesn't agree with what is said
in patch 1:

  +In case this list is expanded in the future, if a value for `hook.runHookDir` is
  +given which Git does not recognize, Git should discard that config entry. For
  +example, if "warn" was specified at system level and "junk" was specified at
  +global level, Git would resolve the value to "warn"; if the only time the config
  +was set was to "junk", Git would use the default value of "yes".

But having said that, I would prefer if Git just errored out in this
case.

The rest looks good.
Emily Shaffer Feb. 9, 2021, 10:31 p.m. UTC | #2
On Sat, Jan 30, 2021 at 07:35:03PM -0800, Jonathan Tan wrote:
> 
> > Include hooks specified in the hook directory in the list of hooks to
> > run. These hooks do need to be treated differently from config-specified
> > ones - they do not need to run in a shell, and later on may be disabled
> > or warned about based on a config setting.
> > 
> > Because they are at least as local as the local config, we'll run them
> > last - to keep the hook execution order from global to local.
> 
> This commit message doesn't seem to match the code change. Firstly,
> we're teaching hook.runHookDir, not respecting it (since it did not
> exist before this commit), and it's about showing it in "list" and not
> about running it at all.

Yeah, thanks for noticing. Now that I'm rereading it, it looks like this
is the old commit message for "include hookdir hook in list". Yikes.
> 
> Perhaps just "hook: teach hook.runHookDir" as the subject and as the
> body:
> 
>   For now, this just affects the output of "git hook list". In the
>   future, this will affect the behavior of "git hook run" and when Git
>   runs hooks before or after its operations.
> 
> > +	switch (should_run_hookdir) {
> > +		case HOOKDIR_NO:
> > +			strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
> > +			break;
> > +		case HOOKDIR_INTERACTIVE:
> > +			strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
> > +			break;
> > +		case HOOKDIR_WARN:
> > +		case HOOKDIR_UNKNOWN:
> 
> Hmm...UNKNOWN is the same as WARN? This doesn't agree with what is said
> in patch 1:
> 
>   +In case this list is expanded in the future, if a value for `hook.runHookDir` is
>   +given which Git does not recognize, Git should discard that config entry. For
>   +example, if "warn" was specified at system level and "junk" was specified at
>   +global level, Git would resolve the value to "warn"; if the only time the config
>   +was set was to "junk", Git would use the default value of "yes".
> 
> But having said that, I would prefer if Git just errored out in this
> case.

Eh, I think I'd still like it to be tolerant. Thanks for keeping me
honest :) I added a test to enforce the tolerant behavior, too.

> 
> The rest looks good.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
index 71449ecbc7..75312754ae 100644
--- a/Documentation/config/hook.txt
+++ b/Documentation/config/hook.txt
@@ -7,3 +7,8 @@  hookcmd.<name>.command::
 	A command to execute during a hook for which <name> has been specified
 	as a command. This can be an executable on your device or a oneliner for
 	your shell. See linkgit:git-hook[1].
+
+hook.runHookDir::
+	Controls how hooks contained in your hookdir are executed. Can be any of
+	"yes", "warn", "interactive", or "no". Defaults to "yes". See
+	linkgit:git-hook[1] and linkgit:git-config[1] "core.hooksPath").
diff --git a/builtin/hook.c b/builtin/hook.c
index a0013ae4d7..d087e6f5b0 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -11,6 +11,8 @@  static const char * const builtin_hook_usage[] = {
 	NULL
 };
 
+static enum hookdir_opt should_run_hookdir;
+
 static int list(int argc, const char **argv, const char *prefix)
 {
 	struct list_head *head, *pos;
@@ -41,6 +43,26 @@  static int list(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
+	switch (should_run_hookdir) {
+		case HOOKDIR_NO:
+			strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
+			break;
+		case HOOKDIR_INTERACTIVE:
+			strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
+			break;
+		case HOOKDIR_WARN:
+		case HOOKDIR_UNKNOWN:
+			strbuf_addstr(&hookdir_annotation, _(" (will warn)"));
+			break;
+		case HOOKDIR_YES:
+		/*
+		 * The default behavior should agree with
+		 * hook.c:configured_hookdir_opt().
+		 */
+		default:
+			break;
+	}
+
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct hook, list);
 		if (item) {
@@ -64,16 +86,42 @@  static int list(int argc, const char **argv, const char *prefix)
 
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
+	const char *run_hookdir = NULL;
+
 	struct option builtin_hook_options[] = {
+		OPT_STRING(0, "run-hookdir", &run_hookdir, N_("option"),
+			   N_("what to do with hooks found in the hookdir")),
 		OPT_END(),
 	};
-	if (argc < 2)
+
+	argc = parse_options(argc, argv, prefix, builtin_hook_options,
+			     builtin_hook_usage, 0);
+
+	/* after the parse, we should have "<command> <hookname> <args...>" */
+	if (argc < 1)
 		usage_with_options(builtin_hook_usage, builtin_hook_options);
 
 	git_config(git_default_config, NULL);
 
-	if (!strcmp(argv[1], "list"))
-		return list(argc - 1, argv + 1, prefix);
+
+	/* argument > config */
+	if (run_hookdir)
+		if (!strcmp(run_hookdir, "no"))
+			should_run_hookdir = HOOKDIR_NO;
+		else if (!strcmp(run_hookdir, "yes"))
+			should_run_hookdir = HOOKDIR_YES;
+		else if (!strcmp(run_hookdir, "warn"))
+			should_run_hookdir = HOOKDIR_WARN;
+		else if (!strcmp(run_hookdir, "interactive"))
+			should_run_hookdir = HOOKDIR_INTERACTIVE;
+		else
+			die(_("'%s' is not a valid option for --run-hookdir "
+			      "(yes, warn, interactive, no)"), run_hookdir);
+	else
+		should_run_hookdir = configured_hookdir_opt();
+
+	if (!strcmp(argv[0], "list"))
+		return list(argc, argv, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index ffbdcfd987..ed52e85159 100644
--- a/hook.c
+++ b/hook.c
@@ -97,6 +97,27 @@  static int hook_config_lookup(const char *key, const char *value, void *cb_data)
 	return 0;
 }
 
+enum hookdir_opt configured_hookdir_opt(void)
+{
+	const char *key;
+	if (git_config_get_value("hook.runhookdir", &key))
+		return HOOKDIR_YES; /* by default, just run it. */
+
+	if (!strcmp(key, "no"))
+		return HOOKDIR_NO;
+
+	if (!strcmp(key, "yes"))
+		return HOOKDIR_YES;
+
+	if (!strcmp(key, "warn"))
+		return HOOKDIR_WARN;
+
+	if (!strcmp(key, "interactive"))
+		return HOOKDIR_INTERACTIVE;
+
+	return HOOKDIR_UNKNOWN;
+}
+
 struct list_head* hook_list(const struct strbuf* hookname)
 {
 	struct strbuf hook_key = STRBUF_INIT;
diff --git a/hook.h b/hook.h
index 5750634c83..ccdf6272f2 100644
--- a/hook.h
+++ b/hook.h
@@ -21,6 +21,21 @@  struct hook
  */
 struct list_head* hook_list(const struct strbuf *hookname);
 
+enum hookdir_opt
+{
+	HOOKDIR_NO,
+	HOOKDIR_WARN,
+	HOOKDIR_INTERACTIVE,
+	HOOKDIR_YES,
+	HOOKDIR_UNKNOWN,
+};
+
+/*
+ * Provides the hookdir_opt specified in the config without consulting any
+ * command line arguments.
+ */
+enum hookdir_opt configured_hookdir_opt(void);
+
 /* Free memory associated with a 'struct hook' */
 void free_hook(struct hook *ptr);
 /* Empties the list at 'head', calling 'free_hook()' on each entry */
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 0f12af4659..91127a50a4 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -104,4 +104,47 @@  test_expect_success 'git hook list shows hooks from the hookdir' '
 	test_cmp expected actual
 '
 
+test_expect_success 'hook.runHookDir = no is respected by list' '
+	setup_hookdir &&
+
+	test_config hook.runHookDir "no" &&
+
+	cat >expected <<-EOF &&
+	hookdir: $(pwd)/.git/hooks/pre-commit (will not run)
+	EOF
+
+	git hook list pre-commit >actual &&
+	# the hookdir annotation is translated
+	test_i18ncmp expected actual
+'
+
+test_expect_success 'hook.runHookDir = warn is respected by list' '
+	setup_hookdir &&
+
+	test_config hook.runHookDir "warn" &&
+
+	cat >expected <<-EOF &&
+	hookdir: $(pwd)/.git/hooks/pre-commit (will warn)
+	EOF
+
+	git hook list pre-commit >actual &&
+	# the hookdir annotation is translated
+	test_i18ncmp expected actual
+'
+
+
+test_expect_success 'hook.runHookDir = interactive is respected by list' '
+	setup_hookdir &&
+
+	test_config hook.runHookDir "interactive" &&
+
+	cat >expected <<-EOF &&
+	hookdir: $(pwd)/.git/hooks/pre-commit (will prompt)
+	EOF
+
+	git hook list pre-commit >actual &&
+	# the hookdir annotation is translated
+	test_i18ncmp expected actual
+'
+
 test_done