From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 20 Feb 2026 13:53:34 GMT
Subject: Re: [PATCH v2 3/8] hook: add "git hook list" command
Message-ID: <87wm07wmg1.fsf@collabora.com>
In-Reply-To: <aZhXlrWRGDS_Bhvm@pks.im>

On Fri, 20 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> On Thu, Feb 19, 2026 at 12:23:47AM +0200, Adrian Ratiu wrote:
>> diff --git a/builtin/hook.c b/builtin/hook.c
>> index 7afec380d2..51660c4941 100644
>> --- a/builtin/hook.c
>> +++ b/builtin/hook.c
>> @@ -20,6 +24,61 @@ static const char * const builtin_hook_run_usage[] = {
>>  	NULL
>>  };
>>  
>> +static int list(int argc, const char **argv, const char *prefix,
>> +		 struct repository *repo)
>> +{
>> +	static const char *const builtin_hook_list_usage[] = {
>> +		BUILTIN_HOOK_LIST_USAGE,
>> +		NULL
>> +	};
>> +	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);
>
> Error messages typically start with a lower-case letter.

Ack, will fix.

>> +	hookname = argv[0];
>> +
>> +	head = list_hooks(repo, hookname, NULL);
>> +
>> +	if (!head->nr) {
>> +		warning(_("No hooks found for event '%s'"), hookname);
>
> Warnings, too.

Ack, will fix.

>> +		ret = 1; /* no hooks found */
>> +		goto cleanup;
>> +	}
>> +
>> +	for_each_string_list_item(item, head) {
>> +		struct hook *h = item->util;
>> +
>> +		switch (h->kind) {
>> +		case HOOK_TRADITIONAL:
>> +			printf("%s\n", _("hook from hookdir"));
>> +			break;
>> +		default:
>> +			BUG("unknown hook kind");
>> +		}
>
> Good that we're being defensive here.

Yes, I remembered correctly then, will add a BUG() to the other patch as
well.