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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 14, 2019, 02:26 UTC
Message-ID: <20190514022653.GA34155@google.com>
In-Reply-To: <20190514015928.GG7458@genre.crustytoothpaste.net>

Hi,

brian m. carlson wrote:

> On Mon, May 13, 2019 at 05:51:01PM -0700, Jonathan Nieder wrote:
>> brian m. carlson wrote:

Show 24 quoted lines

>>>                          the fact that inheritance in the configuration
>>> is in-order and can't be easily modified means that it's not likely to
>>> be very useful, but it is likely to be quite surprising for the average
>>> user.
>>
>> Can we discuss this some more?  What would it take to make it likely
>> to be useful in your view?
>
> 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.
>
> I don't think there's a good way to make it easier unless we radically
> customize the way configuration is done.

Wouldn't a separate config item e.g. to reverse order (or to perform whatever other customization seems appropriate) cover this?

In other words, use the standard config convention for the set of hooks, and treat the order in which they are invoked as a separate question. You could even use the hooks.d style alphabetical order convention.

[...]

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

I agree that it's underdocumented and underimplemented. But I'm not aware of any other method that Git provides to reset a configuration item. What is it inconsistent with?

Show 6 quoted lines

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

Indeed, in the long term I think we should remove the .git/hooks/ mechanism entirely.

In the shorter term, I think the kind of inconsistency you're referring to applies to hooks.d as well.

> The new behavior, which puts everything in the same directory
> (.git/hooks) is much easier to reason about.

That's a good point: a .git/hooks/README sounds like it would be helpful here.

[...]

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

This hasn't been a problem for remote helpers, merge drivers, etc in the past. Why are hooks different?

To be clear, I think it's a reasonable problem to solve, and I've actually been surprised that it hasn't been a problem for people.

[...]

> I agree this is an advantage if they don't hit the ordering issue.

Wonderful. Sounds like if I do some work on the ordering issue, then we have a path forward.

>                                                                    I
> think a lot of the common use cases where this approach has benefits can
> be handled well with core.hooksPath and hooks that can turn themselves
> on or off depending on the repository config.

I think core.hooksPath attempted to solve this problem, but it has several deficiencies:

1. It assumes a single, centrally managed hooks directory, and there's
   no standard for where that directory lives.  This means that it
   can't be counted on by tools like "git secrets" --- instead, each
   particular installation has to set up a custom hooks directory for
   themselves.

2. Since it assumes a single, centrally managed hooks directory,
   customizations in a single repository (e.g. to enable or disable a
   single hook) require duplicating the entire directory.

3. It had no migration path defined to becoming the default, so it
   doesn't end up being discoverable.  core.hooksPath is designed as
   a special case, making it hard to debug, instead of being a
   mainstream setting that can be recommended as a future default.

Show 7 quoted lines

> What might be an interesting approach that would address these concerns
> is a core.globalHooks[0] option that points to a set (or sets,
> depending) of multiple-hook directories. We then enumerate hooks in sort
> order, considering both the global and the local directories as one
> unit, perhaps with some way of disabling hooks. I'm not planning on
> working on this myself, but I wouldn't be opposed to seeing someone else
> work on it.

This sounds overflexible to me. Because of that, I don't think it would end up as a default, so we wouldn't have a path to improving our security stature.

If I implement a config based multiple hooks feature with name based ordering, would that be useful to you?

Thanks, Jonathan

> [0] Better name suggestions are, of course, welcome.