Re: [PATCH v2 0/7] Multiple hook support

From: Jeff King <peff@peff.net>
Date: May 16, 2019, 04:51 UTC
Message-ID: <20190516045140.GA7241@sigill.intra.peff.net>
In-Reply-To: <20190514015928.GG7458@genre.crustytoothpaste.net>

On Tue, May 14, 2019 at 01:59:28AM +0000, brian m. carlson wrote:

Show 13 quoted lines

> There are two aspects here which I think are worth discussing. Let's
> discuss the inheritance issue first.
> 
> Order with multiple hooks matters. The best hook as an example for this
> is prepare-commit-msg. If I have a hook which I want to run on every
> repository, such as a hook that inserts some sort of ID (bug tracker,
> Gerrit, etc.), that hook, due to inheritance, *has* to be first, before
> any other prepare-commit-msg hooks. If I want a hook that runs before
> it, perhaps because a particular repository needs a different
> configuration, I have to wipe the list and insert both hooks. I'm now
> maintaining two separate locations for the command lines instead of just
> inserting a symlink to the global hook and dropping a new hook before
> it.

This part confuses me. In the config based scheme you describe, you have to mention the original hook again. But isn't that exactly what creating a symlink in your hooks directory is doing?

And in fact, I think a config based scheme can be a lot more flexible in the end, simply because the parser _does_ see all of the hooks (whereas in the hook directory scheme, we only ever look in one directory). So we can have an option to sort them before running: alphabetically, system-to-repo order, repo-to-system order, etc.

Show 8 quoted lines

> The second issue here is that it's surprising. Users don't know how to
> reset a configuration option because we don't have a consistent way to
> do that. Users will not expect for there to be multiple ways to set
> hooks. Users will also not expect that their hooks in their
> configuration aren't run if there are hooks in .git/hooks. Tooling that
> has so far used .git/hooks will compete with users' global configuration
> options, which I guarantee you will be a surprise for users using older
> versions of tools.

I don't agree here. If the rule is "config takes precedence", and "if config is absent "behave as if .git/hooks/whatever was in the config", then the transition is easy to explain. And nobody sees any change at all until they decide to set the config. It would probably also be prudent to issue a warning if there's config _and_ an on-disk hook file (or even run them both, though then you open up the question of ordering).

Which isn't to say it's impossible for a person to get confused (new tool sets the config, disabling their old hook?). But I think any transition to multi-hook support (including your directory scheme) is going to have some possibility that tools automatically setting things up behind the scenes is going to confuse a user.

> It also provides a convenient place for hooks to live, which a
> config-based option doesn't. We'll need to invoke things using /bin/sh,
> so will they all have to live in PATH? What about one-offs that don't
> really belong in PATH?

Actually, my biggest beef with the current hooks mechanism is that it's an _inconvenient_ place for them to live.

  - it's not version controlled, and putting a repository inside another
    repository is awkward (though it at least works for the bare case)

  - touching the filesystem is awkward and expensive if you have a large
    number of repositories whose hooks you want to update

  - it requires a manual step to modify the filesystem after a fresh
    clone (as opposed to putting things in ~/.gitconfig). Technically
    one can solve this by modifying .../share/git/templates, but that
    has its own issues.

In my world-view config-based hooks would just be run with SHELL_PATH, just like our other config options. If you don't want one-offs in your PATH, then use absolute paths (just like you would have to for symlinks). If you really don't know where to put one-off hooks, perhaps we could document a base directory from which the hook script is run, and you could put it in a directory in .git and use a relative path? You could even call that directory "hooks", but I suspect that would be too confusing. :)

-Peff