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.

4 files changed, 79 insertions, 43 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 8f177895b08..7666ffaa3b5 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
-    mark_merge_request_mergeable
+    @merge_request.check_mergeability
 
     respond_to do |format|
       format.html do
@@ -253,10 +253,6 @@ 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 merge_when_pipeline_succeeds is enabled 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 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
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb
index 87147d90c32..8670b9ccf3d 100644
--- a/app/services/merge_requests/merge_to_ref_service.rb
+++ b/app/services/merge_requests/merge_to_ref_service.rb
@@ -20,20 +20,14 @@ module MergeRequests
 
       raise_error('Conflicts detected during merge') unless commit_id
 
-      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
+      success(commit_id: commit_id)
+    rescue MergeError, ArgumentError => error
       error(error.message)
     end
 
     private
 
     def validate!
-      authorization_check!
       error_check!
     end
 
@@ -43,21 +37,13 @@ module MergeRequests
       error =
         if !hooks_validation_pass?(merge_request)
           hooks_validation_error(merge_request)
-        elsif !@merge_request.mergeable_to_ref?
-          "Merge request is not mergeable to #{target_ref}"
-        elsif !source
+        elsif source.blank?
           '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
new file mode 100644
index 00000000000..d277d38127c
--- /dev/null
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+module MergeRequests
+  class MergeabilityCheckService < ::BaseService
+    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('Invalid argument') unless merge_request
+      return ServiceResponse.error('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
+
+      ServiceResponse.success
+    end
+
+    private
+
+    attr_reader :merge_request
+
+    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