750: Teach ad-hoc hooks line parsing

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
 

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 /^(?<file>[^:]+):(?<line>[0-9]+):(?<type>[^ ]+)/
+      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,

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 =
+      /^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]* (?<type>[^ ]+)/.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.

lib/overcommit/hook_loader/plugin_hook_loader.rb

@@ -74,23 +74,37 @@ 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
             [:fail, result.stdout + result.stderr]
           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

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)

spec/overcommit/hook_loader/plugin_hook_loader_spec.rb

@@ -0,0 +1,199 @@
+# 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
+
+  describe 'if line-aware' do
+    let(:config_contents) do
+      <<-'YML'
+        PreCommit:
+          FooLint:
+            enabled: true
+            command: ["foo", "lint"]
+            ad_hoc:
+              message_pattern: !ruby/regexp /^(?<file>[^:]+):(?<line>[0-9]+):(?<type>[^ ]+)/
+              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