Re: [PATCH 0/5] Multiple hook support

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 24, 2019, 08:10 UTC
Message-ID: <87tvensu1a.fsf@evledraar.gmail.com>
In-Reply-To: <20190424004948.728326-1-sandals@crustytoothpaste.net>

On Wed, Apr 24 2019, brian m. carlson wrote:

Show 12 quoted lines

> Oftentimes, people want to use multiple of the same kind of hook. This
> may be because software or a project they use requires a given hook, but
> they would also like to have a custom hook, or because they're using
> multiple pieces of software that both require involvement with the same
> hook.
>
> This series introduces support for multiple hooks by using a ".d"
> directory. For example, instead of using a single
> ".git/hooks/post-checkout" file, you'd use multiple files within
> ".git/hooks/post-checkout.d". This is the standard Unix paradigm for
> multiple files and should be familiar to most people. Hooks are executed
> in sorted order.

I think it's interesting if people can chime in with current in-the-wild implementations of this.

I know GitLab's the best, not because I was in any way involved in it, I've just dealt with writing hooks for it: https://docs.gitlab.com/ee/administration/custom_hooks.html#chained-hooks-support

There:

 1. The instance has a 'hooks' dir in the .git repo that's a symlink to
    /my/global/hooks. They could also use a /etc/gitconfig
    core.hooksPath for this part, but whatever.

 2. That /my/global/hooks has e.g. a /my/global/hooks/pre-receive that
    git itself runs, which is a trampoline script that runs all over the
    place and executes global/per-project hooks (which live in
    .git/custom_hooks/).

 3. "The hooks of the same type are executed in order and execution
     stops on the first script exiting with a non-zero value."

I wonder if the eventual goal of this facility would be to get such users on board with using git's native feature for this. This series is most of the way there for GitLab's case, but not quite.

Show 13 quoted lines

> To preserve backwards compatibility, we don't run the hooks in the ".d"
> directory if the single file is a valid hook (i.e. it exists and is
> executable). This is because some people already have multiple hook
> scripts configured, and if we ran them both, we'd run the hooks twice.
> This would be bad for e.g. the prepare-commit-msg hook. This is also the
> least surprising behavior.
>
> We check each hook for its exit status, even if the hook normally
> ignores exit status, and if it fails, we abort executing further hooks.
> This provides an easy way to reason about what the exit status is when a
> hook fails; we need not consider how to handle multiple failing hooks.
> It's also consistent among all hooks, whether they care about exit
> status or not.

Others have replied to this already. I linked to the 2016 discussion of my RFC for this in https://public-inbox.org/git/87wojjsv9p.fsf@evledraar.gmail.com where I made the same choice. Some points on that:

 * There was the mention of "but what if someone wants to run them all",
   e.g. for logging to N logging systems where one fails, that's been
   brought up again this time around by others.

 * The case I find more interesting is the ability to run the hooks in
   parallel. Saying "glob order and exit on first fail" categorically
   closes the door to that.

Show 5 quoted lines

> I've talked with some people about this approach, and they've indicated
> they would prefer a configuration-based approach. I've tried to make the
> series such that it can be replaced with such an approach if that's the
> decision we make. It should be easy enough to simply replace find_hooks
> with an appropriate implementation and update the test framework.

I think whatever opinions we all have on the current implementation it's OK to get this in in *some* form and just made it configurable or whatever later.

Most of the work is the ability to run N hooks, how exactly that happens can be tweaked later, and if this series lands and someone isn't 100% happy with the semantics they're no worse off than they are now.

I.e. we could get something like this in its current form, and later have:

    core.hooksOrder = glob | random
    core.hooksHaltOnError = true | never

Where we'd say that what this series does is core.hooksOrder=glob and core.hooksHaltOnError=true, but that e.g. parallel "run them all" could be done in the future with core.hooksOrder=random & core.hooksHaltOnError=never