Re: [PATCH 3/6] hook: add --list mode

[Bert Wesarg <bert.wesarg@googlemail.com>]

On Tue, Dec 10, 2019 at 3:34 AM Emily Shaffer <emilyshaffer@google.com> 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.
>
> The hook config format is "hook.<hookname> = <order>:<path-to-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 <emilyshaffer@google.com>
> ---
>  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 <hook-name>
>
>  DESCRIPTION
>  -----------
>  You can list, add, and modify hooks with this command.
>
> +This command parses the default configuration files for lines which look like
> +"hook.<hook-name> = <order number>:<hook command>", 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 <hook-name>. Hooks appear
> +       in the order they should be run. Output of this command follows the
> +       format '<order number> <origin config> <hook command>'.
> +
>  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 <hookname>"),

Its "<hook-name>" 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 <hookname>"),


Its "<hook-name>" 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
>