Subject: Re: [PATCH v3 2/6] hook: allow parallel hook execution

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 24 Aug 2021 17:01:09 +0200

On Wed, Aug 18 2021, Emily Shaffer wrote:

> -'git hook' run [--to-stdin=<path>] [--ignore-missing] <hook-name> [-- <hook-args>]
> +'git hook' run [--to-stdin=<path>] [--ignore-missing] [(-j|--jobs) <n>]
> +	<hook-name> [-- <hook-args>]

As an aside I wondered if it shouldn't be [[-j|--jobs] <n>], but grepped
around and found that (x|y|z) means a mandatory pick of x, y or z, but
[x|y|z] means that, plus possibly picking none, I think.

So this is fine, just something I wondered about...

> +-j::
> +--jobs::
> +	Only valid for `run`.
> ++
> +Specify how many hooks to run simultaneously. If this flag is not specified, use
> +the value of the `hook.jobs` config. If the config is not specified, use the

s/use the value/uses the value/

Also we usually say "of the XYZ config, see linkgit:git-config[1]", or
something to that effect when we mention config variables. Perhaps we
should do the same here.

> +number of CPUs on the current system. Some hooks may be ineligible for
> +parallelization: for example, 'commit-msg' intends hooks modify the commit
> +message body and cannot be parallelized.

Not something that *needs* to happen in this series, but I wonder if we
shouldn't have per-type config here too, so users could force it even
for those hook types if they want.

> -#define RUN_HOOKS_OPT_INIT { \
> -	.env = STRVEC_INIT, \
> -	.args = STRVEC_INIT, \
> -}
> -
>  /*
>   * To specify a 'struct string_list', set 'run_hooks_opt.feed_pipe_ctx' to the
>   * string_list and set 'run_hooks_opt.feed_pipe' to 'pipe_from_string_list()'.
> @@ -111,6 +113,18 @@ struct hook_cb_data {
>  	int *invoked_hook;
>  };
>  
> +#define RUN_HOOKS_OPT_INIT_SERIAL { \
> +	.jobs = 1, \
> +	.env = STRVEC_INIT, \
> +	.args = STRVEC_INIT, \
> +}
> +
> +#define RUN_HOOKS_OPT_INIT_PARALLEL { \
> +	.jobs = 0, \
> +	.env = STRVEC_INIT, \
> +	.args = STRVEC_INIT, \
> +}
> +

Ditto earlier comments about whitespace churn, i.e. I can just move this
around in the base topic, so the diff here is the change/addition, not
also moving things around.