Re: [PATCH v4 3/9] hook: add list command

[Phillip Wood <phillip.wood123@gmail.com>]

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
>