Update ReplaceBranch hook to avoid running on --amend by default

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