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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> 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.

> 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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux