From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Wed, 18 Mar 2026 18:02:23 GMT
Subject: Re: [PATCH v3 4/9] hook: allow parallel hook execution
Message-ID: <871phh2ez4.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <xmqqjyvcstvi.fsf@gitster.g>

On Sun, 15 Mar 2026, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Yes, I will use the double negation here on the re-roll, it's the same
and much cleaner (thank you), since it's a simple on/off toggle.

it'sAs you saw on other series, double negation just doesn't occur to me. :)

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

Yes, that was my reasoning as well.

I'll expand the comment to explain a bit better why we're doing it.

>> +	/*
>> +	 * 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;
> 		}
> 	}

Indeed, that is much easier to read, will do it in the next re-roll,
thanks again.

I was thiking in pseudo-code something like this when I wrote it:

if (jobs > 1); then
   for each hook in list do
       if hook is configured and not parallel then
          jobs = 1;

Then just integrated the (jobs > 1) condition into the for loop below
it, the break short-circuit didn't occur to me. :)

>
>
> Other than that, very cleanly written.

Many thanks again, this function I think is the "hairiest" part of the
series, at least it's what gave me the most trouble and also the part
where I spent most time until I could convince myself it's correct.