Re: [PATCH 2/4] hook: introduce "git hook list"
- From
Adrian Ratiu <adrian.ratiu@collabora.com>
- Date
- Feb 9, 2026, 18:26 UTC
- Message-ID
- <875x8522p5.fsf@collabora.com>
- In-Reply-To
- <aYnu8ltBUUflVgm2@pks.im>
On Mon, 09 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
Show 13 quoted lines
> 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.
Show 15 quoted lines
>> 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.
Show 29 quoted lines
>> +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.
Show 6 quoted lines
>> + 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.
Show 7 quoted lines
>> + 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.
Show 5 quoted lines
> > 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.
Show 15 quoted lines
>> 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.