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

On Tue, Aug 24, 2021 at 05:01:09PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Wed, Aug 18 2021, Emily Shaffer wrote:

quoted

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

Hm, I guess I hadn't see [x|y|z] (or paid attention to it). Anyway, yes,
I think [(-j|--jobs) <n>] is probably most correct here: "entirely
optionally you can provide a jobs arg; but if you do it must start with
-j or --jobs and it must contain a number". I guess you could also say
[-j <n> | --jobs <n>] but that might leave the reader to consider
whether <n> has a different meaning in each? Anyway, doesn't matter
really, and thanks for completely nerd-sniping me ;)

quoted

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

Done, thanks.

quoted

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

Yeah, I'm happy to implement that later when someone complains... ;)

quoted

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

Nah, I think I just wasn't paying much attention to where it goes. It
makes more sense to put this by the 'struct run_hooks_opt' decl, I'll
move mine instead.

 - Emily