Re: [PATCH v2 00/13] hook.[ch]: new library to run hooks + simple hook conversion

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Oct 12, 2021 at 03:30:25PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> This series is part 2 of an incremental restart of the greater
> "config-based hooks" topic by Emily Shaffer that I've been helping
> with. See [1] for part 1, and [2] (search for "mark2") for a summary
> of parts to come.
> 
> This goes on top of the "ab/config-based-hooks-1" topic that's already
> in "next" and marked for graduation to "master".
> 
> In this topic we build upon the skeleton hook.[ch] library and build
> infrastructure added in part 1 and add a hook running library and new
> "git hook" command.
> 
> The new "git hook" command is (for now) only for the benefit of
> in-tree non-C programs that need to run hooks, and this series
> converts git-send-email and git-p4 to use it.
> 
> At the end of the series we remove
> "run_hook_{le,ve}()" from run-command.c, as we've migrated all its
> callers.
> 
> What we don't do is convert any of the more complex in-tree hooks that
> require input on stdin such as "pre-receive" and the like, that's in
> later parts once this lands.
> 
> This series is approximately patch 6-20/36 of the previous 36 patch
> ab/config-based-hooks-base topic. A range-diff to that v5[3] is
> included below.
> 
> The changes since that v5 are rather trivial, they are:
> 
>  * Formatting changes to reduce the diff to parts that come after this
>    (which Emily & I were juggling at the time), and re-flowing some
>    overly long lines.
> 
>  * The new test is now marked for SANITIZE=leak testing, with the new
>    test facility I've added recently (and which just landed in
>    "master"). They all pass, the new API doesn't leak.
> 
>  * I rewrote some of the git-send-email.perl code to avoid
>    de-duplication and hardcoding (just using intermediate variables).
> 
> Things to focus on reviewing:
> 
>  * This should all be pretty solid and well tested, but the git-p4.py
>    part in particular I've never tested for real (not having access to
>    p4), and think Emily hasn't either[4].

Correct. I seem to remember wanting to give this a test and not having access
to real perforce to try it against. 

> 
>    The relevant patch looks trivially correct to me[5], and I've tried it
>    out in the Python REPL. But if any of the CC'd people active in
>    git-p4.py development could give it some end-to-end testing that
>    would be much appreciated.
> 
>  * Both Python and Perl code now calls the in-between "git hook run"
>    command rather than calling hooks directly. Will this behave
>    differently due to any special behavior running via a git built-in
>    adds?
> 
>    I vaguely recall a third-party "git-foo" program breaking in the
>    past when invoked as "git foo" but not "git-foo", due to git
>    squatting on SIGINT, but none of that should be relevant here
>    (we're not starting a pager etc.).

I would be very surprised, assuming that we invoke 'git hook' in the same way
that those Python and Perl scripts invoke other 'git whatever' commands...

That said, once we invoke a user hook, all bets are off as far as "we're not
starting a pager or anything". A determined user could find a reason to invoke
a pager, editor, whatever during a hook. I'm curious to know more about this
remembered breakage.

Last time I looked at the range-diff; this time I'll look at the individual
patches, instead.

Note: I looked at a bunch of these offline, so sorry for the sudden
ingress of mails today ;)

 - Emily