| Message ID | 20200909004939.1942347-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | propose config-based hooks | expand |
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@google.com> > --- > 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 >
On Fri, Sep 11, 2020 at 02:27:42PM +0100, Phillip Wood wrote: > > Hi Emily > > > +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 Sure. > > > +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. Oh, that's a good idea - you're absolutely right. I'll do that. > > +/* > > + * 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()`? Yeah, I agree. I'll make that change with the next reroll. Thanks for reading. - Emily
> $ git hook list pre-commit > ~/baz/from/hookcmd.sh > ~/bar.sh In the tests below, there is a "local:" prefix (or similar). It's clearer if the commit message has that too. Also, looking at a later commit, the "list" command probably should include the legacy hook if it exists. > +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); > +} It might be odd to a programmer reading this that an existing "struct hook" with the same name is not reused - the scanning of the list done in hook_config_lookup() could probably go here instead. > +test_expect_success 'git hook list orders by config order' ' > + setup_hooks && > + > + cat >expected <<-EOF && > + global: $ROOT/path/def > + local: $ROOT/path/ghi Will the "global" strings etc. be translated? If yes, it's probably not worth it to align the paths in this way.
Hi Emily, On Wed, 9 Sep 2020 at 02:54, Emily Shaffer <emilyshaffer@google.com> wrote: > DESCRIPTION > ----------- > You can list, add, and modify hooks with this command. (BTW, I think this patch could teach this to say "You can list hooks with this command." If/when we add the other commands, we can expand on this.) > +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 I propose s/"hook"/`hook`/ and similar to set this as monospace since we are discussing configuration sections. If we want to avoid starting sentences with "hook" (or `hookcmd`; do we?), maybe something like "The section `hook` ..." would work fine. > +particular hook event; commands are run in config order. "hookcmd" is used to "config order" feels a bit too colloquial/vague. You use the same phrase in the commit message and I think it works well there for the indented audience. But for this document, I'm not so sure. How about Commands are run in the order they are encountered as the Git configuration files are processed (see linkgit:git-config[1]). ? It's also quite possible that "config order" hits the exact right tone -- please trust your judgment. > +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: > + [hook "post-commit"] > + command = "linter" > + command = "~/typocheck.sh" > + > + [hookcmd "linter"] > + command = "/bin/linter --c" Hmm. "hook", "command" and "hookcmd". Should that be "cmd", or "hookcommand"? I'd favour the latter, but the current proposal somehow feels asymmetric. (If code uses, and is consistent about using, "hookcmd" that's another thing entirely, I think. It's just that for the configuration, it looks a bit odd.) > +List the hooks which have been configured for <hook-name>. Hooks appear `<hook-name>` with backticks. > +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). I had to read and re-read this a few times. The "and note the" does not mean "and please observe that", but rather "and they make note of". Not sure how that can be done clearer. The second thing that tripped me up was that last part. Maybe end the sentence after "specified", then add something like "The scope is not affected by if and where `hookcmd.<hook-name>.command` appears.". I think you could add CONFIGURATION ------------- include::config/hook.txt[] here and add such a file hook.<hook-name>.command:: ... hookcmd.<hook-name>.command:: ... where you define/describe those items. And you can include it from config.txt as well. Martin
Emily Shaffer wrote: > --- a/Documentation/git-hook.txt > +++ b/Documentation/git-hook.txt > @@ -8,12 +8,47 @@ git-hook - Manage configured hooks [...] > +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). A little bit of futureproofing: this may want to mention that the output is intended to be human-readable and is subject to change over time (scripters beware!). Thanks, Jonathan
On Sun, Sep 27, 2020 at 09:23:35PM +0200, Martin Ågren wrote: > > Hi Emily, Firstly, thanks for the doc review - this is great stuff. > > On Wed, 9 Sep 2020 at 02:54, Emily Shaffer <emilyshaffer@google.com> wrote: > > > DESCRIPTION > > ----------- > > You can list, add, and modify hooks with this command. > > (BTW, I think this patch could teach this to say "You can list hooks > with this command." If/when we add the other commands, we can expand > on this.) Done. I sort of glued this together with Jonathan Nieder's suggestion in the setup patch, and ended up saying "later you will be able to blah". > > > +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 > > I propose s/"hook"/`hook`/ and similar to set this as monospace since we > are discussing configuration sections. If we want to avoid starting > sentences with "hook" (or `hookcmd`; do we?), maybe something like "The > section `hook` ..." would work fine. Nice - done. I don't see much problem with starting a sentence with monospaced lower-cased section name... someone can disagree with me :) > > > +particular hook event; commands are run in config order. "hookcmd" is used to > > "config order" feels a bit too colloquial/vague. You use the same phrase > in the commit message and I think it works well there for the indented > audience. But for this document, I'm not so sure. How about > > Commands are run in the order they are encountered as the Git > configuration files are processed (see linkgit:git-config[1]). I don't mind colloquial - I think that improves the readability of user documentation - but you're right that it's vague. "...commands are run in the order Git encounters them during the configuration parse (see linkgitblah)" seemed like an okay balance to me. > > ? It's also quite possible that "config order" hits the exact right tone > -- please trust your judgment. Nah, I think you're right that "config order" is easily understood by Git devs, but probably not by Git users. I like that linking out to the config doc invites users to also learn a little more about how config files work :) > > > +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: > > > + [hook "post-commit"] > > + command = "linter" > > + command = "~/typocheck.sh" > > + > > + [hookcmd "linter"] > > + command = "/bin/linter --c" > > Hmm. "hook", "command" and "hookcmd". Should that be "cmd", or > "hookcommand"? I'd favour the latter, but the current proposal somehow > feels asymmetric. (If code uses, and is consistent about using, > "hookcmd" that's another thing entirely, I think. It's just that for the > configuration, it looks a bit odd.) I'm not entirely in love with the name "hookcmd" but somehow I like "hookcommand" even less - especially since you end up with "hook.command" referencing a "hookcommand" which also has a "hookcommand.command" - blech. Some possible alternatives to "hookcmd": - hookmodule/hook-module - reusable-hook - hook-with-options/hook-options (nah, this sounds like it means "options for hook execution") - hook-details/detailed-hook - named-hook I'll think on this more. I like "named-hook" quite a lot. Very interested in hearing other ideas - "the two hardest problems in computer science are naming, cache invalidation, and off-by-one errors" ;) > > > +List the hooks which have been configured for <hook-name>. Hooks appear > > `<hook-name>` with backticks. > > > +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). > > I had to read and re-read this a few times. The "and note the" does not > mean "and please observe that", but rather "and they make note of". Not > sure how that can be done clearer. The second thing that tripped me up > was that last part. Maybe end the sentence after "specified", then add > something like "The scope is not affected by if and where > `hookcmd.<hook-name>.command` appears.". Occam's Razor suggests "Hooks appear in the order they should be run, and print the config scope blah". Thanks for pointing out "and note that" collision - I never use that phrase so it didn't occur to me! > > I think you could add > > CONFIGURATION > ------------- > include::config/hook.txt[] > > here and add such a file > > hook.<hook-name>.command:: > ... > > hookcmd.<hook-name>.command:: > ... > > where you define/describe those items. And you can include it from > config.txt as well. Yes, totally. Thanks. - Emily
On Wed, Sep 23, 2020 at 04:04:51PM -0700, Jonathan Tan wrote: > > > $ git hook list pre-commit > > ~/baz/from/hookcmd.sh > > ~/bar.sh > > In the tests below, there is a "local:" prefix (or similar). It's > clearer if the commit message has that too. > > Also, looking at a later commit, the "list" command probably should > include the legacy hook if it exists. Have added it as a separate patch for v5, hopefully that will make more sense. > > > +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); > > +} > > It might be odd to a programmer reading this that an existing "struct > hook" with the same name is not reused - the scanning of the list done > in hook_config_lookup() could probably go here instead. Sure, done. > > > +test_expect_success 'git hook list orders by config order' ' > > + setup_hooks && > > + > > + cat >expected <<-EOF && > > + global: $ROOT/path/def > > + local: $ROOT/path/ghi > > Will the "global" strings etc. be translated? If yes, it's probably not > worth it to align the paths in this way. Asked more offline. Jonathan was saying that translation might result in scope name + tab character leaving the path in different columns depending on the scope anyways, so there's no point in using a tab character instead of a space character here. That seems reasonable; I'll switch. - 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 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); + +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)) + 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 +' + + +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
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 | 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