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
