Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

From: Jeff King <peff@peff.net>
Date: Mar 3, 2026, 13:28 UTC
Message-ID: <20260303132855.GA748945@coredump.intra.peff.net>
In-Reply-To: <20260302191704.1814567-2-adrian.ratiu@collabora.com>

On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:

> It is important to call the hook_exists() API provided by hook.[ch]
> because it covers both config-defined hooks and the "traditional"
> hooks from the hookdir. find_hook() only covers the hookdir hooks.

Ah, OK. Traditionally hook_exists() was just a thin wrapper over find_hook(), but it looks like that changed in your series. But either way, it much more clearly expresses the intent to use hook_exists().

So this obviously looks good, but just some random thoughts below.

Show 6 quoted lines

> @@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,
>  	int saved_stderr = -1;
>  	int ret;
>  
> +	if (!hook_exists(the_repository, hook_name))
> +		return 0;

It is a little inelegant that we have to look up the hook data separately ourselves here, and then it will be done again in run_hooks_opt(). But I don't think there is an easy way to reorganize it, short of something like:

  struct hook myhook = HOOK_INIT;

  load_hooks(&myhook, hook_name);
  if (myhook.nr)
	return 0; /* no hooks of this type */

  ...other prep work...

  run_hooks_opt(repo, &myhook, &opt);

I doubt that is worth it, as the lookup process should not be too expensive. It looks like we cache the config parts of the lookup already. We call access() to find the traditional hooks on each lookup, but that is also true of the code before your series.

It would not matter at all for pre-receive, for example, but for something like update, I guess we are doing a bunch of pointless access() calls that could be cached. But again, not new in your series, and nobody has really noticed. So we can either treat it as an optimization for later, or just leave it be forever.

-Peff