feat: skip running overcommit --install if env var is set

[G-Rath]

While a useful tool, overcommit uses ruby for its actual scripts, and so requires being run in an environment that has a stable ruby setup (including bundler being stable & gems installed).

While this is a reasonable assumption for devs using Linux & Mac, it's not for devs running Windows, where Ruby (& its gems) are often difficult to work with and for which we very strongly recommend devs using Windows to use WSL.

In this Windows+WSL setup, typically Windows is used for the GUI and WSL for anything on the commandline, meaning commands like git clone, bin/setup, bundle install, rubocop, rbenv, etc work fine as they're being run in Linux, and GUI tools work as they're running on the Windows host with proper GPU access.

However, one major exception to this is with git: the majority of GUI applications that interact with git on Windows cannot do so via WSL - instead they use git-for-windows, which runs git using a BASH emulator. This means that when git attempts to run overcommits hooks, it attempts to find Ruby on the Windows host instead of within WSL.

The exact result depends on what you've got setup: for me it would just straight crash since I don't have any SDKs installed on my Windows host. For @mokomokoy it would error due to ruby version being wrong, and that gems were missing.

This means that having overcommit hooks installed on Windows blocks you from being able to work with git with GUI applications such as IDEs, and can be particularly painful for developers less familiar with git hooks and who've not worked with these tools before, as the error messages can be quite scary.

This PR adds the ability to disable automatically installing overcommit hooks when calling bin/setup via an env value, which lets us Windows users set & forget in our .bashrc files.

[joshmcarthur]

The test failure is probably because this is the first build since a Rubocop update. In terms of the change, I understand why it's necessary thanks to your nice lengthy description. I wish there was a nicer test we could do so that if overcommit was run within WSL it would do it's thing, and skip on Windows - but I can't see any overcommit config that supports that.

[G-Rath]

The test failure is probably because this is the first build since a Rubocop update.

For sure - I'm already correcting them :) (there's a bunch for the devise variant, but otherwise the rest are just the one I posted)

I can't see any overcommit config that supports that.

The problem is it's a catch-22: the script requires ruby to run, which is after the point that you want to know if it's running in an env like WSL. An ideal solution would be for overcommit to use a sh script that does a quick check on if it should run and if that passes call overcommit.rb (which is what husky does), but I can't think of any easy way to wrap overcommit ourselves without resorting to some hacks or something extreme like publishing our own gem.

[eoinkelly]

@G-Rath Do we need something in the README of this project (not the generated README) to tell folks to set that env var if they are running in WSL?

[joshmcarthur]

🐘 this could be a helper method - run "bundle exec overcommit --install" if install_overcommit?.

[G-Rath]

Do we need something in the README of this project (not the generated README)

I'd want it the other way around: this is about the applications that are generated by the template, so sticking a note in the templates README wouldn't be very effective.

[Rabid-Dan]

I'd be keen to see usage of the env var documented somewhere as well. Other than that it LGTM.

[joshmcarthur]

@G-Rath can you check that my addition to the README is correct?

[G-Rath]

@joshmcarthur thanks for pushing this forward - I've not had the time to swing back around to this.

Currently what you've put in the README isn't actually correct: the env variable controls if --install is called when you run bin/setup, which happens locally and in CI; having it set when doing rails new won't do much (if anything tbh).

Because of this I think it should be documented in the generated readme as well as the templates own readme, as it pertains to both of those.

I also think the documentation should recommend using it as a general "when you don't want overcommit installed", with CI & WSL as examples, as this behaviour is the opposite of overcommits default and what devs might be use to.

[joshmcarthur]

@G-Rath where it gets documented aside, this env variable will have effect when the application is generated, as the rails template application calls bin/setup during generation, therefore overcommit will not be set up.

I'm personally somewhat reluctant to encourage disabling of overcommit, as I'd want a solid reason for it to not be installed for a project under active development.

[G-Rath]

this env variable will have effect when the application is generated

I figured that'd be the case, 👍

I'd want a solid reason for it to not be installed for a project under active development.

I already outlined what I think is a solid reason in the body of this PR, and also in #111, which echos that this is not just a WSL specific problem in the third cost point of the OP. It also points out that this extends beyond projects that are in active development, as we don't remove the install call in bin/setup at anypoint, meaning it affects projects with any form of development.

Another point that I've encountered since #111 was opened is that the hooks are also applied in CI - while in theory this shouldn't be a problem, at best it's confusing (as we have "nothing to update" messages popping up at odd times, leaving you to track down git as the source of these messages), at worse it could result in some nasty CI problems (i.e in CWP projects CI generates files which it commits and pushed to another branch).

[eoinkelly]

@G-Rath will make new PR based on discussions