From 6fce0d0eb3a76b693468a2866a4a519bed877692 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Thu, 22 Sep 2022 12:15:35 -0400 Subject: [PATCH] Apply push rules to squash commit messages Update MergeRequests::MergeService to validate squash commit message when squashing commits Contributes to: https://gitlab.com/gitlab-org/gitlab/-/issues/345359 Changelog: fixed EE: true --- .../projects/merge_requests_controller.rb | 5 +- .../merge_requests/merge_base_service.rb | 4 +- .../ee/merge_requests/merge_base_service.rb | 32 ++++++++-- .../merge_requests/merge_service_spec.rb | 3 +- .../merge_to_ref_service_spec.rb | 3 +- .../merge_merge_requests_shared_examples.rb | 59 +++++++++++++++++-- 6 files changed, 90 insertions(+), 16 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5a212e9a1520da..8abffa4f2bd1c3 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 3e630d40b3d779..2a3c1e8bc266d4 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 5195c46b66a5df..96921c5aeffaf0 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 bb8a0705733bbf..ebdeafd9de083a 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 90c2846de2d0bc..d142a9d6f28d67 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 65a3f666468d07..8fd331ec83f760 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 -- GitLab