From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Mon, 09 Feb 2026 18:26:30 GMT
Subject: Re: [PATCH 2/4] hook: introduce "git hook list"
Message-ID: <875x8522p5.fsf@collabora.com>
In-Reply-To: <aYnu8ltBUUflVgm2@pks.im>

On Mon, 09 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> 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.

Ack, will fix in v2.

>> 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()`.

Ack, will fix in v2.

>> +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.

Indeed, it should be possible to avoid using the_repository here. Will
do for v2 or document why it can't be done.

>> +	if (!head->nr) {
>> +		ret = 1; /* no hooks found */
>> +		goto cleanup;
>> +	}
>
> Do we want to print an error message in this case?

Good idea. Will do.

>> +	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.

Yes, I'll very likely do this in v2, certainly I will replace the
reliance on the empty hook name.

>
> 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.

I can do this, certainly.

>> 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"?

Good idea. Will do.