From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 09 Feb 2026 14:28:02 GMT
Subject: Re: [PATCH 2/4] hook: introduce "git hook list"
Message-ID: <aYnu8ltBUUflVgm2@pks.im>
In-Reply-To: <20260204165126.1548805-3-adrian.ratiu@collabora.com>

On Wed, Feb 04, 2026 at 06:51:24PM +0200, Adrian Ratiu wrote:
> From: Emily Shaffer <emilyshaffer@google.com>
> 
> If more than one hook will be run, it may be useful to see a list of
> which hooks should be run. At very least, it will be useful for us to
> test the semantics of multihooks ourselves.
> 
> For now, only list the hooks which will run in the order they will run
> in; later, it might be useful to include more information like where the
> hooks were configured and whether or not they will run.

I think the commit message could be adapted a bit again to first explain
the problem we're about to solve.

> diff --git a/builtin/hook.c b/builtin/hook.c
> index 7afec380d2..4cc6dac45a 100644
> --- a/builtin/hook.c
> +++ b/builtin/hook.c
> @@ -20,6 +24,54 @@ static const char * const builtin_hook_run_usage[] = {
>  	NULL
>  };
>  
> +static const char *const builtin_hook_list_usage[] = {
> +	BUILTIN_HOOK_LIST_USAGE,
> +	NULL
> +};
> +

This constant can be declared inside `list()`.

> +static int list(int argc, const char **argv, const char *prefix,
> +		 struct repository *repo UNUSED)
> +{
> +	struct string_list *head;
> +	struct string_list_item *item;
> +	const char *hookname = NULL;
> +	int ret = 0;
> +
> +	struct option list_options[] = {
> +		OPT_END(),
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, list_options,
> +			     builtin_hook_list_usage, 0);
> +
> +	/*
> +	 * The only unnamed argument provided should be the hook-name; if we add
> +	 * arguments later they probably should be caught by parse_options.
> +	 */
> +	if (argc != 1)
> +		usage_msg_opt(_("You must specify a hook event name to list."),
> +			      builtin_hook_list_usage, list_options);
> +
> +	hookname = argv[0];
> +
> +	head = list_hooks(the_repository, hookname);

We can use the `repo` parameter instead. The git-hook(1) command is
declared with `RUN_SETUP`, so it will always be set.

> +	if (!head->nr) {
> +		ret = 1; /* no hooks found */
> +		goto cleanup;
> +	}

Do we want to print an error message in this case?

> +	for_each_string_list_item(item, head) {
> +		printf("%s\n", *item->string ? item->string
> +			     : _("hook from hookdir"));
> +	}

This is another case where we could avoid special-casing if the string
list contained the hook paths.

I also wonder whether we should add a "-z" mode to NUL-terminate the
output. In theory, hooks may be configured with a newline in their path.
Probably not all that common, but somehow special cases like this always
end up being encountered eventually.

> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index ed28a2fadb..d2d4a8760c 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -10,6 +10,8 @@ test_expect_success 'git hook usage' '
>  	test_expect_code 129 git hook run &&
>  	test_expect_code 129 git hook run -h &&
>  	test_expect_code 129 git hook run --unknown 2>err &&
> +	test_expect_code 129 git hook list &&
> +	test_expect_code 129 git hook list -h &&
>  	grep "unknown option" err
>  '

Shouldn't we also have some tests that show that this is working as
expected with a configured hook in ".git/hooks"?

Patrick