Re: [PATCH v4 00/36] Run hooks via "git run hook" & hook library

From: Emily Shaffer <emilyshaffer@google.com>
Date: Aug 19, 2021, 00:17 UTC
Message-ID: <YR2jLdYQA5CVzX5h@google.com>
In-Reply-To: <cover-v4-00.36-00000000000-20210803T191505Z-avarab@gmail.com>

On Tue, Aug 03, 2021 at 09:38:26PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 
> This is a v4 re-roll of the "Base for "config-based-hooks" topic.

I performed the rebase of my series on top of this one, and did not find very much objectionable there, but do not consider this email a thorough review of the interdiff. I expect to have time for that later in the week.

Show 10 quoted lines

> Other updates:
> 
>  * In the base topic the s/Signed-off-by/Reviewed-by/g from René
>    change that Junio applied locally has been folded in.
> 
>  * Almost all the callers were just "one-shot" callers, I introduced a
>    new run_hooks_oneshot() function for those, which gets rid of the
>    verbosity around memory management, see e.g. the "builtin/gc.c" in
>    the range-diff below. That run_hooks_oneshot() can also take a NULL
>    set of options.

I am not so wild about this, to be honest, only because after my patch 2, it is hard to understand whether or not a hook is running in parallel or in series. It is extra code-reader overhead to know that run_hooks_oneshot() assumes that a hook can be parallelized, and I think that is unfortunate.

But that is a pretty minor complaint, and I do like the lower complexity, and I agree the defaults are "good enough" for most hook events. So I guess I'm asking for ideas on how to make it more obvious what this oneshot call does in terms of parallelism.

Thanks, and I hope to get to a fuller review of the interdiff soon.

 - Emily