From d8bddb16624f34600069bb5d3540960b25176381 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 10 Apr 2019 11:39:45 +0800 Subject: Validate MR branch names Prevents refspec as branch name, which would bypass branch protection when used in conjunction with rebase. HEAD seems to be a special case with lots of occurrence, so it is considered valid for now. Another special case is `refs/head/*`, which can be imported. --- app/models/merge_request.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'app/models/merge_request.rb') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 458c57c1dc6..368772a5cf4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -620,6 +620,8 @@ class MergeRequest < ApplicationRecord return end + [:source_branch, :target_branch].each { |attr| validate_branch_name(attr) } + if opened? similar_mrs = target_project .merge_requests @@ -640,6 +642,16 @@ class MergeRequest < ApplicationRecord end end + def validate_branch_name(attr) + return unless changes_include?(attr) + + branch = read_attribute(attr) + + return unless branch + + errors.add(attr) unless Gitlab::GitRefValidator.validate_merge_request_branch(branch) + end + def validate_target_project return true if target_project.merge_requests_enabled? -- cgit v1.2.1 From b965009ddddcd50e76841dbc97d2767292e88a0a Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Tue, 21 May 2019 18:14:22 -0300 Subject: Automatically update MR merge-ref along merge status This couples the code that transitions the `MergeRequest#merge_status` and refs/merge-requests/:iid/merge ref update. In general, instead of directly telling `MergeToRefService` to update the merge ref, we should rely on `MergeabilityCheckService` to keep both the merge status and merge ref synced. Now, if the merge_status is `can_be_merged` it means the merge-ref is also updated to the latest. We've also updated the logic to be more systematic and less user-based. --- app/models/merge_request.rb | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) (limited to 'app/models/merge_request.rb') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 311ba1ce6bd..25ad8f67496 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -712,19 +712,16 @@ class MergeRequest < ApplicationRecord MergeRequests::ReloadDiffsService.new(self, current_user).execute end - # rubocop: enable CodeReuse/ServiceClass - - def check_if_can_be_merged - return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write? - can_be_merged = - !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch) + def check_mergeability + MergeRequests::MergeabilityCheckService.new(self).execute + end + # rubocop: enable CodeReuse/ServiceClass - if can_be_merged - mark_as_mergeable - else - mark_as_unmergeable - end + # Returns boolean indicating the merge_status should be rechecked in order to + # switch to either can_be_merged or cannot_be_merged. + def recheck_merge_status? + self.class.state_machines[:merge_status].check_state?(merge_status) end def merge_event @@ -750,7 +747,7 @@ class MergeRequest < ApplicationRecord def mergeable?(skip_ci_check: false) return false unless mergeable_state?(skip_ci_check: skip_ci_check) - check_if_can_be_merged + check_mergeability can_be_merged? && !should_be_rebased? end @@ -765,15 +762,6 @@ class MergeRequest < ApplicationRecord true end - def mergeable_to_ref? - return false unless mergeable_state?(skip_ci_check: true, skip_discussions_check: true) - - # Given the `merge_ref_path` will have the same - # state the `target_branch` would have. Ideally - # we need to check if it can be merged to it. - project.repository.can_be_merged?(diff_head_sha, target_branch) - end - def ff_merge_possible? project.repository.ancestor?(target_branch_sha, diff_head_sha) end @@ -1090,6 +1078,18 @@ class MergeRequest < ApplicationRecord target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path) end + # Returns the current merge-ref HEAD commit. + # + def merge_ref_head + project.repository.commit(merge_ref_path) + end + + # Returns the updated merge-ref HEAD commit. + # + def merge_ref_head! + merge_ref_head if check_mergeability.success? + end + def ref_path "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head" end -- cgit v1.2.1 From 96db70a4448fd1e736c10100dccf3a803ec553c0 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 30 May 2019 20:25:25 +0000 Subject: Simplify merge_ref_head methods --- app/models/merge_request.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'app/models/merge_request.rb') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 25ad8f67496..38b9b886e5f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1080,16 +1080,14 @@ class MergeRequest < ApplicationRecord # Returns the current merge-ref HEAD commit. # + # Consider calling mergeability_check method _before_ this if you need + # the latest possible version of it (it's already automatically updated + # along the merge_status). + # def merge_ref_head project.repository.commit(merge_ref_path) end - # Returns the updated merge-ref HEAD commit. - # - def merge_ref_head! - merge_ref_head if check_mergeability.success? - end - def ref_path "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head" end -- cgit v1.2.1 From 4246a62118d919e62b94d75eba641ed374c3f241 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 31 May 2019 17:18:27 -0300 Subject: Add payload to the service response This introduces payload to the ServiceResponse with the merge ref HEAD commit data --- app/models/merge_request.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'app/models/merge_request.rb') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 38b9b886e5f..cf63a7e79bd 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1080,10 +1080,6 @@ class MergeRequest < ApplicationRecord # Returns the current merge-ref HEAD commit. # - # Consider calling mergeability_check method _before_ this if you need - # the latest possible version of it (it's already automatically updated - # along the merge_status). - # def merge_ref_head project.repository.commit(merge_ref_path) end -- cgit v1.2.1 From d4b46936633a3b2a0248b4572b4a1dc7b2ba8531 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 22 May 2019 18:45:27 +0700 Subject: Abstract auto merge processes We have one auto merge strategy today - Merge When Pipeline Succeeds. In order to add more strategies for Merge Train feature, we abstract the architecture to be more extensible. Removed arguments Fix spec --- app/models/merge_request.rb | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) (limited to 'app/models/merge_request.rb') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 311ba1ce6bd..2602738901b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -165,7 +165,7 @@ class MergeRequest < ApplicationRecord validates :source_branch, presence: true validates :target_project, presence: true validates :target_branch, presence: true - validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing? + validates :merge_user, presence: true, if: :auto_merge_enabled?, unless: :importing? validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?] validate :validate_fork, unless: :closed_without_fork? validate :validate_target_project, on: :create @@ -196,6 +196,7 @@ class MergeRequest < ApplicationRecord alias_attribute :project, :target_project alias_attribute :project_id, :target_project_id + alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds def self.reference_prefix '!' @@ -391,7 +392,7 @@ class MergeRequest < ApplicationRecord def merge_participants participants = [author] - if merge_when_pipeline_succeeds? && !participants.include?(merge_user) + if auto_merge_enabled? && !participants.include?(merge_user) participants << merge_user end @@ -782,7 +783,7 @@ class MergeRequest < ApplicationRecord project.ff_merge_must_be_possible? && !ff_merge_possible? end - def can_cancel_merge_when_pipeline_succeeds?(current_user) + def can_cancel_auto_merge?(current_user) can_be_merged_by?(current_user) || self.author == current_user end @@ -801,6 +802,16 @@ class MergeRequest < ApplicationRecord Gitlab::Utils.to_boolean(merge_params['force_remove_source_branch']) end + def auto_merge_strategy + return unless auto_merge_enabled? + + merge_params['auto_merge_strategy'] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS + end + + def auto_merge_strategy=(strategy) + merge_params['auto_merge_strategy'] = strategy + end + def remove_source_branch? should_remove_source_branch? || force_remove_source_branch? end @@ -973,15 +984,16 @@ class MergeRequest < ApplicationRecord end end - def reset_merge_when_pipeline_succeeds - return unless merge_when_pipeline_succeeds? + def reset_auto_merge + return unless auto_merge_enabled? - self.merge_when_pipeline_succeeds = false + self.auto_merge_enabled = false self.merge_user = nil if merge_params merge_params.delete('should_remove_source_branch') merge_params.delete('commit_message') merge_params.delete('squash_commit_message') + merge_params.delete('auto_merge_strategy') end self.save -- cgit v1.2.1