From: Emily Shaffer <emilyshaffer@google.com>
Date: Thu, 26 Aug 2021 22:36:26 GMT
Subject: Re: [PATCH v3 2/6] hook: allow parallel hook execution
Message-ID: <YSgXarfqwQVmO8Af@google.com>
In-Reply-To: <87wnoaevbe.fsf@evledraar.gmail.com>

On Tue, Aug 24, 2021 at 05:01:09PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> 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...

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 ;)

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

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

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