diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5a212e9a1520da82a3bf6aadd4162345a891e51b..8abffa4f2bd1c33f12fbdcfede7f38c3d610c96d 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -451,15 +451,16 @@ def merge! return :failed end + squashing = params.fetch(:squash, false) merge_service = ::MergeRequests::MergeService.new(project: @project, current_user: current_user, params: merge_params) - unless merge_service.hooks_validation_pass?(@merge_request) + unless merge_service.hooks_validation_pass?(@merge_request, validate_squash_message: squashing) return :hook_validation_error end return :sha_mismatch if params[:sha] != @merge_request.diff_head_sha - @merge_request.update(merge_error: nil, squash: params.fetch(:squash, false)) + @merge_request.update(merge_error: nil, squash: squashing) if auto_merge_requested? if merge_request.auto_merge_enabled? diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index 3e630d40b3d779a902fed7645b114193d3cf6e54..2a3c1e8bc266d42366b87cd27742fe001e1f9774 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -9,12 +9,12 @@ class MergeBaseService < MergeRequests::BaseService attr_reader :merge_request # Overridden in EE. - def hooks_validation_pass?(_merge_request) + def hooks_validation_pass?(merge_request, validate_squash_message: false) true end # Overridden in EE. - def hooks_validation_error(_merge_request) + def hooks_validation_error(merge_request, validate_squash_message: false) # No-op end diff --git a/ee/app/services/ee/merge_requests/merge_base_service.rb b/ee/app/services/ee/merge_requests/merge_base_service.rb index 5195c46b66a5dfa23320e233c5c56e58eb4a4045..96921c5aeffaf032999504050cb811bc6db843fc 100644 --- a/ee/app/services/ee/merge_requests/merge_base_service.rb +++ b/ee/app/services/ee/merge_requests/merge_base_service.rb @@ -13,12 +13,12 @@ def error_check! end override :hooks_validation_pass? - def hooks_validation_pass?(merge_request) + def hooks_validation_pass?(merge_request, validate_squash_message: false) # handle_merge_error needs this. We should move that to a separate # object instead of relying on the order of method calls. @merge_request = merge_request # rubocop:disable Gitlab/ModuleWithInstanceVariables - hooks_error = hooks_validation_error(merge_request) + hooks_error = hooks_validation_error(merge_request, validate_squash_message: validate_squash_message) return true unless hooks_error @@ -31,11 +31,13 @@ def hooks_validation_pass?(merge_request) end override :hooks_validation_error - def hooks_validation_error(merge_request) - return if project.merge_requests_ff_only_enabled - return unless project.feature_available?(:push_rules) + def hooks_validation_error(merge_request, validate_squash_message: false) + if project.merge_requests_ff_only_enabled + return squash_message_validation_error if validate_squash_message + + return + end - push_rule = merge_request.project.push_rule return unless push_rule if !push_rule.commit_message_allowed?(params[:commit_message]) @@ -44,11 +46,29 @@ def hooks_validation_error(merge_request) "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'" elsif !push_rule.author_email_allowed?(current_user.commit_email_or_default) "Author's commit email '#{current_user.commit_email_or_default}' does not follow the pattern '#{push_rule.author_email_regex}'" + elsif validate_squash_message + squash_message_validation_error end end private + def push_rule + strong_memoize(:push_rule) do + merge_request.project.push_rule if project.feature_available?(:push_rules) + end + end + + def squash_message_validation_error + return unless push_rule + + if !push_rule.commit_message_allowed?(params[:squash_commit_message]) + "Squash commit message does not follow the pattern '#{push_rule.commit_message_regex}'" + elsif push_rule.commit_message_blocked?(params[:squash_commit_message]) + "Squash commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'" + end + end + def check_size_limit if size_checker.above_size_limit? raise ::MergeRequests::MergeService::MergeError, size_checker.error_message.merge_error diff --git a/ee/spec/services/merge_requests/merge_service_spec.rb b/ee/spec/services/merge_requests/merge_service_spec.rb index bb8a0705733bbf353023e9c2606ca5dae21717b0..ebdeafd9de083ac2e569e6d6aa65708fa7bc3b4e 100644 --- a/ee/spec/services/merge_requests/merge_service_spec.rb +++ b/ee/spec/services/merge_requests/merge_service_spec.rb @@ -8,7 +8,8 @@ let(:user) { create(:user) } let(:merge_request) { create(:merge_request, :simple) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: user, params: { sha: merge_request.diff_head_sha, commit_message: 'Awesome message' }) } + let(:service) { described_class.new(project: project, current_user: user, params: params) } + let(:params) { { sha: merge_request.diff_head_sha, commit_message: 'Awesome message' } } before do project.add_maintainer(user) diff --git a/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb b/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb index 90c2846de2d0bce6c48642747c816a56263c4ecc..d142a9d6f28d67994d8cc00a1738af22e70f7798 100644 --- a/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -6,7 +6,8 @@ let(:user) { create(:user) } let(:merge_request) { create(:merge_request, :simple) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: user, params: { commit_message: 'Awesome message' }) } + let(:service) { described_class.new(project: project, current_user: user, params: params) } + let(:params) { { commit_message: 'Awesome message' } } before do project.add_maintainer(user) diff --git a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb index 65a3f666468d07d3879025cfa4ef099aae1a4c4e..8fd331ec83f760410e0263da25159f987c21ee1f 100644 --- a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true RSpec.shared_examples 'merge validation hooks' do |args| - def hooks_error - service.hooks_validation_error(merge_request) + def hooks_error(squashing: false) + service.hooks_validation_error(merge_request, validate_squash_message: squashing) end - def hooks_pass? - service.hooks_validation_pass?(merge_request) + def hooks_pass?(squashing: false) + service.hooks_validation_pass?(merge_request, validate_squash_message: squashing) end shared_examples 'hook validations are skipped when push rules unlicensed' do @@ -95,6 +95,57 @@ def hooks_pass? expect(hooks_error).to be_nil end end + + shared_examples 'squashing commits' do + let(:squash_commit_message) { 'Squashed messages' } + let(:params) { super().merge(squash_commit_message: squash_commit_message) } + + context 'and the project has a push rule for required characters' do + before do + allow(project).to receive(:push_rule) { build(:push_rule, commit_message_regex: params[:commit_message]) } + end + + it 'returns false and saves error when invalid' do + expect(hooks_pass?(squashing: true)).to be(false) + expect(hooks_error(squashing: true)).not_to be_empty + + if args[:persisted] + expect(merge_request.merge_error).to eq(hooks_error(squashing: true)) + else + expect(merge_request.merge_error).to be_nil + end + end + end + + context 'and the project has a push rule for forbidden characters' do + before do + allow(project).to receive(:push_rule) do + build(:push_rule, commit_message_negative_regex: squash_commit_message) + end + end + + it 'returns false and saves error when invalid' do + expect(hooks_pass?(squashing: true)).to be(false) + expect(hooks_error(squashing: true)).not_to be_empty + + if args[:persisted] + expect(merge_request.merge_error).to eq(hooks_error(squashing: true)) + else + expect(merge_request.merge_error).to be_nil + end + end + end + end + + it_behaves_like 'squashing commits' + + context 'when the project uses the fast-forward merge method' do + before do + allow(project).to receive(:merge_requests_ff_only_enabled) { true } + end + + it_behaves_like 'squashing commits' + end end RSpec.shared_examples 'service with multiple reviewers' do