From 6af9c8e069a83c96e410f989f91eb17b73d3348d Mon Sep 17 00:00:00 2001 From: Nikita Date: Tue, 10 Nov 2026 15:13:39 +0300 Subject: [PATCH] Skip the ReplaceBranch prefix when the message already starts with it --- CHANGELOG.md | 2 + .../hook/prepare_commit_msg/replace_branch.rb | 55 +++++- .../prepare_commit_msg/replace_branch_spec.rb | 175 ++++++++++++++++++ 3 files changed, 229 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db3837b3..32be533a 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb index 8852b0b5..dd005a47 100644 --- a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb +++ b/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 diff --git a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb b/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb index c5902aa9..3e7332c5 100644 --- a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb +++ b/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' }