Revert "Automatically update MR merge-ref along merge status"

5 files changed, 50 insertions, 109 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 135117926be..a3ceb76845e 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -33,7 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   def show
     close_merge_request_if_no_source_project
-    @merge_request.check_mergeability
+    mark_merge_request_mergeable
 
     respond_to do |format|
       format.html do
@@ -251,6 +251,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @merge_request.has_no_commits? && !@merge_request.target_branch_exists?
   end
 
+  def mark_merge_request_mergeable
+    @merge_request.check_if_can_be_merged
+  end
+
   def merge!
     # Disable the CI check if auto_merge_strategy is specified since we have
     # to wait until CI completes to know
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4fcaac75655..7481f1939ad 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -725,16 +725,19 @@ class MergeRequest < ApplicationRecord
 
     MergeRequests::ReloadDiffsService.new(self, current_user).execute
   end
-
-  def check_mergeability
-    MergeRequests::MergeabilityCheckService.new(self).execute
-  end
   # rubocop: enable CodeReuse/ServiceClass
 
-  # 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)
+  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)
+
+    if can_be_merged
+      mark_as_mergeable
+    else
+      mark_as_unmergeable
+    end
   end
 
   def merge_event
@@ -760,7 +763,7 @@ class MergeRequest < ApplicationRecord
   def mergeable?(skip_ci_check: false)
     return false unless mergeable_state?(skip_ci_check: skip_ci_check)
 
-    check_mergeability
+    check_if_can_be_merged
 
     can_be_merged? && !should_be_rebased?
   end
@@ -775,6 +778,15 @@ 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
@@ -1087,12 +1099,6 @@ 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
-
   def ref_path
     "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
   end
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb
index 8670b9ccf3d..87147d90c32 100644
--- a/app/services/merge_requests/merge_to_ref_service.rb
+++ b/app/services/merge_requests/merge_to_ref_service.rb
@@ -20,14 +20,20 @@ module MergeRequests
 
       raise_error('Conflicts detected during merge') unless commit_id
 
-      success(commit_id: commit_id)
-    rescue MergeError, ArgumentError => error
+      commit = project.commit(commit_id)
+      target_id, source_id = commit.parent_ids
+
+      success(commit_id: commit.id,
+              target_id: target_id,
+              source_id: source_id)
+    rescue MergeError => error
       error(error.message)
     end
 
     private
 
     def validate!
+      authorization_check!
       error_check!
     end
 
@@ -37,13 +43,21 @@ module MergeRequests
       error =
         if !hooks_validation_pass?(merge_request)
           hooks_validation_error(merge_request)
-        elsif source.blank?
+        elsif !@merge_request.mergeable_to_ref?
+          "Merge request is not mergeable to #{target_ref}"
+        elsif !source
           'No source for merge'
         end
 
       raise_error(error) if error
     end
 
+    def authorization_check!
+      unless Ability.allowed?(current_user, :admin_merge_request, project)
+        raise_error("You are not allowed to merge to this ref")
+      end
+    end
+
     def target_ref
       merge_request.merge_ref_path
     end
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
deleted file mode 100644
index ef833774e65..00000000000
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ /dev/null
@@ -1,82 +0,0 @@
-# frozen_string_literal: true
-
-module MergeRequests
-  class MergeabilityCheckService < ::BaseService
-    include Gitlab::Utils::StrongMemoize
-
-    delegate :project, to: :@merge_request
-    delegate :repository, to: :project
-
-    def initialize(merge_request)
-      @merge_request = merge_request
-    end
-
-    # Updates the MR merge_status. Whenever it switches to a can_be_merged state,
-    # the merge-ref is refreshed.
-    #
-    # Returns a ServiceResponse indicating merge_status is/became can_be_merged
-    # and the merge-ref is synced. Success in case of being/becoming mergeable,
-    # error otherwise.
-    def execute
-      return ServiceResponse.error(message: 'Invalid argument') unless merge_request
-      return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
-
-      update_merge_status
-
-      unless merge_request.can_be_merged?
-        return ServiceResponse.error(message: 'Merge request is not mergeable')
-      end
-
-      unless payload.fetch(:merge_ref_head)
-        return ServiceResponse.error(message: 'Merge ref was not found')
-      end
-
-      ServiceResponse.success(payload: payload)
-    end
-
-    private
-
-    attr_reader :merge_request
-
-    def payload
-      strong_memoize(:payload) do
-        {
-          merge_ref_head: merge_ref_head_payload
-        }
-      end
-    end
-
-    def merge_ref_head_payload
-      commit = merge_request.merge_ref_head
-
-      return unless commit
-
-      target_id, source_id = commit.parent_ids
-
-      {
-        commit_id: commit.id,
-        source_id: source_id,
-        target_id: target_id
-      }
-    end
-
-    def update_merge_status
-      return unless merge_request.recheck_merge_status?
-
-      if can_git_merge?
-        merge_to_ref && merge_request.mark_as_mergeable
-      else
-        merge_request.mark_as_unmergeable
-      end
-    end
-
-    def can_git_merge?
-      !merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
-    end
-
-    def merge_to_ref
-      result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
-      result[:status] == :success
-    end
-  end
-end
diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index f3437ba16de..1de30e68d87 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -1,20 +1,19 @@
 # frozen_string_literal: true
 
 class ServiceResponse
-  def self.success(message: nil, payload: {})
-    new(status: :success, message: message, payload: payload)
+  def self.success(message: nil)
+    new(status: :success, message: message)
   end
 
-  def self.error(message:, payload: {}, http_status: nil)
-    new(status: :error, message: message, payload: payload, http_status: http_status)
+  def self.error(message:, http_status: nil)
+    new(status: :error, message: message, http_status: http_status)
   end
 
-  attr_reader :status, :message, :http_status, :payload
+  attr_reader :status, :message, :http_status
 
-  def initialize(status:, message: nil, payload: {}, http_status: nil)
+  def initialize(status:, message: nil, http_status: nil)
     self.status = status
     self.message = message
-    self.payload = payload
     self.http_status = http_status
   end
 
@@ -28,5 +27,5 @@ class ServiceResponse
 
   private
 
-  attr_writer :status, :message, :http_status, :payload
+  attr_writer :status, :message, :http_status
 end