Merge branch 'sync-merge-ref-upon-mergeability-check' into 'master'

Automatically update MR merge-ref along merge status

See merge request gitlab-org/gitlab-ce!29569

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

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

app/services/merge_requests/merge_to_ref_service.rb

@@ -11,6 +11,8 @@ module MergeRequests
   # be executed regardless of the `target_ref` current state).
   #
   class MergeToRefService < MergeRequests::MergeBaseService
+    extend ::Gitlab::Utils::Override
+
     def execute(merge_request)
       @merge_request = merge_request
 
@@ -26,14 +28,18 @@ module MergeRequests
       success(commit_id: commit.id,
               target_id: target_id,
               source_id: source_id)
-    rescue MergeError => error
+    rescue MergeError, ArgumentError => error
       error(error.message)
     end
 
     private
 
+    override :source
+    def source
+      merge_request.diff_head_sha
+    end
+
     def validate!
-      authorization_check!
       error_check!
     end
 
@@ -43,21 +49,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

app/services/merge_requests/mergeability_check_service.rb

+# 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 the current merge_status is
+    # can_be_merged or cannot_be_merged and merge-ref is outdated.
+    # 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
+
+      unless merge_ref_auto_sync_enabled?
+        return ServiceResponse.error(message: 'Merge ref is outdated due to disabled feature')
+      end
+
+      unless payload.fetch(:merge_ref_head)
+        return ServiceResponse.error(message: 'Merge ref cannot be updated')
+      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?
+      return false unless merge_ref_auto_sync_enabled?
+      return false unless merge_request.open?
+
+      return true unless ref_head = merge_request.merge_ref_head
+      return true unless target_sha = merge_request.target_branch_sha
+      return true unless source_sha = merge_request.source_branch_sha
+
+      ref_head.parent_ids != [target_sha, source_sha]
+    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
+      return true unless merge_ref_auto_sync_enabled?
+
+      result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+      result[:status] == :success
+    end
+
+    def merge_ref_auto_sync_enabled?
+      Feature.enabled?(:merge_ref_auto_sync, project, default_enabled: true)
+    end
+  end
+end

app/services/service_response.rb

 # 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

changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml

+---
+title: Sync merge ref upon mergeability check
+merge_request: 29569
+author:
+type: added

db/post_migrate/20190620112608_enqueue_reset_merge_status_second_run.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class EnqueueResetMergeStatusSecondRun < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 10_000
+  MIGRATION = 'ResetMergeStatus'
+  DELAY_INTERVAL = 5.minutes.to_i
+
+  disable_ddl_transaction!
+
+  def up
+    say 'Scheduling `ResetMergeStatus` jobs'
+
+    # We currently have more than ~5_000_000 merge request records on GitLab.com.
+    # This means it'll schedule ~500 jobs (10k MRs each) with a 5 minutes gap,
+    # so this should take ~41 hours for all background migrations to complete.
+    # ((5_000_000 / 10_000) * 5) / 60 => 41.6666..
+    queue_background_migration_jobs_by_range_at_intervals(MergeRequest, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20190619175843) do
+ActiveRecord::Schema.define(version: 20190620112608) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

doc/api/merge_requests.md

@@ -1191,33 +1191,29 @@ Parameters:
 }
 ```
 
-## Merge to default merge ref path
+## Returns the up to date merge-ref HEAD commit
 
 Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge` 
-ref, of the target project repository. This ref will have the state the target branch would have if
+ref, of the target project repository, if possible. This ref will have the state the target branch would have if
 a regular merge action was taken.
 
-This is not a regular merge action given it doesn't change the merge request state in any manner.
+This is not a regular merge action given it doesn't change the merge request target branch state in any manner.
 
-This ref (`refs/merge-requests/:iid/merge`) is **always** overwritten when submitting
-requests to this API, so none of its state is kept or used in the process.
+This ref (`refs/merge-requests/:iid/merge`) isn't necessarily overwritten when submitting
+requests to this API, though it'll make sure the ref has the latest possible state.
 
-If the merge request has conflicts, is empty or already merged,
-you'll get a `400` and a descriptive error message. If you don't have permissions to do so, 
-you'll get a `403`.
+If the merge request has conflicts, is empty or already merged, you'll get a `400` and a descriptive error message.
 
-It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in
-case of `200`.
+It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in case of `200`.
 
 ```
-PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref
+GET /projects/:id/merge_requests/:merge_request_iid/merge_ref
 ```
 
 Parameters:
 
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
 - `merge_request_iid` (required)            - Internal ID of MR
