diff --git a/CHANGELOG.md b/CHANGELOG.md index db3837b3..02e7728d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## main (unreleased) +* Include command file contents in hook signatures when an absolute path is configured. + * Add a configurable `Vale` pre-commit hook to check spelling and style in text and source files (#758). * Honor Git commit-message cleanup settings when handling comment-prefixed titles (#736). diff --git a/lib/overcommit/hook_signer.rb b/lib/overcommit/hook_signer.rb index 3bbf3c6e..1e2d96f1 100644 --- a/lib/overcommit/hook_signer.rb +++ b/lib/overcommit/hook_signer.rb @@ -45,7 +45,7 @@ def hook_path 'must be tracked by Git in order to be signed' end - File.join(Overcommit::Utils.repo_root, command.first.to_s) + File.expand_path(command.first.to_s, Overcommit::Utils.repo_root) end end end diff --git a/spec/overcommit/command_splitter_spec.rb b/spec/overcommit/command_splitter_spec.rb index 5db2e82f..1b780a46 100644 --- a/spec/overcommit/command_splitter_spec.rb +++ b/spec/overcommit/command_splitter_spec.rb @@ -3,6 +3,47 @@ require 'spec_helper' describe Overcommit::CommandSplitter do + describe 'platform command limits' do + around do |example| + previous = described_class.instance_variable_get(:@max_command_length) + described_class.instance_variable_set(:@max_command_length, nil) + example.run + ensure + described_class.instance_variable_set(:@max_command_length, previous) + end + + shared_examples 'a 2048-byte command limit' do + it 'splits commands at the platform limit' do + argument = 'a' * 2045 + result = Overcommit::Subprocess::Result.new(0, '', '') + expect(Overcommit::Subprocess).to receive(:spawn).with(['cmd', argument], {}). + ordered.and_return(result) + expect(Overcommit::Subprocess).to receive(:spawn).with(%w[cmd b], {}). + ordered.and_return(result) + + expect(described_class.execute(['cmd'], args: [argument, 'b'])).to be_success + end + end + + context 'on Windows' do + before do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class).not_to receive(:`) + end + + include_examples 'a 2048-byte command limit' + end + + context 'on Unix' do + before do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(described_class).to receive(:`).with('getconf ARG_MAX').once.and_return("4096\n") + end + + include_examples 'a 2048-byte command limit' + end + end + describe '.execute' do let(:args_prefix) { %w[cmd] } let(:max_command_length) { 10 } diff --git a/spec/overcommit/configuration_validator_spec.rb b/spec/overcommit/configuration_validator_spec.rb index 52320633..d6b04da5 100644 --- a/spec/overcommit/configuration_validator_spec.rb +++ b/spec/overcommit/configuration_validator_spec.rb @@ -55,6 +55,15 @@ end end + context 'and a variable name contains an equals sign' do + let(:env) { { 'INVALID=NAME' => 'value' } } + + it 'rejects the name and explains which variable is invalid' do + expect { subject }.to raise_error(Overcommit::Exceptions::ConfigurationError) + expect(output.string).to include('variable name `INVALID=NAME` cannot contain `=`') + end + end + context 'and it is a hash with integer values' do let(:env) { { 'OVERCOMMIT_ENV_VAR' => 1, 'OVERCOMMIT_ENV_VAR_2' => 2 } } diff --git a/spec/overcommit/gem_plugin_spec.rb b/spec/overcommit/gem_plugin_spec.rb index 5069ea1c..89fe970b 100644 --- a/spec/overcommit/gem_plugin_spec.rb +++ b/spec/overcommit/gem_plugin_spec.rb @@ -229,6 +229,17 @@ def changed_config(hash) /does not satisfy/) end + it 'rejects conflicting versions encountered through a dependency cycle' do + dependency.add_runtime_dependency('company-hooks', '>= 1.0') + conflicting = specification.dup + conflicting.version = '2.0.0' + allow(Gem::Specification).to receive(:find_by_name). + with('company-hooks', anything).and_return(specification, conflicting) + + expect { plugin.update_signature! }.to raise_error(Overcommit::Exceptions::HookLoadError, + /Conflicting versions of company-hooks/) + end + it 'activates the resolved gems only when explicitly requested to load them' do expect(specification).to receive(:activate) expect(dependency).to receive(:activate) diff --git a/spec/overcommit/hook/pre_push/protected_branches_spec.rb b/spec/overcommit/hook/pre_push/protected_branches_spec.rb index 0f8db55a..717702ad 100644 --- a/spec/overcommit/hook/pre_push/protected_branches_spec.rb +++ b/spec/overcommit/hook/pre_push/protected_branches_spec.rb @@ -117,4 +117,26 @@ it { should pass } end + + context 'with configured branch names and patterns' do + let(:hook_config) { { 'branches' => ['stable'], 'branch_patterns' => ['release/*'] } } + let(:pushed_ref_name) { 'stable' } + + before do + allow(subject).to receive(:branch_configurations).and_call_original + allow(pushed_ref).to receive(:destructive?).and_return(true) + end + + it 'protects explicitly named branches' do + expect(subject).to fail_hook + end + + context 'when the branch matches a configured pattern' do + let(:pushed_ref_name) { 'release/1.0' } + + it 'protects matching branches' do + expect(subject).to fail_hook + end + end + end end diff --git a/spec/overcommit/hook/pre_rebase/merged_commits_spec.rb b/spec/overcommit/hook/pre_rebase/merged_commits_spec.rb index acab0138..769868b1 100644 --- a/spec/overcommit/hook/pre_rebase/merged_commits_spec.rb +++ b/spec/overcommit/hook/pre_rebase/merged_commits_spec.rb @@ -55,6 +55,18 @@ end it { should fail_hook } + + it 'uses configured protected branches and names them in the failure' do + settings = { 'PreRebase' => { 'MergedCommits' => { 'branches' => ['release'] } } } + configured = config.merge(Overcommit::Configuration.new(settings)) + allow(Overcommit::GitRepo).to receive(:branches_containing_commit). + with(commit_sha1).and_return(['release']) + hook = described_class.new(configured, context) + + expect(hook.run).to eq([ + :fail, 'Cannot rebase commits that have already been merged into one of release' + ]) + end end end end diff --git a/spec/overcommit/hook_loader/base_spec.rb b/spec/overcommit/hook_loader/base_spec.rb new file mode 100644 index 00000000..1b60722a --- /dev/null +++ b/spec/overcommit/hook_loader/base_spec.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::HookLoader::Base do + it 'requires subclasses to implement hook loading' do + loader = described_class.new(double('config'), double('context'), Overcommit::Logger.silent) + expect { loader.load_hooks }.to raise_error(NotImplementedError) + end +end diff --git a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb index 054643e7..c6ac1fa3 100644 --- a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -260,4 +260,10 @@ allow(reloaded).to receive(:execute).and_return(result) expect(reloaded.run.first.file).to eq('file.foo') end + + it 'wraps missing ad hoc hook base classes in a hook loading error' do + hide_const('Overcommit::Hook::PreCommit::Base') + expect { loader.load_hooks }.to raise_error(Overcommit::Exceptions::HookLoadError, + /Unable to load hook 'CustomScript'/) + end end diff --git a/spec/overcommit/hook_loader/repository_plugins_spec.rb b/spec/overcommit/hook_loader/repository_plugins_spec.rb new file mode 100644 index 00000000..d41a6dbf --- /dev/null +++ b/spec/overcommit/hook_loader/repository_plugins_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::HookLoader::PluginHookLoader, 'with repository plugins' do + let(:enable_verification) { true } + let(:config) { Overcommit::ConfigurationLoader.default_configuration } + let(:context) { Overcommit::HookContext.create('pre-commit', config, [], StringIO.new) } + let(:loader) { described_class.new(config, context, Overcommit::Logger.silent) } + let(:plugin_path) { File.join(config.plugin_directory, 'pre_commit/coverage_fixture.rb') } + + around do |example| + repo do + example.run + ensure + $LOADED_FEATURES.delete(plugin_path) + hook_module = Overcommit::Hook::PreCommit + if hook_module.const_defined?(:CoverageFixture, false) + hook_module.send(:remove_const, :CoverageFixture) + end + end + end + + before do + FileUtils.mkdir_p(File.dirname(plugin_path)) + File.write(plugin_path, <<~RUBY) + module Overcommit::Hook::PreCommit + class CoverageFixture < Base + def run + :pass + end + end + end + RUBY + Overcommit::Utils.execute(['git', 'add', plugin_path]) + end + + it 'loads and instantiates a signed repository hook' do + loader.update_signatures + hook = loader.load_hooks.fetch(0) + expect(hook.name).to eq('CoverageFixture') + expect(hook.run).to eq(:pass) + end + + it 'rejects changed plugin contents before requiring them' do + loader.update_signatures + File.write(plugin_path, "raise 'unapproved code ran'") + expect { loader.load_hooks }.to raise_error(Overcommit::Exceptions::InvalidHookSignature) + expect(Overcommit::Hook::PreCommit.const_defined?(:CoverageFixture, false)).to eq(false) + end +end diff --git a/spec/overcommit/hook_runner_scheduling_spec.rb b/spec/overcommit/hook_runner_scheduling_spec.rb new file mode 100644 index 00000000..85d36f94 --- /dev/null +++ b/spec/overcommit/hook_runner_scheduling_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'timeout' + +describe Overcommit::HookRunner, 'processor reservations' do + let(:config) { double('config', concurrency: 2) } + let(:resource) { ConditionVariable.new } + let(:runner) do + condition = resource + allow(ConditionVariable).to receive(:new).and_return(condition) + described_class.new(config, Overcommit::Logger.silent, double('context'), double('printer')) + end + let(:waiting) { Queue.new } + let(:acquired) { Queue.new } + let(:hook) { double('hook', parallelize?: true, processors: 2) } + + [1, 2].each do |reserved| + it "waits with #{reserved} processors occupied and resumes after they are released" do + occupied = double('running hook', parallelize?: true, processors: reserved) + scheduler = runner + scheduler.send(:wait_for_slot, occupied) + allow(resource).to receive(:wait).and_wrap_original do |method, *args| + waiting << true + method.call(*args) + end + + worker = Thread.new do + scheduler.send(:wait_for_slot, hook) + acquired << true + end + + Timeout.timeout(5) do + waiting.pop + expect(acquired).to be_empty + scheduler.send(:release_slot, occupied) + acquired.pop + worker.join + end + + # A second reservation can consume every processor after the waiter releases them. + scheduler.send(:release_slot, hook) + Timeout.timeout(5) { scheduler.send(:wait_for_slot, hook) } + ensure + worker&.kill + worker&.join + end + end +end diff --git a/spec/overcommit/hook_signer_files_spec.rb b/spec/overcommit/hook_signer_files_spec.rb new file mode 100644 index 00000000..36bb0aa4 --- /dev/null +++ b/spec/overcommit/hook_signer_files_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::HookSigner, 'with repository commands' do + let(:enable_verification) { true } + let(:script_path) { File.expand_path('hook-script') } + let(:command) { script_path } + let(:config) do + Overcommit::Configuration.new( + 'PreCommit' => { 'CustomScript' => { 'enabled' => true, 'command' => [command] } } + ) + end + let(:context) { double('context', hook_type_name: 'pre_commit', hook_class_name: 'PreCommit') } + let(:signer) { described_class.new('CustomScript', config, context) } + + around do |example| + repo { example.run } + end + + before do + File.write(script_path, 'original command') + end + + it 'rejects untracked repository commands when signing is enabled' do + expect { signer.update_signature! }.to raise_error(Overcommit::Exceptions::InvalidHookDefinition, + /must be tracked by Git/) + end + + shared_examples 'a signed repository command' do + it 'signs the actual file and requires fresh approval when its contents change' do + Overcommit::Utils.execute(['git', 'add', script_path]) + expect(File.realpath(signer.hook_path)).to eq(File.realpath(script_path)) + signer.update_signature! + expect(signer.signature_verified?).to eq(true) + + File.write(script_path, 'modified command') + expect(signer.signature_verified?).to eq(false) + + File.write(script_path, 'original command') + expect(signer.signature_verified?).to eq(true) + end + end + + include_examples 'a signed repository command' + + context 'with a relative command path' do + let(:command) { ".#{Overcommit::OS.windows? ? '\\' : '/'}hook-script" } + + include_examples 'a signed repository command' + end +end diff --git a/spec/overcommit/utils_spec.rb b/spec/overcommit/utils_spec.rb index d9e63a0e..89e2686a 100644 --- a/spec/overcommit/utils_spec.rb +++ b/spec/overcommit/utils_spec.rb @@ -211,6 +211,18 @@ it { should be_nil } end + context 'when the process ancestry contains only env.exe launchers' do + let(:processes) do + [double('launcher', Name: 'env.exe', ParentProcessId: Process.pid)] + end + + it 'stops after four queries without launching a fallback' do + expect(service).to receive(:ExecQuery).exactly(4).times.and_return(processes) + expect(described_class).not_to receive(:execute) + expect(subject).to be_nil + end + end + context 'when the command line is inaccessible' do let(:command) { nil }