Re: [PATCH 2/9] hook: allow parallel hook execution

> 
> 
> On Thu, Jul 15 2021, Emily Shaffer wrote:
> 
> > In many cases, there's no reason not to allow hooks to execute in
> > parallel. run_processes_parallel() is well-suited - it's a task queue
> > that runs its housekeeping in series, which means users don't
> > need to worry about thread safety on their callback data. True
> > multithreaded execution with the async_* functions isn't necessary here.
> > Synchronous hook execution can be achieved by only allowing 1 job to run
> > at a time.
> >
> > Teach run_hooks() to use that function for simple hooks which don't
> > require stdin or capture of stderr.
> 
> This doesn't mention...
> 
> >  	int ret;
> > -	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> > +	struct run_hooks_opt opt;
> >  
> > +	run_hooks_opt_init_sync(&opt);
> 
> 
> ...why we need to bring the s/macro/func/ init pattern, back, but looking ahead...
> 
> > +int configured_hook_jobs(void)a
> > +{
> > +	int n = online_cpus();
> > +	git_config_get_int("hook.jobs", &n);
> > +
> > +	return n;
> > +}
> > +
> >  int hook_exists(const char *name)
> >  {
> >  	return !!find_hook(name);
> > @@ -117,6 +125,26 @@ struct list_head* hook_list(const char* hookname)
> >  	return hook_head;
> >  }
> >  
> > +void run_hooks_opt_init_sync(struct run_hooks_opt *o)
> > +{
> > +	strvec_init(&o->env);
> > +	strvec_init(&o->args);
> > +	o->path_to_stdin = NULL;
> > +	o->jobs = 1;
> > +	o->dir = NULL;
> > +	o->feed_pipe = NULL;
> > +	o->feed_pipe_ctx = NULL;
> > +	o->consume_sideband = NULL;
> > +	o->invoked_hook = NULL;
> > +	o->absolute_path = 0;
> > +}
> > +
> > +void run_hooks_opt_init_async(struct run_hooks_opt *o)
> > +{
> > +	run_hooks_opt_init_sync(o);
> > +	o->jobs = configured_hook_jobs();
> > +}
> 
> ...okey, so it's because you brought back the "call jobs function" in
> one of the init functions.
> 
> I had a comment in a previous round, I found
> https://lore.kernel.org/git/87lf7bzbrk.fsf@evledraar.gmail.com/, but I
> think there was a later one where I commented on the "jobs" field
> specifically.
> 
> Anyway, it seems much easier to me to just keep the simpler macro init
> and then:
> 
> > -	if (options->jobs != 1)
> > -		BUG("we do not handle %d or any other != 1 job number yet", options->jobs);
> > -
> >  	run_processes_parallel_tr2(options->jobs,
> >  				   pick_next_hook,
> >  				   notify_start_failure,
> 
> There's this one place where we use the "jobs" parameter, just do
> something like this there:
>         
>         int configured_hook_jobs(void)
>         {
>                 static int jobs;
>                 if (!jobs)
>                     return jobs;
>                 if (git_config_get_int("hook.jobs", &jobs))
>                     jobs = online_cpus();
>                 return jobs;
>         }
> 
> I.e. you also needlessly call online_cpus() when we're about to override
> it in the config. The git_config_get_int()'s return value indicates
> whether you need to do that. Then just:
> 
>     int jobs = options->jobs ? options->jobs : configured_hook_jobs();
>     run_processes_parallel_tr2(jobs, [...]);

> 
> Or some such, i.e. we can defer getting the job number away from startup
> to when we actually need to start those jobs, and your whole use of a
> function init pattern came down to doing that really early.
> 
> As an aside if you /do/ need to do init-via-function my 5726a6b4012 (*.c
> *_init(): define in terms of corresponding *_INIT macro, 2021-07-01) in
> "next" shows a much nicer way to do that. I.e. you'd just do:
> 
>     void run_hooks_opt_init_sync(struct run_hooks_opt *o)
>     {
>          struct run_hooks_opt blank = RUN_HOOKS_OPT_INIT;
>          memcpy(o, &blank, sizeof(*o));
>     }
> 
>     void run_hooks_opt_init_async(struct run_hooks_opt *o)
>     {
>         run_hooks_opt_init_sync(o);
>         o->jobs = configured_hook_jobs();
>     }
> 
> In some cases we do actually need to do init via functions, but can init
> a large option via the macro, which IMO is nicer to read, but here I
> think we don't need the functions at all per the above.