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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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