From 9e9c87f835901fce5164e09f858e5518e6249f46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillaume=20D=C3=A9flache?= Date: Fri, 11 Jun 2021 17:03:14 +0200 Subject: [PATCH 1/8] Add some tests for ad-hoc (pre-commit) Git hooks --- .../hook_loader/plugin_hook_loader_spec.rb | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 spec/overcommit/hook_loader/plugin_hook_loader_spec.rb 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..d8cdc505 --- /dev/null +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'ad-hoc pre-commit hook' do + subject do + hook_loader = Overcommit::HookLoader::PluginHookLoader.new( + config, + context, + Overcommit::Logger.new(STDOUT) + ) + hooks = hook_loader.load_hooks + hooks.find { |h| h.name == hook_name } + end + let(:config) do + config = Overcommit::Configuration.new( + YAML.safe_load(config_contents, [Regexp]), { + validate: false + } + ) + Overcommit::ConfigurationLoader.default_configuration.merge(config) + end + let(:context) do + empty_stdin = File.open(File::NULL) # pre-commit hooks don't take input + context = Overcommit::HookContext.create('pre-commit', config, applicable_files, empty_stdin) + context + end + + around do |example| + repo do + example.run + end + end + + describe 'if not line-aware' do + let(:config_contents) do + <<-'YML' + PreCommit: + FooGitHook: + enabled: true + command: "foocmd" + YML + end + let(:hook_name) { 'FooGitHook' } + let(:applicable_files) { nil } + + before do + context.stub(:execute_hook).with(%w[foocmd]). + and_return(result) + end + + context 'when command succeeds' do + let(:result) do + double(success?: true, stdout: '') + end + + it { should pass } + end + + context 'when command fails' do + let(:result) do + double(success?: false, stdout: '', stderr: '') + end + + it { should fail_hook } + end + end +end From a359629d74072e3eeb06f127c98c833b233437cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillaume=20D=C3=A9flache?= Date: Fri, 11 Jun 2021 19:37:38 +0200 Subject: [PATCH 2/8] Add "ad-hoc" line-aware command hooks --- CHANGELOG.md | 1 + README.md | 62 +++++++++ lib/overcommit/hook_loader/base.rb | 28 ++++ .../hook_loader/plugin_hook_loader.rb | 26 +++- lib/overcommit/utils/messages_utils.rb | 8 ++ .../hook_loader/plugin_hook_loader_spec.rb | 131 ++++++++++++++++++ 6 files changed, 250 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f27e6208..a154d308 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## master (unreleased) * Add `--disable-pending-cops` as default flag to `RuboCop` pre-commit hook to ignore non-existent cops. Requires RuboCop `0.82.0` or newer. +* Add "ad-hoc" line-aware command hooks ## 0.58.0 diff --git a/README.md b/README.md index 36126a2c..d0404762 100644 --- a/README.md +++ b/README.md @@ -240,6 +240,7 @@ 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. +`ad_hoc` | *["Ad-hoc" line-aware command hooks](#adding-existing-line-aware-commands) only.* In addition to the built-in configuration options, each hook can expose its own unique configuration options. The `AuthorEmail` hook, for example, allows @@ -671,6 +672,67 @@ 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 + +Or in other words "low-code error format support." + +If you use tools that analyze files and report their findings line-by-line, +and that Overcommit does not yet support, you may be able to integrate them +with Overcommit without writing any Ruby code in a similar way as +[for existing Git hooks](#adding-existing-git-hooks). + +These special line-aware command hooks behave and are configured the same way +as the Git ones, except only file arguments get passed to them. +Also they must have the `ad_hoc` option, so that, using the command output: +- differentiating between warnings and errors becomes possible +- modified lines can be detected and acted upon as defined by + the `problem_on_unmodified_line`, `requires_files`, `include` and `exclude` + [hook options](#hook-options) + +**Warning**: Only the command's standard output stream is considered for now, +*not* its standard error stream. + +To differentiate between warning and error messages, +the `warning_message_type_pattern` suboption may be specified: +the `type` field of the `message_pattern` regexp below must then include +the `warning_message_type_pattern` option's text. + +The `message_pattern` suboption specifies the format of the command's messages. +It is a optional [(Ruby) regexp][RubyRE], which if present must at least define +a `file` [named capture group][RubyRENCG]. +The only other allowed ones are `line` and `type`, which when specified +enable detection of modified lines and warnings respectively. + +**Note**: The default value for this option is often adequate: +it generalizes the quasi-standard [GNU/Emacs-style error format][GNUEerrf], +adding the most frequently used warning syntax to it. + +For example: + +```yaml +PreCommit: + CustomScript: + enabled: true + command: './bin/custom-script' + ad_hoc: + message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ + warning_message_type_pattern: warning +``` + +**Tip**: To get the syntax of the regexps right, a Ruby interpreter like `irb` +can help: + +```ruby +require('yaml'); puts YAML.dump(/MY-REGEXP/) +``` + +Then copy the output line text as the YAML option's value, thereby +omitting the `---` prefix. + +[RubyRE]: https://ruby-doc.org/core-2.4.1/Regexp.html +[RubyRENCG]: https://ruby-doc.org/core-2.4.1/Regexp.html#class-Regexp-label-Capturing +[GNUEerrf]: https://www.gnu.org/prep/standards/standards.html#Errors + ## Security While Overcommit can make managing Git hooks easier and more convenient, diff --git a/lib/overcommit/hook_loader/base.rb b/lib/overcommit/hook_loader/base.rb index 9173e9f1..0f501a82 100644 --- a/lib/overcommit/hook_loader/base.rb +++ b/lib/overcommit/hook_loader/base.rb @@ -22,6 +22,34 @@ def load_hooks private + # GNU/Emacs-style error format: + AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN = + /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/.freeze + + 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? + + extract_messages(@config['ad_hoc'], result) + end + + def extract_messages(ad_hoc_config, result) + warning_message_type_pattern = ad_hoc_config['warning_message_type_pattern'] + Overcommit::Utils::MessagesUtils.extract_messages( + result.stdout.split("\n"), + ad_hoc_config['message_pattern'] || + AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN, + Overcommit::Utils::MessagesUtils.create_type_categorizer( + 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 d168a0de..2c829854 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_modified_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,24 @@ 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['ad_hoc'] + create_line_aware_command_hook_class(hook_base) + else + create_git_hook_class(hook_base) + end + + # Only to avoid warnings in unit tests...: + if hook_module.const_defined?(hook_name) + return hook_module.const_get(hook_name).new(@config, @context) + 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 c92a6010..a7baecb2 100644 --- a/lib/overcommit/utils/messages_utils.rb +++ b/lib/overcommit/utils/messages_utils.rb @@ -37,6 +37,14 @@ 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| + type.include?(warning_pattern) ? :warning : :error + end + end + private def extract_file(match, message) diff --git a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb index d8cdc505..dad1f28f 100644 --- a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -65,4 +65,135 @@ it { should fail_hook } end end + + describe 'if line-aware' do + let(:config_contents) do + <<-'YML' + PreCommit: + FooLint: + enabled: true + command: ["foo", "lint"] + ad_hoc: + message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ + warning_message_type_pattern: warning + flags: + - "--format=emacs" + include: '**/*.foo' + FooLintDefault: + enabled: true + command: ["foo", "lint"] + ad_hoc: + warning_message_type_pattern: warning + flags: + - "--format=emacs" + include: '**/*.foo' + FooLintDefaultNoWarnings: + enabled: true + command: ["foo", "lint"] + ad_hoc: + flags: + - "--format=emacs" + include: '**/*.foo' + YML + end + let(:hook_name) { 'FooLint' } + let(:applicable_files) { %w[file.foo] } + + before do + subject.stub(:applicable_files).and_return(applicable_files) + subject.stub(:execute).with(%w[foo lint --format=emacs], args: applicable_files). + and_return(result) + end + + context 'when command succeeds' do + let(:result) do + double(success?: true, stdout: '') + end + + it { should pass } + end + + context 'when command fails with empty stdout' do + let(:result) do + double(success?: false, stdout: '', stderr: '') + end + + it { should pass } + end + + context 'when command fails with some warning message' do + let(:result) do + double( + success?: false, + stdout: "A:1:warning...\n", + stderr: '' + ) + end + + it { should warn } + end + + context 'when command fails with some error message' do + let(:result) do + double( + success?: false, + stdout: "A:1:???\n", + stderr: '' + ) + end + + it { should fail_hook } + end + + describe '(using default pattern)' do + let(:hook_name) { 'FooLintDefault' } + + context 'when command fails with some warning message' do + let(:result) do + double( + success?: false, + stdout: <<-MSG, +B:1: warning: ??? + MSG + stderr: '' + ) + end + + it { should warn } + end + + context 'when command fails with some error message' do + let(:result) do + double( + success?: false, + stdout: <<-MSG, +A:1:80: error + MSG + stderr: '' + ) + end + + it { should fail_hook } + end + end + + describe '(using defaults)' do + let(:hook_name) { 'FooLintDefaultNoWarnings' } + + context 'when command fails with some messages' do + let(:result) do + double( + success?: false, + stdout: <<-MSG, +A:1:80: error +B:1: warning: ??? + MSG + stderr: '' + ) + end + + it { should fail_hook } + end + end + end end From ad88bf750b6fed0bba9921f51983888f03223a46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillaume=20D=C3=A9flache?= Date: Fri, 16 Jul 2021 12:35:43 +0200 Subject: [PATCH 3/8] Flatten YAML configuration as per review comments --- README.md | 16 ++++++++----- lib/overcommit/hook_loader/base.rb | 23 +++++++++++++++---- .../hook_loader/plugin_hook_loader.rb | 2 +- .../hook_loader/plugin_hook_loader_spec.rb | 10 ++++---- 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index d0404762..cb1cd430 100644 --- a/README.md +++ b/README.md @@ -683,7 +683,8 @@ with Overcommit without writing any Ruby code in a similar way as These special line-aware command hooks behave and are configured the same way as the Git ones, except only file arguments get passed to them. -Also they must have the `ad_hoc` option, so that, using the command output: +Also to enable the feature, they must use at least one of the following options, +so that, using the command output: - differentiating between warnings and errors becomes possible - modified lines can be detected and acted upon as defined by the `problem_on_unmodified_line`, `requires_files`, `include` and `exclude` @@ -691,13 +692,17 @@ Also they must have the `ad_hoc` option, so that, using the command output: **Warning**: Only the command's standard output stream is considered for now, *not* its standard error stream. +If you do not need to change the default values for any other option, +then the `extract_messages_from` option has to be specified. +Its value does not matter for now, but it should be set to `stdout` +to avoid problems in the future. To differentiate between warning and error messages, -the `warning_message_type_pattern` suboption may be specified: +the `warning_message_type_pattern` option may be specified: the `type` field of the `message_pattern` regexp below must then include the `warning_message_type_pattern` option's text. -The `message_pattern` suboption specifies the format of the command's messages. +The `message_pattern` option specifies the format of the command's messages. It is a optional [(Ruby) regexp][RubyRE], which if present must at least define a `file` [named capture group][RubyRENCG]. The only other allowed ones are `line` and `type`, which when specified @@ -714,9 +719,8 @@ PreCommit: CustomScript: enabled: true command: './bin/custom-script' - ad_hoc: - message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ - warning_message_type_pattern: warning + message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ + warning_message_type_pattern: warning ``` **Tip**: To get the syntax of the regexps right, a Ruby interpreter like `irb` diff --git a/lib/overcommit/hook_loader/base.rb b/lib/overcommit/hook_loader/base.rb index 0f501a82..3a6251fe 100644 --- a/lib/overcommit/hook_loader/base.rb +++ b/lib/overcommit/hook_loader/base.rb @@ -26,21 +26,34 @@ def load_hooks AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN = /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/.freeze - def create_line_aware_command_hook_class(hook_base) + def is_hook_line_aware(hook_config) + hook_config['extract_messages_from'] || + hook_config['message_pattern'] || + hook_config['warning_message_type_pattern'] + end + + def create_line_aware_command_hook_class(hook_base) # rubocop:disable Metrics/MethodLength Class.new(hook_base) do + def line_aware_config + { + message_pattern: @config['message_pattern'], + warning_message_type_pattern: @config['warning_message_type_pattern'] + } + end + def run result = execute(command, args: applicable_files) return :pass if result.success? - extract_messages(@config['ad_hoc'], result) + extract_messages(line_aware_config, result) end - def extract_messages(ad_hoc_config, result) - warning_message_type_pattern = ad_hoc_config['warning_message_type_pattern'] + def extract_messages(line_aware_config, result) + warning_message_type_pattern = line_aware_config[:warning_message_type_pattern] Overcommit::Utils::MessagesUtils.extract_messages( result.stdout.split("\n"), - ad_hoc_config['message_pattern'] || + line_aware_config[:message_pattern] || AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN, Overcommit::Utils::MessagesUtils.create_type_categorizer( warning_message_type_pattern diff --git a/lib/overcommit/hook_loader/plugin_hook_loader.rb b/lib/overcommit/hook_loader/plugin_hook_loader.rb index 2c829854..dfa2e88c 100644 --- a/lib/overcommit/hook_loader/plugin_hook_loader.rb +++ b/lib/overcommit/hook_loader/plugin_hook_loader.rb @@ -95,7 +95,7 @@ def create_ad_hoc_hook(hook_name) hook_config = @config.for_hook(hook_name, @context.hook_class_name) hook_class = - if hook_config['ad_hoc'] + if is_hook_line_aware(hook_config) create_line_aware_command_hook_class(hook_base) else create_git_hook_class(hook_base) diff --git a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb index dad1f28f..6c64232b 100644 --- a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -73,24 +73,22 @@ FooLint: enabled: true command: ["foo", "lint"] - ad_hoc: - message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ - warning_message_type_pattern: warning + message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ + warning_message_type_pattern: warning flags: - "--format=emacs" include: '**/*.foo' FooLintDefault: enabled: true command: ["foo", "lint"] - ad_hoc: - warning_message_type_pattern: warning + warning_message_type_pattern: warning flags: - "--format=emacs" include: '**/*.foo' FooLintDefaultNoWarnings: enabled: true command: ["foo", "lint"] - ad_hoc: + extract_messages_from: stdout flags: - "--format=emacs" include: '**/*.foo' From 9f1c8ba60fe6d67d2ddf606c213799bac5f65976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillaume=20D=C3=A9flache?= Date: Mon, 11 Oct 2021 19:27:10 +0200 Subject: [PATCH 4/8] Link to feature more + avoid "ad-hoc" term in docs --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index cb1cd430..6174a87b 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,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) @@ -41,6 +42,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) * [Contributing](#contributing) * [Community](#community) @@ -240,7 +242,7 @@ 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. -`ad_hoc` | *["Ad-hoc" line-aware command hooks](#adding-existing-line-aware-commands) only.* +[...]`message_`[...] | *[Line-aware command hooks](#adding-existing-line-aware-commands) only.* In addition to the built-in configuration options, each hook can expose its own unique configuration options. The `AuthorEmail` hook, for example, allows From 3dc1d149b933e540e219640209a4526e6f160d2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillaume=20D=C3=A9flache?= Date: Mon, 11 Oct 2021 20:14:47 +0200 Subject: [PATCH 5/8] Remove magic default value as per review comment --- README.md | 17 ++++++++--------- lib/overcommit/hook_loader/base.rb | 11 ++--------- .../hook_loader/plugin_hook_loader_spec.rb | 15 ++++++++------- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 6174a87b..7ca87a3f 100644 --- a/README.md +++ b/README.md @@ -685,8 +685,8 @@ with Overcommit without writing any Ruby code in a similar way as These special line-aware command hooks behave and are configured the same way as the Git ones, except only file arguments get passed to them. -Also to enable the feature, they must use at least one of the following options, -so that, using the command output: +Also to enable them and for optimal use, one must configure them as explained +below, so that, using the command output: - differentiating between warnings and errors becomes possible - modified lines can be detected and acted upon as defined by the `problem_on_unmodified_line`, `requires_files`, `include` and `exclude` @@ -694,10 +694,6 @@ so that, using the command output: **Warning**: Only the command's standard output stream is considered for now, *not* its standard error stream. -If you do not need to change the default values for any other option, -then the `extract_messages_from` option has to be specified. -Its value does not matter for now, but it should be set to `stdout` -to avoid problems in the future. To differentiate between warning and error messages, the `warning_message_type_pattern` option may be specified: @@ -705,13 +701,16 @@ the `type` field of the `message_pattern` regexp below must then include the `warning_message_type_pattern` option's text. The `message_pattern` option specifies the format of the command's messages. -It is a optional [(Ruby) regexp][RubyRE], which if present must at least define +It is mandatory, must be a [(Ruby) regexp][RubyRE], and must define at least a `file` [named capture group][RubyRENCG]. The only other allowed ones are `line` and `type`, which when specified enable detection of modified lines and warnings respectively. -**Note**: The default value for this option is often adequate: -it generalizes the quasi-standard [GNU/Emacs-style error format][GNUEerrf], +**Tip**: The following value for this option is often adequate: +```ruby +/^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ +``` +It generalizes the quasi-standard [GNU/Emacs-style error format][GNUEerrf], adding the most frequently used warning syntax to it. For example: diff --git a/lib/overcommit/hook_loader/base.rb b/lib/overcommit/hook_loader/base.rb index 3a6251fe..612b436f 100644 --- a/lib/overcommit/hook_loader/base.rb +++ b/lib/overcommit/hook_loader/base.rb @@ -22,14 +22,8 @@ def load_hooks private - # GNU/Emacs-style error format: - AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN = - /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/.freeze - def is_hook_line_aware(hook_config) - hook_config['extract_messages_from'] || - hook_config['message_pattern'] || - hook_config['warning_message_type_pattern'] + hook_config['message_pattern'] end def create_line_aware_command_hook_class(hook_base) # rubocop:disable Metrics/MethodLength @@ -53,8 +47,7 @@ def extract_messages(line_aware_config, result) warning_message_type_pattern = line_aware_config[:warning_message_type_pattern] Overcommit::Utils::MessagesUtils.extract_messages( result.stdout.split("\n"), - line_aware_config[:message_pattern] || - AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN, + line_aware_config[:message_pattern], Overcommit::Utils::MessagesUtils.create_type_categorizer( warning_message_type_pattern ) diff --git a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb index 6c64232b..3dca7319 100644 --- a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -78,17 +78,18 @@ flags: - "--format=emacs" include: '**/*.foo' - FooLintDefault: + FooLintRecommendedPattern: enabled: true command: ["foo", "lint"] + message_pattern: !ruby/regexp /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ warning_message_type_pattern: warning flags: - "--format=emacs" include: '**/*.foo' - FooLintDefaultNoWarnings: + FooLintRecommendedPatternNoWarnings: enabled: true command: ["foo", "lint"] - extract_messages_from: stdout + message_pattern: !ruby/regexp /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ flags: - "--format=emacs" include: '**/*.foo' @@ -143,8 +144,8 @@ it { should fail_hook } end - describe '(using default pattern)' do - let(:hook_name) { 'FooLintDefault' } + describe '(using recommended pattern)' do + let(:hook_name) { 'FooLintRecommendedPattern' } context 'when command fails with some warning message' do let(:result) do @@ -175,8 +176,8 @@ end end - describe '(using defaults)' do - let(:hook_name) { 'FooLintDefaultNoWarnings' } + describe '(using recommended pattern w/o warnings)' do + let(:hook_name) { 'FooLintRecommendedPatternNoWarnings' } context 'when command fails with some messages' do let(:result) do From b3178484b97f0f095af7ae55655d3cd14cf16154 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Mon, 9 Nov 2026 22:35:09 -0800 Subject: [PATCH 6/8] Complete configurable command diagnostics and cover issue #750 requirements --- README.md | 83 ++--- lib/overcommit/configuration_loader.rb | 2 +- lib/overcommit/hook_loader/base.rb | 39 +- .../hook_loader/plugin_hook_loader.rb | 9 +- lib/overcommit/utils/messages_utils.rb | 8 +- spec/overcommit/configuration_loader_spec.rb | 17 + .../hook_loader/plugin_hook_loader_spec.rb | 340 ++++++++++-------- 7 files changed, 283 insertions(+), 215 deletions(-) diff --git a/README.md b/README.md index 51ad3a63..8105b070 100644 --- a/README.md +++ b/README.md @@ -258,7 +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_`[...] | *[Line-aware command hooks](#adding-existing-line-aware-commands) only.* +`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 @@ -710,67 +711,49 @@ of hook, see the [git-hooks documentation][GHD]. ### Adding Existing Line-Aware Commands -Or in other words "low-code error format support." - -If you use tools that analyze files and report their findings line-by-line, -and that Overcommit does not yet support, you may be able to integrate them -with Overcommit without writing any Ruby code in a similar way as -[for existing Git hooks](#adding-existing-git-hooks). - -These special line-aware command hooks behave and are configured the same way -as the Git ones, except only file arguments get passed to them. -Also to enable them and for optimal use, one must configure them as explained -below, so that, using the command output: -- differentiating between warnings and errors becomes possible -- modified lines can be detected and acted upon as defined by - the `problem_on_unmodified_line`, `requires_files`, `include` and `exclude` - [hook options](#hook-options) - -**Warning**: Only the command's standard output stream is considered for now, -*not* its standard error stream. - -To differentiate between warning and error messages, -the `warning_message_type_pattern` option may be specified: -the `type` field of the `message_pattern` regexp below must then include -the `warning_message_type_pattern` option's text. - -The `message_pattern` option specifies the format of the command's messages. -It is mandatory, must be a [(Ruby) regexp][RubyRE], and must define at least -a `file` [named capture group][RubyRENCG]. -The only other allowed ones are `line` and `type`, which when specified -enable detection of modified lines and warnings respectively. - -**Tip**: The following value for this option is often adequate: -```ruby -/^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ -``` -It generalizes the quasi-standard [GNU/Emacs-style error format][GNUEerrf], -adding the most frequently used warning syntax to it. +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. -For example: +`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' - message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ - warning_message_type_pattern: warning + include: '**/*.rb' + message_pattern: !ruby/regexp /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ + warning_message_type_pattern: !ruby/regexp /^warning:/i ``` -**Tip**: To get the syntax of the regexps right, a Ruby interpreter like `irb` -can help: +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. -```ruby -require('yaml'); puts YAML.dump(/MY-REGEXP/) -``` +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`. -Then copy the output line text as the YAML option's value, thereby -omitting the `---` prefix. +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.4.1/Regexp.html -[RubyRENCG]: https://ruby-doc.org/core-2.4.1/Regexp.html#class-Regexp-label-Capturing -[GNUEerrf]: https://www.gnu.org/prep/standards/standards.html#Errors +[RubyRE]: https://ruby-doc.org/core-2.7.3/Regexp.html ## Security 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 612b436f..46e6bd14 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,34 +24,33 @@ def load_hooks private - def is_hook_line_aware(hook_config) - hook_config['message_pattern'] + 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) # rubocop:disable Metrics/MethodLength + def create_line_aware_command_hook_class(hook_base) Class.new(hook_base) do - def line_aware_config - { - message_pattern: @config['message_pattern'], - warning_message_type_pattern: @config['warning_message_type_pattern'] - } - end - 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? - extract_messages(line_aware_config, result) - end - - def extract_messages(line_aware_config, result) - warning_message_type_pattern = line_aware_config[:warning_message_type_pattern] Overcommit::Utils::MessagesUtils.extract_messages( - result.stdout.split("\n"), - line_aware_config[:message_pattern], + output, + @config['message_pattern'], Overcommit::Utils::MessagesUtils.create_type_categorizer( - warning_message_type_pattern + @config['warning_message_type_pattern'] ) ) end diff --git a/lib/overcommit/hook_loader/plugin_hook_loader.rb b/lib/overcommit/hook_loader/plugin_hook_loader.rb index db1a620d..0fdeb092 100644 --- a/lib/overcommit/hook_loader/plugin_hook_loader.rb +++ b/lib/overcommit/hook_loader/plugin_hook_loader.rb @@ -95,15 +95,16 @@ def create_ad_hoc_hook(hook_name) hook_config = @config.for_hook(hook_name, @context.hook_class_name) hook_class = - if is_hook_line_aware(hook_config) + 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 - # Only to avoid warnings in unit tests...: - if hook_module.const_defined?(hook_name) - return hook_module.const_get(hook_name).new(@config, @context) + # 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) diff --git a/lib/overcommit/utils/messages_utils.rb b/lib/overcommit/utils/messages_utils.rb index adb3923e..b83d3ac0 100644 --- a/lib/overcommit/utils/messages_utils.rb +++ b/lib/overcommit/utils/messages_utils.rb @@ -41,7 +41,13 @@ def create_type_categorizer(warning_pattern) return nil if warning_pattern.nil? lambda do |type| - type.include?(warning_pattern) ? :warning : :error + warning = + if warning_pattern.is_a?(Regexp) + type && warning_pattern.match?(type) + else + type&.include?(warning_pattern) + end + warning ? :warning : :error end end 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 index 3dca7319..893c9317 100644 --- a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -2,197 +2,257 @@ require 'spec_helper' -describe 'ad-hoc pre-commit hook' do - subject do - hook_loader = Overcommit::HookLoader::PluginHookLoader.new( - config, - context, - Overcommit::Logger.new(STDOUT) - ) - hooks = hook_loader.load_hooks - hooks.find { |h| h.name == hook_name } - end +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 - config = Overcommit::Configuration.new( - YAML.safe_load(config_contents, [Regexp]), { - validate: false - } + Overcommit::ConfigurationLoader.default_configuration.merge( + Overcommit::Configuration.new( + 'PreCommit' => { 'CustomScript' => hook_config.merge(config_changes) } + ) ) - Overcommit::ConfigurationLoader.default_configuration.merge(config) - end - let(:context) do - empty_stdin = File.open(File::NULL) # pre-commit hooks don't take input - context = Overcommit::HookContext.create('pre-commit', config, applicable_files, empty_stdin) - context 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 do - example.run + 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 - describe 'if not line-aware' do - let(:config_contents) do - <<-'YML' - PreCommit: - FooGitHook: - enabled: true - command: "foocmd" - YML + context 'with a message pattern' do + let(:hook_config) do + super().merge( + 'message_pattern' => /^(?[^:]+):(?\d+): (?\w+):/, + 'warning_message_type_pattern' => /^warn(?:ing)?$/i, + 'flags' => ['--format=emacs'], + 'include' => '**/*.foo', + 'exclude' => 'excluded.foo' + ) end - let(:hook_name) { 'FooGitHook' } - let(:applicable_files) { nil } before do - context.stub(:execute_hook).with(%w[foocmd]). + 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 - context 'when command succeeds' do - let(:result) do - double(success?: true, stdout: '') - 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 { should pass } + 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 - context 'when command fails' do - let(:result) do - double(success?: false, stdout: '', stderr: '') - end + it 'warns on warning messages' do + expect(hook.run_and_transform.first).to eq(:warn) + end - it { should fail_hook } + 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 - end - describe 'if line-aware' do - let(:config_contents) do - <<-'YML' - PreCommit: - FooLint: - enabled: true - command: ["foo", "lint"] - message_pattern: !ruby/regexp /^(?[^:]+):(?[0-9]+):(?[^ ]+)/ - warning_message_type_pattern: warning - flags: - - "--format=emacs" - include: '**/*.foo' - FooLintRecommendedPattern: - enabled: true - command: ["foo", "lint"] - message_pattern: !ruby/regexp /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ - warning_message_type_pattern: warning - flags: - - "--format=emacs" - include: '**/*.foo' - FooLintRecommendedPatternNoWarnings: - enabled: true - command: ["foo", "lint"] - message_pattern: !ruby/regexp /^(?(?:\w:)?[^:]+):(?\d+):[^ ]* (?[^ ]+)/ - flags: - - "--format=emacs" - include: '**/*.foo' - YML - end - let(:hook_name) { 'FooLint' } - let(:applicable_files) { %w[file.foo] } + 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 - before do - subject.stub(:applicable_files).and_return(applicable_files) - subject.stub(:execute).with(%w[foo lint --format=emacs], args: applicable_files). - and_return(result) + 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 - context 'when command succeeds' do - let(:result) do - double(success?: true, stdout: '') - 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 { should pass } + 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 - context 'when command fails with empty stdout' do - let(:result) do - double(success?: false, stdout: '', stderr: '') + 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 - it { should pass } + 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 'when command fails with some warning message' do - let(:result) do - double( - success?: false, - stdout: "A:1:warning...\n", - stderr: '' - ) + context 'with a file-only pattern' do + let(:config_changes) { { 'message_pattern' => /^(?[^:]+):/ } } + + 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' => /^(?[^:]+):(?\d+):(?: (?error):)?/ } end - it { should warn } + it 'defaults to an error' do + expect(hook.run.first.type).to eq(:error) + end end - context 'when command fails with some error message' do - let(:result) do - double( - success?: false, - stdout: "A:1:???\n", - stderr: '' - ) + 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 { should fail_hook } + 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 - describe '(using recommended pattern)' do - let(:hook_name) { 'FooLintRecommendedPattern' } + context 'with errors on unmodified lines' do + let(:stdout) { "file.foo:1: error: old problem\n" } - context 'when command fails with some warning message' do - let(:result) do - double( - success?: false, - stdout: <<-MSG, -B:1: warning: ??? - MSG - stderr: '' - ) + 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 { should warn } + it 'fails' do + expect(hook.run_and_transform.first).to eq(:fail) + end end - context 'when command fails with some error message' do - let(:result) do - double( - success?: false, - stdout: <<-MSG, -A:1:80: error - MSG - stderr: '' - ) + 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 { should fail_hook } + it 'reports a configuration error' do + expect { hook }.to raise_error(Overcommit::Exceptions::HookLoadError, /message_pattern/) end end - describe '(using recommended pattern w/o warnings)' do - let(:hook_name) { 'FooLintRecommendedPatternNoWarnings' } + context 'when message_pattern has no file capture' do + let(:pattern) { /(?\d+)/ } - context 'when command fails with some messages' do - let(:result) do - double( - success?: false, - stdout: <<-MSG, -A:1:80: error -B:1: warning: ??? - MSG - stderr: '' - ) - end + it 'explains the required capture' do + expect { hook }.to raise_error(Overcommit::Exceptions::HookLoadError, /file/) + end + end - it { should fail_hook } + 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' => /^(?[^:]+):(?\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' => /^(?[^:]+):/ } } + ) + ) + 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 From 6380372549f95c9a2e6af9f5c4e8cfbbfa8bf7e3 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Mon, 9 Nov 2026 22:35:24 -0800 Subject: [PATCH 7/8] Separate successful command guard from diagnostic parsing --- lib/overcommit/hook_loader/base.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/overcommit/hook_loader/base.rb b/lib/overcommit/hook_loader/base.rb index 46e6bd14..4ecfab7f 100644 --- a/lib/overcommit/hook_loader/base.rb +++ b/lib/overcommit/hook_loader/base.rb @@ -43,6 +43,7 @@ def create_line_aware_command_hook_class(hook_base) 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? From a15939bef311ff781f9ec789e357557dde943ac0 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Mon, 9 Nov 2026 22:38:44 -0800 Subject: [PATCH 8/8] Exercise drive-letter diagnostics in command hook tests --- .../hook_loader/plugin_hook_loader_spec.rb | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb index 893c9317..054643e7 100644 --- a/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb +++ b/spec/overcommit/hook_loader/plugin_hook_loader_spec.rb @@ -49,7 +49,7 @@ context 'with a message pattern' do let(:hook_config) do super().merge( - 'message_pattern' => /^(?[^:]+):(?\d+): (?\w+):/, + 'message_pattern' => /^(?(?:\w:)?[^:]+):(?\d+): (?\w+):/, 'warning_message_type_pattern' => /^warn(?:ing)?$/i, 'flags' => ['--format=emacs'], 'include' => '**/*.foo', @@ -72,6 +72,11 @@ ]) 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 @@ -131,7 +136,7 @@ end context 'with a file-only pattern' do - let(:config_changes) { { 'message_pattern' => /^(?[^:]+):/ } } + let(:config_changes) { { 'message_pattern' => /^(?(?:\w:)?[^:]+):/ } } it 'supports missing line and type captures' do message = hook.run.first @@ -141,7 +146,7 @@ context 'with an unmatched optional type capture' do let(:config_changes) do - { 'message_pattern' => /^(?[^:]+):(?\d+):(?: (?error):)?/ } + { 'message_pattern' => /^(?(?:\w:)?[^:]+):(?\d+):(?: (?error):)?/ } end it 'defaults to an error' do @@ -235,7 +240,7 @@ hook_config.merge!( 'command' => [RbConfig.ruby, '-e', 'puts ARGV.first + ":2: warning: check this"; exit 1', '--'], - 'message_pattern' => /^(?[^:]+):(?\d+): (?\w+):/, + 'message_pattern' => /^(?(?:\w:)?[^:]+):(?\d+): (?\w+):/, 'warning_message_type_pattern' => /^warning$/ ) status, output = hook.run_and_transform @@ -247,7 +252,7 @@ loader.load_hooks updated_config = config.merge( Overcommit::Configuration.new( - 'PreCommit' => { 'CustomScript' => { 'message_pattern' => /^(?[^:]+):/ } } + 'PreCommit' => { 'CustomScript' => { 'message_pattern' => /^(?(?:\w:)?[^:]+):/ } } ) ) updated_loader = described_class.new(updated_config, context, Overcommit::Logger.silent)