From 1feffcafe64bd451d6d39f49d145f983432dc647 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 18:46:06 -0500 Subject: [PATCH 1/8] 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. --- config/default.yml | 2 +- lib/overcommit/hook/prepare_commit_msg/replace_branch.rb | 4 +++- .../overcommit/hook/prepare_commit_msg/replace_branch_spec.rb | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/config/default.yml b/config/default.yml index 55fa74f6..c8a7ee35 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1237,7 +1237,7 @@ 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'] on_fail: warn diff --git a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb index c64f4c22..9f87b4fe 100644 --- a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb +++ b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb @@ -5,6 +5,8 @@ module Overcommit::Hook::PrepareCommitMsg # It's possible to reference parts of the branch name through the captures in # the `branch_pattern` regex. class ReplaceBranch < Base + DEFAULT_BRANCH_PATTERN = /\A(\d+)-(\w+).*\z/ + def run return :pass if skipped_commit_types.include? commit_message_source @@ -31,7 +33,7 @@ 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 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 71eb7c13..b619e3d6 100644 --- a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb +++ b/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb @@ -25,7 +25,7 @@ File.delete(prepare_commit_message_file) unless ENV['APPVEYOR'] end - let(:new_head) { 'userbeforeid-12345-branch-description' } + let(:new_head) { '123-topic' } describe '#run' do context 'when the checked out branch matches the pattern' do @@ -38,7 +38,7 @@ hook.stub(:replacement_text).and_return('Id is: \1') end - it { is_expected.to eq('Id is: 12345') } + it { is_expected.to eq('Id is: 123') } end end From c40c74654baf0dd5b6c1233247457aaac3dc3120 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 18:56:24 -0500 Subject: [PATCH 2/8] 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. --- .../hook/prepare_commit_msg/replace_branch_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 b619e3d6..699827be 100644 --- a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb +++ b/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb @@ -35,10 +35,10 @@ subject(:template) { hook.new_template } before do - hook.stub(:replacement_text).and_return('Id is: \1') + hook.stub(:replacement_text).and_return('[#\1]') end - it { is_expected.to eq('Id is: 123') } + it { is_expected.to eq('[#123]') } end end @@ -64,7 +64,7 @@ describe '#replacement_text' do subject(:replacement_text) { hook.replacement_text } let(:replacement_template_file) { 'valid_filename.txt' } - let(:replacement) { 'Id is: \1' } + let(:replacement) { '[#\1]' } context 'when the replacement text points to a valid filename' do before do From 656617ae7897e1f83b75539efd57bbb2e8e05970 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 18:59:43 -0500 Subject: [PATCH 3/8] improve class comment describing ReplaceBranch usage This makes the usage of this (hopefully) obvious to anyone reading it. --- .../hook/prepare_commit_msg/replace_branch.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb index 9f87b4fe..bd2edf7c 100644 --- a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb +++ b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb @@ -2,8 +2,32 @@ 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/ From 579d8a7a918d3fa181f4e8b1c360606c1efd8c42 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 19:36:59 -0500 Subject: [PATCH 4/8] 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. --- config/default.yml | 7 ++++++- .../hook/prepare_commit_msg/replace_branch_spec.rb | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/config/default.yml b/config/default.yml index c8a7ee35..27794ffe 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1239,7 +1239,12 @@ PrepareCommitMsg: description: 'Prepends the commit message with text based on the branch name' 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 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 699827be..7ad60d61 100644 --- a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb +++ b/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb @@ -7,7 +7,7 @@ let(:config) { Overcommit::ConfigurationLoader.default_configuration } let(:context) do Overcommit::HookContext::PrepareCommitMsg.new( - config, [prepare_commit_message_file, 'commit'], StringIO.new + config, [prepare_commit_message_file], StringIO.new ) end From 0bcc8dd194b39617da09a21cc8298c7b34fa93f8 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 20:44:30 -0500 Subject: [PATCH 5/8] remove extra hard-coded space in ReplaceBranch This removes a trailing space after the repacement text, as well as `#strip`ing the replacement text. --- .../hook/prepare_commit_msg/replace_branch.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb index bd2edf7c..f1dd7b2f 100644 --- a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb +++ b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb @@ -32,7 +32,7 @@ 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}" @@ -43,14 +43,14 @@ 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 ||= Overcommit::GitRepo.current_branch.gsub(branch_pattern, replacement_text).strip end def branch_pattern @@ -79,5 +79,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 From 821641c5be1c6a40a4c452d1e05aa6295cbabc78 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 20:55:11 -0500 Subject: [PATCH 6/8] 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 --- .../prepare_commit_msg/replace_branch_spec.rb | 104 ++++++++++-------- 1 file changed, 56 insertions(+), 48 deletions(-) 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 7ad60d61..070072d8 100644 --- a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb +++ b/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb @@ -4,77 +4,85 @@ 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], 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) { '123-topic' } + before { allow(Overcommit::Utils).to receive_message_chain(:log, :debug) } + + let(:config) { new_config } + let(:normal_context) { new_context(config, ['COMMIT_EDITMSG']) } + subject(:hook) { hook_for(config, normal_context) } describe '#run' do - context 'when the checked out branch matches the pattern' do - it { is_expected.to pass } + before { add_file 'COMMIT_EDITMSG', '' } + after { remove_file 'COMMIT_EDITMSG' } - context 'template contents' do - subject(:template) { hook.new_template } + context 'when the checked out branch matches the pattern' do + before { checkout_branch '123-topic' } + before { hook.run } - before do - hook.stub(:replacement_text).and_return('[#\1]') - end + it { is_expected.to pass } - it { is_expected.to eq('[#123]') } + it 'prepends the replacement text' do + expect(File.read('COMMIT_EDITMSG')).to eq("[#123]\n") 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 the checked out branch doesn't matches the pattern" do + before { checkout_branch 'topic-123' } + before { hook.run } - 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 warn } + end - it { is_expected.to pass } - 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' } - context 'when the commit type is not in `skipped_commit_types`' do - it { is_expected.to warn } - end - end - end + let(:config) { new_config('replacement_text' => 'replacement_text.txt') } + let(:normal_context) { new_context(config, ['COMMIT_EDITMSG']) } + subject(:hook) { hook_for(config, normal_context) } - describe '#replacement_text' do - subject(:replacement_text) { hook.replacement_text } - let(:replacement_template_file) { 'valid_filename.txt' } - let(:replacement) { '[#\1]' } + before { hook.run } - 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) - end + 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 From 3675504df5168f45b81b08dd69ac252c3e5ed394 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Fri, 7 Aug 2020 22:21:41 -0500 Subject: [PATCH 7/8] update ReplaceBranch specs to test all `skipped_commit_types` --- .../prepare_commit_msg/replace_branch_spec.rb | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) 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 070072d8..3844f77a 100644 --- a/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb +++ b/spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb @@ -42,6 +42,11 @@ def remove_file(name) 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 @@ -63,7 +68,44 @@ def remove_file(name) before { checkout_branch 'topic-123' } before { hook.run } - it { is_expected.to warn } + context 'with the default `skipped_commit_types`' do + it { is_expected.to warn } + end + + 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) } + + 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 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 + + 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) } + + 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 From f236b50fc2c92f3585ab052a01c30d92a5272873 Mon Sep 17 00:00:00 2001 From: Mark Delk Date: Tue, 11 Aug 2020 20:48:55 -0500 Subject: [PATCH 8/8] shorten a long line for the linter --- lib/overcommit/hook/prepare_commit_msg/replace_branch.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb index f1dd7b2f..5739d124 100644 --- a/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb +++ b/lib/overcommit/hook/prepare_commit_msg/replace_branch.rb @@ -50,7 +50,11 @@ def run end def new_template - @new_template ||= Overcommit::GitRepo.current_branch.gsub(branch_pattern, replacement_text).strip + @new_template ||= + begin + curr_branch = Overcommit::GitRepo.current_branch + curr_branch.gsub(branch_pattern, replacement_text).strip + end end def branch_pattern