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

From: SZEDER Gábor <[email-masked]>
Date: Thu, 12 Dec 2019 11:58:55 +0100

On Mon, Dec 09, 2019 at 06:33:32PM -0800, 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.
> 
> 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 <[email-masked]>
> ---
>  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/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>"),
>  	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>"),
> +			    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(),
>  	};