Skip the ReplaceBranch prefix when the message already starts with it

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## main (unreleased)
 
+* Skip prepending in the `ReplaceBranch` prepare-commit-msg hook when the commit message already starts with the replacement text or with an autosquash prefix, and document a ticket ID config (#875).
+
 * Add a configurable `Vale` pre-commit hook to check spelling and style in text and source files (#758).
 
 * Honor Git commit-message cleanup settings when handling comment-prefixed titles (#736).

lib/overcommit/hook/prepare_commit_msg/replace_branch.rb

@@ -17,6 +17,28 @@ module Overcommit::Hook::PrepareCommitMsg
   #
   # Similarly, a replacement text of `[\1][\2]` would result in `[123][topic]`.
   #
+  # The hook does not prepend the text if the commit message already starts
+  # with it, or with an autosquash prefix (`fixup! `, `squash! `, `amend! `).
+  # This matters when you remove `message` or `commit` from
+  # `skipped_commit_types`: `git commit -m '[#123] Fix'`, `--amend`, and
+  # `--fixup` then keep one prefix.
+  #
+  # === Ticket IDs
+  #
+  # This config prepends `[PROJ-123] ` on a branch such as
+  # `task/PROJ-123/fix-login`, also for `-m` and `--amend` commits:
+  #
+  #    PrepareCommitMsg:
+  #      ReplaceBranch:
+  #        enabled: true
+  #        branch_pattern: '\A.*?([A-Z][A-Z0-9]+-\d+).*\z'
+  #        replacement_text: '[\1] '
+  #        skipped_commit_types: ['template', 'merge', 'squash']
+  #        on_warn: pass # do not warn on branches without a ticket ID
+  #
+  # It keeps `template` skipped, because a commit template can start with a
+  # `#` comment line, and the prefix would turn that line into the subject.
+  #
   # == When to run this hook
   #
   # You can configure this to run only for specific types of commits by setting
@@ -31,6 +53,10 @@ module Overcommit::Hook::PrepareCommitMsg
   class ReplaceBranch < Base
     DEFAULT_BRANCH_PATTERN = /\A(\d+)-(\w+).*\z/.freeze
 
+    # `git rebase --autosquash` finds these commits only when the prefix starts
+    # the subject.
+    AUTOSQUASH_PREFIX = /\A(?:fixup|squash|amend)! /.freeze
+
     def run
       return :pass if skip?
 
@@ -40,10 +66,14 @@ def run
 
       return :warn unless branch_pattern.match?(Overcommit::GitRepo.current_branch)
 
-      Overcommit::Utils.log.debug("Writing #{commit_message_filename} with #{new_template}")
-
       modify_commit_message do |old_contents|
-        "#{new_template}#{old_contents}"
+        if keep_message?(old_contents)
+          Overcommit::Utils.log.debug("Keeping #{commit_message_filename} unchanged")
+          old_contents
+        else
+          Overcommit::Utils.log.debug("Writing #{commit_message_filename} with #{new_template}")
+          "#{new_template}#{old_contents}"
+        end
       end
 
       :pass
@@ -57,6 +87,25 @@ def new_template
         end
     end
 
+    # Compare bytes: String methods such as `lstrip` raise on a message that is
+    # not valid in the default encoding, for example an amended Latin-1 commit.
+    def keep_message?(contents)
+      message = contents.b.lstrip
+      message.match?(AUTOSQUASH_PREFIX) || starts_with_new_template?(message)
+    end
+
+    # Strip the template, because users type the prefix with or without the
+    # trailing space of `replacement_text`. Without that space, a template that
+    # ends in a word character, such as `PROJ-12`, must not match `PROJ-123`.
+    # A blank template never matches, so a whitespace-only `replacement_text`
+    # is still prepended as before.
+    def starts_with_new_template?(message)
+      template = new_template.b.strip
+      return false if template.empty? || !message.start_with?(template)
+
+      !(template.match?(/\w\z/) && message[template.size]&.match?(/\w/))
+    end
+
     def branch_pattern
       @branch_pattern ||=
         begin

spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb

@@ -140,6 +140,181 @@ def remove_file(name)
       end
     end
 
+    context 'when the message is given and the branch matches the pattern' do
+      let(:replacement_text) { '[#\1] ' }
+      let(:config) do
+        new_config('replacement_text' => replacement_text, 'skipped_commit_types' => [])
+      end
+      subject(:hook) { hook_for(config, message_context) }
+
+      before do
+        checkout_branch '123-topic'
+        add_file 'COMMIT_EDITMSG', message
+        hook.run
+      end
+
+      context 'when the message does not start with the replacement text' do
+        let(:message) { 'Fix login' }
+
+        it 'prepends the replacement text' do
+          expect(File.read('COMMIT_EDITMSG')).to eq("[#123] Fix login\n")
+        end
+      end
+
+      context 'when the message already starts with the replacement text' do
+        let(:message) { '[#123] Fix login' }
+
+        it 'does not prepend the replacement text again' do
+          expect(File.read('COMMIT_EDITMSG')).to eq("[#123] Fix login\n")
+        end
+      end
+
+      context 'when the message starts with the replacement text without its trailing space' do
+        let(:message) { '[#123]Fix login' }
+
+        it 'does not prepend the replacement text again' do
+          expect(File.read('COMMIT_EDITMSG')).to eq("[#123]Fix login\n")
+        end
+      end
+
+      context 'when the message starts with the replacement text after blank lines' do
+        let(:message) { "\n\n[#123] Fix login" }
+
+        it 'does not prepend the replacement text again' do
+          expect(File.read('COMMIT_EDITMSG')).to eq("\n\n[#123] Fix login\n")
+        end
+      end
+
+      context 'when the message contains the replacement text after its start' do
+        let(:message) { 'Revert "[#123] Fix login"' }
+
+        it 'prepends the replacement text' do
+          expect(File.read('COMMIT_EDITMSG')).to eq(%{[#123] Revert "[#123] Fix login"\n})
+        end
+      end
+
+      %w[fixup squash amend].each do |autosquash_prefix|
+        context "when the message starts with `#{autosquash_prefix}! `" do
+          let(:message) { "#{autosquash_prefix}! [#123] Fix login" }
+
+          it 'keeps the autosquash prefix at the start' do
+            expect(File.read('COMMIT_EDITMSG')).to eq("#{message}\n")
+          end
+        end
+      end
+
+      context 'when the replacement text ends in a word character' do
+        let(:replacement_text) { '#\1 ' }
+
+        context 'when the message starts with the same ID' do
+          let(:message) { '#123 Fix login' }
+
+          it 'does not prepend the replacement text again' do
+            expect(File.read('COMMIT_EDITMSG')).to eq("#123 Fix login\n")
+          end
+        end
+
+        context 'when the message starts with a longer ID' do
+          let(:message) { '#1234 Fix login' }
+
+          it 'prepends the replacement text' do
+            expect(File.read('COMMIT_EDITMSG')).to eq("#123 #1234 Fix login\n")
+          end
+        end
+      end
+
+      context 'when the replacement text is only whitespace' do
+        let(:replacement_text) { ' ' }
+        let(:message) { 'Fix login' }
+
+        it 'prepends the replacement text' do
+          expect(File.read('COMMIT_EDITMSG')).to eq(" Fix login\n")
+        end
+      end
+    end
+
+    context 'when the message is not valid UTF-8' do
+      let(:config) do
+        new_config('replacement_text' => '[#\1] ', 'skipped_commit_types' => [])
+      end
+      subject(:hook) { hook_for(config, message_context) }
+
+      before do
+        checkout_branch '123-topic'
+        File.binwrite('COMMIT_EDITMSG', "\xC4nderung".b)
+
+        # The locale sets the encoding that `File.read` returns. Use UTF-8, the
+        # usual locale, because `lstrip` raises on invalid UTF-8 but not on
+        # invalid US-ASCII.
+        allow(File).to receive(:read).and_call_original
+        allow(File).to receive(:read).with('COMMIT_EDITMSG') do |name|
+          File.binread(name).force_encoding(Encoding::UTF_8)
+        end
+      end
+
+      it 'prepends the replacement text' do
+        expect(hook.run).to eq(:pass)
+        expect(File.binread('COMMIT_EDITMSG')).to eq("[#123] \xC4nderung".b)
+      end
+    end
+
+    context 'with the ticket ID config from the hook documentation' do
+      let(:config) do
+        new_config(
+          'branch_pattern' => '\A.*?([A-Z][A-Z0-9]+-\d+).*\z',
+          'replacement_text' => '[\1] ',
+          'skipped_commit_types' => %w[template merge squash],
+          'on_warn' => 'pass',
+        )
+      end
+      let(:branch) { 'task/PROJ-123/fix-login' }
+      let(:message) { 'Fix login' }
+      let(:hook_context) { message_context }
+      subject(:hook) { hook_for(config, hook_context) }
+
+      before do
+        checkout_branch branch
+        add_file 'COMMIT_EDITMSG', message
+      end
+
+      context 'when the message is given on a branch with a ticket ID' do
+        it 'prepends the ticket ID' do
+          expect(hook.run_and_transform).to eq([:pass, nil])
+          expect(File.read('COMMIT_EDITMSG')).to eq("[PROJ-123] Fix login\n")
+        end
+      end
+
+      context 'when amending a commit that already has the ticket ID' do
+        let(:message) { '[PROJ-123] Fix login' }
+        let(:hook_context) { commit_context }
+
+        it 'keeps one ticket ID' do
+          expect(hook.run_and_transform).to eq([:pass, nil])
+          expect(File.read('COMMIT_EDITMSG')).to eq("[PROJ-123] Fix login\n")
+        end
+      end
+
+      %w[template merge squash].each do |commit_type|
+        context "when the commit type is `#{commit_type}`" do
+          let(:hook_context) { public_send("#{commit_type}_context") }
+
+          it 'does not change the commit message' do
+            expect(hook.run_and_transform).to eq([:pass, nil])
+            expect(File.read('COMMIT_EDITMSG')).to eq("Fix login\n")
+          end
+        end
+      end
+
+      context 'when the branch has no ticket ID' do
+        let(:branch) { 'main' }
+
+        it 'passes without a warning and does not change the commit message' do
+          expect(hook.run_and_transform).to eq([:pass, nil])
+          expect(File.read('COMMIT_EDITMSG')).to eq("Fix login\n")
+        end
+      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' }