Add Gem hook plugin support

[seanmil]

Support loading Overcommit hooks supplied by Gems in the same
fashion as hooks that can be provided in Repo-Specific hooks.

[seanmil]

I don't know why the Appveyor run is failing with a Rubocop error when Travis can run Rubocop fine. It doesn't look to be related to anything in this PR though.

[sds]

Thanks for the pull request, @seanmil.

Very supportive of a system to enable sharing of hooks via gems.

If we can avoid the creation of a new concept + loading mechanism, we might be able to simplify what is needed to support this (but it is understandable why you implemented in this way).

Before we go into that, some more concrete feedback:

• Loading the gems in the ConfigurationValidator allows you execute arbitrary code, which would break expectations users have about Overcommit today. If you look at how the current plugin hook loader is implemented, it checks to see if signatures have changed before attempting to load the plugins. This needs to be fixed before we move forward.
• It's not clear what would happen if a gem defined a hook that eventually became a built-in hook that shipped with Overcommit. Which one would we expect to run?

This raises some questions about perhaps fundamentally changing how Overcommit loads hooks. Based on #745, it sounds like there is also appetite to share configuration as well, so perhaps we can take some cues from RuboCop to support sharing both configuration and actual code.

Therefore, the Overcommit configuration file itself probably needs to change to make it explicit about where configuration or code is coming from. All of this needs to be done in a way to preserve the property that any change in any of the upstream gems or configuration would require the user to accept (if they have signature verification enabled).

I don't have bandwidth to dig into this more deeply, but I hope these points explain why we cannot merge this PR as-is. I can also appreciate that you may not have bandwidth to research these questions either. But if you do, very open to a different pull request that attempts to address these concerns holistically.

[sds]

I may be misunderstanding, but since Overcommit hooks are themselves in the load path, won't this still return true even for built-in hooks?

[seanmil]

Thank you for your thorough and thoughtful response.

If we can avoid the creation of a new concept + loading mechanism, we might be able to simplify what is needed to support this (but it is understandable why you implemented in this way).

My initial implementation was built within the core hooks loader, but there were drawbacks. It didn't provide any way for a user to indicate they didn't want to load the hooks, and then I later realized/remembered that when invoked outside of a bundler context the $LOAD_PATH doesn't have the gem path on it until it is required. Once I got done looking at trying to fix both of those problems it seemed to make more sense to put them in a separate loader. But I agree that folding them into an existing one might be useful.

* Loading the gems in the `ConfigurationValidator` allows you execute arbitrary code, which would break expectations users have about Overcommit today. If you look at how the current plugin hook loader is implemented, [it checks to see if signatures have changed](https://github.com/sds/overcommit/blob/192c84bad54c6d4ef2c3ab887968bb2ccd26cc0b/lib/overcommit/hook_loader/plugin_hook_loader.rb#L10) _before_ attempting to load the plugins. This needs to be fixed before we move forward.

I decided that was okay because my thinking was that unlike hooks which are delivered from a repository, hooks from a Ruby gem have to be installed by the system/user. Either via gem install at the system level or by the user via a bundle install. Even using the gemfile Overcommit configuration option won't just go to the Internet and fetch uninstalled gems - it tells you to use bundle install. So, in both scenarios the user (or authorized system administrator) has had to be involved in intentionally putting the gems on the system. I didn't see the code exposure path here being any different than any other gem in the system.

That said, if you strongly believe that gem-based hooks also need to be covered by the signature bit that probably wouldn't be hard to implement.

* It's not clear what would happen if a gem defined a hook that eventually became a built-in hook that shipped with Overcommit. Which one would we expect to run?

That's a great point. I debated quite a bit about putting the hooks in the same load path as the built-in Overcommit hooks. After reading your feedback I think that probably the best thing is to expect the hooks to be in some other load path and provide some way for an Overcommit hook add-on gem to provide that path to Overcommit. In terms of load precedence, I think the safest is that the built-in hooks always win.

Maybe supporting a configuration file somewhat like this makes sense:

gem_plugins:
  Custom: 'overcommit-custom'

PreCommit:
  Custom::MyCustomPreCommitHook:
    enabled: true

It would be clear which were external/custom hooks, which gem provided them (if there were multiple) and there would be no chance of namespace conflicts.

Then in the custom hook Overcommit expects to be able to require overcommit-custom/hooks (the path provided in the config, plus hooks) and in there lives the normal hooks structure.

This raises some questions about perhaps fundamentally changing how Overcommit loads hooks. Based on #745, it sounds like there is also appetite to share configuration as well, so perhaps we can take some cues from RuboCop to support sharing both configuration and actual code.

I could see the above syntax being extendable to something like:

inherit_gem:
  Custom: './default.yml`

Where the path is resolved based on the gem path. I'm not sure I want to tackle inheritable configuration as part of this, just noting that I think it could be added.

Let me know what you think about the syntax above. I don't know if I'll have any more time to work on it in the near future, but I may.