From 3db733e5b7dd479baee1c8e587aa86ed73e5011f Mon Sep 17 00:00:00 2001 From: Matt Brictson Date: Sun, 16 Feb 2025 13:04:54 -0800 Subject: [PATCH] Restore ability to specify "gemfile: false" in config (#863) Setting `gemfile: false` in `.overcommit.yml` is supposed to disable Bundler. However, a recently-introduced bug causes `false` to be interpreted as the name of the gemfile. Bundler looks for a gemfile named "false", which fails, leading overcommit's hooks to crash. This PR fixes the bug by adjusting the regex used to parse the `gemfile:` line in the config. Now, `false` is no longer interpreted as a gemfile name. I added an integration test to verify the fix. Fixes #862 --- spec/integration/gemfile_option_spec.rb | 211 +++++++++++++++--------- template-dir/hooks/commit-msg | 2 +- template-dir/hooks/overcommit-hook | 2 +- template-dir/hooks/post-checkout | 2 +- template-dir/hooks/post-commit | 2 +- template-dir/hooks/post-merge | 2 +- template-dir/hooks/post-rewrite | 2 +- template-dir/hooks/pre-commit | 2 +- template-dir/hooks/pre-push | 2 +- template-dir/hooks/pre-rebase | 2 +- template-dir/hooks/prepare-commit-msg | 2 +- 11 files changed, 144 insertions(+), 87 deletions(-) diff --git a/spec/integration/gemfile_option_spec.rb b/spec/integration/gemfile_option_spec.rb index 7a0175da..a6a6a4ba 100644 --- a/spec/integration/gemfile_option_spec.rb +++ b/spec/integration/gemfile_option_spec.rb @@ -3,99 +3,156 @@ require 'spec_helper' describe 'specifying `gemfile` option in Overcommit configuration' do - let(:repo_root) { File.expand_path(File.join('..', '..'), File.dirname(__FILE__)) } - let(:fake_gem_path) { File.join('lib', 'my_fake_gem') } - - # We point the overcommit gem back to this repo since we can't assume the gem - # has already been installed in a test environment - let(:gemfile) { normalize_indent(<<-RUBY) } - source 'https://rubygems.org' - - gem 'overcommit', path: '#{repo_root}' - gem 'my_fake_gem', path: '#{fake_gem_path}' - gem 'ffi' if Gem.win_platform? # Necessary for test to pass on Windows - RUBY - - let(:gemspec) { normalize_indent(<<-RUBY) } - Gem::Specification.new do |s| - s.name = 'my_fake_gem' - s.version = '1.0.0' - s.author = 'John Doe' - s.license = 'MIT' - s.homepage = 'https://example.com' - s.email = 'john.doe@example.com' - s.summary = 'A fake gem' - s.files = [File.join('lib', 'my_fake_gem.rb')] - end - RUBY - - # Specify a hook that depends on an external gem to test Gemfile loading - let(:hook) { normalize_indent(<<-RUBY) } - module Overcommit::Hook::PreCommit - class FakeHook < Base - def run - require 'my_fake_gem' - :pass + context 'given a project that uses a Gemfile' do + let(:repo_root) { File.expand_path(File.join('..', '..'), File.dirname(__FILE__)) } + let(:fake_gem_path) { File.join('lib', 'my_fake_gem') } + + # We point the overcommit gem back to this repo since we can't assume the gem + # has already been installed in a test environment + let(:gemfile) { normalize_indent(<<-RUBY) } + source 'https://rubygems.org' + + gem 'overcommit', path: '#{repo_root}' + gem 'my_fake_gem', path: '#{fake_gem_path}' + gem 'ffi' if Gem.win_platform? # Necessary for test to pass on Windows + RUBY + + let(:gemspec) { normalize_indent(<<-RUBY) } + Gem::Specification.new do |s| + s.name = 'my_fake_gem' + s.version = '1.0.0' + s.author = 'John Doe' + s.license = 'MIT' + s.homepage = 'https://example.com' + s.email = 'john.doe@example.com' + s.summary = 'A fake gem' + s.files = [File.join('lib', 'my_fake_gem.rb')] + end + RUBY + + # Specify a hook that depends on an external gem to test Gemfile loading + let(:hook) { normalize_indent(<<-RUBY) } + module Overcommit::Hook::PreCommit + class FakeHook < Base + def run + require 'my_fake_gem' + :pass + end + end + end + RUBY + + let(:config) { normalize_indent(<<-YAML) } + verify_signatures: false + + CommitMsg: + ALL: + enabled: false + + PreCommit: + ALL: + enabled: false + FakeHook: + enabled: true + requires_files: false + YAML + + around do |example| + repo do + # Since RSpec is being run within a Bundler context we need to clear it + # in order to not taint the test + Bundler.with_unbundled_env do + FileUtils.mkdir_p(File.join(fake_gem_path, 'lib')) + echo(gemspec, File.join(fake_gem_path, 'my_fake_gem.gemspec')) + touch(File.join(fake_gem_path, 'lib', 'my_fake_gem.rb')) + + echo(gemfile, '.overcommit_gems.rb') + `bundle install --gemfile=.overcommit_gems.rb` + + echo(config, '.overcommit.yml') + + # Set BUNDLE_GEMFILE so we load Overcommit from the current repo + ENV['BUNDLE_GEMFILE'] = '.overcommit_gems.rb' + `bundle exec overcommit --install > #{File::NULL}` + FileUtils.mkdir_p(File.join('.git-hooks', 'pre_commit')) + echo(hook, File.join('.git-hooks', 'pre_commit', 'fake_hook.rb')) + + Overcommit::Utils.with_environment 'OVERCOMMIT_NO_VERIFY' => '1' do + example.run + end end end end - RUBY - - let(:config) { normalize_indent(<<-YAML) } - verify_signatures: false - - CommitMsg: - ALL: - enabled: false - - PreCommit: - ALL: - enabled: false - FakeHook: - enabled: true - requires_files: false - YAML - - around do |example| - repo do - # Since RSpec is being run within a Bundler context we need to clear it - # in order to not taint the test - Bundler.with_unbundled_env do - FileUtils.mkdir_p(File.join(fake_gem_path, 'lib')) - echo(gemspec, File.join(fake_gem_path, 'my_fake_gem.gemspec')) - touch(File.join(fake_gem_path, 'lib', 'my_fake_gem.rb')) - - echo(gemfile, '.overcommit_gems.rb') - `bundle install --gemfile=.overcommit_gems.rb` + subject { shell(%w[git commit --allow-empty -m Test]) } + + context 'when configuration specifies the gemfile' do + let(:config) { "gemfile: .overcommit_gems.rb\n" + super() } + + it 'runs the hook successfully' do + subject.status.should == 0 + end + end + + context 'when configuration does not specify the gemfile' do + it 'fails to run the hook' do + subject.status.should_not == 0 + end + end + end + + context 'given a project that does not use a Gemfile' do + let(:hook) { normalize_indent(<<-RUBY) } + module Overcommit::Hook::PreCommit + class NoInvalidGemfileHook < Base + def run + if (gemfile = ENV["BUNDLE_GEMFILE"]) + raise unless File.exist?(gemfile) + end + + :pass + end + end + end + RUBY + + let(:config) { normalize_indent(<<-YAML) } + verify_signatures: false + + CommitMsg: + ALL: + enabled: false + + PreCommit: + ALL: + enabled: false + NoInvalidGemfileHook: + enabled: true + requires_files: false + YAML + + around do |example| + repo do echo(config, '.overcommit.yml') - # Set BUNDLE_GEMFILE so we load Overcommit from the current repo - ENV['BUNDLE_GEMFILE'] = '.overcommit_gems.rb' - `bundle exec overcommit --install > #{File::NULL}` + `overcommit --install > #{File::NULL}` FileUtils.mkdir_p(File.join('.git-hooks', 'pre_commit')) - echo(hook, File.join('.git-hooks', 'pre_commit', 'fake_hook.rb')) + echo(hook, File.join('.git-hooks', 'pre_commit', 'no_invalid_gemfile_hook.rb')) Overcommit::Utils.with_environment 'OVERCOMMIT_NO_VERIFY' => '1' do example.run end end end - end - - subject { shell(%w[git commit --allow-empty -m Test]) } - context 'when configuration specifies the gemfile' do - let(:config) { "gemfile: .overcommit_gems.rb\n" + super() } + subject { shell(%w[git commit --allow-empty -m Test]) } - it 'runs the hook successfully' do - subject.status.should == 0 - end - end + context 'when configuration explicitly sets the gemfile to false' do + let(:config) { "gemfile: false\n" + super() } - context 'when configuration does not specify the gemfile' do - it 'fails to run the hook' do - subject.status.should_not == 0 + it 'runs the hook successfully' do + subject.status.should == 0 + end end end end diff --git a/template-dir/hooks/commit-msg b/template-dir/hooks/commit-msg index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/commit-msg +++ b/template-dir/hooks/commit-msg @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/overcommit-hook b/template-dir/hooks/overcommit-hook index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/overcommit-hook +++ b/template-dir/hooks/overcommit-hook @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/post-checkout b/template-dir/hooks/post-checkout index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/post-checkout +++ b/template-dir/hooks/post-checkout @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/post-commit b/template-dir/hooks/post-commit index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/post-commit +++ b/template-dir/hooks/post-commit @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/post-merge b/template-dir/hooks/post-merge index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/post-merge +++ b/template-dir/hooks/post-merge @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/post-rewrite b/template-dir/hooks/post-rewrite index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/post-rewrite +++ b/template-dir/hooks/post-rewrite @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/pre-commit b/template-dir/hooks/pre-commit index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/pre-commit +++ b/template-dir/hooks/pre-commit @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/pre-push b/template-dir/hooks/pre-push index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/pre-push +++ b/template-dir/hooks/pre-push @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/pre-rebase b/template-dir/hooks/pre-rebase index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/pre-rebase +++ b/template-dir/hooks/pre-rebase @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile diff --git a/template-dir/hooks/prepare-commit-msg b/template-dir/hooks/prepare-commit-msg index 377c892b..87ffeb58 100755 --- a/template-dir/hooks/prepare-commit-msg +++ b/template-dir/hooks/prepare-commit-msg @@ -27,7 +27,7 @@ if hook_type == 'overcommit-hook' end # Check if Overcommit should invoke a Bundler context for loading gems -config = File.read('.overcommit.yml') =~ /gemfile: ['"]?(.*)['"]?/ +File.read('.overcommit.yml') =~ /gemfile: (?:false|['"]?(.*)['"]?)/ gemfile = Regexp.last_match(1) if gemfile