Check compatibility with overcommit

[jzaefferer]

See https://github.com/brigade/overcommit

Sounds similar to husky.

[lencioni]

Relevant overcommit issue: sds/overcommit#405

[alisianoi]

First impressions on a project with hooks set up (jQuery Core, actually):

gem install overcommit
overcommit --install
overcommit --sign

at this point overcommit backs up all existing hooks. The two hooks from husky that jQuery uses (precommit and commitmsg) are moved to .git/hooks/old-hooks and replaced as symbolic links to a ruby script .git/hooks/overcommit-hook. So, overcommit takes full control over all the preexisting hooks.

A very interesting command is overcommit --list. If we are talking about integration, then we should be looking at CommitMsg hook of overcommit. By the looks of it, none of the default checks contradict the checks performed by commitplease, so we just need to be called from there.

Ok, the way to get called by overcommit is to configure its .overcommit.yml. I have tried the following:

CommitMsg:
  commitplease:
    enabled: true
    command: ['node node_modules/commitplease']

That fails with:

Hook must specify a `required_executable` or `command` that is tracked by git (i.e. is a path relative to the root of the repository) so that it can be signed

I am investigating further

[alisianoi]

I have tried several combinations of setting the .overcommit.yml, unsuccessfully yet. Primarily used sds/overcommit#338 for info, here is what I have tried so far:

CommitMsg:
  commitplease:
    enabled: true
    required_executable: 'node'
    command: ['node', './node_modules/.bin/commitplease']

Since jQuery Core also has a dedicated script in its package.json, I have tried it (no luck):

CommitMsg:
  commitplease:
    enabled: true
    required_executable: 'npm'
    command: ['npm', 'run-script', 'commitmsg']

Then I came across a very nice file in overcommit that shows how other npm modules are supported. It looks like the expected way is to support global install.

npm install -g commitplease results in a long list of "Module is inside a symlinked module" warnings for me and a "command not found: commitplease" once I try to run commitplease.

@jzaefferer do you think we should:

1. fix/find a way to support npm install -g commitplease? It looks like all available-by-default overcommit scripts that come from javascript world do that.
2. just keep trying to get overcommit to run a local install of commitplease via .overcommit.yml?

@lencioni do you perhaps see any obvious thing that I am doing wrong?

[lencioni]

We typically set up the overcommit hooks to be able to run globally by default. People in their own projects can override the required_executable to be the local version if they happen to be using npm and have it in their devDependencies. For example, a project's .overcomm.yml file might include the following configuration for eslint:

EsLint:
    enabled: true
    flags: ['--ext=.js,.jsx', '--format=compact']
    required_executable: './node_modules/.bin/eslint'
    include:
      - '**/*.js'
      - '**/*.jsx'

Borrowing from the ESLint default config, I think this might make sense for commitplease:

  Commitplease:
    enabled: false
    description: 'Analyze with Commitplease'
    required_executable: 'commitplease'
    install_command: 'npm install -g commitplease'

And then allow folks to specify the local version if they want, when they enable it, like this:

Commitplease:
    enabled: true
    required_executable: './node_modules/.bin/commitplease'

Of course, you would need to make commitplease able to be installed globally for this setup to work. If you don't want to do that, you could probably roll with something like this for the default:

  Commitplease:
    enabled: false
    description: 'Analyze with Commitplease'
    required_executable: './node_modules/.bin/commitplease'
    install_command: 'npm install --save-dev commitplease'

I hope this helps. Let me know if you have any more questions.

[alisianoi]

@lencioni thank you! My issue appears to have been security-related and here is what worked for me:

Create a project and install the two packages:

mkdir test-project && cd test-project && npm init --yes && git init
npm i commitplease --save-dev
overcommit --install
overcommit --sign

At this point, I have to begin the security dance [remember these two lines for later]:

git add ./node_modules/.bin/commitplease
git commit -m "Add commitplease to source control to fit overcommit security ways"

Now I configure .overcommit.yml to contain the suggested lines:

  CommitMsg:
    Commitplease:
      enabled: true
      description: 'Analyze with Commitplease'
      required_executable: './node_modules/.bin/commitplease'
      install_command: 'npm install --save-dev commitplease'

And continue the security dance:

overcommit --sign commit-msg

And at this point it starts to work! Thanks!

Usability problem:

