Re: [PATCH v3 4/9] hook: allow parallel hook execution

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2026, 20:46 UTC
Message-ID: <xmqqjyvcstvi.fsf@gitster.g>
In-Reply-To: <20260309133739.294555-5-adrian.ratiu@collabora.com>

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

Show 6 quoted lines

> +hook.<name>.parallel::
> +	Whether the hook `hook.<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`.

This is very conservative and safe default.

Show 5 quoted lines

> @@ -307,6 +316,7 @@ static void build_hook_config_map(struct repository *r,
>  			entry->command = command ? xstrdup(command) : NULL;
>  			entry->scope = scope;
>  			entry->disabled = is_disabled;
> +			entry->parallel = (int)(uintptr_t)par;

Hmm. The source "par" is (void *) and the destination .parallel member is a single bit, so would this

			entry->parallel = !!par;

be the same? A cast first to uintptr_t, presumably not to lose bits, and then casting it down to potentially narrower int made me wonder what else is going on here that is tricky.

Show 17 quoted lines

> +/* 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)
> +{
> +	unsigned int jobs;
> +
> +	/*
> +	 * Hooks needing separate output streams must run sequentially. Next
> +	 * commits will add an extension to allow parallelizing these as well.
> +	 */
> +	if (!options->stdout_to_stderr)
> +		return 1;
> +
> +	/* An explicit job count (FORCE_SERIAL jobs=1, or -j from CLI). */
> +	if (options->jobs)
> +		return options->jobs;

This could be risky for hooks that claim they do not want to run with others at the same time, but the CLI user ought to know what they are using, so this override is very much appreciated. After all, the override may be serializing an overly optimisitic set of hooks that want to run in parallel to avoid interaction between them.

Show 21 quoted lines

> +	/*
> +	 * Use hook.jobs from the already-parsed config cache (in-repo), or
> +	 * fall back to a direct config lookup (out-of-repo).  Default to 1.
> +	 */
> +	if (r && r->gitdir && r->hook_config_cache)
> +		/* Use the already-parsed cache (in-repo) */
> +		jobs = r->hook_config_cache->jobs ? r->hook_config_cache->jobs : 1;
> +	else
> +		/* No cache present (out-of-repo call), use direct cfg lookup */
> +		jobs = repo_config_get_uint(r, "hook.jobs", &jobs) ? 1 : jobs;
> +
> +	/*
> +	 * Cap to serial any configured hook not marked as parallel = true.
> +	 * This enforces the parallel = false default, even for "traditional"
> +	 * hooks from the hookdir which cannot be marked parallel = true.
> +	 */
> +	for (size_t i = 0; jobs > 1 && i < hook_list->nr; i++) {
> +		struct hook *h = hook_list->items[i].util;
> +		if (h->kind == HOOK_CONFIGURED && !h->parallel)
> +			jobs = 1;
> +	}

Losing "jobs > 1 &&" from the termination condition and instead explicitly "break;" out when we demote jobs to 1 would be easier to read, even though it would spend two more lines, i.e.,

	for (size_t i = 0; i < hook_list->nr; i++) {
        	struct hook *h = ...;
		if (...) {
			jobs = 1;
			break;
		}
	}

Other than that, very cleanly written.

Thanks.