Hi Emily
On 09/09/2020 01:49, Emily Shaffer wrote:
Teach 'git hook list <hookname>', which checks the known configs in
order to create an ordered list of hooks to run on a given hook event.
Multiple commands can be specified for a given hook by providing
multiple "hook.<hookname>.command = <path-to-hook>" lines. Hooks will be
run in config order. If more properties need to be set on a given hook
in the future, commands can also be specified by providing
"hook.<hookname>.command = <hookcmd-name>", as well as a "[hookcmd
<hookcmd-name>]" subsection; at minimum, this subsection must contain a
"hookcmd.<hookcmd-name>.command = <path-to-hook>" line.
For example:
$ git config --list | grep ^hook
hook.pre-commit.command=baz
hook.pre-commit.command=~/bar.sh
hookcmd.baz.command=~/baz/from/hookcmd.sh
$ git hook list pre-commit
~/baz/from/hookcmd.sh
~/bar.sh
Signed-off-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
---
Documentation/git-hook.txt | 37 +++++++++++-
Makefile | 1 +
builtin/hook.c | 55 ++++++++++++++++--
hook.c | 102 ++++++++++++++++++++++++++++++++++
hook.h | 15 +++++
t/t1360-config-based-hooks.sh | 68 ++++++++++++++++++++++-
6 files changed, 271 insertions(+), 7 deletions(-)
create mode 100644 hook.c
create mode 100644 hook.h
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 2d50c414cc..e458586e96 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,12 +8,47 @@ git-hook - Manage configured hooks
SYNOPSIS
--------
[verse]
-'git hook'
+'git hook' list <hook-name>
DESCRIPTION
-----------
You can list, 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
+particular hook event; commands are run in config order. "hookcmd" is used to
+describe attributes of a specific command. If additional attributes don't need
+to be specified, a command to run can be specified directly in the "hook"
+section; if a "hookcmd" by that name isn't found, Git will attempt to run the
+provided value directly. For example:
+
+Global config
+----
+ [hook "post-commit"]
+ command = "linter"
+ command = "~/typocheck.sh"
+
+ [hookcmd "linter"]
+ command = "/bin/linter --c"
+----
+
+Local config
+----
+ [hook "prepare-commit-msg"]
+ command = "linter"
+ [hook "post-commit"]
+ command = "python ~/run-test-suite.py"
+----
I think it would be helpful to have a couple of lines explaining what
the example configuration sets up
+COMMANDS
+--------
+
+list <hook-name>::
+
+List the hooks which have been configured for <hook-name>. Hooks appear
+in the order they should be run, and note the config scope where the relevant
+`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
Thanks for clarifying that it is the origin of the
hook.<hook-name>.command that is printed. An example of the output of
the config above would be useful I think.
GIT
---
Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 6eee75555e..804de45b16 100644
--- a/Makefile
+++ b/Makefile
@@ -890,6 +890,7 @@ LIB_OBJS += grep.o
LIB_OBJS += hashmap.o
LIB_OBJS += help.o
LIB_OBJS += hex.o
+LIB_OBJS += hook.o
LIB_OBJS += ident.o
LIB_OBJS += interdiff.o
LIB_OBJS += json-writer.o
diff --git a/builtin/hook.c b/builtin/hook.c
index b2bbc84d4d..a0759a4c26 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -1,21 +1,68 @@
#include "cache.h"
#include "builtin.h"
+#include "config.h"
+#include "hook.h"
#include "parse-options.h"
+#include "strbuf.h"
static const char * const builtin_hook_usage[] = {
- N_("git hook"),
+ N_("git hook list <hookname>"),
NULL
};
-int cmd_hook(int argc, const char **argv, const char *prefix)
+static int list(int argc, const char **argv, const char *prefix)
{
- struct option builtin_hook_options[] = {
+ struct list_head *head, *pos;
+ struct hook *item;
+ struct strbuf hookname = STRBUF_INIT;
+
+ struct option list_options[] = {
OPT_END(),
};
- argc = parse_options(argc, argv, prefix, builtin_hook_options,
+ argc = parse_options(argc, argv, prefix, list_options,
builtin_hook_usage, 0);
+ if (argc < 1) {
+ usage_msg_opt("a hookname must be provided to operate on.",
+ builtin_hook_usage, list_options);
+ }
+
+ strbuf_addstr(&hookname, argv[0]);
+
+ head = hook_list(&hookname);
+
+ if (list_empty(head)) {
+ printf(_("no commands configured for hook '%s'\n"),
+ hookname.buf);
+ return 0;
+ }
+
+ list_for_each(pos, head) {
+ item = list_entry(pos, struct hook, list);
+ if (item)
+ printf("%s:\t%s\n",
+ config_scope_name(item->origin),
+ item->command.buf);
+ }
+
+ clear_hook_list();
+ strbuf_release(&hookname);
+
return 0;
}
+
+int cmd_hook(int argc, const char **argv, const char *prefix)
+{
+ struct option builtin_hook_options[] = {
+ OPT_END(),
+ };
+ if (argc < 2)
+ usage_with_options(builtin_hook_usage, builtin_hook_options);
+
+ if (!strcmp(argv[1], "list"))
+ return list(argc - 1, argv + 1, prefix);
+
+ usage_with_options(builtin_hook_usage, builtin_hook_options);
+}
diff --git a/hook.c b/hook.c
new file mode 100644
index 0000000000..b006950eb8
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,102 @@
+#include "cache.h"
+
+#include "hook.h"
+#include "config.h"
+
+/*
+ * NEEDSWORK: a stateful hook_head means we can't run two hook events in the
+ * background at the same time - which might be ok, or might not.
+ *
+ * Maybe it's better to cache a list head per hookname, since we can probably
+ * guess that the hook list won't change during a user-initiated operation. For
+ * now, within list_hooks, call clear_hook_list() at the outset.
+ */
+static LIST_HEAD(hook_head);
I can see a cache might be useful for the sequencer which needs to run
the prepare-msg hook for each commit (it should probably not be running
the post-commit hook but does at the moment) and for am which runs some
hooks for each patch but until then I'm not sure why we need a global
variable here, can't we just declare `hook_head` in `list_hook()`?
+void free_hook(struct hook *ptr)
+{
+ if (ptr) {
+ strbuf_release(&ptr->command);
+ free(ptr);
+ }
+}
+
+static void emplace_hook(struct list_head *pos, const char *command)
+{
+ 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);
+
+ list_add_tail(&to_add->list, pos);
+}
+
+static void remove_hook(struct list_head *to_remove)
+{
+ struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
+ list_del(to_remove);
+ free_hook(hook_to_remove);
+}
+
+void clear_hook_list(void)
+{
+ struct list_head *pos, *tmp;
+ list_for_each_safe(pos, tmp, &hook_head)
+ remove_hook(pos);
+}
+
+static int hook_config_lookup(const char *key, const char *value, void *hook_key_cb)
+{
+ const char *hook_key = hook_key_cb;
+
+ if (!strcmp(key, hook_key)) {
+ const char *command = value;
+ struct strbuf hookcmd_name = STRBUF_INIT;
+ struct list_head *pos = NULL, *tmp = NULL;
+
+ /* Check if a hookcmd with that name exists. */
+ strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
+ git_config_get_value(hookcmd_name.buf, &command);
+
+ if (!command)
+ BUG("git_config_get_value overwrote a string it shouldn't have");
+
+ /*
+ * TODO: implement an option-getting callback, e.g.
+ * get configs by pattern hookcmd.$value.*
+ * for each key+value, do_callback(key, value, cb_data)
+ */
+
+ list_for_each_safe(pos, tmp, &hook_head) {
+ struct hook *hook = list_entry(pos, struct hook, list);
+ /*
+ * The list of hooks to run can be reordered by being redeclared
+ * in the config. Options about hook ordering should be checked
+ * here.
+ */
+ if (0 == strcmp(hook->command.buf, command))
We normally write this as !strcmp(...)
+ remove_hook(pos);
+ }
+ emplace_hook(pos, command);
+ }
+
+ return 0;
+}
+
+struct list_head* hook_list(const struct strbuf* hookname)
+{
+ struct strbuf hook_key = STRBUF_INIT;
+
+ if (!hookname)
+ return NULL;
+
+ /* hook_head is stateful */
+ clear_hook_list();
+
+ strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
+
+ git_config(hook_config_lookup, (void*)hook_key.buf);
+
+ return &hook_head;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 0000000000..aaf6511cff
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,15 @@
+#include "config.h"
+#include "list.h"
+#include "strbuf.h"
+
+struct hook
+{
+ struct list_head list;
+ enum config_scope origin;
+ struct strbuf command;
+};
+
+struct list_head* hook_list(const struct strbuf *hookname);
+
+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 34b0df5216..46d1ed354a 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -4,8 +4,72 @@ test_description='config-managed multihooks, including git-hook command'
. ./test-lib.sh
-test_expect_success 'git hook command does not crash' '
- git hook
+ROOT=
+if test_have_prereq MINGW
+then
+ # In Git for Windows, Unix-like paths work only in shell scripts;
+ # `git.exe`, however, will prefix them with the pseudo root directory
+ # (of the Unix shell). Let's accommodate for that.
+ ROOT="$(cd / && pwd)"
+fi
+
+setup_hooks () {
+ test_config hook.pre-commit.command "/path/ghi" --add
+ test_config_global hook.pre-commit.command "/path/def" --add
+}
+
+setup_hookcmd () {
+ test_config hook.pre-commit.command "abc" --add
+ test_config_global hookcmd.abc.command "/path/abc" --add
+}
+
+test_expect_success 'git hook rejects commands without a mode' '
+ test_must_fail git hook pre-commit
+'
Thanks for changing the tests to be independent of each other
Best Wishes
Phillip
+
+test_expect_success 'git hook rejects commands without a hookname' '
+ test_must_fail git hook list
+'
+
+test_expect_success 'git hook list orders by config order' '
+ setup_hooks &&
+
+ cat >expected <<-EOF &&
+ global: $ROOT/path/def
+ local: $ROOT/path/ghi
+ EOF
+
+ git hook list pre-commit >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'git hook list dereferences a hookcmd' '
+ setup_hooks &&
+ setup_hookcmd &&
+
+ cat >expected <<-EOF &&
+ global: $ROOT/path/def
+ local: $ROOT/path/ghi
+ local: $ROOT/path/abc
+ EOF
+
+ git hook list pre-commit >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'git hook list reorders on duplicate commands' '
+ setup_hooks &&
+
+ test_config hook.pre-commit.command "/path/def" --add &&
+
+ cat >expected <<-EOF &&
+ local: $ROOT/path/ghi
+ local: $ROOT/path/def
+ EOF
+
+ git hook list pre-commit >actual &&
+ test_cmp expected actual
'
test_done
