diff --git a/CHANGELOG.md b/CHANGELOG.md index dc4ced45..66691322 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Overcommit Changelog +## main (unreleased) + +* Add configurable line parsing for command hooks (#750). + ## 0.75.0 * Remember all locally approved configuration and hook signatures across rebases diff --git a/README.md b/README.md index 59bc5ff2..8105b070 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,8 @@ In addition to supporting a wide variety of hooks that can be used across multiple repositories, you can also define hooks specific to a repository which are stored in source control. You can also easily -[add your existing hook scripts](#adding-existing-git-hooks) without writing +[add your existing hook scripts](#adding-existing-git-hooks) or +[use some of your existing line-oriented analysis tools](#adding-existing-line-aware-commands) without writing any Ruby code. - [Requirements](#requirements) @@ -47,6 +48,7 @@ any Ruby code. - [PreRebase](#prerebase) - [Repo-Specific hooks](#repo-specific-hooks) - [Adding Existing Git Hooks](#adding-existing-git-hooks) + - [Adding Existing Line-Aware Commands](#adding-existing-line-aware-commands) - [Security](#security) - [Disabling Signature Checking](#disabling-signature-checking) - [Contributing](#contributing) @@ -256,6 +258,8 @@ Option | Description `install_command` | Command the user can run to install the `required_executable` (or alternately the specified `required_libraries`). This is intended for documentation purposes, as Overcommit does not install software on your behalf since there are too many edge cases where such behavior would result in incorrectly configured installations (e.g. installing a Python package in the global package space instead of in a virtual environment). `skip_file_checkout` | Whether to skip this hook for file checkouts (e.g. `git checkout some-ref -- file`). Only applicable to `PostCheckout` hooks. `skip_if` | Array of arguments to be executed to determine whether or not the hook should run. For example, setting this to a value of `['bash', '-c', '! which my-executable']` would allow you to skip running this hook if `my-executable` was not in the bin path. +`message_pattern` | Regex used to parse diagnostics from [line-aware command hooks](#adding-existing-line-aware-commands). Requires a named `file` capture; optional `line` and `type` captures provide line handling and warning classification. +`warning_message_type_pattern` | Optional regex (or literal substring) identifying warnings in the `type` capture of a line-aware command hook. All other diagnostics are errors. In addition to the built-in configuration options, each hook can expose its own unique configuration options. The `AuthorEmail` hook, for example, allows @@ -705,6 +709,52 @@ of hook, see the [git-hooks documentation][GHD]. [GHD]: https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks +### Adding Existing Line-Aware Commands + +Tools that report file diagnostics can use Overcommit's message handling without +writing a Ruby hook. Add a `message_pattern` regex to an existing command hook to +parse each line of its standard output. The command receives the applicable file +paths after its configured command and flags, instead of Git hook arguments or +standard input. + +`message_pattern` is required to enable this behavior and has no default. It must +be a Ruby regex with a named `file` capture. An optional `line` capture enables +`problem_on_unmodified_line` handling. An optional `type` capture can be classified +using `warning_message_type_pattern`, which accepts a regex or a literal substring. +Without a matching warning type, diagnostics are errors. Other captures are ignored. + +For example, to parse GNU/Emacs-style diagnostics such as +`file.rb:12: warning: unused variable`: + +```yaml +PreCommit: + CustomScript: + enabled: true + command: './bin/custom-script' + include: '**/*.rb' + message_pattern: !ruby/regexp /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ + warning_message_type_pattern: !ruby/regexp /^warning:/i +``` + +The normal `include`, `exclude`, and `requires_files` options control which files +are passed to the command and whether it runs. `problem_on_unmodified_line` +controls how diagnostics on unchanged lines affect the result. + +A successful command passes. For a failed command, the parsed diagnostics determine +whether the hook fails or warns. A failed command with no standard output still +fails and displays its standard error. Unexpected output that does not match +`message_pattern` raises a message processing error instead of being silently ignored. + +Only standard output is parsed. For tools that write diagnostics to standard error, +use a wrapper script that redirects it with `2>&1`. + +These options apply to command hooks without an existing built-in or repository +Ruby implementation. See the [Ruby regex documentation][RubyRE] for named captures. +To serialize a regex as YAML, use `require 'yaml'; puts YAML.dump(/MY-REGEXP/)` in +`irb` and omit the leading `---` from the result. + +[RubyRE]: https://ruby-doc.org/core-2.7.3/Regexp.html + ## Security While Overcommit can make managing Git hooks easier and more convenient, diff --git a/lib/overcommit/configuration_loader.rb b/lib/overcommit/configuration_loader.rb index 5473e8ce..882eeddb 100644 --- a/lib/overcommit/configuration_loader.rb +++ b/lib/overcommit/configuration_loader.rb @@ -28,7 +28,7 @@ def load_from_file(file, options = {}) # default. Explicitly enable aliases if the option is available. yaml = begin - YAML.load_file(file, aliases: true) + YAML.load_file(file, permitted_classes: [Regexp, Symbol], aliases: true) rescue ArgumentError YAML.load_file(file) end diff --git a/lib/overcommit/hook_loader/base.rb b/lib/overcommit/hook_loader/base.rb index 9173e9f1..4ecfab7f 100644 --- a/lib/overcommit/hook_loader/base.rb +++ b/lib/overcommit/hook_loader/base.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'overcommit/utils/messages_utils' + module Overcommit::HookLoader # Responsible for loading hooks from a file. class Base @@ -22,6 +24,40 @@ def load_hooks private + def validate_line_aware_config(hook_config) + pattern = hook_config['message_pattern'] + unless pattern.is_a?(Regexp) && pattern.names.include?('file') + raise Overcommit::Exceptions::HookLoadError, + '`message_pattern` must be a Regexp with a named `file` capture group' + end + + warning_pattern = hook_config['warning_message_type_pattern'] + unless warning_pattern.nil? || warning_pattern.is_a?(Regexp) || warning_pattern.is_a?(String) + raise Overcommit::Exceptions::HookLoadError, + '`warning_message_type_pattern` must be a Regexp or String' + end + end + + def create_line_aware_command_hook_class(hook_base) + Class.new(hook_base) do + def run + result = execute(command, args: applicable_files) + return :pass if result.success? + + output = result.stdout.split("\n") + return [:fail, result.stderr] if output.empty? + + Overcommit::Utils::MessagesUtils.extract_messages( + output, + @config['message_pattern'], + Overcommit::Utils::MessagesUtils.create_type_categorizer( + @config['warning_message_type_pattern'] + ) + ) + end + end + end + attr_reader :log # Load and return a {Hook} from a CamelCase hook name. diff --git a/lib/overcommit/hook_loader/plugin_hook_loader.rb b/lib/overcommit/hook_loader/plugin_hook_loader.rb index 12d4fe45..0fdeb092 100644 --- a/lib/overcommit/hook_loader/plugin_hook_loader.rb +++ b/lib/overcommit/hook_loader/plugin_hook_loader.rb @@ -74,16 +74,12 @@ def check_for_unverified_plugins raise Overcommit::Exceptions::InvalidHookSignature end - def create_ad_hoc_hook(hook_name) - hook_module = Overcommit::Hook.const_get(@context.hook_class_name) - hook_base = hook_module.const_get('Base') - + def create_git_hook_class(hook_base) # Implement a simple class that executes the command and returns pass/fail # based on the exit status - hook_class = Class.new(hook_base) do + Class.new(hook_base) do def run result = @context.execute_hook(command) - if result.success? :pass else @@ -91,6 +87,25 @@ def run end end end + end + + def create_ad_hoc_hook(hook_name) + hook_module = Overcommit::Hook.const_get(@context.hook_class_name) + hook_base = hook_module.const_get('Base') + + hook_config = @config.for_hook(hook_name, @context.hook_class_name) + hook_class = + if hook_config['message_pattern'] + validate_line_aware_config(hook_config) + create_line_aware_command_hook_class(hook_base) + else + create_git_hook_class(hook_base) + end + + # A later load may configure the same hook to use a different command format. + if hook_module.const_defined?(hook_name, false) + hook_module.send(:remove_const, hook_name) + end hook_module.const_set(hook_name, hook_class).new(@config, @context) rescue LoadError, NameError => e diff --git a/lib/overcommit/utils/messages_utils.rb b/lib/overcommit/utils/messages_utils.rb index 31c0f8db..b83d3ac0 100644 --- a/lib/overcommit/utils/messages_utils.rb +++ b/lib/overcommit/utils/messages_utils.rb @@ -37,6 +37,20 @@ def extract_messages(output_messages, regex, type_categorizer = nil) end end + def create_type_categorizer(warning_pattern) + return nil if warning_pattern.nil? + + lambda do |type| + warning = + if warning_pattern.is_a?(Regexp) + type && warning_pattern.match?(type) + else + type&.include?(warning_pattern) + end + warning ? :warning : :error + end + end + private def extract_file(match, message) diff --git a/spec/overcommit/configuration_loader_spec.rb b/spec/overcommit/configuration_loader_spec.rb index 90497201..163a3a3b 100644 --- a/spec/overcommit/configuration_loader_spec.rb +++ b/spec/overcommit/configuration_loader_spec.rb @@ -44,6 +44,23 @@ subject.plugin_directory.should == File.expand_path('some-directory') end + context 'and command hooks use regular expressions in YAML' do + let(:config_contents) { <<-'CFG' } + PreCommit: + CustomScript: + enabled: true + command: custom-script + message_pattern: !ruby/regexp /^(?[^:]+):(?\d+):[ ](?\w+):/ + warning_message_type_pattern: !ruby/regexp /^warning$/i + CFG + + it 'loads both patterns as regular expressions' do + hook_config = subject.for_hook('CustomScript', 'PreCommit') + expect(hook_config['message_pattern']).to be_a(Regexp) + expect(hook_config['warning_message_type_pattern']).to eq(/^warning$/i) + end + end + context 'and the configuration file contains a hook with no `enabled` option' do let(:config_contents) { <<-CFG } PreCommit: diff --git a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb new file mode 100644 index 00000000..054643e7 --- /dev/null +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -0,0 +1,263 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::HookLoader::PluginHookLoader do + subject(:hook) { loader.load_hooks.find { |loaded| loaded.name == 'CustomScript' } } + + let(:loader) { described_class.new(config, context, Overcommit::Logger.silent) } + let(:config) do + Overcommit::ConfigurationLoader.default_configuration.merge( + Overcommit::Configuration.new( + 'PreCommit' => { 'CustomScript' => hook_config.merge(config_changes) } + ) + ) + end + let(:hook_config) { { 'enabled' => true, 'command' => %w[custom-script] } } + let(:context) { Overcommit::HookContext.create('pre-commit', config, [], StringIO.new) } + let(:config_changes) { {} } + let(:result) { double(success?: false, stdout: stdout, stderr: '') } + let(:stdout) { "file.foo:2: warning: check this\n" } + + around do |example| + repo { example.run } + end + + after do + hook_module = Overcommit::Hook::PreCommit + if hook_module.const_defined?(:CustomScript, false) + hook_module.send(:remove_const, :CustomScript) + end + end + + context 'without a message pattern' do + before do + allow(context).to receive(:execute_hook).with(%w[custom-script]).and_return(result) + end + + it 'preserves the exit status and both output streams for existing Git hooks' do + allow(result).to receive(:stderr).and_return('stderr detail') + expect(hook.run).to eq([:fail, "#{stdout}stderr detail"]) + end + + it 'passes when the command succeeds' do + allow(result).to receive(:success?).and_return(true) + expect(hook.run).to eq(:pass) + end + end + + context 'with a message pattern' do + let(:hook_config) do + super().merge( + 'message_pattern' => /^(?(?:\w:)?[^:]+):(?\d+): (?\w+):/, + 'warning_message_type_pattern' => /^warn(?:ing)?$/i, + 'flags' => ['--format=emacs'], + 'include' => '**/*.foo', + 'exclude' => 'excluded.foo' + ) + end + + before do + allow(context).to receive(:modified_files). + and_return(%w[file.foo excluded.foo other.txt].map { |file| File.expand_path(file) }) + allow(context).to receive(:modified_lines_in_file).with('file.foo').and_return([2]) + allow(hook).to receive(:execute). + with(%w[custom-script --format=emacs], args: [File.expand_path('file.foo')]). + and_return(result) + end + + it 'extracts file, line, type, and original message using configured regexes' do + expect(hook.run).to eq([ + Overcommit::Hook::Message.new(:warning, 'file.foo', 2, stdout.chomp) + ]) + end + + it 'preserves Windows drive letters in diagnostic paths' do + allow(result).to receive(:stdout).and_return("C:/project/file.foo:2: warning: check this\n") + expect(hook.run.first.file).to eq('C:/project/file.foo') + end + + it 'fails on errors on modified lines' do + allow(result).to receive(:stdout).and_return("file.foo:2: error: fix this\n") + status, output = hook.run_and_transform + expect(status).to eq(:fail) + expect(output).to include('Errors on modified lines:', 'fix this') + end + + it 'warns on warning messages' do + expect(hook.run_and_transform.first).to eq(:warn) + end + + it 'keeps errors and warnings distinct in mixed output' do + allow(result).to receive(:stdout).and_return("#{stdout}file.foo:2: error: fix this\n") + expect(hook.run.map(&:type)).to eq([:warning, :error]) + expect(hook.run_and_transform.first).to eq(:fail) + end + + it 'passes on a successful exit even without output' do + allow(result).to receive_messages(success?: true, stdout: '') + expect(hook.run).to eq(:pass) + end + + it 'fails when the command fails without stdout, preserving stderr' do + allow(result).to receive_messages(stdout: '', stderr: 'command crashed') + expect(hook.run_and_transform).to eq([:fail, 'command crashed']) + end + + it 'fails when the command fails silently' do + allow(result).to receive(:stdout).and_return('') + expect(hook.run_and_transform.first).to eq(:fail) + end + + it 'fails when stdout contains only blank lines' do + allow(result).to receive(:stdout).and_return("\n\n") + expect(hook.run_and_transform.first).to eq(:fail) + end + + it 'reports unexpected output rather than silently discarding it' do + allow(result).to receive(:stdout).and_return('unrecognized diagnostic') + expect { hook.run }.to raise_error(Overcommit::Exceptions::MessageProcessingError) + end + + context 'with a literal warning string' do + let(:config_changes) { { 'warning_message_type_pattern' => 'warn' } } + + it 'classifies warnings by substring' do + expect(hook.run.first.type).to eq(:warning) + end + end + + context 'without a warning pattern' do + let(:hook_config) { super().reject { |key, _| key == 'warning_message_type_pattern' } } + + it 'defaults to errors' do + expect(hook.run.first.type).to eq(:error) + end + end + + context 'with a file-only pattern' do + let(:config_changes) { { 'message_pattern' => /^(?(?:\w:)?[^:]+):/ } } + + it 'supports missing line and type captures' do + message = hook.run.first + expect([message.file, message.line, message.type]).to eq(['file.foo', nil, :error]) + end + end + + context 'with an unmatched optional type capture' do + let(:config_changes) do + { 'message_pattern' => /^(?(?:\w:)?[^:]+):(?\d+):(?: (?error):)?/ } + end + + it 'defaults to an error' do + expect(hook.run.first.type).to eq(:error) + end + end + + context 'with required_executable in place of command' do + let(:hook_config) do + super().reject { |key, _| key == 'command' }. + merge('required_executable' => 'custom-script') + end + + it 'executes the configured executable' do + expect(hook.run.first.file).to eq('file.foo') + end + end + + it 'does not run when requires_files is set and no files match' do + allow(context).to receive(:modified_files).and_return([File.expand_path('other.txt')]) + expect(hook.run?).to eq(false) + end + + context 'with errors on unmodified lines' do + let(:stdout) { "file.foo:1: error: old problem\n" } + + it 'fails by default' do + expect(hook.run_and_transform.first).to eq(:fail) + end + + context 'when configured to warn about problems' do + let(:config_changes) { { 'problem_on_unmodified_line' => 'warn' } } + + it 'warns' do + expect(hook.run_and_transform.first).to eq(:warn) + end + end + + context 'when configured to report problems' do + let(:config_changes) { { 'problem_on_unmodified_line' => 'report' } } + + it 'fails' do + expect(hook.run_and_transform.first).to eq(:fail) + end + end + + context 'when configured to ignore problems' do + let(:config_changes) { { 'problem_on_unmodified_line' => 'ignore' } } + + it 'passes and omits ignored problems' do + expect(hook.run_and_transform).to eq([:pass, '']) + end + end + end + end + + context 'with invalid line-aware options' do + let(:config_changes) { { 'message_pattern' => pattern } } + + context 'when message_pattern is a string' do + let(:pattern) { 'not a Ruby regexp' } + + it 'reports a configuration error' do + expect { hook }.to raise_error(Overcommit::Exceptions::HookLoadError, /message_pattern/) + end + end + + context 'when message_pattern has no file capture' do + let(:pattern) { /(?\d+)/ } + + it 'explains the required capture' do + expect { hook }.to raise_error(Overcommit::Exceptions::HookLoadError, /file/) + end + end + + context 'when warning_message_type_pattern is not a string or regexp' do + let(:config_changes) do + { 'message_pattern' => /(?.+)/, 'warning_message_type_pattern' => true } + end + + it 'explains the invalid option' do + expect { hook }.to raise_error(Overcommit::Exceptions::HookLoadError, + /warning_message_type_pattern/) + end + end + end + + it 'runs a real command against staged files and processes its diagnostics' do + File.write('file.foo', "first line\nsecond line\n") + Overcommit::Utils.execute(%w[git add file.foo]) + hook_config.merge!( + 'command' => [RbConfig.ruby, '-e', 'puts ARGV.first + ":2: warning: check this"; exit 1', + '--'], + 'message_pattern' => /^(?(?:\w:)?[^:]+):(?\d+): (?\w+):/, + 'warning_message_type_pattern' => /^warning$/ + ) + status, output = hook.run_and_transform + expect(status).to eq(:warn) + expect(output).to include('Warnings on modified lines:', 'file.foo:2: warning: check this') + end + + it 'uses the current configuration when reloading the same hook name' do + loader.load_hooks + updated_config = config.merge( + Overcommit::Configuration.new( + 'PreCommit' => { 'CustomScript' => { 'message_pattern' => /^(?(?:\w:)?[^:]+):/ } } + ) + ) + updated_loader = described_class.new(updated_config, context, Overcommit::Logger.silent) + reloaded = updated_loader.load_hooks.find { |loaded| loaded.name == 'CustomScript' } + allow(reloaded).to receive(:execute).and_return(result) + expect(reloaded.run.first.file).to eq('file.foo') + end +end