Add support for commit-msg hooks

Change-Id: Ib159889f57131a40e0868c75f33429534a6db123
Reviewed-on: http://gerrit.causes.com/35560
Reviewed-by: Shane da Silva <shane@causes.com>
Tested-by: Shane da Silva <shane@causes.com>

Author: sds

TODO

@@ -1,7 +1,7 @@
-Handle case where hooks are installed but overcommit isn't
+# Handle case where hooks are installed but overcommit isn't
 Add user-friendly installation instructions (e.g. run `overcommit` in git
 repo and get asked to install if it is not installed)
-Symlink all scripts to single Overcommit file
+#Symlink all scripts to single Overcommit file
 Ask to remove sample files
 Parallelize hook checks
 Stash non-indexed changes before running checks so real filename can be used
@@ -15,6 +15,9 @@ Allow hooks to specify required executable (`executable` option, coupled with
 Allow hooks to specify whether they only apply to lines the user modified
 Split out Whitespace check into individual checks
 Allow SKIP arguments to be case insensitive
+Move ReleaseNote check into Causes repo
+Enable hook by simply including it in repo-specific .overcommit.yml (no need
+to specify enabled: true)
 
 
 Allow features of hooks to be customized (stealth, required, etc.)

bin/run-hook

@@ -21,7 +21,7 @@ pre_commit:
   AuthorName:
     requires_files: false
     required: true
-    silent: true
+    quiet: true
 
   CoffeeLint:
     description: 'Analyzing with coffeelint'
@@ -75,8 +75,15 @@ prepare_commit_msg:
 # project.
 commit_msg:
   ALL:
+    requires_files: false
     quiet: false
 
+  GerritChangeId:
+    enabled: false
+    description: 'Ensuring Gerrit Change-Id is present'
+    required: true
+    quiet: true
+
   HardTabs:
     description: 'Checking for hard tabs'
 

config/default.yml

@@ -47,11 +47,11 @@ def apply_environment!(hook_type, env)
       skipped_hooks = "#{env['SKIP']} #{env['SKIP_CHECKS']}".split(/[:, ]/)
 
       if skipped_hooks.include?('all') || skipped_hooks.include?('ALL')
-        @hash[hook_type]['ALL']['enabled'] = false
+        @hash[hook_type]['ALL']['skip'] = true
       else
         skipped_hooks.each do |hook_name|
           @hash[hook_type][hook_name] ||= {}
-          @hash[hook_type][hook_name]['enabled'] = false
+          @hash[hook_type][hook_name]['skip'] = true
         end
       end
     end

lib/overcommit/configuration.rb

@@ -3,8 +3,9 @@
 module Overcommit::Hook
   # Functionality common to all hooks.
   class Base
-    def initialize(config)
+    def initialize(config, hook_runner)
       @config = config.hook_config(self)
+      @hook_runner = hook_runner
     end
 
     # Runs the hook.
@@ -24,8 +25,8 @@ def required?
       @config['required']
     end
 
-    def silent?
-      @config['silent']
+    def quiet?
+      @config['quiet']
     end
 
     def enabled?
@@ -37,8 +38,13 @@ def requires_modified_files?
     end
 
     def skip?
-      !(enabled? || required?) ||
-        (requires_modified_files? && applicable_files.empty?)
+      @config['skip']
+    end
+
+    def run?
+      enabled? &&
+        (!skip? || required?) &&
+        !(requires_modified_files? && applicable_files.empty?)
     end
 
     # Gets a list of staged files that apply to this hook based on its
@@ -49,13 +55,8 @@ def applicable_files
 
   private
 
-    # Get a list of added, copied, or modified files that have been staged.
-    # Renames and deletions are ignored, since there should be nothing to check.
     def staged_files
-      @staged_files ||=
-        `git diff --cached --name-only --diff-filter=ACM --ignore-submodules=all`.
-          split("\n").
-          map { |relative_file| File.expand_path(relative_file) }
+      @hook_runner.staged_files
     end
 
     # Returns whether the specified file is applicable to this hook based on the

lib/overcommit/hook/base.rb

@@ -0,0 +1,8 @@
+module Overcommit::Hook::CommitMsg
+  # Functionality common to all commit-msg hooks.
+  class Base < Overcommit::Hook::Base
+    def commit_message
+      @hook_runner.commit_message
+    end
+  end
+end

lib/overcommit/hook/commit_msg/base.rb

@@ -0,0 +1,16 @@
+module Overcommit::Hook::CommitMsg
+  # Ensures a Gerrit Change-Id line is included in the commit message.
+  #
+  # It may seem odd to do this here instead of in a prepare-commit-msg hook, but
+  # the reality is that if you want to _ensure_ the Change-Id is included then
+  # you need to do it in a commit-msg hook. This is because the user could still
+  # edit the message after a prepare-commit-msg hook was run.
+  class GerritChangeId < Base
+    SCRIPT_LOCATION = Overcommit::Utils.script_path('gerrit-change-id')
+
+    def run
+      command "#{SCRIPT_LOCATION} #{commit_message_file}"
+      :good
+    end
+  end
+end

lib/overcommit/hook/commit_msg/gerrit_change_id.rb

@@ -1,10 +1,9 @@
-module Overcommit::GitHook
-  class HardTabs < HookSpecificCheck
-    include HookRegistry
-
-    def run_check
+module Overcommit::Hook::CommitMsg
+  # Checks for hard tabs in commit messages.
+  class HardTabs < Base
+    def run
       # Catches hard tabs entered by the user (not auto-generated)
-      if commit_message.join.index /\t/
+      if commit_message.join.index(/\t/)
         return :warn, "Don't use hard tabs in commit messages"
       end
 

lib/overcommit/plugins/commit_msg/hard_tabs.rb lib/overcommit/hook/commit_msg/hard_tabs.rb

@@ -1,11 +1,9 @@
-module Overcommit::GitHook
-  class RussianNovel < HookSpecificCheck
-    include HookRegistry
-
-    stealth!
-
+module Overcommit::Hook::CommitMsg
+  # Checks for long commit messages (not good or bad--just fun to point out)
+  class RussianNovel < Base
     RUSSIAN_NOVEL_LENGTH = 30
-    def run_check
+
+    def run
       if commit_message.length > RUSSIAN_NOVEL_LENGTH
         return :warn, 'You seem to have authored a Russian novel; congratulations!'
       end

lib/overcommit/plugins/commit_msg/russian_novel.rb lib/overcommit/hook/commit_msg/russian_novel.rb

@@ -0,0 +1,12 @@
+module Overcommit::Hook::CommitMsg
+  # Ensures commit message subject lines are followed by a blank line.
+  class SingleLineSubject < Base
+    def run
+      unless commit_message[1].to_s.strip.empty?
+        return :warn, 'Subject should be one line and followed by a blank line'
+      end
+
+      :good
+    end
+  end
+end

lib/overcommit/hook/commit_msg/single_line_subject.rb

@@ -1,8 +1,8 @@
-module Overcommit::GitHook
-  class TextWidth < HookSpecificCheck
-    include HookRegistry
-
-    def run_check
+module Overcommit::Hook::CommitMsg
+  # Ensures the number of columns the subject and commit message lines occupy is
+  # under the preferred limits.
+  class TextWidth < Base
+    def run
       if commit_message.first.size > 60
         return :warn, 'Please keep the subject < ~60 characters'
       end

lib/overcommit/plugins/commit_msg/text_width.rb lib/overcommit/hook/commit_msg/text_width.rb

@@ -1,8 +1,7 @@
-module Overcommit::GitHook
-  class TrailingPeriod < HookSpecificCheck
-    include HookRegistry
-
-    def run_check
+module Overcommit::Hook::CommitMsg
+  # Ensures commit message subject lines do not have a trailing period
+  class TrailingPeriod < Base
+    def run
       if commit_message[0].rstrip.end_with?('.')
         return :warn, 'Please omit trailing period from commit message subject'
       end

lib/overcommit/plugins/commit_msg/trailing_period.rb lib/overcommit/hook/commit_msg/trailing_period.rb

@@ -7,12 +7,12 @@ def run
       # Catches hard tabs
       result = command("grep -Inl \"\\t\" #{paths}")
       unless result.stdout.empty?
-        return :stop, "Don't use hard tabs:\n#{result.stdout}"
+        return :bad, "Don't use hard tabs:\n#{result.stdout}"
       end
 
       # Catches trailing whitespace, conflict markers etc
       result = command('git diff --check --cached')
-      return :stop, result.stdout unless result.success?
+      return :bad, result.stdout unless result.success?
 
       :good
     end

lib/overcommit/hook/pre_commit/whitespace.rb

@@ -9,11 +9,15 @@ def initialize(config)
 
     # Loads and runs the hooks registered for this {HookRunner}.
     def run(args, input, logger)
-      #stash_unstaged_files
+      @args = args
+      @input = input
+      @logger = logger
+
+      # stash_unstaged_files
       load_hooks
-      run_hooks(args, input, logger)
+      run_hooks
     ensure
-      #restore_unstaged_files
+      # restore_unstaged_files
     end
 
     # Returns the type of hook this runner deals with (e.g. "CommitMsg",
@@ -26,14 +30,23 @@ def underscored_hook_type
       @underscored_hook_type ||= Overcommit::Utils.underscorize(hook_type)
     end
 
+    # Get a list of added, copied, or modified files that have been staged.
+    # Renames and deletions are ignored, since there should be nothing to check.
+    def staged_files
+      @staged_files ||=
+        `git diff --cached --name-only --diff-filter=ACM --ignore-submodules=all`.
+          split("\n").
+          map { |relative_file| File.expand_path(relative_file) }
+    end
+
   private
 
-    def run_hooks(args, input, log)
-      reporter = Overcommit::Reporter.new(self, @hooks, @config, log)
+    def run_hooks
+      reporter = Overcommit::Reporter.new(self, @hooks, @config, @logger)
 
       reporter.start_hook_run
 
-      @hooks.reject { |hook| hook.skip? }.
+      @hooks.select { |hook| hook.run? }.
              each do |hook|
         reporter.with_status(hook) do
           hook.run
@@ -72,8 +85,7 @@ def load_hook_plugins
       Dir[File.join(directory, '*.rb')].sort do |plugin|
         require plugin
 
-        hook_name = Overcommit::HookRunner.hook_type_to_class_name(File.basename(plugin, '.rb'))
-
+        hook_name = self.class.hook_type_to_class_name(File.basename(plugin, '.rb'))
         @hooks << create_hook(hook_name)
       end
     end
@@ -82,7 +94,7 @@ def load_hook_plugins
     # hook configuration.
     def create_hook(hook_name)
       Overcommit::Hook.const_get("#{hook_type}::#{hook_name}").
-                       new(@config)
+                       new(@config, self)
     rescue LoadError, NameError => error
       raise Overcommit::Exceptions::HookLoadError,
             "Unable to load hook '#{hook_name}': #{error}",

lib/overcommit/hook_runner/base.rb

@@ -0,0 +1,25 @@
+module Overcommit::HookRunner
+  # Manages loading and running commit-msg hooks.
+  class CommitMsg < Base
+    # User commit message stripped of comments and diff (from verbose output)
+    def commit_message
+      @commit_message ||= raw_commit_message.
+        reject     { |line| line =~ /^#/ }.
+        take_while { |line| !line.start_with?('diff --git') }
+    end
+
+    def raw_commit_message
+      @raw_commit_message ||= ::IO.readlines(commit_message_file)
+    end
+
+  private
+
+    def commit_message_file
+      unless @args[0] && ::File.exist?(@args[0])
+        fail 'Not running in the context of a commit message'
+      end
+
+      @args[0]
+    end
+  end
+end

lib/overcommit/hook_runner/commit_msg.rb

@@ -8,7 +8,7 @@ def initialize(hook_runner, hooks, config, log)
       @config  = config
       @name    = hook_runner.underscored_hook_type.gsub('_', '-')
       @hooks   = hooks
-      @width   = 60 - (@hooks.map { |s| s.name.length }.max || 0)
+      @width   = [@hooks.map { |s| s.description.length }.max + 3, 60].max
       @results = []
     end
 
@@ -18,7 +18,7 @@ def start_hook_run
 
     def with_status(hook, &block)
       title = "  #{hook.description}"
-      unless hook.silent?
+      unless hook.quiet?
         log.partial title
         print '.' * (@width - title.length)
       end
@@ -37,6 +37,8 @@ def finish_hook_run
       else
         log.error "✗ One or more #{@name} checks failed"
       end
+
+      log.log # Newline
     end
 
     def checks_passed?
@@ -46,7 +48,7 @@ def checks_passed?
   private
 
     def print_hook_result(hook, title, status, output)
-      if hook.silent?
+      if hook.quiet?
         return if status == :good
         log.partial title
       end