From: Adrian Ratiu Date: Mon, 09 Feb 2026 19:10:25 GMT Subject: Re: [PATCH 3/4] hook: include hooks from the config Message-ID: <87343920ny.fsf@collabora.com> In-Reply-To: On Mon, 09 Feb 2026, Patrick Steinhardt wrote: > On Wed, Feb 04, 2026 at 06:51:25PM +0200, Adrian Ratiu wrote: >> diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc >> new file mode 100644 >> index 0000000000..49c7ffd82e >> --- /dev/null >> +++ b/Documentation/config/hook.adoc >> @@ -0,0 +1,17 @@ >> +hook..command:: >> + A command to execute whenever `hook.` is invoked. `` should >> + be a unique "friendly" name which you can use to identify this hook >> + command. (You can specify when to invoke this command with >> + `hook..event`.) The value can be an executable on your device or a >> + oneliner for your shell. If more than one value is specified for the >> + same ``, the last value parsed will be the only command executed. >> + See linkgit:git-hook[1]. >> + >> +hook..event:: >> + The hook events which should invoke `hook.`. `` should be a >> + unique "friendly" name which you can use to identify this hook. The >> + value should be the name of a hook event, like "pre-commit" or "update". >> + (See linkgit:githooks[5] for a complete list of hooks Git knows about.) >> + On the specified event, the associated `hook..command` will be >> + executed. More than one event can be specified if you wish for >> + `hook.` to execute on multiple events. See linkgit:git-hook[1]. > > I'm not exactly sure how you'd execute it on multiple events after > reading this description. Is this by comma-delimiting them? Or is the > event key a multivalue variable? If the latter (which I think would make > most sense), can you also reset the value by e.g. saying "event = " like > we support with other multivalued keys? Yes, event is a multivalue variable, however the current implementation does not allow resetting using "event = ". I'll allow it in v2 and also make the doc clearer. > I would have also expected a "hook..enabled" option so that you > can disable globally configured hooks. > Good idea. I can add that in v2. >> +git config hook..command >> +git config --add hook..event > > Let's use the modern equivalents: > > git config set hook..command > git config set --append hook..event > Ack, will do in v2. >> @@ -46,6 +128,46 @@ OPTIONS >> tools that want to do a blind one-shot run of a hook that may >> or may not be present. >> >> +WRAPPERS >> +-------- >> + >> +`git hook run` has been designed to make it easy for tools which wrap Git to >> +configure and execute hooks using the Git hook infrastructure. It is possible to >> +provide arguments and stdin via the command line, as well as specifying parallel >> +or series execution if the user has provided multiple hooks. >> + >> +Assuming your wrapper wants to support a hook named "mywrapper-start-tests", you >> +can have your users specify their hooks like so: >> + >> + [hook "setup-test-dashboard"] >> + event = mywrapper-start-tests >> + command = ~/mywrapper/setup-dashboard.py --tap > > Ah, interesting. Would that then also look for > ".git/hooks/mywrapper-start-tests"? Yes, because list_hooks() gathers all hooks for the event, including the "legacy/default" hooks from the hookdir. I also just noticed this doc mentions parallel execution, which is not yet possible (that is a separate series). I fix this and add the parallel comment back in the other series. >> diff --git a/hook.c b/hook.c >> index 949c907b59..9f59ebd0bd 100644 >> --- a/hook.c >> +++ b/hook.c >> @@ -47,24 +47,74 @@ const char *find_hook(struct repository *r, const char *name) >> return path.buf; >> } >> >> +struct hook_config_cb >> +{ > > Formatting: the curly brace should not go on a standalone line. Ack, will fix. >> + /* Look for "hook.friendly-name.event = hook_event" */ >> + if (parse_config_key(key, "hook", &name, &name_len, &event_key) || >> + strcmp(event_key, "event")) >> + return 0; > > So we only care about parsing "hook..event" here, and I assume > that we would look up the command at a later point in time? That design > is somewhat weird -- I would have expected that we scan through all > config keys and already pull out both the command and event so that we > don't have to extract the config a second time. > > But I guess this is another consequence of the string list only caring > about the name, not about the actual path. That is correct. I do plan to change this design in v2 and we might as well also look up the path at the same time. I see nothing preventing us from doing it. >> + /* Extract the hook name */ >> + hook_name = xmemdupz(name, name_len); >> + >> + /* Remove the hook if already in the list, so we append in config order. */ >> + if ((item = unsorted_string_list_lookup(data->list, hook_name))) >> + unsorted_string_list_delete_item(data->list, item - data->list->items, 0); >> + >> + /* The list takes ownership of hook_name, so append with nodup */ >> + string_list_append_nodup(data->list, hook_name); >> + >> + return 0; >> +} >> + >> struct string_list *list_hooks(struct repository *r, const char *hookname) >> { >> - struct string_list *hook_head; >> + struct hook_config_cb cb_data; >> >> if (!hookname) >> BUG("null hookname was provided to hook_list()!"); >> >> - hook_head = xmalloc(sizeof(struct string_list)); >> - string_list_init_dup(hook_head); >> + cb_data.hook_event = hookname; >> + cb_data.list = xmalloc(sizeof(struct string_list)); >> + string_list_init_dup(cb_data.list); >> + >> + /* Add the hooks from the config, e.g. hook.myhook.event = pre-commit */ >> + repo_config(r, hook_config_lookup, &cb_data); > > There are cases where we invoke the same hook multiple times per > process. One such candidate is for example the "reference-transaction" > hook, which is executed thrice per ref transaction. Would it make sense > if we cached the value in `r` so that we can avoid repeatedly parsing > the configuration for this hook? > > We could even go one step further: instead of caching the value for > _one_ hook, we could parse _all_ hooks lazily and store them in a map. I think this can be done, yes, though I'm unsure of the performance gain vs the added complexity. Since after the first lookup the config files are hot in the OS page cache, the next lookups are cheap/fast and we'd be shaving off only a few microseconds of parsing. So I'm unsure if this added complexity is worth it or not. >> @@ -125,6 +175,24 @@ static int pick_next_hook(struct child_process *cp, >> hook_path = absolute_path(hook_path); >> >> strvec_push(&cp->args, hook_path); >> + } else { >> + /* ...from config */ >> + struct strbuf cmd_key = STRBUF_INIT; >> + char *command = NULL; >> + >> + /* to enable oneliners, let config-specified hooks run in shell. */ >> + cp->use_shell = true; >> + >> + strbuf_addf(&cmd_key, "hook.%s.command", to_run->string); >> + if (repo_config_get_string(hook_cb->repository, >> + cmd_key.buf, &command)) { >> + die(_("'hook.%s.command' must be configured or" >> + "'hook.%s.event' must be removed; aborting.\n"), >> + to_run->string, to_run->string); >> + } >> + strbuf_release(&cmd_key); >> + >> + strvec_push_nodup(&cp->args, command); >> } >> >> if (!cp->args.nr) > > Okay, so here we do an extra step to extract the commands. I think this > would fit more naturally in `hook_config_lookup`. Agreed, especially after I implement all other feedback to not rely on the "empty" name to distinguish hooks, I think these can be consolidated in one place.