-- `merge_commit_message` (optional)         - Custom merge commit message
 
 ```json
 {

lib/api/entities.rb

@@ -703,7 +703,7 @@ module API
       # See https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 for more
       # information.
       expose :merge_status do |merge_request|
-        merge_request.check_if_can_be_merged
+        merge_request.check_mergeability
         merge_request.merge_status
       end
       expose :diff_head_sha, as: :sha

lib/api/merge_requests.rb

@@ -397,28 +397,16 @@ module API
         present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
       end
 
-      desc 'Merge a merge request to its default temporary merge ref path'
-      params do
-        optional :merge_commit_message, type: String, desc: 'Custom merge commit message'
-      end
-      put ':id/merge_requests/:merge_request_iid/merge_to_ref' do
+      desc 'Returns the up to date merge-ref HEAD commit'
+      get ':id/merge_requests/:merge_request_iid/merge_ref' do
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
-        authorize! :admin_merge_request, user_project
-
-        merge_params = {
-          commit_message: params[:merge_commit_message]
-        }
-
-        result = ::MergeRequests::MergeToRefService
-          .new(merge_request.target_project, current_user, merge_params)
-          .execute(merge_request)
+        result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
 
-        if result[:status] == :success
-          present result.slice(:commit_id), 200
+        if result.success?
+          present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
         else
-          http_status = result[:http_status] || 400
-          render_api_error!(result[:message], http_status)
+          render_api_error!(result.message, 400)
         end
       end
 

spec/migrations/enqueue_reset_merge_status_second_run_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190620112608_enqueue_reset_merge_status_second_run.rb')
+
+describe EnqueueResetMergeStatusSecondRun, :migration, :sidekiq do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
+  let(:merge_requests) { table(:merge_requests) }
+
+  def create_merge_request(id, extra_params = {})
+    params = {
+      id: id,
+      target_project_id: project.id,
+      target_branch: 'master',
+      source_project_id: project.id,
+      source_branch: 'mr name',
+      title: "mr name#{id}"
+    }.merge(extra_params)
+
+    merge_requests.create!(params)
+  end
+
+  it 'correctly schedules background migrations' do
+    create_merge_request(1, state: 'opened', merge_status: 'can_be_merged')
+    create_merge_request(2, state: 'opened', merge_status: 'can_be_merged')
+    create_merge_request(3, state: 'opened', merge_status: 'can_be_merged')
+    create_merge_request(4, state: 'merged', merge_status: 'can_be_merged')
+    create_merge_request(5, state: 'opened', merge_status: 'unchecked')
+
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(5.minutes, 1, 2)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(10.minutes, 3, 4)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(15.minutes, 5, 5)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(3)
+      end
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -1996,57 +1996,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#check_if_can_be_merged' do
-    let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
-
-    shared_examples 'checking if can be merged' do
-      context 'when it is not broken and has no conflicts' do
-        before do
-          allow(subject).to receive(:broken?) { false }
-          allow(project.repository).to receive(:can_be_merged?).and_return(true)
-        end
-
-        it 'is marked as mergeable' do
-          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
-        end
-      end
-
-      context 'when broken' do
-        before do
-          allow(subject).to receive(:broken?) { true }
-          allow(project.repository).to receive(:can_be_merged?).and_return(false)
-        end
-
-        it 'becomes unmergeable' do
-          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
-        end
-      end
-
-      context 'when it has conflicts' do
-        before do
-          allow(subject).to receive(:broken?) { false }
-          allow(project.repository).to receive(:can_be_merged?).and_return(false)
-        end
-
-        it 'becomes unmergeable' do
-          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
-        end
-      end
-    end
-
-    context 'when merge_status is unchecked' do
-      subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
-
-      it_behaves_like 'checking if can be merged'
-    end
-
-    context 'when merge_status is unchecked' do
-      subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) }
-
-      it_behaves_like 'checking if can be merged'
-    end
-  end
-
   describe '#mergeable?' do
     let(:project) { create(:project) }
 
@@ -2060,7 +2009,7 @@ describe MergeRequest do
 
     it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do
       allow(subject).to receive(:mergeable_state?) { true }
-      expect(subject).to receive(:check_if_can_be_merged)
+      expect(subject).to receive(:check_mergeability)
       expect(subject).to receive(:can_be_merged?) { true }
 
       expect(subject.mergeable?).to be_truthy
@@ -2074,7 +2023,7 @@ describe MergeRequest do
 
     it 'checks if merge request can be merged' do
       allow(subject).to receive(:mergeable_ci_state?) { true }
-      expect(subject).to receive(:check_if_can_be_merged)
+      expect(subject).to receive(:check_mergeability)
 
       subject.mergeable?
     end
@@ -3142,38 +3091,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#mergeable_to_ref?' do
-    it 'returns true when merge request is mergeable' do
-      subject = create(:merge_request)
-
-      expect(subject.mergeable_to_ref?).to be(true)
-    end
-
-    it 'returns false when merge request is already merged' do
-      subject = create(:merge_request, :merged)
-
-      expect(subject.mergeable_to_ref?).to be(false)
-    end
-
-    it 'returns false when merge request is closed' do
-      subject = create(:merge_request, :closed)
-
-      expect(subject.mergeable_to_ref?).to be(false)
-    end
-
-    it 'returns false when merge request is work in progress' do
-      subject = create(:merge_request, title: 'WIP: The feature')
-
-      expect(subject.mergeable_to_ref?).to be(false)
-    end
-
-    it 'returns false when merge request has no commits' do
-      subject = create(:merge_request, source_branch: 'empty-branch', target_branch: 'master')
-
-      expect(subject.mergeable_to_ref?).to be(false)
-    end
-  end
-
   describe '#merge_participants' do
     it 'contains author' do
       expect(subject.merge_participants).to eq([subject.author])

spec/requests/api/merge_requests_spec.rb

@@ -1546,52 +1546,80 @@ describe API::MergeRequests do
     end
   end
 
-  describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do
-    let(:pipeline) { create(:ci_pipeline_without_jobs) }
+  describe "GET /projects/:id/merge_requests/:merge_request_iid/merge_ref" do
+    before do
+      merge_request.mark_as_unchecked!
+    end
+
+    let(:merge_request_iid) { merge_request.iid }
+
     let(:url) do
-      "/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref"
+      "/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref"
     end
 
     it 'returns the generated ID from the merge service in case of success' do
-      put api(url, user), params: { merge_commit_message: 'Custom message' }
-
-      commit = project.commit(json_response['commit_id'])
+      get api(url, user)
 
       expect(response).to have_gitlab_http_status(200)
-      expect(json_response['commit_id']).to be_present
-      expect(commit.message).to eq('Custom message')
+      expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
     end
 
-    it "returns 400 if branch can't be merged" do
-      merge_request.update!(state: 'merged')
+    context 'when merge-ref is not synced with merge status' do
+      before do
+        merge_request.update!(merge_status: 'cannot_be_merged')
+      end
 
-      put api(url, user)
+      it 'returns 200 if MR can be merged' do
+        get api(url, user)
 
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message'])
-        .to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
+      end
+
+      it 'returns 400 if MR cannot be merged' do
+        expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_request|
+          expect(merge_request).to receive(:execute) { { status: :failed } }
+        end
+
+        get api(url, user)
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response['message']).to eq('Merge request is not mergeable')
+      end
     end
 
-    it 'returns 403 if user has no permissions to merge to the ref' do
-      user2 = create(:user)
-      project.add_reporter(user2)
+    context 'when user has no access to the MR' do
+      let(:project) { create(:project, :private) }
+      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
 
-      put api(url, user2)
+      it 'returns 404' do
+        project.add_guest(user)
 
-      expect(response).to have_gitlab_http_status(403)
-      expect(json_response['message']).to eq('403 Forbidden')
+        get api(url, user)
+
+        expect(response).to have_gitlab_http_status(404)
+        expect(json_response['message']).to eq('404 Not found')
+      end
     end
 
-    it 'returns 404 for an invalid merge request IID' do
-      put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user)
+    context 'when invalid merge request IID' do
+      let(:merge_request_iid) { '12345' }
 
-      expect(response).to have_gitlab_http_status(404)
+      it 'returns 404' do
+        get api(url, user)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
     end
 
-    it "returns 404 if the merge request id is used instead of iid" do
-      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
+    context 'when merge request ID is used instead IID' do
+      let(:merge_request_iid) { merge_request.id }
 
-      expect(response).to have_gitlab_http_status(404)
+      it 'returns 404' do
+        get api(url, user)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
     end
   end
 

spec/services/merge_requests/merge_to_ref_service_spec.rb

@@ -72,10 +72,6 @@ describe MergeRequests::MergeToRefService do
   let(:merge_request) { create(:merge_request, :simple) }
   let(:project) { merge_request.project }
 
-  before do
-    project.add_maintainer(user)
-  end
-
   describe '#execute' do
     let(:service) do
       described_class.new(project, user, commit_message: 'Awesome message',
@@ -92,6 +88,12 @@ describe MergeRequests::MergeToRefService do
     it_behaves_like 'successfully evaluates pre-condition checks'
 
     context 'commit history comparison with regular MergeService' do
+      before do
+        # The merge service needs an authorized user while merge-to-ref
+        # doesn't.
+        project.add_maintainer(user)
+      end
+
       let(:merge_ref_service) do
         described_class.new(project, user, {})
       end
@@ -104,12 +106,18 @@ describe MergeRequests::MergeToRefService do
         it_behaves_like 'MergeService for target ref'
       end
 
-      context 'when merge commit with squash', :quarantine do
+      context 'when merge commit with squash' do
         before do
-          merge_request.update!(squash: true, source_branch: 'master', target_branch: 'feature')
+          merge_request.update!(squash: true)
         end
 
         it_behaves_like 'MergeService for target ref'
+
+        it 'does not squash before merging' do
+          expect(MergeRequests::SquashService).not_to receive(:new)
+
+          process_merge_to_ref
+        end
       end
     end
 
@@ -136,9 +144,9 @@ describe MergeRequests::MergeToRefService do
         let(:merge_method) { :merge }
 
         it 'returns error' do
-          allow(merge_request).to receive(:mergeable_to_ref?) { false }
+          allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil }
 
-          error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}"
+          error_message = 'Conflicts detected during merge'
 
           result = service.execute(merge_request)
 
@@ -170,28 +178,5 @@ describe MergeRequests::MergeToRefService do
 
       it { expect(todo).not_to be_done }
     end
-
-    context 'when merge request is WIP state' do
-      it 'fails to merge' do
-        merge_request = create(:merge_request, title: 'WIP: The feature')
-
-        result = service.execute(merge_request)
-
-        expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
-      end
-    end
-
-    it 'returns error when user has no authorization to admin the merge request' do
-      unauthorized_user = create(:user)
-      project.add_reporter(unauthorized_user)
-
-      service = described_class.new(project, unauthorized_user)
-
-      result = service.execute(merge_request)
-
-      expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq('You are not allowed to merge to this ref')
-    end
   end
 end

spec/services/merge_requests/mergeability_check_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::MergeabilityCheckService do
+  shared_examples_for 'unmergeable merge request' do
+    it 'updates or keeps merge status as cannot_be_merged' do
+      subject
+
+      expect(merge_request.merge_status).to eq('cannot_be_merged')
+    end
+
+    it 'does not change the merge ref HEAD' do
+      expect { subject }.not_to change(merge_request, :merge_ref_head)
+    end
+
+    it 'returns ServiceResponse.error' do
+      result = subject
+
+      expect(result).to be_a(ServiceResponse)
+      expect(result).to be_error
+    end
+  end
+
+  shared_examples_for 'mergeable merge request' do
+    it 'updates or keeps merge status as can_be_merged' do
+      subject
+
+      expect(merge_request.merge_status).to eq('can_be_merged')
+    end
+
+    it 'updates the merge ref' do
+      expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
+    end
+
+    it 'returns ServiceResponse.success' do
+      result = subject
+
+      expect(result).to be_a(ServiceResponse)
+      expect(result).to be_success
+    end
+
+    it 'ServiceResponse has merge_ref_head payload' do
+      result = subject
+
+      expect(result.payload.keys).to contain_exactly(:merge_ref_head)
+      expect(result.payload[:merge_ref_head].keys)
+        .to contain_exactly(:commit_id, :target_id, :source_id)
+    end
+  end
+
+  describe '#execute' do
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
+    let(:repo) { project.repository }
+
+    subject { described_class.new(merge_request).execute }
+
+    before do
+      project.add_developer(merge_request.author)
+    end
+
+    it_behaves_like 'mergeable merge request'
+
+    context 'when multiple calls to the service' do
+      it 'returns success' do
+        subject
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.success?).to be(true)
+      end
+
+      it 'second call does not change the merge-ref' do
+        expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
+        expect { subject }.not_to change(merge_request, :merge_ref_head)
+      end
+    end
+
+    context 'disabled merge ref sync feature flag' do
+      before do
+        stub_feature_flags(merge_ref_auto_sync: false)
+      end
+
+      it 'returns error and no payload' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge ref is outdated due to disabled feature')
+        expect(result.payload).to be_empty
+      end
+
+      it 'ignores merge-ref and updates merge status' do
+        expect { subject }.to change(merge_request, :merge_status).from('unchecked').to('can_be_merged')
+      end
+    end
+
+    context 'when broken' do
+      before do
+        allow(merge_request).to receive(:broken?) { true }
+        allow(project.repository).to receive(:can_be_merged?) { false }
+      end
+
+      it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
+    end
+
+    context 'when it has conflicts' do
+      before do
+        allow(merge_request).to receive(:broken?) { false }
+        allow(project.repository).to receive(:can_be_merged?) { false }
+      end
+
+      it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
+    end
+
+    context 'when MR cannot be merged and has no merge ref' do
+      before do
+        merge_request.mark_as_unmergeable!
+      end
+
+      it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
+    end
+
+    context 'when MR cannot be merged and has outdated merge ref' do
+      before do
+        MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+        merge_request.mark_as_unmergeable!
+      end
+
+      it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
+    end
+
+    context 'when merge request is not given' do
+      subject { described_class.new(nil).execute }
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.message).to eq('Invalid argument')
+      end
+    end
+
+    context 'when read only DB' do
+      it 'returns ServiceResponse.error' do
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.message).to eq('Unsupported operation')
+      end
+    end
+
+    context 'when fails to update the merge-ref' do
+      before do
+        expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref|
+          expect(merge_to_ref).to receive(:execute).and_return(status: :failed)
+        end
+      end
+
+      it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
+    end
+
+    context 'recheck enforced' do
+      subject { described_class.new(merge_request).execute(recheck: true) }
+
+      context 'when MR is mergeable and merge-ref auto-sync is disabled' do
+        before do
+          stub_feature_flags(merge_ref_auto_sync: false)
+          merge_request.mark_as_mergeable!
+        end
+
+        it 'returns ServiceResponse.error' do
+          result = subject
+
+          expect(result).to be_a(ServiceResponse)
+          expect(result.error?).to be(true)
+          expect(result.message).to eq('Merge ref is outdated due to disabled feature')
+          expect(result.payload).to be_empty
+        end
+
+        it 'merge status is not changed' do
+          subject
+
+          expect(merge_request.merge_status).to eq('can_be_merged')
+        end
+      end
+
+      context 'when MR is marked as mergeable, but repo is not mergeable and MR is not opened' do
+        before do
+          # Making sure that we don't touch the merge-status after
+          # the MR is not opened any longer. Source branch might
+          # have been removed, etc.
+          allow(merge_request).to receive(:broken?) { true }
+          merge_request.mark_as_mergeable!
+          merge_request.close!
+        end
+
+        it 'returns ServiceResponse.error' do
+          result = subject
+
+          expect(result).to be_a(ServiceResponse)
+          expect(result.error?).to be(true)
+          expect(result.message).to eq('Merge ref cannot be updated')
+          expect(result.payload).to be_empty
+        end
+
+        it 'does not change the merge status' do
+          expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
+        end
+      end
+
+      context 'when MR is mergeable but merge-ref does not exists' do
+        before do
+          merge_request.mark_as_mergeable!
+        end
+
+        it_behaves_like 'mergeable merge request'
+      end
+
+      context 'when MR is mergeable but merge-ref is already updated' do
+        before do
+          MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+          merge_request.mark_as_mergeable!
+        end
+
+        it 'returns ServiceResponse.success' do
+          result = subject
+
+          expect(result).to be_a(ServiceResponse)
+          expect(result).to be_success
+          expect(result.payload[:merge_ref_head]).to be_present
+        end
+
+        it 'does not recreate the merge-ref' do
+          expect(MergeRequests::MergeToRefService).not_to receive(:new)
+
+          subject
+        end
+      end
+    end
+  end
+end

spec/services/service_response_spec.rb

@@ -16,6 +16,13 @@ describe ServiceResponse do
       expect(response).to be_success
       expect(response.message).to eq('Good orange')
     end
+
+    it 'creates a successful response with payload' do
+      response = described_class.success(payload: { good: 'orange' })
+
+      expect(response).to be_success
+      expect(response.payload).to eq(good: 'orange')
+    end
   end
 
   describe '.error' do
@@ -33,6 +40,15 @@ describe ServiceResponse do
       expect(response.message).to eq('Bad apple')
       expect(response.http_status).to eq(400)
     end
+
+    it 'creates a failed response with payload' do
+      response = described_class.error(message: 'Bad apple',
+                                       payload: { bad: 'apple' })
+
+      expect(response).to be_error
+      expect(response.message).to eq('Bad apple')
+      expect(response.payload).to eq(bad: 'apple')
+    end
   end
 
   describe '#success?' do