# [PATCH 0/6] configuration-based hook management 19 messages from 2019-12-10 to 2019-12-12. Participants: Emily Shaffer, Philip Oakley, Junio C Hamano, Bert Wesarg, SZEDER Gábor. Thread: https://gitlist.dev/t/52420 ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 0/6] configuration-based hook management Message-ID: <20191210023335.49987-1-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-1-emilyshaffer%40google.com ``` An implementation of the first piece of the proposal given in lore.kernel.org/git/20191116011125.GG22855@google.com. Teaches a new command, 'git hook', which will someday include 'git hook --add ...', 'git hook --edit ...', and maybe more. For now, just teach it how to check the config files with 'git hook --list ...'. The hooks-to-run list is collected in a new library, hook.o, which can someday reimplement find_hook() or otherwise be invoked to run all hooks for a given hookname (e.g. "pre-commit"). The change to config.[ch] allows us to display a similar scope name to the one a user may use to 'git config --add' or later 'git hook --add' a hook at a certain scope, e.g.: $ git hook --list pre-commit 001 global ~/foo.sh $ git hook --add --global pre-commit 005 ~/bar.sh Added. 001 global ~/foo.sh 005 global ~/bar.sh There are config examples in many of the commit messages in this chain. Before I consider "--list" to be done, I also want to add support to check "hook.runHookDir" and take .git/hooks/* into account. But I wanted us to spend time chewing on the config format for a while before I got too far. It's also very possible (likely, even!) to put this feature behind an experimental flag, which gives us more room to change the config format if we want before the feature is "done". In the discussion thread with brian, I also mentioned a self-paced deprecation of hooks which live in .git/hooks/, which I'm aware some users may not want to follow. However, it occurred to me that we may be able to hide a Git-paced deprecation behind a config macro (since those are new and shiny) which is opt-in, and handles something like: hook.runHookDir = true hook.warnHookDir = false {some months pass, we are sure config-based hooks are working nicely} hook.runHookDir = true hook.warnHookDir = true {so start yelling at users to move away, and wait some more months/years} hook.runHookDir = false {users who have opted into the hookdir phaseout macro are no longer using the hookdir} As it's opt-in (and easily reversible by changing configs) this might be a good middle ground for the "deprecate or not" discussion brian and I had. Thanks. - Emily Emily Shaffer (6): hook: scaffolding for git-hook subcommand config: add string mapping for enum config_scope hook: add --list mode hook: support reordering of hook list hook: remove prior hook with '---' hook: teach --porcelain mode .gitignore | 1 + Documentation/git-hook.txt | 53 ++++++++++++++++++++ Makefile | 2 + builtin.h | 1 + builtin/hook.c | 80 ++++++++++++++++++++++++++++++ config.c | 17 +++++++ config.h | 1 + git.c | 1 + hook.c | 93 +++++++++++++++++++++++++++++++++++ hook.h | 14 ++++++ t/t1360-config-based-hooks.sh | 89 +++++++++++++++++++++++++++++++++ 11 files changed, 352 insertions(+) create mode 100644 Documentation/git-hook.txt create mode 100644 builtin/hook.c create mode 100644 hook.c create mode 100644 hook.h create mode 100755 t/t1360-config-based-hooks.sh -- 2.24.0.393.g34dc348eaf-goog ``` ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 1/6] hook: scaffolding for git-hook subcommand Message-ID: <20191210023335.49987-2-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-2-emilyshaffer%40google.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` Introduce infrastructure for a new subcommand, git-hook, which will be used to ease config-based hook management. This command will handle parsing configs to compose a list of hooks to run for a given event, as well as adding or modifying hook configs in an interactive fashion. Signed-off-by: Emily Shaffer --- .gitignore | 1 + Documentation/git-hook.txt | 19 +++++++++++++++++++ Makefile | 1 + builtin.h | 1 + builtin/hook.c | 21 +++++++++++++++++++++ git.c | 1 + t/t1360-config-based-hooks.sh | 11 +++++++++++ 7 files changed, 55 insertions(+) create mode 100644 Documentation/git-hook.txt create mode 100644 builtin/hook.c create mode 100755 t/t1360-config-based-hooks.sh diff --git a/.gitignore b/.gitignore index 89b3b79c1a..9ef59b9baa 100644 --- a/.gitignore +++ b/.gitignore @@ -74,6 +74,7 @@ /git-grep /git-hash-object /git-help +/git-hook /git-http-backend /git-http-fetch /git-http-push diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt new file mode 100644 index 0000000000..2d50c414cc --- /dev/null +++ b/Documentation/git-hook.txt @@ -0,0 +1,19 @@ +git-hook(1) +=========== + +NAME +---- +git-hook - Manage configured hooks + +SYNOPSIS +-------- +[verse] +'git hook' + +DESCRIPTION +----------- +You can list, add, and modify hooks with this command. + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index 58b92af54b..83263505c0 100644 --- a/Makefile +++ b/Makefile @@ -1074,6 +1074,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o BUILTIN_OBJS += builtin/grep.o BUILTIN_OBJS += builtin/hash-object.o BUILTIN_OBJS += builtin/help.o +BUILTIN_OBJS += builtin/hook.o BUILTIN_OBJS += builtin/index-pack.o BUILTIN_OBJS += builtin/init-db.o BUILTIN_OBJS += builtin/interpret-trailers.o diff --git a/builtin.h b/builtin.h index 5cf5df69f7..d4ca2ac9a5 100644 --- a/builtin.h +++ b/builtin.h @@ -173,6 +173,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix); int cmd_grep(int argc, const char **argv, const char *prefix); int cmd_hash_object(int argc, const char **argv, const char *prefix); int cmd_help(int argc, const char **argv, const char *prefix); +int cmd_hook(int argc, const char **argv, const char *prefix); int cmd_index_pack(int argc, const char **argv, const char *prefix); int cmd_init_db(int argc, const char **argv, const char *prefix); int cmd_interpret_trailers(int argc, const char **argv, const char *prefix); diff --git a/builtin/hook.c b/builtin/hook.c new file mode 100644 index 0000000000..b2bbc84d4d --- /dev/null +++ b/builtin/hook.c @@ -0,0 +1,21 @@ +#include "cache.h" + +#include "builtin.h" +#include "parse-options.h" + +static const char * const builtin_hook_usage[] = { + N_("git hook"), + NULL +}; + +int cmd_hook(int argc, const char **argv, const char *prefix) +{ + struct option builtin_hook_options[] = { + OPT_END(), + }; + + argc = parse_options(argc, argv, prefix, builtin_hook_options, + builtin_hook_usage, 0); + + return 0; +} diff --git a/git.c b/git.c index ce6ab0ece2..c8344b9ab7 100644 --- a/git.c +++ b/git.c @@ -513,6 +513,7 @@ static struct cmd_struct commands[] = { { "grep", cmd_grep, RUN_SETUP_GENTLY }, { "hash-object", cmd_hash_object }, { "help", cmd_help }, + { "hook", cmd_hook, RUN_SETUP }, { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT }, { "init", cmd_init_db }, { "init-db", cmd_init_db }, diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh new file mode 100755 index 0000000000..34b0df5216 --- /dev/null +++ b/t/t1360-config-based-hooks.sh @@ -0,0 +1,11 @@ +#!/bin/bash + +test_description='config-managed multihooks, including git-hook command' + +. ./test-lib.sh + +test_expect_success 'git hook command does not crash' ' + git hook +' + +test_done -- 2.24.0.393.g34dc348eaf-goog ``` ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 2/6] config: add string mapping for enum config_scope Message-ID: <20191210023335.49987-3-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-3-emilyshaffer%40google.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` If a user is interacting with their config files primarily by the 'git config' command, using the location flags (--global, --system, etc) then they may be more interested to see the scope of the config file they are editing, rather than the filepath. Signed-off-by: Emily Shaffer --- config.c | 17 +++++++++++++++++ config.h | 1 + 2 files changed, 18 insertions(+) diff --git a/config.c b/config.c index e7052b3977..a20110e016 100644 --- a/config.c +++ b/config.c @@ -3312,6 +3312,23 @@ enum config_scope current_config_scope(void) return current_parsing_scope; } +const char *config_scope_to_string(enum config_scope scope) +{ + switch (scope) { + case CONFIG_SCOPE_SYSTEM: + return _("system"); + case CONFIG_SCOPE_GLOBAL: + return _("global"); + case CONFIG_SCOPE_REPO: + return _("repo"); + case CONFIG_SCOPE_CMDLINE: + return _("cmdline"); + case CONFIG_SCOPE_UNKNOWN: + default: + return _("unknown"); + } +} + int lookup_config(const char **mapping, int nr_mapping, const char *var) { int i; diff --git a/config.h b/config.h index f0ed464004..612f43acd0 100644 --- a/config.h +++ b/config.h @@ -139,6 +139,7 @@ enum config_scope { }; enum config_scope current_config_scope(void); +const char *config_scope_to_string(enum config_scope); const char *current_config_origin_type(void); const char *current_config_name(void); -- 2.24.0.393.g34dc348eaf-goog ``` ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 3/6] hook: add --list mode Message-ID: <20191210023335.49987-4-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-4-emilyshaffer%40google.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` Teach 'git hook --list ', which checks the known configs in order to create an ordered list of hooks to run on a given hook event. The hook config format is "hook. = :". This paves the way for multiple hook support; hooks should be run in the order specified by the user in the config, and in the case of an order number collision, configuration order should be used (e.g. global hook 004 will run before repo hook 004). For example: $ grep -A2 "\[hook\]" ~/.gitconfig [hook] pre-commit = 001:~/test.sh pre-commit = 999:~/baz.sh $ grep -A1 "\[hook\]" ~/git/.git/config [hook] pre-commit = 900:~/bar.sh $ ./bin-wrappers/git hook --list pre-commit 001 global ~/test.sh 900 repo ~/bar.sh 999 global ~/baz.sh Signed-off-by: Emily Shaffer --- Documentation/git-hook.txt | 17 +++++++- Makefile | 1 + builtin/hook.c | 54 ++++++++++++++++++++++- hook.c | 81 +++++++++++++++++++++++++++++++++++ hook.h | 14 ++++++ t/t1360-config-based-hooks.sh | 43 ++++++++++++++++++- 6 files changed, 206 insertions(+), 4 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..a141884239 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -8,12 +8,27 @@ git-hook - Manage configured hooks SYNOPSIS -------- [verse] -'git hook' +'git hook' -l | --list DESCRIPTION ----------- You can list, add, and modify hooks with this command. +This command parses the default configuration files for lines which look like +"hook. = :", e.g. "hook.pre-commit = +010:/path/to/script.sh". In this way, multiple scripts can be run during a +single hook. Hooks are sorted in ascending order by order number; in the event +of an order number conflict, they are sorted in configuration order. + +OPTIONS +------- + +-l:: +--list:: + List the hooks which have been configured for . Hooks appear + in the order they should be run. Output of this command follows the + format ' '. + GIT --- Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index 83263505c0..21b3a82208 100644 --- a/Makefile +++ b/Makefile @@ -892,6 +892,7 @@ LIB_OBJS += hashmap.o LIB_OBJS += linear-assignment.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..8261302b27 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -1,21 +1,73 @@ #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 "), NULL }; +enum hook_command { + HOOK_NO_COMMAND = 0, + HOOK_LIST, +}; + +static int print_hook_list(const struct strbuf *hookname) +{ + struct list_head *head, *pos; + struct hook *item; + + head = hook_list(hookname); + + list_for_each(pos, head) { + item = list_entry(pos, struct hook, list); + if (item) + printf("%.3d\t%s\t%s\n", item->order, + config_scope_to_string(item->origin), + item->command.buf); + } + + return 0; +} + int cmd_hook(int argc, const char **argv, const char *prefix) { + enum hook_command command = 0; + struct strbuf hookname = STRBUF_INIT; + struct option builtin_hook_options[] = { + OPT_CMDMODE('l', "list", &command, + N_("list scripts which will be run for "), + HOOK_LIST), OPT_END(), }; argc = parse_options(argc, argv, prefix, builtin_hook_options, builtin_hook_usage, 0); + if (argc < 1) { + usage_msg_opt("a hookname must be provided to operate on.", + builtin_hook_usage, builtin_hook_options); + } + + strbuf_addstr(&hookname, "hook."); + strbuf_addstr(&hookname, argv[0]); + + switch(command) { + case HOOK_LIST: + return print_hook_list(&hookname); + break; + default: + usage_msg_opt("no command given.", builtin_hook_usage, + builtin_hook_options); + } + + clear_hook_list(); + strbuf_release(&hookname); + return 0; } diff --git a/hook.c b/hook.c new file mode 100644 index 0000000000..f8d1109084 --- /dev/null +++ b/hook.c @@ -0,0 +1,81 @@ +#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, int order, const char *command) +{ + struct hook *to_add = malloc(sizeof(struct hook)); + to_add->order = order; + 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() +{ + struct list_head *pos, *tmp; + list_for_each_safe(pos, tmp, &hook_head) + remove_hook(pos); +} + +static int check_config_for_hooks(const char *var, const char *value, void *hookname) +{ + struct list_head *pos, *p; + struct hook *item; + const struct strbuf *hookname_strbuf = hookname; + + if (!strcmp(var, hookname_strbuf->buf)) { + int order = 0; + // TODO this is bad - open to overflows + char command[256]; + int added = 0; + if (!sscanf(value, "%d:%s", &order, command)) + die(_("hook config '%s' doesn't match expected format"), + value); + + list_for_each_safe(pos, p, &hook_head) { + item = list_entry(pos, struct hook, list); + + /* + * the new entry should go just before the first entry + * which has a higher order number than it. + */ + if (item->order > order && !added) { + emplace_hook(pos, order, command); + added = 1; + } + } + + if (!added) + emplace_hook(pos, order, command); + } + + return 0; +} + +struct list_head* hook_list(const struct strbuf* hookname) +{ + git_config(check_config_for_hooks, (void*)hookname); + + return &hook_head; +} diff --git a/hook.h b/hook.h new file mode 100644 index 0000000000..104df4c088 --- /dev/null +++ b/hook.h @@ -0,0 +1,14 @@ +#include "config.h" + +struct hook +{ + struct list_head list; + int order; + 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(); diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 34b0df5216..1434051db3 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -4,8 +4,47 @@ 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 system, global, and local' ' + git config --add --global hook.pre-commit "010:/path/def" && + git config --add --global hook.pre-commit "999:/path/uvw" && + + git config --add --local hook.pre-commit "100:/path/ghi" && + git config --add --local hook.pre-commit "990:/path/rst" +' + +test_expect_success 'git hook --list orders by order number' ' + cat >expected <<-\EOF && + 010 global /path/def + 100 repo /path/ghi + 990 repo /path/rst + 999 global /path/uvw + EOF + + git hook --list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'order number collisions resolved in config order' ' + cat >expected <<-\EOF && + 010 global /path/def + 010 repo /path/abc + 100 repo /path/ghi + 990 repo /path/rst + 999 global /path/uvw + EOF + + git config --add --local hook.pre-commit "010:/path/abc" && + git hook --list pre-commit >actual && + test_cmp expected actual ' test_done -- 2.24.0.393.g34dc348eaf-goog ``` ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 4/6] hook: support reordering of hook list Message-ID: <20191210023335.49987-5-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-5-emilyshaffer%40google.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` It's possible that in most cases a user wants to run pre-commit hook 'A', but in exactly one repo that user wants to run pre-commit hook 'A' first instead. Teach 'git hook' to support this by allowing a user to specify a new order number for a hook after the initial hook has been specified. For example: $ grep -A2 "\[hook\]" ~/.gitconfig [hook] pre-commit = 001:~/test.sh pre-commit = 999:~/baz.sh $ grep -A2 "\[hook\]" ~/git/.git/config [hook] pre-commit = 900:~/bar.sh pre-commit = 050:~/baz.sh $ ./bin-wrappers/git hook --list pre-commit 001 global ~/test.sh 050 repo ~/baz.sh 900 repo ~/bar.sh In the above example, '~/baz.sh' is provided in the global config with order position 999. Then, in the local config, that order is overridden to 050. Instead of running ~/baz.sh twice (at order 050 and at order 999), only run it once, in the position specified last in config order. Signed-off-by: Emily Shaffer --- Documentation/git-hook.txt | 8 ++++++++ hook.c | 7 +++++++ t/t1360-config-based-hooks.sh | 14 ++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index a141884239..0f7115f826 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -20,6 +20,14 @@ This command parses the default configuration files for lines which look like single hook. Hooks are sorted in ascending order by order number; in the event of an order number conflict, they are sorted in configuration order. +The order number of a hook can be changed at a more local scope, e.g.: + + git config --add --global hook.pre-commit "001:/foo.sh" + git config --add --local hook.pre-commit "005:/foo.sh" + +When the order number is respecified this way, the previously specified hook +configuration is overridden. + OPTIONS ------- diff --git a/hook.c b/hook.c index f8d1109084..a7dcd18a2e 100644 --- a/hook.c +++ b/hook.c @@ -64,6 +64,13 @@ static int check_config_for_hooks(const char *var, const char *value, void *hook emplace_hook(pos, order, command); added = 1; } + + /* + * if the command already exists, this entry should be + * replacing it. + */ + if (!strcmp(item->command.buf, command)) + remove_hook(pos); } if (!added) diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 1434051db3..1af43ef18d 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -47,4 +47,18 @@ test_expect_success 'order number collisions resolved in config order' ' test_cmp expected actual ' +test_expect_success 'adding a command with a different number reorders list' ' + cat >expected <<-\EOF && + 010 repo /path/abc + 050 repo /path/def + 100 repo /path/ghi + 990 repo /path/rst + 999 global /path/uvw + EOF + + git config --add --local hook.pre-commit "050:/path/def" && + git hook --list pre-commit >actual && + test_cmp expected actual +' + test_done -- 2.24.0.393.g34dc348eaf-goog ``` ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 5/6] hook: remove prior hook with '---' Message-ID: <20191210023335.49987-6-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-6-emilyshaffer%40google.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` It's possible a user may want to run a hook for nearly every repo, except for one. Rather than requiring the user to specify the hook locally in all but one repo, teach 'git hook' how to interpret config lines intended to remove hooks specified earlier during the config parse. This means a user can specify such a hook at the system or global level and override it at the local level. For example: $ grep -A2 "\[hook\]" ~/.gitconfig [hook] pre-commit = 001:~/test.sh pre-commit = 999:~/baz.sh $ grep -A2 "\[hook\]" ~/git/.git/config [hook] pre-commit = 900:~/bar.sh pre-commit = ---:~/baz.sh $ ./bin-wrappers/git hook --list pre-commit 001 global ~/test.sh 900 repo ~/bar.sh Signed-off-by: Emily Shaffer --- Documentation/git-hook.txt | 8 ++++++++ hook.c | 15 ++++++++++----- t/t1360-config-based-hooks.sh | 13 +++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index 0f7115f826..b4a992d43f 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -28,6 +28,14 @@ The order number of a hook can be changed at a more local scope, e.g.: When the order number is respecified this way, the previously specified hook configuration is overridden. +A hook specified at a more global scope can be removed by specifying "---" +instead of an order number, e.g.: + + git config --add --global hook.pre-commit "001:/foo.sh" + git config --add --local hook.pre-commit "---:/foo.sh" + +When the hook is removed in this way, `/foo.sh` will not be run at all. + OPTIONS ------- diff --git a/hook.c b/hook.c index a7dcd18a2e..e7afa140c8 100644 --- a/hook.c +++ b/hook.c @@ -49,9 +49,14 @@ static int check_config_for_hooks(const char *var, const char *value, void *hook // TODO this is bad - open to overflows char command[256]; int added = 0; - if (!sscanf(value, "%d:%s", &order, command)) - die(_("hook config '%s' doesn't match expected format"), - value); + int remove = 0; + if (!sscanf(value, "%d:%s", &order, command)) { + if (sscanf(value, "---:%s", command)) + remove = 1; + else + die(_("hook config '%s' doesn't match expected format"), + value); + } list_for_each_safe(pos, p, &hook_head) { item = list_entry(pos, struct hook, list); @@ -60,7 +65,7 @@ static int check_config_for_hooks(const char *var, const char *value, void *hook * the new entry should go just before the first entry * which has a higher order number than it. */ - if (item->order > order && !added) { + if (item->order > order && !added && !remove) { emplace_hook(pos, order, command); added = 1; } @@ -73,7 +78,7 @@ static int check_config_for_hooks(const char *var, const char *value, void *hook remove_hook(pos); } - if (!added) + if (!added && !remove) emplace_hook(pos, order, command); } diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 1af43ef18d..66e70ae222 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -61,4 +61,17 @@ test_expect_success 'adding a command with a different number reorders list' ' test_cmp expected actual ' +test_expect_success 'remove a command with "---:/path/to/cmd"' ' + cat >expected <<-\EOF && + 010 repo /path/abc + 050 repo /path/def + 100 repo /path/ghi + 990 repo /path/rst + EOF + + git config --add --local hook.pre-commit "---:/path/uvw" && + git hook --list pre-commit >actual && + test_cmp expected actual +' + test_done -- 2.24.0.393.g34dc348eaf-goog ``` ## Emily Shaffer, 2019-12-10 02:33 Subject: [PATCH 6/6] hook: teach --porcelain mode Message-ID: <20191210023335.49987-7-emilyshaffer@google.com> URL: https://gitlist.dev/e/20191210023335.49987-7-emilyshaffer%40google.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` It might be desirable - for a user script, or a scripted Git command - to run the appropriate set of hooks from outside of the compiled Git binary. So, teach --porcelain in a way that enables the following: git hook --list --porcelain pre-commit | xargs -I% sh "%" Signed-off-by: Emily Shaffer --- Documentation/git-hook.txt | 5 ++++- builtin/hook.c | 19 +++++++++++++------ t/t1360-config-based-hooks.sh | 12 ++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index b4a992d43f..34276f5bce 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -8,7 +8,7 @@ git-hook - Manage configured hooks SYNOPSIS -------- [verse] -'git hook' -l | --list +'git hook' -l | --list [--porcelain] DESCRIPTION ----------- @@ -45,6 +45,9 @@ OPTIONS in the order they should be run. Output of this command follows the format ' '. +--porcelain:: + Print in a machine-readable format suitable for scripting. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/hook.c b/builtin/hook.c index 8261302b27..b76dd3ad8f 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -16,7 +16,7 @@ enum hook_command { HOOK_LIST, }; -static int print_hook_list(const struct strbuf *hookname) +static int print_hook_list(const struct strbuf *hookname, int porcelain) { struct list_head *head, *pos; struct hook *item; @@ -25,10 +25,14 @@ static int print_hook_list(const struct strbuf *hookname) list_for_each(pos, head) { item = list_entry(pos, struct hook, list); - if (item) - printf("%.3d\t%s\t%s\n", item->order, - config_scope_to_string(item->origin), - item->command.buf); + if (item) { + if (porcelain) + printf("%s\n", item->command.buf); + else + printf("%.3d\t%s\t%s\n", item->order, + config_scope_to_string(item->origin), + item->command.buf); + } } return 0; @@ -38,11 +42,14 @@ int cmd_hook(int argc, const char **argv, const char *prefix) { enum hook_command command = 0; struct strbuf hookname = STRBUF_INIT; + int porcelain = 0; struct option builtin_hook_options[] = { OPT_CMDMODE('l', "list", &command, N_("list scripts which will be run for "), HOOK_LIST), + OPT_BOOL(0, "porcelain", &porcelain, + N_("display in machine parseable format")), OPT_END(), }; @@ -59,7 +66,7 @@ int cmd_hook(int argc, const char **argv, const char *prefix) switch(command) { case HOOK_LIST: - return print_hook_list(&hookname); + return print_hook_list(&hookname, porcelain); break; default: usage_msg_opt("no command given.", builtin_hook_usage, diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 66e70ae222..6f16ea1dd8 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -33,6 +33,18 @@ test_expect_success 'git hook --list orders by order number' ' test_cmp expected actual ' +test_expect_success 'git hook --list --porcelain' ' + cat >expected <<-\EOF && + /path/def + /path/ghi + /path/rst + /path/uvw + EOF + + git hook --list --porcelain pre-commit >actual && + test_cmp expected actual +' + test_expect_success 'order number collisions resolved in config order' ' cat >expected <<-\EOF && 010 global /path/def -- 2.24.0.393.g34dc348eaf-goog ``` ## Philip Oakley, 2019-12-10 11:16 Subject: Re: [PATCH 2/6] config: add string mapping for enum config_scope Message-ID: URL: https://gitlist.dev/e/e0bcedec-dd1c-9cb7-1b0f-a8fda0bbf610%40iee.email In-Reply-To: <20191210023335.49987-3-emilyshaffer@google.com> ``` Hi Emily, On 10/12/2019 02:33, Emily Shaffer wrote: > If a user is interacting with their config files primarily by the 'git > config' command, using the location flags (--global, --system, etc) then > they may be more interested to see the scope of the config file they are > editing, rather than the filepath. There's asimilar issue being worked on under Git-for-Windows with some proposed code for this very 'problem' https://github.com/git-for-windows/git/pull/2399 and a GitGitGadget PR https://github.com/gitgitgadget/git/pull/478 cc'ing Matthew to help coordination. Philip > > Signed-off-by: Emily Shaffer > --- > config.c | 17 +++++++++++++++++ > config.h | 1 + > 2 files changed, 18 insertions(+) > > diff --git a/config.c b/config.c > index e7052b3977..a20110e016 100644 > --- a/config.c > +++ b/config.c > @@ -3312,6 +3312,23 @@ enum config_scope current_config_scope(void) > return current_parsing_scope; > } > > +const char *config_scope_to_string(enum config_scope scope) > +{ > + switch (scope) { > + case CONFIG_SCOPE_SYSTEM: > + return _("system"); > + case CONFIG_SCOPE_GLOBAL: > + return _("global"); > + case CONFIG_SCOPE_REPO: > + return _("repo"); > + case CONFIG_SCOPE_CMDLINE: > + return _("cmdline"); > + case CONFIG_SCOPE_UNKNOWN: > + default: > + return _("unknown"); > + } > +} > + > int lookup_config(const char **mapping, int nr_mapping, const char *var) > { > int i; > diff --git a/config.h b/config.h > index f0ed464004..612f43acd0 100644 > --- a/config.h > +++ b/config.h > @@ -139,6 +139,7 @@ enum config_scope { > }; > > enum config_scope current_config_scope(void); > +const char *config_scope_to_string(enum config_scope); > const char *current_config_origin_type(void); > const char *current_config_name(void); > ``` ## Philip Oakley, 2019-12-10 17:21 Subject: Re: [PATCH 2/6] config: add string mapping for enum config_scope Message-ID: URL: https://gitlist.dev/e/f5d109b0-edc3-914a-1b73-5bae8ddd9eee%40iee.email In-Reply-To: ``` correcting Matt's email address. original thread https://lore.kernel.org/git/20191210023335.49987-3-emilyshaffer@google.com/ On 10/12/2019 11:16, Philip Oakley wrote: > On 10/12/2019 02:33, Emily Shaffer wrote: >> If a user is interacting with their config files primarily by the 'git >> config' command, using the location flags (--global, --system, etc) then >> they may be more interested to see the scope of the config file they are >> editing, rather than the filepath. > There's asimilar issue being worked on under Git-for-Windows with some > proposed code for this very 'problem' > https://github.com/git-for-windows/git/pull/2399 and a GitGitGadget PR > https://github.com/gitgitgadget/git/pull/478 > > cc'ing Matthew to help coordination. Philip ``` ## Junio C Hamano, 2019-12-11 19:21 Subject: Re: [PATCH 4/6] hook: support reordering of hook list Message-ID: URL: https://gitlist.dev/e/xmqqlfrioegv.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20191210023335.49987-5-emilyshaffer@google.com> ``` Emily Shaffer writes: > $ grep -A2 "\[hook\]" ~/.gitconfig > [hook] > pre-commit = 001:~/test.sh > pre-commit = 999:~/baz.sh > $ grep -A2 "\[hook\]" ~/git/.git/config > [hook] > pre-commit = 900:~/bar.sh > pre-commit = 050:~/baz.sh > $ ./bin-wrappers/git hook --list pre-commit > 001 global ~/test.sh > 050 repo ~/baz.sh > 900 repo ~/bar.sh > > In the above example, '~/baz.sh' is provided in the global config with > order position 999. Then, in the local config, that order is overridden > to 050. Instead of running ~/baz.sh twice (at order 050 and at order > 999), only run it once, in the position specified last in config order. Doesn't that depend on the nature of the hook? A hook that is general enough to be used to inspect if another hook's effect is sane and reject the result may want to be run after invocation of each hook that is not itself, so I would prefer to avoid a design that forbids the same command to be specified twice. I would love it if it were possible without the precedence order and instead the order of appearance in git_config() stream were usable to decide the order these hooks are executed. Unfortunately, there is a fixed order that the configuration files are read, and I do not see a way short of adding : prefix like this design does to ensure that a hook defined in the local config can run before or after a hook defined in the global config, so : in the above design is probably a necessary evil X-<. Having said that, I have a suspicion that the config file itself should be kept simple---if a hook appears twice with different numbers, they would be run twice, for example---and the tooling around it (e.g. "git hook add/edit/replace/reorder") should implement such a policy (e.g. "the same hook can run only once, so remove the other entries when adding the same") if desired. Which would mean that overriding/disabling an entry in the same configuration file should be done by replacing or removing the entry. Adding another entry for the same command with different precedence should mean the command would run twice. And you would need a notation to override or disable an entry in a different configuration file (e.g. global tells us to run foo.sh at level 50 with "hook.pre-commit=50:foo.sh"; repository wants to say not to run it at all, or run it at 80 instead). I would think you'd just need a notation to kill an existing entry (e.g. the local one adds "hook.pre-commit=-50:foo.sh" to countermand the entry in the earlier example, and then can add another one at level 80 if it desires). I am also tempted to say that the precedence level may not stay to be the only attribute for a pair wants to keep. Instead of [hook] pre-commit = 900:bar.sh it may have to become more like [hook "pre-commit"] level = 900 path = bar.sh if we do not want to paint us into a corner from which we cannot get out of. I dunno. Doesn't this require coordination between the three configuration sources how numbers are assigned and used, by the way? Between the per-user and the per-repository config, they are set by the same person anyway, so there is not much to coordinate, but I am not sure what the expectations are to allow reading from the system-wide configuration (or, should we just disable reading from the system-wide configuration for, say, security reasons?) ``` ## Junio C Hamano, 2019-12-11 19:33 Subject: Re: [PATCH 6/6] hook: teach --porcelain mode Message-ID: URL: https://gitlist.dev/e/xmqqimmmodwt.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20191210023335.49987-7-emilyshaffer@google.com> ``` Emily Shaffer writes: > It might be desirable - for a user script, or a scripted Git command - > to run the appropriate set of hooks from outside of the compiled Git > binary. So, teach --porcelain in a way that enables the following: > > git hook --list --porcelain pre-commit | xargs -I% sh "%" > > Signed-off-by: Emily Shaffer > --- > +--porcelain:: > + Print in a machine-readable format suitable for scripting. > + > ... > +static int print_hook_list(const struct strbuf *hookname, int porcelain) > { > struct list_head *head, *pos; > struct hook *item; > @@ -25,10 +25,14 @@ static int print_hook_list(const struct strbuf *hookname) > > list_for_each(pos, head) { > item = list_entry(pos, struct hook, list); > + if (item) { > + if (porcelain) > + printf("%s\n", item->command.buf); > + else > + printf("%.3d\t%s\t%s\n", item->order, > + config_scope_to_string(item->origin), > + item->command.buf); > + } So, a Porcelain script cannot learn where the hook command comes from, or what the precedence order of each line of the output is? ``` ## Emily Shaffer, 2019-12-11 22:00 Subject: Re: [PATCH 6/6] hook: teach --porcelain mode Message-ID: <20191211220039.GB8464@google.com> URL: https://gitlist.dev/e/20191211220039.GB8464%40google.com In-Reply-To: ``` On Wed, Dec 11, 2019 at 11:33:38AM -0800, Junio C Hamano wrote: > Emily Shaffer writes: > > > It might be desirable - for a user script, or a scripted Git command - > > to run the appropriate set of hooks from outside of the compiled Git > > binary. So, teach --porcelain in a way that enables the following: > > > > git hook --list --porcelain pre-commit | xargs -I% sh "%" > > > > Signed-off-by: Emily Shaffer > > --- > > > +--porcelain:: > > + Print in a machine-readable format suitable for scripting. > > + > > ... > > +static int print_hook_list(const struct strbuf *hookname, int porcelain) > > { > > struct list_head *head, *pos; > > struct hook *item; > > @@ -25,10 +25,14 @@ static int print_hook_list(const struct strbuf *hookname) > > > > list_for_each(pos, head) { > > item = list_entry(pos, struct hook, list); > > + if (item) { > > + if (porcelain) > > + printf("%s\n", item->command.buf); > > + else > > + printf("%.3d\t%s\t%s\n", item->order, > > + config_scope_to_string(item->origin), > > + item->command.buf); > > + } > > So, a Porcelain script cannot learn where the hook command comes > from, Not as I had envisioned. > or what the precedence order of each line of the output is? > They're printed in the order they should be executed; the explicit order isn't provided. I suppose I had considered really just the one use case listed in the commit message, especially since other inquiry into the hooks to be run can be done against the config files themselves. But - I'm of course open to use cases. What did you have in mind? Maybe this can be solved better with a --pretty=format type of argument. ``` ## Junio C Hamano, 2019-12-11 22:07 Subject: Re: [PATCH 6/6] hook: teach --porcelain mode Message-ID: URL: https://gitlist.dev/e/xmqqwob2ms7i.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20191211220039.GB8464@google.com> ``` Emily Shaffer writes: >> So, a Porcelain script cannot learn where the hook command comes >> from, > > Not as I had envisioned. > >> or what the precedence order of each line of the output is? >> > > They're printed in the order they should be executed; the explicit order > isn't provided. > > > I suppose I had considered really just the one use case listed in the > commit message, especially since other inquiry into the hooks to be run > can be done against the config files themselves. But - I'm of course > open to use cases. What did you have in mind? A tool to diagnose why the hooks are not firing in the order the user intended them to, for example? Or a tool to help editing the list of hooks. ``` ## Junio C Hamano, 2019-12-11 22:42 Subject: Re: [PATCH 0/6] configuration-based hook management Message-ID: URL: https://gitlist.dev/e/xmqqo8wemqlr.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20191210023335.49987-1-emilyshaffer@google.com> ``` Emily Shaffer writes: > An implementation of the first piece of the proposal given in > lore.kernel.org/git/20191116011125.GG22855@google.com. > > Teaches a new command, 'git hook', which will someday include 'git hook > --add ...', 'git hook --edit ...', and maybe more. For now, just teach > it how to check the config files with 'git hook --list ...'. > > The hooks-to-run list is collected in a new library, hook.o, which can > someday reimplement find_hook() or otherwise be invoked to run all hooks > for a given hookname (e.g. "pre-commit"). Nice to see the endgame vision upfront. A few things that I'd like to see in the endgame that you did not mention here are: - We may probably not want to have an authoritative "these are the hooks Git runs" catalog, so it would be great if the resulting system can operate without one. - There are at least two kinds of hooks wrt the style of input they take. Some take their input on their command line, which makes it quite easy to run multiple of them in a row. Others take their input from their standard input stream, which probably means that there needs a cache of the input stream to feed to each such hook script (unless Git process itself is generating the stream to drive the hook, in which case we could run the generation of the stream multiple times) if we want to run multiple of them. . With the design goal of *not* having an authoritiative catalog, we'd probably need some way to annotate each entry in the [hook] configuration which kind of invication the hook program wants. . There may be more than the above two styles. The system should be designed to be extensible to accomodate yet more. Thanks. ``` ## Emily Shaffer, 2019-12-11 23:15 Subject: Re: [PATCH 6/6] hook: teach --porcelain mode Message-ID: <20191211231535.GD8464@google.com> URL: https://gitlist.dev/e/20191211231535.GD8464%40google.com In-Reply-To: ``` On Wed, Dec 11, 2019 at 02:07:45PM -0800, Junio C Hamano wrote: > Emily Shaffer writes: > > >> So, a Porcelain script cannot learn where the hook command comes > >> from, > > > > Not as I had envisioned. > > > >> or what the precedence order of each line of the output is? > >> > > > > They're printed in the order they should be executed; the explicit order > > isn't provided. > > > > > > I suppose I had considered really just the one use case listed in the > > commit message, especially since other inquiry into the hooks to be run > > can be done against the config files themselves. But - I'm of course > > open to use cases. What did you have in mind? > > A tool to diagnose why the hooks are not firing in the order the > user intended them to, for example? > > Or a tool to help editing the list of hooks. FWIW, the next step for this 'git hook' tool is just such a mode, although I certainly won't argue with anybody who wants to interact with them somewhat differently. Does allowing a format string solve this, then? Maybe it's less Git-idiomatic, but it seems to me to be a very explicit format contract that the scripter can write, and probably more useful than guessing what info one might want when scripting. It also doesn't paint us into a corner if we add other interesting info later. Unless you have a complaint about it, I'll try to add that kind of argument instead of --porcelain for this command. - Emily ``` ## Bert Wesarg, 2019-12-12 09:38 Subject: Re: [PATCH 3/6] hook: add --list mode Message-ID: URL: https://gitlist.dev/e/CAKPyHN04hEK7PKZQvF7LkGPkbBnV2jccBAxJSiukrXb%3D4g4g_Q%40mail.gmail.com In-Reply-To: <20191210023335.49987-4-emilyshaffer@google.com> ``` On Tue, Dec 10, 2019 at 3:34 AM Emily Shaffer wrote: > > Teach 'git hook --list ', which checks the known configs in > order to create an ordered list of hooks to run on a given hook event. > > The hook config format is "hook. = :". > This paves the way for multiple hook support; hooks should be run in the > order specified by the user in the config, and in the case of an order > number collision, configuration order should be used (e.g. global hook > 004 will run before repo hook 004). > > For example: > > $ grep -A2 "\[hook\]" ~/.gitconfig > [hook] > pre-commit = 001:~/test.sh > pre-commit = 999:~/baz.sh > > $ grep -A1 "\[hook\]" ~/git/.git/config > [hook] > pre-commit = 900:~/bar.sh > > $ ./bin-wrappers/git hook --list pre-commit > 001 global ~/test.sh > 900 repo ~/bar.sh > 999 global ~/baz.sh > > Signed-off-by: Emily Shaffer > --- > Documentation/git-hook.txt | 17 +++++++- > Makefile | 1 + > builtin/hook.c | 54 ++++++++++++++++++++++- > hook.c | 81 +++++++++++++++++++++++++++++++++++ > hook.h | 14 ++++++ > t/t1360-config-based-hooks.sh | 43 ++++++++++++++++++- > 6 files changed, 206 insertions(+), 4 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..a141884239 100644 > --- a/Documentation/git-hook.txt > +++ b/Documentation/git-hook.txt > @@ -8,12 +8,27 @@ git-hook - Manage configured hooks > SYNOPSIS > -------- > [verse] > -'git hook' > +'git hook' -l | --list > > DESCRIPTION > ----------- > You can list, add, and modify hooks with this command. > > +This command parses the default configuration files for lines which look like > +"hook. = :", e.g. "hook.pre-commit = > +010:/path/to/script.sh". In this way, multiple scripts can be run during a > +single hook. Hooks are sorted in ascending order by order number; in the event > +of an order number conflict, they are sorted in configuration order. > + > +OPTIONS > +------- > + > +-l:: > +--list:: > + List the hooks which have been configured for . Hooks appear > + in the order they should be run. Output of this command follows the > + format ' '. > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/Makefile b/Makefile > index 83263505c0..21b3a82208 100644 > --- a/Makefile > +++ b/Makefile > @@ -892,6 +892,7 @@ LIB_OBJS += hashmap.o > LIB_OBJS += linear-assignment.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..8261302b27 100644 > --- a/builtin/hook.c > +++ b/builtin/hook.c > @@ -1,21 +1,73 @@ > #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 "), Its "" in Documentation/git-hook.txt > NULL > }; > > +enum hook_command { > + HOOK_NO_COMMAND = 0, > + HOOK_LIST, > +}; > + > +static int print_hook_list(const struct strbuf *hookname) > +{ > + struct list_head *head, *pos; > + struct hook *item; > + > + head = hook_list(hookname); > + > + list_for_each(pos, head) { > + item = list_entry(pos, struct hook, list); > + if (item) > + printf("%.3d\t%s\t%s\n", item->order, > + config_scope_to_string(item->origin), > + item->command.buf); > + } > + > + return 0; > +} > + > int cmd_hook(int argc, const char **argv, const char *prefix) > { > + enum hook_command command = 0; > + struct strbuf hookname = STRBUF_INIT; > + > struct option builtin_hook_options[] = { > + OPT_CMDMODE('l', "list", &command, > + N_("list scripts which will be run for "), Its "" in Documentation/git-hook.txt > + HOOK_LIST), > OPT_END(), > }; > > argc = parse_options(argc, argv, prefix, builtin_hook_options, > builtin_hook_usage, 0); > > + if (argc < 1) { > + usage_msg_opt("a hookname must be provided to operate on.", > + builtin_hook_usage, builtin_hook_options); > + } > + > + strbuf_addstr(&hookname, "hook."); > + strbuf_addstr(&hookname, argv[0]); The arg is never checked, if this is a valid/known hook. Bert > + > + switch(command) { > + case HOOK_LIST: > + return print_hook_list(&hookname); > + break; > + default: > + usage_msg_opt("no command given.", builtin_hook_usage, > + builtin_hook_options); > + } > + > + clear_hook_list(); > + strbuf_release(&hookname); > + > return 0; > } > diff --git a/hook.c b/hook.c > new file mode 100644 > index 0000000000..f8d1109084 > --- /dev/null > +++ b/hook.c > @@ -0,0 +1,81 @@ > +#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, int order, const char *command) > +{ > + struct hook *to_add = malloc(sizeof(struct hook)); > + to_add->order = order; > + 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() > +{ > + struct list_head *pos, *tmp; > + list_for_each_safe(pos, tmp, &hook_head) > + remove_hook(pos); > +} > + > +static int check_config_for_hooks(const char *var, const char *value, void *hookname) > +{ > + struct list_head *pos, *p; > + struct hook *item; > + const struct strbuf *hookname_strbuf = hookname; > + > + if (!strcmp(var, hookname_strbuf->buf)) { > + int order = 0; > + // TODO this is bad - open to overflows > + char command[256]; > + int added = 0; > + if (!sscanf(value, "%d:%s", &order, command)) > + die(_("hook config '%s' doesn't match expected format"), > + value); > + > + list_for_each_safe(pos, p, &hook_head) { > + item = list_entry(pos, struct hook, list); > + > + /* > + * the new entry should go just before the first entry > + * which has a higher order number than it. > + */ > + if (item->order > order && !added) { > + emplace_hook(pos, order, command); > + added = 1; > + } > + } > + > + if (!added) > + emplace_hook(pos, order, command); > + } > + > + return 0; > +} > + > +struct list_head* hook_list(const struct strbuf* hookname) > +{ > + git_config(check_config_for_hooks, (void*)hookname); > + > + return &hook_head; > +} > diff --git a/hook.h b/hook.h > new file mode 100644 > index 0000000000..104df4c088 > --- /dev/null > +++ b/hook.h > @@ -0,0 +1,14 @@ > +#include "config.h" > + > +struct hook > +{ > + struct list_head list; > + int order; > + 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(); > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh > index 34b0df5216..1434051db3 100755 > --- a/t/t1360-config-based-hooks.sh > +++ b/t/t1360-config-based-hooks.sh > @@ -4,8 +4,47 @@ 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 system, global, and local' ' > + git config --add --global hook.pre-commit "010:/path/def" && > + git config --add --global hook.pre-commit "999:/path/uvw" && > + > + git config --add --local hook.pre-commit "100:/path/ghi" && > + git config --add --local hook.pre-commit "990:/path/rst" > +' > + > +test_expect_success 'git hook --list orders by order number' ' > + cat >expected <<-\EOF && > + 010 global /path/def > + 100 repo /path/ghi > + 990 repo /path/rst > + 999 global /path/uvw > + EOF > + > + git hook --list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'order number collisions resolved in config order' ' > + cat >expected <<-\EOF && > + 010 global /path/def > + 010 repo /path/abc > + 100 repo /path/ghi > + 990 repo /path/rst > + 999 global /path/uvw > + EOF > + > + git config --add --local hook.pre-commit "010:/path/abc" && > + git hook --list pre-commit >actual && > + test_cmp expected actual > ' > > test_done > -- > 2.24.0.393.g34dc348eaf-goog > ``` ## Bert Wesarg, 2019-12-12 09:41 Subject: Re: [PATCH 1/6] hook: scaffolding for git-hook subcommand Message-ID: URL: https://gitlist.dev/e/CAKPyHN19uDdredXyGv63sHJKKeUue0t9nM6e31RVPYNYfd%3DXmQ%40mail.gmail.com In-Reply-To: <20191210023335.49987-2-emilyshaffer@google.com> ``` On Tue, Dec 10, 2019 at 3:34 AM Emily Shaffer wrote: > > Introduce infrastructure for a new subcommand, git-hook, which will be > used to ease config-based hook management. This command will handle > parsing configs to compose a list of hooks to run for a given event, as > well as adding or modifying hook configs in an interactive fashion. > > Signed-off-by: Emily Shaffer > --- > .gitignore | 1 + > Documentation/git-hook.txt | 19 +++++++++++++++++++ > Makefile | 1 + > builtin.h | 1 + > builtin/hook.c | 21 +++++++++++++++++++++ > git.c | 1 + how about adding also completion support here? Bert > t/t1360-config-based-hooks.sh | 11 +++++++++++ > 7 files changed, 55 insertions(+) > create mode 100644 Documentation/git-hook.txt > create mode 100644 builtin/hook.c > create mode 100755 t/t1360-config-based-hooks.sh > > diff --git a/.gitignore b/.gitignore > index 89b3b79c1a..9ef59b9baa 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -74,6 +74,7 @@ > /git-grep > /git-hash-object > /git-help > +/git-hook > /git-http-backend > /git-http-fetch > /git-http-push > diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt > new file mode 100644 > index 0000000000..2d50c414cc > --- /dev/null > +++ b/Documentation/git-hook.txt > @@ -0,0 +1,19 @@ > +git-hook(1) > +=========== > + > +NAME > +---- > +git-hook - Manage configured hooks > + > +SYNOPSIS > +-------- > +[verse] > +'git hook' > + > +DESCRIPTION > +----------- > +You can list, add, and modify hooks with this command. > + > +GIT > +--- > +Part of the linkgit:git[1] suite > diff --git a/Makefile b/Makefile > index 58b92af54b..83263505c0 100644 > --- a/Makefile > +++ b/Makefile > @@ -1074,6 +1074,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o > BUILTIN_OBJS += builtin/grep.o > BUILTIN_OBJS += builtin/hash-object.o > BUILTIN_OBJS += builtin/help.o > +BUILTIN_OBJS += builtin/hook.o > BUILTIN_OBJS += builtin/index-pack.o > BUILTIN_OBJS += builtin/init-db.o > BUILTIN_OBJS += builtin/interpret-trailers.o > diff --git a/builtin.h b/builtin.h > index 5cf5df69f7..d4ca2ac9a5 100644 > --- a/builtin.h > +++ b/builtin.h > @@ -173,6 +173,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix); > int cmd_grep(int argc, const char **argv, const char *prefix); > int cmd_hash_object(int argc, const char **argv, const char *prefix); > int cmd_help(int argc, const char **argv, const char *prefix); > +int cmd_hook(int argc, const char **argv, const char *prefix); > int cmd_index_pack(int argc, const char **argv, const char *prefix); > int cmd_init_db(int argc, const char **argv, const char *prefix); > int cmd_interpret_trailers(int argc, const char **argv, const char *prefix); > diff --git a/builtin/hook.c b/builtin/hook.c > new file mode 100644 > index 0000000000..b2bbc84d4d > --- /dev/null > +++ b/builtin/hook.c > @@ -0,0 +1,21 @@ > +#include "cache.h" > + > +#include "builtin.h" > +#include "parse-options.h" > + > +static const char * const builtin_hook_usage[] = { > + N_("git hook"), > + NULL > +}; > + > +int cmd_hook(int argc, const char **argv, const char *prefix) > +{ > + struct option builtin_hook_options[] = { > + OPT_END(), > + }; > + > + argc = parse_options(argc, argv, prefix, builtin_hook_options, > + builtin_hook_usage, 0); > + > + return 0; > +} > diff --git a/git.c b/git.c > index ce6ab0ece2..c8344b9ab7 100644 > --- a/git.c > +++ b/git.c > @@ -513,6 +513,7 @@ static struct cmd_struct commands[] = { > { "grep", cmd_grep, RUN_SETUP_GENTLY }, > { "hash-object", cmd_hash_object }, > { "help", cmd_help }, > + { "hook", cmd_hook, RUN_SETUP }, > { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT }, > { "init", cmd_init_db }, > { "init-db", cmd_init_db }, > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh > new file mode 100755 > index 0000000000..34b0df5216 > --- /dev/null > +++ b/t/t1360-config-based-hooks.sh > @@ -0,0 +1,11 @@ > +#!/bin/bash > + > +test_description='config-managed multihooks, including git-hook command' > + > +. ./test-lib.sh > + > +test_expect_success 'git hook command does not crash' ' > + git hook > +' > + > +test_done > -- > 2.24.0.393.g34dc348eaf-goog > ``` ## SZEDER Gábor, 2019-12-12 10:47 Subject: Re: [PATCH 1/6] hook: scaffolding for git-hook subcommand Message-ID: <20191212104701.GI6527@szeder.dev> URL: https://gitlist.dev/e/20191212104701.GI6527%40szeder.dev In-Reply-To: <20191210023335.49987-2-emilyshaffer@google.com> ``` On Mon, Dec 09, 2019 at 06:33:30PM -0800, Emily Shaffer wrote: > Introduce infrastructure for a new subcommand, git-hook, which will be > used to ease config-based hook management. This command will handle > parsing configs to compose a list of hooks to run for a given event, as > well as adding or modifying hook configs in an interactive fashion. > > Signed-off-by: Emily Shaffer > --- > .gitignore | 1 + > Documentation/git-hook.txt | 19 +++++++++++++++++++ > Makefile | 1 + > builtin.h | 1 + > builtin/hook.c | 21 +++++++++++++++++++++ > git.c | 1 + > t/t1360-config-based-hooks.sh | 11 +++++++++++ > 7 files changed, 55 insertions(+) > create mode 100644 Documentation/git-hook.txt > create mode 100644 builtin/hook.c > create mode 100755 t/t1360-config-based-hooks.sh When adding a new command please don't forget the steps noted in 4ed5562925 (myfirstcontrib: add 'psuh' to command-list.txt, 2019-10-31) ;) > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh > new file mode 100755 > index 0000000000..34b0df5216 > --- /dev/null > +++ b/t/t1360-config-based-hooks.sh > @@ -0,0 +1,11 @@ > +#!/bin/bash s/ba// > + > +test_description='config-managed multihooks, including git-hook command' > + > +. ./test-lib.sh > + > +test_expect_success 'git hook command does not crash' ' > + git hook > +' > + > +test_done > -- > 2.24.0.393.g34dc348eaf-goog > ``` ## SZEDER Gábor, 2019-12-12 10:58 Subject: Re: [PATCH 3/6] hook: add --list mode Message-ID: <20191212105855.GJ6527@szeder.dev> URL: https://gitlist.dev/e/20191212105855.GJ6527%40szeder.dev In-Reply-To: <20191210023335.49987-4-emilyshaffer@google.com> ``` On Mon, Dec 09, 2019 at 06:33:32PM -0800, Emily Shaffer wrote: > Teach 'git hook --list ', which checks the known configs in > order to create an ordered list of hooks to run on a given hook event. > > The hook config format is "hook. = :". > This paves the way for multiple hook support; hooks should be run in the > order specified by the user in the config, and in the case of an order > number collision, configuration order should be used (e.g. global hook > 004 will run before repo hook 004). > > For example: > > $ grep -A2 "\[hook\]" ~/.gitconfig > [hook] > pre-commit = 001:~/test.sh > pre-commit = 999:~/baz.sh > > $ grep -A1 "\[hook\]" ~/git/.git/config > [hook] > pre-commit = 900:~/bar.sh > > $ ./bin-wrappers/git hook --list pre-commit > 001 global ~/test.sh > 900 repo ~/bar.sh > 999 global ~/baz.sh > > Signed-off-by: Emily Shaffer > --- > Documentation/git-hook.txt | 17 +++++++- > Makefile | 1 + > builtin/hook.c | 54 ++++++++++++++++++++++- > hook.c | 81 +++++++++++++++++++++++++++++++++++ > hook.h | 14 ++++++ > t/t1360-config-based-hooks.sh | 43 ++++++++++++++++++- > 6 files changed, 206 insertions(+), 4 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..a141884239 100644 > --- a/Documentation/git-hook.txt > +++ b/Documentation/git-hook.txt > @@ -8,12 +8,27 @@ git-hook - Manage configured hooks > SYNOPSIS > -------- > [verse] > -'git hook' > +'git hook' -l | --list > > DESCRIPTION > ----------- > You can list, add, and modify hooks with this command. > > +This command parses the default configuration files for lines which look like > +"hook. = :", e.g. "hook.pre-commit = > +010:/path/to/script.sh". In this way, multiple scripts can be run during a > +single hook. Hooks are sorted in ascending order by order number; in the event > +of an order number conflict, they are sorted in configuration order. > + > +OPTIONS > +------- > + > +-l:: > +--list:: > + List the hooks which have been configured for . Hooks appear > + in the order they should be run. Output of this command follows the > + format ' '. > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/builtin/hook.c b/builtin/hook.c > index b2bbc84d4d..8261302b27 100644 > --- a/builtin/hook.c > +++ b/builtin/hook.c > @@ -1,21 +1,73 @@ > #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 "), > NULL > }; > > +enum hook_command { > + HOOK_NO_COMMAND = 0, > + HOOK_LIST, > +}; > + > +static int print_hook_list(const struct strbuf *hookname) > +{ > + struct list_head *head, *pos; > + struct hook *item; > + > + head = hook_list(hookname); > + > + list_for_each(pos, head) { > + item = list_entry(pos, struct hook, list); > + if (item) > + printf("%.3d\t%s\t%s\n", item->order, > + config_scope_to_string(item->origin), > + item->command.buf); > + } > + > + return 0; > +} > + > int cmd_hook(int argc, const char **argv, const char *prefix) > { > + enum hook_command command = 0; > + struct strbuf hookname = STRBUF_INIT; > + > struct option builtin_hook_options[] = { > + OPT_CMDMODE('l', "list", &command, > + N_("list scripts which will be run for "), > + HOOK_LIST), I'm not sure about '--list' being an option. I don't know what other operations you have in mind for this 'git hook' command, but I suppose that besides listing configured hooks it will be able to at least add, remove, and reorder them as well. These seem to be better implemented as subcommands, along the lines of e.g. how notes and remotes can be added, removed, etc. > OPT_END(), > }; ```