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

[Patrick Steinhardt <ps@pks.im>]

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?

I would have also expected a "hook.<name>.enabled" option so that you
can disable globally configured hooks.

> diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc
> index 93d734f687..5f339dc48b 100644
> --- a/Documentation/git-hook.adoc
> +++ b/Documentation/git-hook.adoc
> @@ -17,12 +17,94 @@ DESCRIPTION
>  A command interface for running git hooks (see linkgit:githooks[5]),
>  for use by other scripted git commands.
>  
> +This command parses the default configuration files for sets of configs like
> +so:
> +
> +  [hook "linter"]
> +    event = pre-commit
> +    command = ~/bin/linter --cpp20
> +
> +In this example, `[hook "linter"]` represents one script - `~/bin/linter
> +--cpp20` - which can be shared by many repos, and even by many hook events, if
> +appropriate.
> +
> +To add an unrelated hook which runs on a different event, for example a
> +spell-checker for your commit messages, you would write a configuration like so:
> +
> +  [hook "linter"]
> +    event = pre-commit
> +    command = ~/bin/linter --cpp20
> +  [hook "spellcheck"]
> +    event = commit-msg
> +    command = ~/bin/spellchecker
> +
> +With this config, when you run 'git commit', first `~/bin/linter --cpp20` will
> +have a chance to check your files to be committed (during the `pre-commit` hook
> +event`), and then `~/bin/spellchecker` will have a chance to check your commit
> +message (during the `commit-msg` hook event).
> +
> +Commands are run in the order Git encounters their associated
> +`hook.<name>.event` configs during the configuration parse (see
> +linkgit:git-config[1]). Although multiple `hook.linter.event` configs can be
> +added, only one `hook.linter.command` event is valid - Git uses "last-one-wins"
> +to determine which command to run.
> +
> +So if you wanted your linter to run when you commit as well as when you push,
> +you would configure it like so:
> +
> +  [hook "linter"]
> +    event = pre-commit
> +    event = pre-push
> +    command = ~/bin/linter --cpp20
> +
> +With this config, `~/bin/linter --cpp20` would be run by Git before a commit is
> +generated (during `pre-commit`) as well as before a push is performed (during
> +`pre-push`).
> +
> +And if you wanted to run your linter as well as a secret-leak detector during
> +only the "pre-commit" hook event, you would configure it instead like so:
> +
> +  [hook "linter"]
> +    event = pre-commit
> +    command = ~/bin/linter --cpp20
> +  [hook "no-leaks"]
> +    event = pre-commit
> +    command = ~/bin/leak-detector
> +
> +With this config, before a commit is generated (during `pre-commit`), Git would
> +first start `~/bin/linter --cpp20` and second start `~/bin/leak-detector`. It
> +would evaluate the output of each when deciding whether to proceed with the
> +commit.
> +
> +For a full list of hook events which you can set your `hook.<name>.event` to,
> +and how hooks are invoked during those events, see linkgit:githooks[5].
> +
> +Git will ignore any `hook.<name>.event` that specifies an event it doesn't
> +recognize. This is intended so that tools which wrap Git can use the hook
> +infrastructure to run their own hooks; see "WRAPPERS" for more guidance.
> +
> +In general, when instructions suggest adding a script to
> +`.git/hooks/<hook-event>`, you can specify it in the config instead by running:
> +
> +----
> +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>

> +----
> +
> +This way you can share the script between multiple repos. That is, `cp
> +~/my-script.sh ~/project/.git/hooks/pre-commit` would become:
> +
> +----
> +git config hook.my-script.command ~/my-script.sh
> +git config --add hook.my-script.event pre-commit
> +----

Likewise.

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

> +	const char *hook_event;
> +	struct string_list *list;
> +};
> +
> +/*
> + * Callback for git_config which adds configured hooks to a hook list.  Hooks
> + * can be configured by specifying both hook.<friendly-name>.command = <path>
> + * and hook.<friendly-name>.event = <hook-event>.
> + */
> +static int hook_config_lookup(const char *key, const char *value,
> +			      const struct config_context *ctx UNUSED,
> +			      void *cb_data)
> +{
> +	struct hook_config_cb *data = cb_data;
> +	const char *name, *event_key;
> +	size_t name_len = 0;
> +	struct string_list_item *item;
> +	char *hook_name;
> +
> +	/*
> +	 * Don't bother doing the expensive parse if there's no
> +	 * chance that the config matches 'hook.myhook.event = hook_event'.
> +	 */
> +	if (!value || strcmp(value, data->hook_event))
> +		return 0;

Okay.

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

Patrick