diff options
Diffstat (limited to 'app/services/merge_requests')
| -rw-r--r-- | app/services/merge_requests/approval_service.rb | 18 | ||||
| -rw-r--r-- | app/services/merge_requests/base_service.rb | 18 | ||||
| -rw-r--r-- | app/services/merge_requests/build_service.rb | 8 | ||||
| -rw-r--r-- | app/services/merge_requests/create_ref_service.rb | 94 | ||||
| -rw-r--r-- | app/services/merge_requests/ff_merge_service.rb | 31 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_base_service.rb | 36 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_service.rb | 97 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_strategies/from_source_branch.rb | 4 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_to_ref_service.rb | 8 | ||||
| -rw-r--r-- | app/services/merge_requests/refresh_service.rb | 4 | ||||
| -rw-r--r-- | app/services/merge_requests/update_service.rb | 2 |
11 files changed, 100 insertions, 220 deletions
diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 8560a15b7c4..dbe5567cbc5 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -5,12 +5,17 @@ module MergeRequests def execute(merge_request) return unless eligible_for_approval?(merge_request) - approval = merge_request.approvals.new(user: current_user) + approval = merge_request.approvals.new( + user: current_user, + patch_id_sha: fetch_patch_id_sha(merge_request) + ) return success unless save_approval(approval) reset_approvals_cache(merge_request) + merge_request_activity_counter.track_approve_mr_action(user: current_user, merge_request: merge_request) + trigger_merge_request_merge_status_updated(merge_request) trigger_merge_request_reviewers_updated(merge_request) trigger_merge_request_approval_state_updated(merge_request) @@ -31,6 +36,17 @@ module MergeRequests private + def fetch_patch_id_sha(merge_request) + diff_refs = merge_request.diff_refs + base_sha = diff_refs&.base_sha + head_sha = diff_refs&.head_sha + + return unless base_sha && head_sha + return if base_sha == head_sha + + merge_request.project.repository.get_patch_id(base_sha, head_sha) + end + def eligible_for_approval?(merge_request) merge_request.eligible_for_approval_by?(current_user) end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 0fc85675e49..f36cad7139a 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -36,10 +36,7 @@ module MergeRequests merge_request.project.execute_integrations(merge_data, :merge_request_hooks) execute_external_hooks(merge_request, merge_data) - - if action == 'open' && Feature.enabled?(:group_mentions, merge_request.project) - execute_group_mention_hooks(merge_request, merge_data) - end + execute_group_mention_hooks(merge_request, merge_data) if action == 'open' enqueue_jira_connect_messages_for(merge_request) end @@ -113,7 +110,7 @@ module MergeRequests # Don't try to print expensive instance variables. def inspect - return "#<#{self.class}>" unless respond_to?(:merge_request) + return "#<#{self.class}>" unless respond_to?(:merge_request) && merge_request "#<#{self.class} #{merge_request.to_reference(full: true)}>" end @@ -176,21 +173,10 @@ module MergeRequests params.delete(:allow_collaboration) end - filter_locked_labels(merge_request) filter_reviewer(merge_request) filter_suggested_reviewers end - # Filter out any locked labels that are requested to be removed. - # Only supported for merged MRs. - def filter_locked_labels(merge_request) - return unless params[:remove_label_ids].present? - return unless merge_request.merged? - return unless Feature.enabled?(:enforce_locked_labels_on_merge, merge_request.project, type: :ops) - - params[:remove_label_ids] -= labels_service.filter_locked_labels_ids_in_param(:remove_label_ids) - end - def filter_reviewer(merge_request) return if params[:reviewer_ids].blank? diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index b8853e8bcbc..bb347096274 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -74,7 +74,7 @@ module MergeRequests # IssuableBaseService#process_label_ids and # IssuableBaseService#process_assignee_ids take care # of the removal. - params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a) + params[:label_ids] = process_label_ids(params, issuable: merge_request, extra_label_ids: merge_request.label_ids.to_a) params[:assignee_ids] = process_assignee_ids(params, extra_assignee_ids: merge_request.assignee_ids.to_a) @@ -130,10 +130,14 @@ module MergeRequests if source_branch_default? && !target_branch_specified? merge_request.target_branch = nil else - merge_request.target_branch ||= target_project.default_branch + merge_request.target_branch ||= get_target_branch end end + def get_target_branch + target_project.default_branch + end + def source_branch_specified? params[:source_branch].present? end diff --git a/app/services/merge_requests/create_ref_service.rb b/app/services/merge_requests/create_ref_service.rb index e0f10183bac..eae6845335a 100644 --- a/app/services/merge_requests/create_ref_service.rb +++ b/app/services/merge_requests/create_ref_service.rb @@ -9,101 +9,117 @@ module MergeRequests CreateRefError = Class.new(StandardError) def initialize( - current_user:, merge_request:, target_ref:, first_parent_ref:, - source_sha: nil, merge_commit_message: nil) - + current_user:, merge_request:, target_ref:, first_parent_ref:, source_sha: nil + ) @current_user = current_user @merge_request = merge_request - @initial_source_sha = source_sha + @source_sha = source_sha @target_ref = target_ref - @merge_commit_message = merge_commit_message + @first_parent_ref = first_parent_ref @first_parent_sha = target_project.commit(first_parent_ref)&.sha end def execute - commit_sha = initial_source_sha # the SHA to be at HEAD of target_ref - source_sha = initial_source_sha # the SHA to be the merged result of the source (minus the merge commit) - expected_old_oid = "" # the SHA we expect target_ref to be at prior to an update (an optimistic lock) - # TODO: Update this message with the removal of FF merge_trains_create_ref_service and update tests # This is for compatibility with MergeToRefService during the rollout. return ServiceResponse.error(message: '3:Invalid merge source') unless first_parent_sha.present? - commit_sha, source_sha, expected_old_oid = maybe_squash!(commit_sha, source_sha, expected_old_oid) - commit_sha, source_sha, expected_old_oid = maybe_rebase!(commit_sha, source_sha, expected_old_oid) - commit_sha, source_sha = maybe_merge!(commit_sha, source_sha, expected_old_oid) - - ServiceResponse.success( - payload: { - commit_sha: commit_sha, - target_sha: first_parent_sha, - source_sha: source_sha - } - ) + result = { + commit_sha: source_sha, # the SHA to be at HEAD of target_ref + expected_old_oid: "", # the SHA we expect target_ref to be at prior to an update (an optimistic lock) + source_sha: source_sha, # for pipeline.source_sha + target_sha: first_parent_sha # for pipeline.target_sha + } + + result = maybe_squash!(**result) + result = maybe_rebase!(**result) + result = maybe_merge!(**result) + + update_merge_request!(merge_request, result) + + ServiceResponse.success(payload: result) rescue CreateRefError => error ServiceResponse.error(message: error.message) end private - attr_reader :current_user, :merge_request, :target_ref, :first_parent_sha, :initial_source_sha + attr_reader :current_user, :merge_request, :target_ref, :first_parent_ref, :first_parent_sha, :source_sha delegate :target_project, to: :merge_request delegate :repository, to: :target_project - def maybe_squash!(commit_sha, source_sha, expected_old_oid) + def maybe_squash!(commit_sha:, **rest) if merge_request.squash_on_merge? squash_result = MergeRequests::SquashService.new( merge_request: merge_request, current_user: current_user, commit_message: squash_commit_message ).execute + raise CreateRefError, squash_result[:message] if squash_result[:status] == :error commit_sha = squash_result[:squash_sha] - source_sha = commit_sha + squash_commit_sha = commit_sha end # squash does not overwrite target_ref, so expected_old_oid remains the same - [commit_sha, source_sha, expected_old_oid] + rest.merge( + commit_sha: commit_sha, + squash_commit_sha: squash_commit_sha + ).compact end - def maybe_rebase!(commit_sha, source_sha, expected_old_oid) + def maybe_rebase!(commit_sha:, expected_old_oid:, squash_commit_sha: nil, **rest) if target_project.ff_merge_must_be_possible? commit_sha = safe_gitaly_operation do repository.rebase_to_ref( current_user, - source_sha: source_sha, + source_sha: commit_sha, target_ref: target_ref, - first_parent_ref: first_parent_sha + first_parent_ref: first_parent_sha, + expected_old_oid: expected_old_oid || "" ) end - source_sha = commit_sha + squash_commit_sha = commit_sha if squash_commit_sha # rebase rewrites commit SHAs after first_parent_sha expected_old_oid = commit_sha end - [commit_sha, source_sha, expected_old_oid] + rest.merge( + commit_sha: commit_sha, + squash_commit_sha: squash_commit_sha, + expected_old_oid: expected_old_oid + ).compact end - def maybe_merge!(commit_sha, source_sha, expected_old_oid) + def maybe_merge!(commit_sha:, expected_old_oid:, **rest) unless target_project.merge_requests_ff_only_enabled commit_sha = safe_gitaly_operation do repository.merge_to_ref( current_user, - source_sha: source_sha, + source_sha: commit_sha, target_ref: target_ref, message: merge_commit_message, first_parent_ref: first_parent_sha, branch: nil, - expected_old_oid: expected_old_oid + expected_old_oid: expected_old_oid || "" ) end - commit = target_project.commit(commit_sha) - _, source_sha = commit.parent_ids + + expected_old_oid = commit_sha + merge_commit_sha = commit_sha end - [commit_sha, source_sha] + rest.merge( + commit_sha: commit_sha, + merge_commit_sha: merge_commit_sha, + expected_old_oid: expected_old_oid + ).compact + end + + def update_merge_request!(merge_request, result) + # overridden in EE end def safe_gitaly_operation @@ -119,12 +135,10 @@ module MergeRequests strong_memoize_attr :squash_commit_message def merge_commit_message - return @merge_commit_message if @merge_commit_message.present? - - @merge_commit_message = ( - merge_request.merge_params['commit_message'].presence || + merge_request.merge_params['commit_message'].presence || merge_request.default_merge_commit_message(user: current_user) - ) end end end + +MergeRequests::CreateRefService.prepend_mod_with('MergeRequests::CreateRefService') diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb deleted file mode 100644 index 1a83bbf9de6..00000000000 --- a/app/services/merge_requests/ff_merge_service.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module MergeRequests - # MergeService class - # - # Do git fast-forward merge and in case of success - # mark merge request as merged and execute all hooks and notifications - # Executed when you do fast-forward merge via GitLab UI - # - class FfMergeService < MergeRequests::MergeService - extend ::Gitlab::Utils::Override - - private - - override :execute_git_merge - def execute_git_merge - repository.ff_merge( - current_user, - source, - merge_request.target_branch, - merge_request: merge_request - ) - end - - override :merge_success_data - def merge_success_data(commit_id) - # There is no merge commit to update, so this is just blank. - {} - end - end -end diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index fa0a4f808e2..0c8795cfd61 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -18,24 +18,8 @@ module MergeRequests # No-op end - def source - strong_memoize(:source) do - if merge_request.squash_on_merge? - squash_sha! - else - merge_request.diff_head_sha - end - end - end - private - def check_source - unless source - raise_error('No source for merge') - end - end - # Overridden in EE. def check_size_limit # No-op @@ -53,26 +37,6 @@ module MergeRequests def handle_merge_error(*args) # No-op end - - def commit_message - params[:commit_message] || - merge_request.default_merge_commit_message(user: current_user) - end - - def squash_sha! - squash_result = ::MergeRequests::SquashService.new( - merge_request: merge_request, - current_user: current_user, - commit_message: params[:squash_commit_message] - ).execute - - case squash_result[:status] - when :success - squash_result[:squash_sha] - when :error - raise ::MergeRequests::MergeService::MergeError, squash_result[:message] - end - end end end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 1398a6dbb67..29aba3c8679 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -16,38 +16,6 @@ module MergeRequests delegate :merge_jid, :state, to: :@merge_request def execute(merge_request, options = {}) - return execute_v2(merge_request, options) if Feature.enabled?(:refactor_merge_service, project) - - if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService) - FfMergeService.new(project: project, current_user: current_user, params: params).execute(merge_request) - return - end - - return if merge_request.merged? - return unless exclusive_lease(merge_request.id).try_obtain - - @merge_request = merge_request - @options = options - jid = merge_jid - - validate! - - merge_request.in_locked_state do - if commit - after_merge - clean_merge_jid - success - end - end - - log_info("Merge process finished on JID #{jid} with state #{state}") - rescue MergeError => e - handle_merge_error(log_message: e.message, save_message_on_model: true) - ensure - exclusive_lease(merge_request.id).cancel - end - - def execute_v2(merge_request, options = {}) return if merge_request.merged? return unless exclusive_lease(merge_request.id).try_obtain @@ -61,7 +29,7 @@ module MergeRequests validate! merge_request.in_locked_state do - if commit_v2 + if commit after_merge clean_merge_jid success @@ -90,28 +58,8 @@ module MergeRequests end end - # Can remove this entire method when :refactor_merge_service is enabled - def error_check! - super - - return if Feature.enabled?(:refactor_merge_service, project) - - check_source - - error = - if @merge_request.should_be_rebased? - 'Only fast-forward merge is allowed for your project. Please update your source branch' - elsif !@merge_request.mergeable?(skip_discussions_check: @options[:skip_discussions_check], check_mergeability_retry_lease: @options[:check_mergeability_retry_lease]) - 'Merge request is not mergeable' - elsif !@merge_request.squash && project.squash_always? - 'This project requires squashing commits when merge requests are accepted.' - end - - raise_error(error) if error - end - def validate_strategy! - @merge_strategy.validate! if Feature.enabled?(:refactor_merge_service, project) + @merge_strategy.validate! end def updated_check! @@ -121,7 +69,7 @@ module MergeRequests end end - def commit_v2 + def commit log_info("Git merge started on JID #{merge_jid}") merge_result = try_merge { @merge_strategy.execute_git_merge! } @@ -131,7 +79,11 @@ module MergeRequests log_info("Git merge finished on JID #{merge_jid} commit #{commit_sha}") - new_merge_request_attributes = merge_result.slice(:merge_commit_sha, :squash_commit_sha) + new_merge_request_attributes = { + merged_commit_sha: commit_sha, + merge_commit_sha: merge_result[:merge_commit_sha], + squash_commit_sha: merge_result[:squash_commit_sha] + }.compact merge_request.update!(new_merge_request_attributes) if new_merge_request_attributes.present? commit_sha @@ -140,35 +92,6 @@ module MergeRequests log_info("Merge request marked in progress") end - def commit - log_info("Git merge started on JID #{merge_jid}") - commit_id = try_merge { execute_git_merge } - - if commit_id - log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}") - else - raise_error(GENERIC_ERROR_MESSAGE) - end - - update_merge_sha_metadata(commit_id) - - commit_id - ensure - merge_request.update_and_mark_in_progress_merge_commit_sha(nil) - log_info("Merge request marked in progress") - end - - def update_merge_sha_metadata(commit_id) - data_to_update = merge_success_data(commit_id) - data_to_update[:squash_commit_sha] = source if merge_request.squash_on_merge? - - merge_request.update!(**data_to_update) if data_to_update.present? - end - - def merge_success_data(commit_id) - { merge_commit_sha: commit_id } - end - def try_merge yield rescue Gitlab::Git::PreReceiveError => e @@ -178,10 +101,6 @@ module MergeRequests raise_error(GENERIC_ERROR_MESSAGE) end - def execute_git_merge - repository.merge(current_user, source, merge_request, commit_message) - end - def after_merge log_info("Post merge started on JID #{merge_jid} with state #{state}") MergeRequests::PostMergeService.new(project: project, current_user: current_user).execute(merge_request) diff --git a/app/services/merge_requests/merge_strategies/from_source_branch.rb b/app/services/merge_requests/merge_strategies/from_source_branch.rb index 9fe5fc5160b..fe0e4d8a90c 100644 --- a/app/services/merge_requests/merge_strategies/from_source_branch.rb +++ b/app/services/merge_requests/merge_strategies/from_source_branch.rb @@ -28,7 +28,7 @@ module MergeRequests check_mergeability_retry_lease: @options[:check_mergeability_retry_lease] ) 'Merge request is not mergeable' - elsif !merge_request.squash && project.squash_always? + elsif merge_request.missing_required_squash? 'This project requires squashing commits when merge requests are accepted.' end @@ -110,3 +110,5 @@ module MergeRequests end end end + +::MergeRequests::MergeStrategies::FromSourceBranch.prepend_mod diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb index 8b79feb5e0f..6e1b56d9651 100644 --- a/app/services/merge_requests/merge_to_ref_service.rb +++ b/app/services/merge_requests/merge_to_ref_service.rb @@ -31,14 +31,13 @@ module MergeRequests private - override :source def source merge_request.diff_head_sha end override :error_check! def error_check! - check_source + raise_error('No source for merge') unless source end ## @@ -55,6 +54,11 @@ module MergeRequests params[:first_parent_ref] || merge_request.target_branch_ref end + def commit_message + params[:commit_message] || + merge_request.default_merge_commit_message(user: current_user) + end + def extracted_merge_to_ref repository.merge_to_ref(current_user, source_sha: source, diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 447f4f9428c..7a7d0dbfef2 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -94,7 +94,9 @@ module MergeRequests ) merge_requests.each do |merge_request| - merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha) + sha = analyzer.get_merge_commit(merge_request.diff_head_sha) + merge_request.merge_commit_sha = sha + merge_request.merged_commit_sha = sha MergeRequests::PostMergeService .new(project: merge_request.target_project, current_user: @current_user) diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 598dbaf93a9..c435048e343 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -94,7 +94,7 @@ module MergeRequests end def track_title_and_desc_edits(changed_fields) - tracked_fields = %w(title description) + tracked_fields = %w[title description] return unless changed_fields.any? { |field| tracked_fields.include?(field) } |