From d8fa43b57a1a59bdca501bc9b1d35077751f3473 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Fri, 12 Feb 2016 10:32:40 -0500 Subject: [PATCH] Improve error output on extract_messages failure The `extract_messages` helper was designed to fail loudly if the input didn't match what was expected by the regex. The goal was to help hook authors debug and catch these issues quickly and easily. However, there are some situations where hook authors did nothing wrong, for example if you are using `rbenv` and on a version of Ruby where a gem hasn't yet been installed (but the shim already exists), you'll get an error like the following: The `rubocop' command exists in these Ruby versions: 1.9.3-p551 2.1.1 2.1.2 2.2.1 2.2.2 2.3.0 jruby-1.7.20 ...which appears like the following in the hook error output: Analyzing with RuboCop..............................[RuboCop] FAILED Hook raised unexpected error Unexpected output: unable to determine line number or type of error/warning for message '' lib/overcommit/hook/pre_commit/base.rb:31:in `block in extract_messages' lib/overcommit/hook/pre_commit/base.rb:28:in `map' lib/overcommit/hook/pre_commit/base.rb:28:in `extract_messages' lib/overcommit/hook/pre_commit/rubo_cop.rb:19:in `run' lib/overcommit/hook/base.rb:45:in `block in run_and_transform' lib/overcommit/utils.rb:259:in `with_environment' lib/overcommit/hook/base.rb:45:in `run_and_transform' lib/overcommit/hook_runner.rb:152:in `run_hook' lib/overcommit/hook_runner.rb:88:in `block in consume' lib/overcommit/hook_runner.rb:85:in `loop' lib/overcommit/hook_runner.rb:85:in `consume' The stacktrace in this case is unsightly and doesn't explain what's actually going on. Fix this by throwing a special class of error and printing the remaining unprocessed output. Fixes #335 --- CHANGELOG.md | 2 ++ lib/overcommit/exceptions.rb | 3 +++ lib/overcommit/hook/pre_commit/base.rb | 20 +++++++++++++------- lib/overcommit/hook_runner.rb | 3 +++ 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06e7331a..0c959dc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ Unicode characters could cause a crash due to invalid byte sequences * Add `ForbiddenBranches` pre-commit hook which prevents creating a commit on any blacklisted branch by name/pattern +* Improve error message output when there is a problem processing messages + via `extract_messages` pre-commit hook helper ## 0.32.0.rc1 diff --git a/lib/overcommit/exceptions.rb b/lib/overcommit/exceptions.rb index dda9f8f0..14d4622d 100644 --- a/lib/overcommit/exceptions.rb +++ b/lib/overcommit/exceptions.rb @@ -42,6 +42,9 @@ class InvalidHookDefinition < StandardError; end # Raised when one or more hook plugin signatures have changed. class InvalidHookSignature < StandardError; end + # Raised when there is a problem processing output into {Hook::Messages}s. + class MessageProcessingError < StandardError; end + # Raised when an installation target already contains non-Overcommit hooks. class PreExistingHooks < StandardError; end end diff --git a/lib/overcommit/hook/pre_commit/base.rb b/lib/overcommit/hook/pre_commit/base.rb index c7c7456a..7d53d565 100644 --- a/lib/overcommit/hook/pre_commit/base.rb +++ b/lib/overcommit/hook/pre_commit/base.rb @@ -22,13 +22,16 @@ class Base < Overcommit::Hook::Base # @param type_categorizer [Proc] function executed against the `type` # capture group to convert it to a `:warning` or `:error` symbol. Assumes # `:error` if `nil`. - # @raise [RuntimeError] line of output did not match regex + # @raise [Overcommit::Exceptions::MessageProcessingError] line of output did + # not match regex # @return [Array] def extract_messages(output_messages, regex, type_categorizer = nil) - output_messages.map do |message| + output_messages.map.with_index do |message, index| unless match = message.match(regex) - raise 'Unexpected output: unable to determine line number or type ' \ - "of error/warning for message '#{message}'" + raise Overcommit::Exceptions::MessageProcessingError, + 'Unexpected output: unable to determine line number or type ' \ + "of error/warning for output:\n" \ + "#{output_messages[index..-1].join("\n")}" end file = extract_file(match, message) @@ -43,7 +46,8 @@ def extract_file(match, message) return unless match.names.include?('file') if match[:file].to_s.empty? - raise "Unexpected output: no file found in '#{message}'" + raise Overcommit::Exceptions::MessageProcessingError, + "Unexpected output: no file found in '#{message}'" end match[:file] @@ -53,7 +57,8 @@ def extract_line(match, message) return unless match.names.include?('line') Integer(match[:line]) rescue ArgumentError, TypeError - raise "Unexpected output: invalid line number found in '#{message}'" + raise Overcommit::Exceptions::MessageProcessingError, + "Unexpected output: invalid line number found in '#{message}'" end def extract_type(match, message, type_categorizer) @@ -61,7 +66,8 @@ def extract_type(match, message, type_categorizer) type_match = match.names.include?('type') ? match[:type] : nil type = type_categorizer.call(type_match) unless Overcommit::Hook::MESSAGE_TYPES.include?(type) - raise "Invalid message type '#{type}' for '#{message}': must " \ + raise Overcommit::Exceptions::MessageProcessingError, + "Invalid message type '#{type}' for '#{message}': must " \ "be one of #{Overcommit::Hook::MESSAGE_TYPES.inspect}" end type diff --git a/lib/overcommit/hook_runner.rb b/lib/overcommit/hook_runner.rb index d55a9f3a..901e25e8 100644 --- a/lib/overcommit/hook_runner.rb +++ b/lib/overcommit/hook_runner.rb @@ -150,6 +150,9 @@ def run_hook(hook) # rubocop:disable Metrics/CyclomaticComplexity return if should_skip?(hook) status, output = hook.run_and_transform + rescue Overcommit::Exceptions::MessageProcessingError => ex + status = :fail + output = ex.message rescue => ex status = :fail output = "Hook raised unexpected error\n#{ex.message}\n#{ex.backtrace.join("\n")}"