On Wed, May 26 2021, Emily Shaffer wrote:
> After much delay and $DAYJOB, here is v9.
>
> - Addressed nits in reviews on v8
> - sendemail-validate hook becomes non-parallelized; updated to use
> Ævar's updated system_or_die() function
> - changed strbuf to char* in hooks_list
> - Attempted to do so in run_command's stdout callback, but this made
> length protection difficult, so stuck with strbuf there.
> - test_i18ncmp -> test_cmp
> - Stop doing i18n lego in run_hooks()
> - Checked that run_hooks_opt_init() is always separated by a space from
> variable decl blocks
> - Checked for early returns which may skip run_hooks_opt_clear(); this
> resulted in minimizing the scope of run_hooks_opt in most places
> - Got rid of native-hooks.txt. It was a nice idea, but not attached to a
> large and slow series like this one.
> - In traces, log the name of the hook (e.g. "pre-commit") instead of the
> name of the executable (e.g. "/home/emily/check-for-debug-strings");
> the executable name is tracelogged as part of argv anyways, and we
> want to be able to tell which hook was responsible for invoking the
> executable in question.
In v8 I had some comments about the overall approach here. I've got to
say I'm disappointed that that's neither been replied to or in any way
addressed:
https://lore.kernel.org/git/87mtv8fww3.fsf@xxxxxxxxxxxxxxxxxxx/
Also referenced in several follow-up discussions, including Junio's
comment of "OK, Emily, I guess the ball is in your court now?":
https://lore.kernel.org/git/?q=87mtv8fww3.fsf%40evledraar.gmail.com
Basically I think that this very large series could be easily split into
much more digestable chunks if it were re-arranged. Right now it's
essentially a 37 patch mix of, in order:
1. Design doc
2. Add "git hook" with "run", "list" etc.
3. Make that more fully featured, support config, legacy hooks et.c
4. Implement parallel hooks
5. Go through N existing hook callers and migrate them to "git hook run"
6. Some misc updates, e.g. to docs
If it were instead:
1. Add "git hook run", only supports "old/legacy" .git/hooks/ hooks
2. Go through N existing hook callers and migrate them to "git hook run"
That would be at least half of this series, and it would be much more
narrow change that would demonstrably retain all existing
semantics. We'd simply call our hooks via a command wrapper instead of
via run_command(<path to name>) as we do now. So we could have that land
and then focus on the actual behavior changes later.
In earlier rounds/the above E-Mail I asked something to the effect of if
you thought that was a good approach, whether disagreed, or just thought
you didn't have time for it.
I'm still keen to help get this in, but given the non-responses don't
know where you stand on it. I suppose I could re-arrange this myself and
submit an alternate "prep" series to rebase this on, but wanted to get
your take first.
Aside from the suggestion of splitting it up:
In the above referenced correspondence I expressed concerns that the
config layout you're proposing needlessly creates complexity requiring a
"git hook list" etc. command to solve, as opposed to not having that, or
have it by a trivial synonym for a "git config --show-origin --list"
invocation.
I'm still interested in what you think of that, as being able to
normalize that more invites getting rid of more complexity without
impacting the end-goal of hooks via config.
But in any case such a discussion could be had later and on a smaller
series if the refactoring of existing hook running was split up from the
big semantic changes here, which are currently tied up with that
refactoring.
Thanks!
