Automatically update MR merge-ref along merge statusosw-sync-merge-ref-upon-mergeability-check

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.

5 files changed, 131 insertions, 50 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 9e7e3ed5afb..58b1f0196fa 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
@@ -251,10 +251,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 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 7481f1939ad..4fcaac75655 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -725,19 +725,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
@@ -763,7 +760,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
@@ -778,15 +775,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
@@ -1099,6 +1087,12 @@ 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 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..505c8594ce0
--- /dev/null
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -0,0 +1,104 @@
+# 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.
+    #
+    # recheck - When given, it'll enforce a merge-ref refresh if it's outdated,
+    # even if the current merge_status is can_be_merged or cannot_be_merged.
+    # Given MergeRequests::RefreshService is called async, it might happen that the target
+    # branch gets updated, but the MergeRequest#merge_status lags behind. So in scenarios
+    # where we need the current state of the merge ref in repository, the `recheck`
+    # argument is required.
+    #
+    # 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(recheck: false)
+      return ServiceResponse.error(message: 'Invalid argument') unless merge_request
+      return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
+
+      recheck! if recheck
+      update_merge_status
+
+      unless merge_request.can_be_merged?
+        return ServiceResponse.error(message: 'Merge request is not mergeable')
+      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 recheck!
+      if !merge_request.recheck_merge_status? && outdated_merge_ref?
+        merge_request.mark_as_unchecked
+      end
+    end
+
+    # Checks if the existing merge-ref is synced with the target branch.
+    #
+    # Returns true if the merge-ref does not exists or is out of sync.
+    def outdated_merge_ref?
+      ref_head = merge_request.merge_ref_head
+
+      return true unless ref_head
+      return true unless target_id = merge_request.target_branch_sha
+
+      ref_head.parent_id != target_id
+    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 1de30e68d87..f3437ba16de 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -1,19 +1,20 @@
 # frozen_string_literal: true
 
 class ServiceResponse
-  def self.success(message: nil)
-    new(status: :success, message: message)
+  def self.success(message: nil, payload: {})
+    new(status: :success, message: message, payload: payload)
   end
 
-  def self.error(message:, http_status: nil)
-    new(status: :error, message: message, http_status: http_status)
+  def self.error(message:, payload: {}, http_status: nil)
+    new(status: :error, message: message, payload: payload, http_status: http_status)
   end
 
-  attr_reader :status, :message, :http_status
+  attr_reader :status, :message, :http_status, :payload
 
-  def initialize(status:, message: nil, http_status: nil)
+  def initialize(status:, message: nil, payload: {}, http_status: nil)
     self.status = status
     self.message = message
+    self.payload = payload
     self.http_status = http_status
   end
 
@@ -27,5 +28,5 @@ class ServiceResponse
 
   private
 
-  attr_writer :status, :message, :http_status
+  attr_writer :status, :message, :http_status, :payload
 end