Update ReplaceBranch hook to avoid running on --amend by default (#725

)

* change default branch pattern for ReplaceBranch

This changes the default branch pattern to a simpler pattern.

This is added to make understanding the hook and it's usage a bit
easier.

* change an example in ReplaceBranch spec to default

This changes the spec to test against the same default branch
replacement pattern specified in the default config.

This is just to make things more consistent.

* improve class comment describing ReplaceBranch usage

This makes the usage of this (hopefully) obvious to anyone reading it.

* add `commit` to default ReplaceBranch skipped types

This makes the default behavior of this hook skip commits that explicitly
give the SHA, i.e, `--amend`, `-c`, or `-C`.

This is added because most people (I'd argue) don't want to run this
hook on `amend`s.

* remove extra hard-coded space in ReplaceBranch

This removes a trailing space after the repacement text, as well as
`#strip`ing the replacement text.

* refactor ReplaceBranch specs to avoid so much mocking

This changes the specs for the ReplaceBranch hook to

- test the contents of the file that `git` writes it's commit message to
- explicitly set config variables instead of mocking them

* update ReplaceBranch specs to test all `skipped_commit_types`

* shorten a long line for the linter

config/default.yml

@@ -1237,9 +1237,14 @@ PrepareCommitMsg:
   ReplaceBranch:
     enabled: false
     description: 'Prepends the commit message with text based on the branch name'
-    branch_pattern: '\A.*\w+[-_](\d+).*\z'
+    branch_pattern: '\A(\d+)-(\w+).*\z'
     replacement_text: '[#\1]'
-    skipped_commit_types: ['message', 'template', 'merge', 'squash']
+    skipped_commit_types:
+     - 'message'  # if message is given via `-m`, `-F`
+     - 'template' # if `-t` is given or `commit.template` is set
+     - 'commit'   # if `-c`, `-C`, or `--amend` is given
+     - 'merge'    # if merging
+     - 'squash'   # if squashing
     on_fail: warn
 
 # Hooks that run during `git push`, after remote refs have been updated but

lib/overcommit/hook/prepare_commit_msg/replace_branch.rb

@@ -2,11 +2,37 @@
 
 module Overcommit::Hook::PrepareCommitMsg
   # Prepends the commit message with a message based on the branch name.
+  #
+  # === What to prepend
+  #
   # It's possible to reference parts of the branch name through the captures in
   # the `branch_pattern` regex.
+  #
+  # For instance, if your current branch is `123-topic` then this config
+  #
+  #    branch_pattern: '(\d+)-(\w+)'
+  #    replacement_text: '[#\1]'
+  #
+  # would make this hook prepend commit messages with `[#123]`.
+  #
+  # Similarly, a replacement text of `[\1][\2]` would result in `[123][topic]`.
+  #
+  # == When to run this hook
+  #
+  # You can configure this to run only for specific types of commits by setting
+  # the `skipped_commit_types`. The allowed types are
+  #
+  # - 'message'  - if message is given via `-m`, `-F`
+  # - 'template' - if `-t` is given or `commit.template` is set
+  # - 'commit'   - if `-c`, `-C`, or `--amend` is given
+  # - 'merge'    - if merging
+  # - 'squash'   - if squashing
+  #
   class ReplaceBranch < Base
+    DEFAULT_BRANCH_PATTERN = /\A(\d+)-(\w+).*\z/
+
     def run
-      return :pass if skipped_commit_types.include? commit_message_source
+      return :pass if skip?
 
       Overcommit::Utils.log.debug(
         "Checking if '#{Overcommit::GitRepo.current_branch}' matches #{branch_pattern}"
@@ -17,21 +43,25 @@ def run
       Overcommit::Utils.log.debug("Writing #{commit_message_filename} with #{new_template}")
 
       modify_commit_message do |old_contents|
-        "#{new_template} #{old_contents}"
+        "#{new_template}#{old_contents}"
       end
 
       :pass
     end
 
     def new_template
-      @new_template ||= Overcommit::GitRepo.current_branch.gsub(branch_pattern, replacement_text)
+      @new_template ||=
+        begin
+          curr_branch = Overcommit::GitRepo.current_branch
+          curr_branch.gsub(branch_pattern, replacement_text).strip
+        end
     end
 
     def branch_pattern
       @branch_pattern ||=
         begin
           pattern = config['branch_pattern']
-          Regexp.new((pattern || '').empty? ? '\A.*\w+[-_](\d+).*\z' : pattern)
+          Regexp.new((pattern || '').empty? ? DEFAULT_BRANCH_PATTERN : pattern)
         end
     end
 
@@ -53,5 +83,9 @@ def replacement_text_config
     def skipped_commit_types
       @skipped_commit_types ||= config['skipped_commit_types'].map(&:to_sym)
     end
+
+    def skip?
+      skipped_commit_types.include?(commit_message_source)
+    end
   end
 end

spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb

@@ -4,77 +4,127 @@
 require 'overcommit/hook_context/prepare_commit_msg'
 
 describe Overcommit::Hook::PrepareCommitMsg::ReplaceBranch do
-  let(:config) { Overcommit::ConfigurationLoader.default_configuration }
-  let(:context) do
-    Overcommit::HookContext::PrepareCommitMsg.new(
-      config, [prepare_commit_message_file, 'commit'], StringIO.new
+  def checkout_branch(branch)
+    allow(Overcommit::GitRepo).to receive(:current_branch).and_return(branch)
+  end
+
+  def new_config(opts = {})
+    default = Overcommit::ConfigurationLoader.default_configuration
+
+    return default if opts.empty?
+
+    default.merge(
+      Overcommit::Configuration.new(
+        'PrepareCommitMsg' => {
+          'ReplaceBranch' => opts.merge('enabled' => true)
+        }
+      )
     )
   end
 
-  let(:prepare_commit_message_file) { 'prepare_commit_message_file.txt' }
+  def new_context(config, argv)
+    Overcommit::HookContext::PrepareCommitMsg.new(config, argv, StringIO.new)
+  end
 
-  subject(:hook) { described_class.new(config, context) }
+  def hook_for(config, context)
+    described_class.new(config, context)
+  end
 
-  before do
-    File.open(prepare_commit_message_file, 'w')
-    allow(Overcommit::Utils).to receive_message_chain(:log, :debug)
-    allow(Overcommit::GitRepo).to receive(:current_branch).and_return(new_head)
+  def add_file(name, contents)
+    File.open(name, 'w') { |f| f.puts contents }
   end
 
-  after do
-    File.delete(prepare_commit_message_file) unless ENV['APPVEYOR']
+  def remove_file(name)
+    File.delete(name)
   end
 
-  let(:new_head) { 'userbeforeid-12345-branch-description' }
+  before { allow(Overcommit::Utils).to receive_message_chain(:log, :debug) }
+
+  let(:config)           { new_config }
+  let(:normal_context)   { new_context(config, ['COMMIT_EDITMSG']) }
+  let(:message_context)  { new_context(config, %w[COMMIT_EDITMSG message]) }
+  let(:commit_context)   { new_context(config, %w[COMMIT_EDITMSG commit HEAD]) }
+  let(:merge_context)    { new_context(config, %w[MERGE_MSG merge]) }
+  let(:squash_context)   { new_context(config, %w[SQUASH_MSG squash]) }
+  let(:template_context) { new_context(config, ['template.txt', 'template']) }
+  subject(:hook)         { hook_for(config, normal_context) }
 
   describe '#run' do
+    before { add_file    'COMMIT_EDITMSG', '' }
+    after  { remove_file 'COMMIT_EDITMSG' }
+
     context 'when the checked out branch matches the pattern' do
+      before { checkout_branch '123-topic' }
+      before { hook.run }
+
       it { is_expected.to pass }
 
-      context 'template contents' do
-        subject(:template) { hook.new_template }
+      it 'prepends the replacement text' do
+        expect(File.read('COMMIT_EDITMSG')).to eq("[#123]\n")
+      end
+    end
 
-        before do
-          hook.stub(:replacement_text).and_return('Id is: \1')
-        end
+    context "when the checked out branch doesn't matches the pattern" do
+      before { checkout_branch 'topic-123' }
+      before { hook.run }
 
-        it { is_expected.to eq('Id is: 12345') }
+      context 'with the default `skipped_commit_types`' do
+        it { is_expected.to warn }
       end
-    end
 
-    context 'when the checked out branch does not match the pattern' do
-      let(:new_head) { "this shouldn't match the default pattern" }
+      context 'when merging, and `skipped_commit_types` includes `merge`' do
+        let(:config)   { new_config('skipped_commit_types' => ['merge']) }
+        subject(:hook) { hook_for(config, merge_context) }
 
-      context 'when the commit type is in `skipped_commit_types`' do
-        let(:context) do
-          Overcommit::HookContext::PrepareCommitMsg.new(
-            config, [prepare_commit_message_file, 'template'], StringIO.new
-          )
-        end
+        it { is_expected.to pass }
+      end
+
+      context 'when merging, and `skipped_commit_types` includes `template`' do
+        let(:config)   { new_config('skipped_commit_types' => ['template']) }
+        subject(:hook) { hook_for(config, template_context) }
 
         it { is_expected.to pass }
       end
 
-      context 'when the commit type is not in `skipped_commit_types`' do
-        it { is_expected.to warn }
+      context 'when merging, and `skipped_commit_types` includes `message`' do
+        let(:config)   { new_config('skipped_commit_types' => ['message']) }
+        subject(:hook) { hook_for(config, message_context) }
+
+        it { is_expected.to pass }
       end
-    end
-  end
 
-  describe '#replacement_text' do
-    subject(:replacement_text) { hook.replacement_text }
-    let(:replacement_template_file) { 'valid_filename.txt' }
-    let(:replacement) { 'Id is: \1' }
+      context 'when merging, and `skipped_commit_types` includes `commit`' do
+        let(:config)   { new_config('skipped_commit_types' => ['commit']) }
+        subject(:hook) { hook_for(config, commit_context) }
 
-    context 'when the replacement text points to a valid filename' do
-      before do
-        hook.stub(:replacement_text_config).and_return(replacement_template_file)
-        File.stub(:exist?).and_return(true)
-        File.stub(:read).with(replacement_template_file).and_return(replacement)
+        it { is_expected.to pass }
+      end
+
+      context 'when merging, and `skipped_commit_types` includes `squash`' do
+        let(:config)   { new_config('skipped_commit_types' => ['squash']) }
+        subject(:hook) { hook_for(config, squash_context) }
+
+        it { is_expected.to pass }
       end
+    end
+
+    context 'when the replacement text points to a valid filename' do
+      before { checkout_branch '123-topic' }
+      before { add_file    'replacement_text.txt', 'FOO' }
+      after  { remove_file 'replacement_text.txt' }
+
+      let(:config) { new_config('replacement_text' => 'replacement_text.txt') }
+      let(:normal_context) { new_context(config, ['COMMIT_EDITMSG']) }
+      subject(:hook)       { hook_for(config, normal_context) }
+
+      before { hook.run }
+
+      it { is_expected.to pass }
+
+      let(:commit_msg) { File.read('COMMIT_EDITMSG') }
 
-      describe 'it reads it as the replacement template' do
-        it { is_expected.to eq(replacement) }
+      it 'uses the file contents as the replacement text' do
+        expect(commit_msg).to eq(File.read('replacement_text.txt'))
       end
     end
   end