On Thu, Jul 22 2021, Emily Shaffer wrote:
> On Fri, Jul 16, 2021 at 10:36:10AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>
>> 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@xxxxxxxxxxxxxxxxxxx/, 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, [...]);
>
> Ahh, and then let RUN_HOOKS_OPT_INIT_ASYNC set jobs to 0 ("go look it
> up"). Yeah, that makes sense.
>
> Shout if somehow you meant to leave just one initializer macro;
> otherwise, I'll do it this way - with RUN_HOOKS_OPT_INIT_ASYNC and
> RUN_HOOKS_OPT_INIT_SYNC. I think it's valuable for hook callers to make
> it very plain at the callsite whether they're parallelizable or not, and
> I think
>
> struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> opt.jobs = 0;
>
> doesn't make that as obvious.
Yes agreed, sorry about the ambiguity, I meant we should have two init
macros, just like e.g. STRING_LIST_INIT_NODUP and STRING_LIST_INIT_DUP.
