| Message ID | 20200521185414.43760-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | propose config-based hooks | expand |
Hi Emily On 21/05/2020 19:54, 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@google.com> > --- > Documentation/git-hook.txt | 37 +++++++++++++- > Makefile | 1 + > builtin/hook.c | 55 +++++++++++++++++++-- > hook.c | 90 +++++++++++++++++++++++++++++++++++ > hook.h | 15 ++++++ > t/t1360-config-based-hooks.sh | 51 +++++++++++++++++++- > 6 files changed, 242 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" > +---- > + > +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). > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/Makefile b/Makefile > index fce6ee154e..b7bbf3be7b 100644 > --- a/Makefile > +++ b/Makefile > @@ -894,6 +894,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..cfd8e388bd 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 (!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..9dfc1a885e > --- /dev/null > +++ b/hook.c > @@ -0,0 +1,90 @@ > +#include "cache.h" > + > +#include "hook.h" > +#include "config.h" > + > +static LIST_HEAD(hook_head); > + > +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); > + strbuf_addstr(&to_add->command, 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); This looks dodgy to me. This code is called by git_config() as it parses the config files, so it has not had a chance to fully populate the config cache used by git_config_get_value(). I think the test below passes because the hookcmd setting is set in the global file and the hook setting is set in the local file so when we have already parsed the hookcmd setting when we come to look it up. The same comment applies to the hypothetical ordering config mentioned below. I think it would be better to collect the list of hook.<event>.command settings in this callback and then look up any hookcmd settings for those hook commands after we've finished reading all of the config files. > + > + 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)) > + 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; > + > + 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..4e46d7dd4e 100755 > --- a/t/t1360-config-based-hooks.sh > +++ b/t/t1360-config-based-hooks.sh > @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command' > > . ./test-lib.sh > > -test_expect_success 'git hook command does not crash' ' > - git hook > +test_expect_success 'git hook rejects commands without a mode' ' > + test_must_fail git hook pre-commit > +' > + > + > +test_expect_success 'git hook rejects commands without a hookname' ' > + test_must_fail git hook list > +' > + > +test_expect_success 'setup hooks in global, and local' ' > + git config --add --local hook.pre-commit.command "/path/ghi" && Can I make a plea for the use of test_config please. Writing tests which rely on previous tests for their set-up creates a chain of hidden dependencies that make it hard to add/alter tests later or run a subset of the tests when developing a new patch. t3404-rebase-interactive.sh is a prime example of this and I dread touching it. > + git config --add --global hook.pre-commit.command "/path/def" > +' > + > +test_expect_success 'git hook list orders by config order' ' > + cat >expected <<-\EOF && > + global: /path/def > + local: /path/ghi > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'git hook list dereferences a hookcmd' ' > + git config --add --local hook.pre-commit.command "abc" && > + git config --add --global hookcmd.abc.command "/path/abc" && > + > + cat >expected <<-\EOF && > + global: /path/def > + local: /path/ghi > + local: /path/abc We should make it clear in the documentation that the config origin applies to the hook setting, even though we display the hookcmd command which is set globally here for the last hook. Best Wishes Phillip > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'git hook list reorders on duplicate commands' ' > + git config --add --local hook.pre-commit.command "/path/def" && > + > + cat >expected <<-\EOF && > + local: /path/ghi > + local: /path/abc > + local: /path/def > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > ' > > test_done >
Hi Emily, On Thu, 21 May 2020, Emily Shaffer wrote: > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh > index 34b0df5216..4e46d7dd4e 100755 > --- a/t/t1360-config-based-hooks.sh > +++ b/t/t1360-config-based-hooks.sh > @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command' > > . ./test-lib.sh > > -test_expect_success 'git hook command does not crash' ' > - git hook > +test_expect_success 'git hook rejects commands without a mode' ' > + test_must_fail git hook pre-commit > +' > + > + > +test_expect_success 'git hook rejects commands without a hookname' ' > + test_must_fail git hook list > +' > + > +test_expect_success 'setup hooks in global, and local' ' > + git config --add --local hook.pre-commit.command "/path/ghi" && > + git config --add --global hook.pre-commit.command "/path/def" > +' > + > +test_expect_success 'git hook list orders by config order' ' > + cat >expected <<-\EOF && > + global: /path/def > + local: /path/ghi > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual This, as well as the next two test cases, won't work on Windows, as you almost certainly realized from looking at the failed GitHub workflow run of your branch. The reason is that Unix-like absolute paths like `/path/def` do _not_ do what you think on Windows: they are relative to the MSYS2 root (because the shell script runs in an MSYS2 Bash). The Git executable, however, has not the slightest idea about MSYS2 and does not handle those. To remedy that, the MSYS2 Bash prefixes those paths with the absolute _Windows-style_ path when passing them to `git.exe` (in your case, actually in the `setup hooks` test case above). So you will need to squash this (or an equivalent fix) into your patch: -- snip -- From f2568d47509130a9c35590d907797d2eb813ac0d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin <johannes.schindelin@gmx.de> Date: Mon, 25 May 2020 15:03:16 +0200 Subject: [PATCH] fixup??? hook: add list command This is needed to make the tests pass on Windows, where Unix-like absolute paths are not what you think they are. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t1360-config-based-hooks.sh | 39 +++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 3296d8af4587..c862655fd4d9 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -18,10 +18,19 @@ test_expect_success 'setup hooks in global, and local' ' git config --add --global hook.pre-commit.command "/path/def" ' +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 + test_expect_success 'git hook list orders by config order' ' - cat >expected <<-\EOF && - global: /path/def - local: /path/ghi + cat >expected <<-EOF && + global: $ROOT/path/def + local: $ROOT/path/ghi EOF git hook list pre-commit >actual && @@ -32,10 +41,10 @@ test_expect_success 'git hook list dereferences a hookcmd' ' git config --add --local hook.pre-commit.command "abc" && git config --add --global hookcmd.abc.command "/path/abc" && - cat >expected <<-\EOF && - global: /path/def - local: /path/ghi - local: /path/abc + cat >expected <<-EOF && + global: $ROOT/path/def + local: $ROOT/path/ghi + local: $ROOT/path/abc EOF git hook list pre-commit >actual && @@ -45,10 +54,10 @@ test_expect_success 'git hook list dereferences a hookcmd' ' test_expect_success 'git hook list reorders on duplicate commands' ' git config --add --local hook.pre-commit.command "/path/def" && - cat >expected <<-\EOF && - local: /path/ghi - local: /path/abc - local: /path/def + cat >expected <<-EOF && + local: $ROOT/path/ghi + local: $ROOT/path/abc + local: $ROOT/path/def EOF git hook list pre-commit >actual && @@ -56,10 +65,10 @@ test_expect_success 'git hook list reorders on duplicate commands' ' ' test_expect_success 'git hook list --porcelain prints just the command' ' - cat >expected <<-\EOF && - /path/ghi - /path/abc - /path/def + cat >expected <<-EOF && + $ROOT/path/ghi + $ROOT/path/abc + $ROOT/path/def EOF git hook list --porcelain pre-commit >actual && -- 2.27.0.rc1.windows.1 -- snap -- Ciao, Dscho > +' > + > +test_expect_success 'git hook list dereferences a hookcmd' ' > + git config --add --local hook.pre-commit.command "abc" && > + git config --add --global hookcmd.abc.command "/path/abc" && > + > + cat >expected <<-\EOF && > + global: /path/def > + local: /path/ghi > + local: /path/abc > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'git hook list reorders on duplicate commands' ' > + git config --add --local hook.pre-commit.command "/path/def" && > + > + cat >expected <<-\EOF && > + local: /path/ghi > + local: /path/abc > + local: /path/def > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > ' > > test_done > -- > 2.27.0.rc0.183.gde8f92d652-goog > > >
On Mon, May 25, 2020 at 01:00:03AM +0200, Johannes Schindelin wrote: > cc: git@vger.kernel.org > > Hi Emily, > > On Thu, 21 May 2020, Emily Shaffer wrote: > > > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh > > index 34b0df5216..4e46d7dd4e 100755 > > --- a/t/t1360-config-based-hooks.sh > > +++ b/t/t1360-config-based-hooks.sh > > @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command' > > > > . ./test-lib.sh > > > > -test_expect_success 'git hook command does not crash' ' > > - git hook > > +test_expect_success 'git hook rejects commands without a mode' ' > > + test_must_fail git hook pre-commit > > +' > > + > > + > > +test_expect_success 'git hook rejects commands without a hookname' ' > > + test_must_fail git hook list > > +' > > + > > +test_expect_success 'setup hooks in global, and local' ' > > + git config --add --local hook.pre-commit.command "/path/ghi" && > > + git config --add --global hook.pre-commit.command "/path/def" > > +' > > + > > +test_expect_success 'git hook list orders by config order' ' > > + cat >expected <<-\EOF && > > + global: /path/def > > + local: /path/ghi > > + EOF > > + > > + git hook list pre-commit >actual && > > + test_cmp expected actual > > This, as well as the next two test cases, won't work on Windows, as you > almost certainly realized from looking at the failed GitHub workflow run > of your branch. Thanks very much for sending this - to be honest, the failed workflow run appeared to be because of the earlier SDK download issue, which I have not rebased on top of a fix for yet, so I missed any actionable failures when I ran the CI locally. I'll take it into account, much appreciated. - Emily
On Fri, May 22, 2020 at 11:27:44AM +0100, Phillip Wood wrote: > > Hi Emily > > On 21/05/2020 19:54, 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@google.com> > > --- > > Documentation/git-hook.txt | 37 +++++++++++++- > > Makefile | 1 + > > builtin/hook.c | 55 +++++++++++++++++++-- > > hook.c | 90 +++++++++++++++++++++++++++++++++++ > > hook.h | 15 ++++++ > > t/t1360-config-based-hooks.sh | 51 +++++++++++++++++++- > > 6 files changed, 242 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" > > +---- > > + > > +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). > > + > > GIT > > --- > > Part of the linkgit:git[1] suite > > diff --git a/Makefile b/Makefile > > index fce6ee154e..b7bbf3be7b 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -894,6 +894,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..cfd8e388bd 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 (!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..9dfc1a885e > > --- /dev/null > > +++ b/hook.c > > @@ -0,0 +1,90 @@ > > +#include "cache.h" > > + > > +#include "hook.h" > > +#include "config.h" > > + > > +static LIST_HEAD(hook_head); > > + > > +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); > > + strbuf_addstr(&to_add->command, 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); > > This looks dodgy to me. This code is called by git_config() as it parses > the config files, so it has not had a chance to fully populate the > config cache used by git_config_get_value(). I think the test below > passes because the hookcmd setting is set in the global file and the > hook setting is set in the local file so when we have already parsed the > hookcmd setting when we come to look it up. The same comment applies to > the hypothetical ordering config mentioned below. I think it would be > better to collect the list of hook.<event>.command settings in this > callback and then look up any hookcmd settings for those hook commands > after we've finished reading all of the config files. git_config_get_value() calls repo_read_config(the_repository) if the config hasn't been fully parsed yet, so I think what you're worrying about is not an issue. It's ugly, I agree, but since the new hotness (git_config_get_value() and friends) doesn't offer the same functionality as the old solution (config origin) this seemed like an okay approach. As I understand it, moving this hookcmd lookup section outside of the config callback will save us up to one additional pass through the configs, at the expense of a more convoluted code path. > > > + > > + 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)) > > + 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; > > + > > + 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..4e46d7dd4e 100755 > > --- a/t/t1360-config-based-hooks.sh > > +++ b/t/t1360-config-based-hooks.sh > > @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command' > > > > . ./test-lib.sh > > > > -test_expect_success 'git hook command does not crash' ' > > - git hook > > +test_expect_success 'git hook rejects commands without a mode' ' > > + test_must_fail git hook pre-commit > > +' > > + > > + > > +test_expect_success 'git hook rejects commands without a hookname' ' > > + test_must_fail git hook list > > +' > > + > > +test_expect_success 'setup hooks in global, and local' ' > > + git config --add --local hook.pre-commit.command "/path/ghi" && > > Can I make a plea for the use of test_config please. Writing tests which > rely on previous tests for their set-up creates a chain of hidden > dependencies that make it hard to add/alter tests later or run a subset > of the tests when developing a new patch. t3404-rebase-interactive.sh is > a prime example of this and I dread touching it. Sure. I'll redo them. > > > + git config --add --global hook.pre-commit.command "/path/def" > > +' > > + > > +test_expect_success 'git hook list orders by config order' ' > > + cat >expected <<-\EOF && > > + global: /path/def > > + local: /path/ghi > > + EOF > > + > > + git hook list pre-commit >actual && > > + test_cmp expected actual > > +' > > + > > +test_expect_success 'git hook list dereferences a hookcmd' ' > > + git config --add --local hook.pre-commit.command "abc" && > > + git config --add --global hookcmd.abc.command "/path/abc" && > > + > > + cat >expected <<-\EOF && > > + global: /path/def > > + local: /path/ghi > > + local: /path/abc > > We should make it clear in the documentation that the config origin > applies to the hook setting, even though we display the hookcmd command > which is set globally here for the last hook. One of the suggestions from our UX team last week was to make this list output clearer to indicate the origin of the command plus the origin of the hookcmd object; I'll try to straighten this out and make sure the doc agrees. - Emily
Hi Emily sorry it has taken me so long to reply On 09/06/2020 22:49, Emily Shaffer wrote: > On Fri, May 22, 2020 at 11:27:44AM +0100, Phillip Wood wrote: >> >> Hi Emily >> >> On 21/05/2020 19:54, Emily Shaffer wrote: >>> [...] >>> +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); >> >> This looks dodgy to me. This code is called by git_config() as it parses >> the config files, so it has not had a chance to fully populate the >> config cache used by git_config_get_value(). I think the test below >> passes because the hookcmd setting is set in the global file and the >> hook setting is set in the local file so when we have already parsed the >> hookcmd setting when we come to look it up. The same comment applies to >> the hypothetical ordering config mentioned below. I think it would be >> better to collect the list of hook.<event>.command settings in this >> callback and then look up any hookcmd settings for those hook commands >> after we've finished reading all of the config files. > > git_config_get_value() calls repo_read_config(the_repository) if the > config hasn't been fully parsed yet, so I think what you're worrying > about is not an issue. It's ugly, I agree, but since the new hotness > (git_config_get_value() and friends) doesn't offer the same > functionality as the old solution (config origin) this seemed like an > okay approach. As I understand it, moving this hookcmd lookup section > outside of the config callback will save us up to one additional pass > through the configs, at the expense of a more convoluted code path. Oh I didn't realize that, thanks for explaining it. Below you mention showing the origin for hookcmds as well as the origin of the command which would mean having to change this code anyway I think. >> >>> + >>> + 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)) >>> + 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; >>> + >>> + 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..4e46d7dd4e 100755 >>> --- a/t/t1360-config-based-hooks.sh >>> +++ b/t/t1360-config-based-hooks.sh >>> @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command' >>> >>> . ./test-lib.sh >>> >>> -test_expect_success 'git hook command does not crash' ' >>> - git hook >>> +test_expect_success 'git hook rejects commands without a mode' ' >>> + test_must_fail git hook pre-commit >>> +' >>> + >>> + >>> +test_expect_success 'git hook rejects commands without a hookname' ' >>> + test_must_fail git hook list >>> +' >>> + >>> +test_expect_success 'setup hooks in global, and local' ' >>> + git config --add --local hook.pre-commit.command "/path/ghi" && >> >> Can I make a plea for the use of test_config please. Writing tests which >> rely on previous tests for their set-up creates a chain of hidden >> dependencies that make it hard to add/alter tests later or run a subset >> of the tests when developing a new patch. t3404-rebase-interactive.sh is >> a prime example of this and I dread touching it. > > Sure. I'll redo them. That's great, thanks Best Wishes Phillip >> >>> + git config --add --global hook.pre-commit.command "/path/def" >>> +' >>> + >>> +test_expect_success 'git hook list orders by config order' ' >>> + cat >expected <<-\EOF && >>> + global: /path/def >>> + local: /path/ghi >>> + EOF >>> + >>> + git hook list pre-commit >actual && >>> + test_cmp expected actual >>> +' >>> + >>> +test_expect_success 'git hook list dereferences a hookcmd' ' >>> + git config --add --local hook.pre-commit.command "abc" && >>> + git config --add --global hookcmd.abc.command "/path/abc" && >>> + >>> + cat >expected <<-\EOF && >>> + global: /path/def >>> + local: /path/ghi >>> + local: /path/abc >> >> We should make it clear in the documentation that the config origin >> applies to the hook setting, even though we display the hookcmd command >> which is set globally here for the last hook. > > One of the suggestions from our UX team last week was to make this list > output clearer to indicate the origin of the command plus the origin of > the hookcmd object; I'll try to straighten this out and make sure the > doc agrees. > > - Emily >
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" +---- + +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). + GIT --- Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index fce6ee154e..b7bbf3be7b 100644 --- a/Makefile +++ b/Makefile @@ -894,6 +894,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..cfd8e388bd 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 (!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..9dfc1a885e --- /dev/null +++ b/hook.c @@ -0,0 +1,90 @@ +#include "cache.h" + +#include "hook.h" +#include "config.h" + +static LIST_HEAD(hook_head); + +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); + strbuf_addstr(&to_add->command, 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)) + 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; + + 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..4e46d7dd4e 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command' . ./test-lib.sh -test_expect_success 'git hook command does not crash' ' - git hook +test_expect_success 'git hook rejects commands without a mode' ' + test_must_fail git hook pre-commit +' + + +test_expect_success 'git hook rejects commands without a hookname' ' + test_must_fail git hook list +' + +test_expect_success 'setup hooks in global, and local' ' + git config --add --local hook.pre-commit.command "/path/ghi" && + git config --add --global hook.pre-commit.command "/path/def" +' + +test_expect_success 'git hook list orders by config order' ' + cat >expected <<-\EOF && + global: /path/def + local: /path/ghi + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'git hook list dereferences a hookcmd' ' + git config --add --local hook.pre-commit.command "abc" && + git config --add --global hookcmd.abc.command "/path/abc" && + + cat >expected <<-\EOF && + global: /path/def + local: /path/ghi + local: /path/abc + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'git hook list reorders on duplicate commands' ' + git config --add --local hook.pre-commit.command "/path/def" && + + cat >expected <<-\EOF && + local: /path/ghi + local: /path/abc + local: /path/def + EOF + + git hook list pre-commit >actual && + test_cmp expected actual ' test_done
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@google.com> --- Documentation/git-hook.txt | 37 +++++++++++++- Makefile | 1 + builtin/hook.c | 55 +++++++++++++++++++-- hook.c | 90 +++++++++++++++++++++++++++++++++++ hook.h | 15 ++++++ t/t1360-config-based-hooks.sh | 51 +++++++++++++++++++- 6 files changed, 242 insertions(+), 7 deletions(-) create mode 100644 hook.c create mode 100644 hook.h