It does not look like I can skip the "add commitplease hook into git" lines that I have marked above. If I just skip straight into configuring .overcommit.yml, it will not install commitplease automatically even though there is an install_command setting. And it took me quite a while to realize that overcommit actually wants me to add a hook into source control.

Since I must add the hook to source control, I have to have it pre-installed before I configure overcommit. So, overcommit does not automatically install the missing hooks for me.

Am I doing something wrong?

[lencioni]

It's really unusual to be checking in things from node_modules to source control. Generally for hooks that depend on npm packages, we default to using a globally installed version and allow for people to specify a locally installed version in their own overcommit configuration--which works well for teams who want to lock down a specific version of the tool. Ideally, the locally installed version shouldn't be the default, so it would be really great if you could get a globally installed version working. I think this is how folks will expect it to work anyway.

Even if you can't get the global one to work, there shouldn't be any reason that you need to check in the commitplease binary into your repo. I think the missing link might be that most hooks have a Ruby file in the overcommit repo that process the command's output and generates useful information for Overcommit. You can see the commit message files here. https://github.com/brigade/overcommit/tree/master/lib/overcommit/hook/commit_msg

Here's a simple one that is written entirely in Ruby that checks for hard tabs: https://github.com/brigade/overcommit/blob/master/lib/overcommit/hook/commit_msg/hard_tabs.rb

Here's one that executes a command to check for spelling mistakes: https://github.com/brigade/overcommit/blob/master/lib/overcommit/hook/commit_msg/spell_check.rb (the configuration for the command that is run can be found at https://github.com/brigade/overcommit/blob/master/config/default.yml#L101)

And here's the eslint one that runs eslint and parses the output for overcommit: https://github.com/brigade/overcommit/blob/master/lib/overcommit/hook/pre_commit/es_lint.rb

To add a hook, you'll probably want to add a ruby file like one of these along with the default configuration.

I hope this helps make sense of it all!

[jawshooah]

overcommit does not automatically install the missing hooks for me

It's not supposed to. install_command is there so contributors to your repository know how to install the tools required by the hooks; there is no automatic installation.

Regarding security, we added a check to ensure that custom hooks are tracked by git in sds/overcommit@7443529. This provides stronger protection against accidentally executing a malicious script outside the repository.

As @lencioni said above, you could contribute a Commitplease hook to the repo to avoid this process (thanks!). Alternatively, you could write a custom Overcommit hook (docs here) and add it to your repo as .git-hooks/commit_msg/commitplease.rb. Then in the configuration you could set required_executable: ./node_modules/.bin/commitplease (or command: ['npm', 'run', 'commitplease']).

[lencioni]

command: ['npm', 'run', 'commitplease'] will only be correct if you have a matching commitplease entry in your package.json "scripts" section that runs commitplease.

[jawshooah]

My mistake, I thought npm run implicitly added node_modules/.bin to the
path.

On Sat, Jul 30, 2016, 8:24 AM Joe Lencioni notifications@github.com wrote:

command: ['npm', 'run', 'commitplease'] will only be correct if you have
a matching commitplease entry in your package.json "scripts" section that
runs commitplease.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#72 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEVyZw1Z8c-7o4W-Zr-MJ707oLBFBwZhks5qa2zDgaJpZM4JVXHS
.

[lencioni]

It does, but only for commands inside of scripts. For example, if you had the eslint package installed, npm run eslint will give you an error because you don't have an eslint entry in your scripts config. However, if you have a script in your scripts config that simply executes eslint, it will work because within the context of the scripts, npm bin is added to the PATH.

[jzaefferer]

As @lencioni said above, you could contribute a Commitplease hook to the repo to avoid this process (thanks!).

Once that lands, is there anything left to do here?

[alisianoi]

@jzaefferer I think that PR is missing a ruby script based on this:

I think the missing link might be that most hooks have a Ruby file in the overcommit repo that process the command's output and generates useful information for Overcommit.

Basically, @lencioni and @jawshooah have described the process of creating that Ruby script really well. Unfortunately, I do not know Ruby, so the simple action of "try out a locally modified ruby gem" is rather slow and painful (that is what I am doing right now --- figuring out how to locally modify overcommit and run it).

So, I am finishing that PR. @lencioni's guess is that this missing Ruby script is causing the usability problem and forcing me to check node_modules/.bin/commitplease into source control