Re: [PATCH 3/4] hook: include hooks from the config

> 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.<name>.command::
>> +	A command to execute whenever `hook.<name>` is invoked. `<name>` 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.<name>.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 `<name>`, the last value parsed will be the only command executed.
>> +	See linkgit:git-hook[1].
>> +
>> +hook.<name>.event::
>> +	The hook events which should invoke `hook.<name>`. `<name>` 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.<name>.command` will be
>> +	executed. More than one event can be specified if you wish for
>> +	`hook.<name>` 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?

>> +git config hook.<some-name>.command <path-to-script>
>> +git config --add hook.<some-name>.event <hook-event>
>
> Let's use the modern equivalents:
>
>     git config set hook.<some-name>.command <path-to-script>
>     git config set --append hook.<some-name>.event <hook-event>
>

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

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

>> +	/* 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.<name>.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.

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

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