From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 24 Mar 2026 09:07:47 GMT
Subject: Re: [PATCH v4 3/9] hook: allow parallel hook execution
Message-ID: <acJUY9N4oplsTgFY@pks.im>
In-Reply-To: <20260320135311.331463-4-adrian.ratiu@collabora.com>

On Fri, Mar 20, 2026 at 03:53:05PM +0200, Adrian Ratiu wrote:
> From: Emily Shaffer <nasamuffin@google.com>
> 
> Hooks always run in sequential order due to the hardcoded jobs == 1
> passed to run_process_parallel(). Remove that hardcoding to allow
> users to run hooks in parallel (opt-in).
> 
> Users need to decide which hooks to run in parallel, by specifying
> "parallel = true" in the config, because git cannot know if their

s/git/Git/

Sorry to be pedantic :)

> diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc
> index b7847f9338..21800db648 100644
> --- a/Documentation/config/hook.adoc
> +++ b/Documentation/config/hook.adoc
> @@ -23,6 +23,19 @@ hook.<friendly-name>.enabled::
>  	in a system or global config file and needs to be disabled for a
>  	specific repository. See linkgit:git-hook[1].
>  
> +hook.<friendly-name>.parallel::
> +	Whether the hook `hook.<friendly-name>` may run in parallel with other hooks
> +	for the same event. Defaults to `false`. Set to `true` only when the
> +	hook script is safe to run concurrently with other hooks for the same
> +	event. If any hook for an event does not have this set to `true`,
> +	all hooks for that event run sequentially regardless of `hook.jobs`.
> +	Only configured (named) hooks need to declare this. Traditional hooks
> +	found in the hooks directory do not need to, and run in parallel when
> +	the effective job count is greater than 1. See linkgit:git-hook[1].

Thanks for adding this setting, this addresses my most important concern
with this patch series.

>  hook.jobs::
>  	Specifies how many hooks can be run simultaneously during parallelized
>  	hook execution. If unspecified, defaults to 1 (serial execution).
> ++
> +This setting has no effect unless all configured hooks for the event have
> +`hook.<friendly-name>.parallel` set to `true`.

I guess that's a fair constraint for now. We can still iterate going
forward and have those marked as parallelizable run in parallel, while
running the others sequentially.

> diff --git a/hook.c b/hook.c
> index c4872d8707..a60dac5a60 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -120,6 +120,7 @@ struct hook_config_cache_entry {
>  	char *command;
>  	enum config_scope scope;
>  	unsigned int disabled:1;
> +	unsigned int parallel:1;
>  };

I'd recommend to use a proper bool here.

> @@ -223,6 +226,10 @@ static int hook_config_lookup_all(const char *key, const char *value,
>  		default:
>  			break; /* ignore unrecognised values */
>  		}
> +	} else if (!strcmp(subkey, "parallel")) {
> +		int v = git_parse_maybe_bool(value);
> +		if (v >= 0)
> +			strmap_put(&data->parallel_hooks, hook_name, (void *)(uintptr_t)v);

Do we want to warn on unparseable values?

> @@ -541,21 +553,75 @@ static void run_hooks_opt_clear(struct run_hooks_opt *options)
>  	strvec_clear(&options->args);
>  }
>  
> +/* Determine how many jobs to use for hook execution. */
> +static unsigned int get_hook_jobs(struct repository *r,
> +				  struct run_hooks_opt *options,
> +				  struct string_list *hook_list)
> +{
> +	/*
> +	 * Hooks needing separate output streams must run sequentially.
> +	 * Next commit will allow parallelizing these as well.
> +	 */
> +	if (!options->stdout_to_stderr)
> +		return 1;
> +
> +	/*
> +	 * An explicit job count overrides everything else: this covers both
> +	 * FORCE_SERIAL callers (for hooks that must never run in parallel)
> +	 * and the -j flag from the CLI. The CLI override is intentional: users
> +	 * may want to serialize hooks declared parallel or to parallelize more
> +	 * aggressively than the default.
> +	 */
> +	if (options->jobs)
> +		return options->jobs;

Hm, okay. I feel like this behaviour is somewhat surprising given that
it now operates different compared to the config value. But arguably,
there is no reason why a caller of git-hook(1) should explicitly set
this flag as it should be under control of the user, unless they have a
very good reason to override the number of jobs.

So maybe this is fine, but I think it needs to be called out explicitly
in our docs.

> +	/*
> +	 * Use hook.jobs from the already-parsed config cache (in-repo), or
> +	 * fallback to a direct config lookup (out-of-repo).
> +	 * Default to 1 (serial execution) on failure.
> +	 */
> +	if (r && r->gitdir && r->hook_config_cache)
> +		/* Use the already-parsed cache (in-repo) */
> +		options->jobs = r->hook_jobs ? r->hook_jobs : 1;
> +	else
> +		/* No cache present (out-of-repo call), use direct cfg lookup */
> +		if (repo_config_get_uint(r, "hook.jobs", &options->jobs))
> +			options->jobs = 1;

We first have a check for `r != NULL`, but if I'm not mistaken
`repo_config_get_uint()` will cause us to unconditionally deref `r`.

> diff --git a/hook.h b/hook.h
> index 7c8c3d471e..494f74345f 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -35,6 +35,13 @@ struct hook {
>  		} configured;
>  	} u;
>  
> +	/**
> +	 * Whether this hook may run in parallel with other hooks for the same
> +	 * event. Only useful for configured (named) hooks. Traditional hooks
> +	 * always default to 0 (serial). Set via `hook.<name>.parallel = true`.
> +	 */
> +	unsigned int parallel:1;
> +
>  	/**
>  	 * Opaque data pointer used to keep internal state across callback calls.
>  	 *

This should likely also be a boolean.

Patrick