Merge branch 'fix-conflict-resolution-with-corrupt-repos' into 'master'

Fix conflict resolution from corrupted upstream

Closes gitlab-ee#2128

See merge request !11298

15 files changed, 268 insertions, 200 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 207fbad7856..b99ccd453b8 100755
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -155,8 +155,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       format.html { define_discussion_vars }
 
       format.json do
-        if @merge_request.conflicts_can_be_resolved_in_ui?
-          render json: @merge_request.conflicts
+        if @conflicts_list.can_be_resolved_in_ui?
+          render json: @conflicts_list
         elsif @merge_request.can_be_merged?
           render json: {
             message: 'The merge conflicts for this merge request have already been resolved. Please return to the merge request.',
@@ -173,9 +173,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def conflict_for_path
-    return render_404 unless @merge_request.conflicts_can_be_resolved_in_ui?
+    return render_404 unless @conflicts_list.can_be_resolved_in_ui?
 
-    file = @merge_request.conflicts.file_for_path(params[:old_path], params[:new_path])
+    file = @conflicts_list.file_for_path(params[:old_path], params[:new_path])
 
     return render_404 unless file
 
@@ -183,7 +183,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def resolve_conflicts
-    return render_404 unless @merge_request.conflicts_can_be_resolved_in_ui?
+    return render_404 unless @conflicts_list.can_be_resolved_in_ui?
 
     if @merge_request.can_be_merged?
       render status: :bad_request, json: { message: 'The merge conflicts for this merge request have already been resolved.' }
@@ -191,7 +191,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
 
     begin
-      MergeRequests::ResolveService.new(@merge_request.source_project, current_user, params).execute(@merge_request)
+      MergeRequests::Conflicts::ResolveService.
+        new(merge_request).
+        execute(current_user, params)
 
       flash[:notice] = 'All merge conflicts were resolved. The merge request can now be merged.'
 
@@ -459,7 +461,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def authorize_can_resolve_conflicts!
-    return render_404 unless @merge_request.conflicts_can_be_resolved_by?(current_user)
+    @conflicts_list = MergeRequests::Conflicts::ListService.new(@merge_request)
+
+    return render_404 unless @conflicts_list.can_be_resolved_by?(current_user)
   end
 
   def module_enabled
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 9e5db53b15e..d7e7ae7a25f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -891,34 +891,6 @@ class MergeRequest < ActiveRecord::Base
     project.repository.keep_around(self.merge_commit_sha)
   end
 
-  def conflicts
-    @conflicts ||= Gitlab::Conflict::FileCollection.new(self)
-  end
-
-  def conflicts_can_be_resolved_by?(user)
-    return false unless source_project
-
-    access = ::Gitlab::UserAccess.new(user, project: source_project)
-    access.can_push_to_branch?(source_branch)
-  end
-
-  def conflicts_can_be_resolved_in_ui?
-    return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui)
-
-    return @conflicts_can_be_resolved_in_ui = false unless cannot_be_merged?
-    return @conflicts_can_be_resolved_in_ui = false unless has_complete_diff_refs?
-
-    begin
-      # Try to parse each conflict. If the MR's mergeable status hasn't been updated,
-      # ensure that we don't say there are conflicts to resolve when there are no conflict
-      # files.
-      conflicts.files.each(&:lines)
-      @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
-    rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing
-      @conflicts_can_be_resolved_in_ui = false
-    end
-  end
-
   def has_commits?
     merge_request_diff && commits_count > 0
   end
diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb
index 255f63db5c2..0db9e31031c 100644
--- a/app/presenters/merge_request_presenter.rb
+++ b/app/presenters/merge_request_presenter.rb
@@ -76,7 +76,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def conflict_resolution_path
-    if conflicts_can_be_resolved_in_ui? && conflicts_can_be_resolved_by?(current_user)
+    if conflicts.can_be_resolved_in_ui? && conflicts.can_be_resolved_by?(current_user)
       conflicts_namespace_project_merge_request_path(project.namespace, project, merge_request)
     end
   end
@@ -141,6 +141,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
 
   private
 
+  def conflicts
+    @conflicts ||= MergeRequests::Conflicts::ListService.new(merge_request)
+  end
+
   def closing_issues
     @closing_issues ||= closes_issues(current_user)
   end
diff --git a/app/services/merge_requests/conflicts/base_service.rb b/app/services/merge_requests/conflicts/base_service.rb
new file mode 100644
index 00000000000..b50875347d9
--- /dev/null
+++ b/app/services/merge_requests/conflicts/base_service.rb
@@ -0,0 +1,11 @@
+module MergeRequests
+  module Conflicts
+    class BaseService
+      attr_reader :merge_request
+
+      def initialize(merge_request)
+        @merge_request = merge_request
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb
new file mode 100644
index 00000000000..9bf82518643
--- /dev/null
+++ b/app/services/merge_requests/conflicts/list_service.rb
@@ -0,0 +1,35 @@
+module MergeRequests
+  module Conflicts
+    class ListService < MergeRequests::Conflicts::BaseService
+      delegate :file_for_path, :to_json, to: :conflicts
+
+      def can_be_resolved_by?(user)
+        return false unless merge_request.source_project
+
+        access = ::Gitlab::UserAccess.new(user, project: merge_request.source_project)
+        access.can_push_to_branch?(merge_request.source_branch)
+      end
+
+      def can_be_resolved_in_ui?
+        return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui)
+
+        return @conflicts_can_be_resolved_in_ui = false unless merge_request.cannot_be_merged?
+        return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs?
+
+        begin
+          # Try to parse each conflict. If the MR's mergeable status hasn't been
+          # updated, ensure that we don't say there are conflicts to resolve
+          # when there are no conflict files.
+          conflicts.files.each(&:lines)
+          @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
+        rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing
+          @conflicts_can_be_resolved_in_ui = false
+        end
+      end
+
+      def conflicts
+        @conflicts ||= Gitlab::Conflict::FileCollection.read_only(merge_request)
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/conflicts/resolve_service.rb b/app/services/merge_requests/conflicts/resolve_service.rb
new file mode 100644
index 00000000000..d74a82effd6
--- /dev/null
+++ b/app/services/merge_requests/conflicts/resolve_service.rb
@@ -0,0 +1,53 @@
+module MergeRequests
+  module Conflicts
+    class ResolveService < MergeRequests::Conflicts::BaseService
+      MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
+
+      def execute(current_user, params)
+        rugged = merge_request.source_project.repository.rugged
+
+        Gitlab::Conflict::FileCollection.for_resolution(merge_request) do |conflicts_for_resolution|
+          merge_index = conflicts_for_resolution.merge_index
+
+          params[:files].each do |file_params|
+            conflict_file = conflicts_for_resolution.file_for_path(file_params[:old_path], file_params[:new_path])
+
+            write_resolved_file_to_index(merge_index, rugged, conflict_file, file_params)
+          end
+
+          unless merge_index.conflicts.empty?
+            missing_files = merge_index.conflicts.map { |file| file[:ours][:path] }
+
+            raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}"
+          end
+
+          commit_params = {
+            message: params[:commit_message] || conflicts_for_resolution.default_commit_message,
+            parents: [conflicts_for_resolution.our_commit, conflicts_for_resolution.their_commit].map(&:oid),
+            tree: merge_index.write_tree(rugged)
+          }
+
+          conflicts_for_resolution.
+            project.
+            repository.
+            resolve_conflicts(current_user, merge_request.source_branch, commit_params)
+        end
+      end
+
+      private
+
+      def write_resolved_file_to_index(merge_index, rugged, file, params)
+        new_file = if params[:sections]
+                     file.resolve_lines(params[:sections]).map(&:text).join("\n")
+                   elsif params[:content]
+                     file.resolve_content(params[:content])
+                   end
+
+        our_path = file.our_path
+
+        merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
+        merge_index.conflict_remove(our_path)
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/resolve_service.rb b/app/services/merge_requests/resolve_service.rb
deleted file mode 100644
index 82cd89d9a0b..00000000000
--- a/app/services/merge_requests/resolve_service.rb
+++ /dev/null
@@ -1,65 +0,0 @@
-module MergeRequests
-  class ResolveService < MergeRequests::BaseService
-    MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
-
-    attr_accessor :conflicts, :rugged, :merge_index, :merge_request
-
-    def execute(merge_request)
-      @conflicts = merge_request.conflicts
-      @rugged = project.repository.rugged
-      @merge_index = conflicts.merge_index
-      @merge_request = merge_request
-
-      fetch_their_commit!
-
-      params[:files].each do |file_params|
-        conflict_file = merge_request.conflicts.file_for_path(file_params[:old_path], file_params[:new_path])
-
-        write_resolved_file_to_index(conflict_file, file_params)
-      end
-
-      unless merge_index.conflicts.empty?
-        missing_files = merge_index.conflicts.map { |file| file[:ours][:path] }
-
-        raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}"
-      end
-
-      commit_params = {
-        message: params[:commit_message] || conflicts.default_commit_message,
-        parents: [conflicts.our_commit, conflicts.their_commit].map(&:oid),
-        tree: merge_index.write_tree(rugged)
-      }
-
-      project.repository.resolve_conflicts(current_user, merge_request.source_branch, commit_params)
-    end
-
-    def write_resolved_file_to_index(file, params)
-      new_file = if params[:sections]
-                   file.resolve_lines(params[:sections]).map(&:text).join("\n")
-                 elsif params[:content]
-                   file.resolve_content(params[:content])
-                 end
-
-      our_path = file.our_path
-
-      merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
-      merge_index.conflict_remove(our_path)
-    end
-
-    # If their commit (in the target project) doesn't exist in the source project, it
-    # can't be a parent for the merge commit we're about to create. If that's the case,
-    # fetch the target branch ref into the source project so the commit exists in both.
-    #
-    def fetch_their_commit!
-      return if rugged.include?(conflicts.their_commit.oid)
-
-      random_string = SecureRandom.hex
-
-      project.repository.fetch_ref(
-        merge_request.target_project.repository.path_to_repo,
-        "refs/heads/#{merge_request.target_branch}",
-        "refs/tmp/#{random_string}/head"
-      )
-    end
-  end
-end
diff --git a/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml b/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml
new file mode 100644
index 00000000000..19a3c56e478
--- /dev/null
+++ b/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent further repository corruption when resolving conflicts from a fork
+  where both the fork and upstream projects require housekeeping
+merge_request:
+author:
diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb
index 990b719ecfd..6e73361cad1 100644
--- a/lib/gitlab/conflict/file_collection.rb
+++ b/lib/gitlab/conflict/file_collection.rb
@@ -3,16 +3,33 @@ module Gitlab
     class FileCollection
       ConflictSideMissing = Class.new(StandardError)
 
-      attr_reader :merge_request, :our_commit, :their_commit
+      attr_reader :merge_request, :our_commit, :their_commit, :project
 
-      def initialize(merge_request)
-        @merge_request = merge_request
-        @our_commit = merge_request.source_branch_head.raw.raw_commit
-        @their_commit = merge_request.target_branch_head.raw.raw_commit
-      end
+      delegate :repository, to: :project
+
+      class << self
+        # We can only write when getting the merge index from the source
+        # project, because we will write to that project. We don't use this all
+        # the time because this fetches a ref into the source project, which
+        # isn't needed for reading.
+        def for_resolution(merge_request)
+          project = merge_request.source_project
+
+          new(merge_request, project).tap do |file_collection|
+            project.
+              repository.
+              with_repo_branch_commit(merge_request.target_project.repository, merge_request.target_branch) do
+
+              yield file_collection
+            end
+          end
+        end
 
-      def repository
-        merge_request.project.repository
+        # We don't need to do `with_repo_branch_commit` here, because the target
+        # project always fetches source refs when creating merge request diffs.
+        def read_only(merge_request)
+          new(merge_request, merge_request.target_project)
+        end
       end
 
       def merge_index
@@ -55,6 +72,15 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc
 #{conflict_filenames.join("\n")}
 EOM
       end
+
+      private
+
+      def initialize(merge_request, project)
+        @merge_request = merge_request
+        @our_commit = merge_request.source_branch_head.raw.raw_commit
+        @their_commit = merge_request.target_branch_head.raw.raw_commit
+        @project = project
+      end
     end
   end
 end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 37a253fde9b..0b3492a8fed 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -916,7 +916,9 @@ describe Projects::MergeRequestsController do
       end
 
       it 'returns the file in JSON format' do
-        content = merge_request_with_conflicts.conflicts.file_for_path(path, path).content
+        content = MergeRequests::Conflicts::ListService.new(merge_request_with_conflicts).
+                    file_for_path(path, path).
+                    content
 
         expect(json_response).to include('old_path' => path,
                                          'new_path' => path,
@@ -1040,11 +1042,15 @@ describe Projects::MergeRequestsController do
 
     context 'when a file has identical content to the conflict' do
       before do
+        content = MergeRequests::Conflicts::ListService.new(merge_request_with_conflicts).
+                    file_for_path('files/ruby/popen.rb', 'files/ruby/popen.rb').
+                    content
+
         resolved_files = [
           {
             'new_path' => 'files/ruby/popen.rb',
             'old_path' => 'files/ruby/popen.rb',
-            'content' => merge_request_with_conflicts.conflicts.file_for_path('files/ruby/popen.rb', 'files/ruby/popen.rb').content
+            'content' => content
           }, {
             'new_path' => 'files/ruby/regex.rb',
             'old_path' => 'files/ruby/regex.rb',
diff --git a/spec/lib/gitlab/conflict/file_collection_spec.rb b/spec/lib/gitlab/conflict/file_collection_spec.rb
index 39d892c18c0..27f23ea70dc 100644
--- a/spec/lib/gitlab/conflict/file_collection_spec.rb
+++ b/spec/lib/gitlab/conflict/file_collection_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::Conflict::FileCollection, lib: true do
   let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') }
-  let(:file_collection) { Gitlab::Conflict::FileCollection.new(merge_request) }
+  let(:file_collection) { described_class.read_only(merge_request) }
 
   describe '#files' do
     it 'returns an array of Conflict::Files' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index e8124c4cbe9..ce870fcc1d3 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1268,71 +1268,6 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#conflicts_can_be_resolved_in_ui?' do
-    def create_merge_request(source_branch)
-      create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
-        mr.mark_as_unmergeable
-      end
-    end
-
-    it 'returns a falsey value when the MR can be merged without conflicts' do
-      merge_request = create_merge_request('master')
-      merge_request.mark_as_mergeable
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the MR is marked as having conflicts, but has none' do
-      merge_request = create_merge_request('master')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the MR has a missing ref after a force push' do
-      merge_request = create_merge_request('conflict-resolvable')
-      allow(merge_request.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError)
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the MR does not support new diff notes' do
-      merge_request = create_merge_request('conflict-resolvable')
-      merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the conflicts contain a large file' do
-      merge_request = create_merge_request('conflict-too-large')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the conflicts contain a binary file' do
-      merge_request = create_merge_request('conflict-binary-file')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do
-      merge_request = create_merge_request('conflict-missing-side')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a truthy value when the conflicts are resolvable in the UI' do
-      merge_request = create_merge_request('conflict-resolvable')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
-    end
-
-    it 'returns a truthy value when the conflicts have to be resolved in an editor' do
-      merge_request = create_merge_request('conflict-contains-conflict-markers')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
-    end
-  end
-
   describe "#source_project_missing?" do
     let(:project)      { create(:empty_project) }
     let(:fork_project) { create(:empty_project, forked_from_project: project) }
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index e599ddaf943..44720fc4448 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/spec/presenters/merge_request_presenter_spec.rb
@@ -73,12 +73,12 @@ describe MergeRequestPresenter do
   describe '#conflict_resolution_path' do
     let(:project) { create :empty_project }
     let(:user) { create :user }
-    let(:path) { described_class.new(resource, current_user: user).conflict_resolution_path }
+    let(:presenter) { described_class.new(resource, current_user: user) }
+    let(:path) { presenter.conflict_resolution_path }
 
     context 'when MR cannot be resolved in UI' do
       it 'does not return conflict resolution path' do
-        allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true }
-        allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { false }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { false }
 
         expect(path).to be_nil
       end
@@ -86,8 +86,8 @@ describe MergeRequestPresenter do
 
     context 'when conflicts cannot be resolved by user' do
       it 'does not return conflict resolution path' do
-        allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { false }
-        allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_by?).with(user) { false }
 
         expect(path).to be_nil
       end
@@ -95,8 +95,8 @@ describe MergeRequestPresenter do
 
     context 'when able to access conflict resolution UI' do
       it 'does return conflict resolution path' do
-        allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true }
-        allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_by?).with(user) { true }
 
         expect(path)
           .to eq("/#{project.full_path}/merge_requests/#{resource.iid}/conflicts")
diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb
new file mode 100644
index 00000000000..e8a305d6130
--- /dev/null
+++ b/spec/services/merge_requests/conflicts/list_service_spec.rb
@@ -0,0 +1,73 @@
+require 'spec_helper'
+
+describe MergeRequests::Conflicts::ListService do
+  describe '#can_be_resolved_in_ui?' do
+    def create_merge_request(source_branch)
+      create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
+        mr.mark_as_unmergeable
+      end
+    end
+
+    def conflicts_service(merge_request)
+      described_class.new(merge_request)
+    end
+
+    it 'returns a falsey value when the MR can be merged without conflicts' do
+      merge_request = create_merge_request('master')
+      merge_request.mark_as_mergeable
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the MR is marked as having conflicts, but has none' do
+      merge_request = create_merge_request('master')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the MR has a missing ref after a force push' do
+      merge_request = create_merge_request('conflict-resolvable')
+      service = conflicts_service(merge_request)
+      allow(service.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError)
+
+      expect(service.can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the MR does not support new diff notes' do
+      merge_request = create_merge_request('conflict-resolvable')
+      merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the conflicts contain a large file' do
+      merge_request = create_merge_request('conflict-too-large')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the conflicts contain a binary file' do
+      merge_request = create_merge_request('conflict-binary-file')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do
+      merge_request = create_merge_request('conflict-missing-side')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a truthy value when the conflicts are resolvable in the UI' do
+      merge_request = create_merge_request('conflict-resolvable')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
+    end
+
+    it 'returns a truthy value when the conflicts have to be resolved in an editor' do
+      merge_request = create_merge_request('conflict-contains-conflict-markers')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
+    end
+  end
+end
diff --git a/spec/services/merge_requests/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb
index 3afd6b92900..19e8d5cc5f1 100644
--- a/spec/services/merge_requests/resolve_service_spec.rb
+++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe MergeRequests::ResolveService do
+describe MergeRequests::Conflicts::ResolveService do
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository) }
 
@@ -24,6 +24,8 @@ describe MergeRequests::ResolveService do
   end
 
   describe '#execute' do
+    let(:service) { described_class.new(merge_request) }
+
     context 'with section params' do
       let(:params) do
         {
@@ -50,7 +52,7 @@ describe MergeRequests::ResolveService do
 
       context 'when the source and target project are the same' do
         before do
-          described_class.new(project, user, params).execute(merge_request)
+          service.execute(user, params)
         end
 
         it 'creates a commit with the message' do
@@ -74,15 +76,26 @@ describe MergeRequests::ResolveService do
             branch_name: 'conflict-start')
         end
 
-        before do
-          described_class.new(fork_project, user, params).execute(merge_request_from_fork)
+        def resolve_conflicts
+          described_class.new(merge_request_from_fork).execute(user, params)
+        end
+
+        it 'gets conflicts from the source project' do
+          expect(fork_project.repository.rugged).to receive(:merge_commits).and_call_original
+          expect(project.repository.rugged).not_to receive(:merge_commits)
+
+          resolve_conflicts
         end
 
         it 'creates a commit with the message' do
+          resolve_conflicts
+
           expect(merge_request_from_fork.source_branch_head.message).to eq(params[:commit_message])
         end
 
         it 'creates a commit with the correct parents' do
+          resolve_conflicts
+
           expect(merge_request_from_fork.source_branch_head.parents.map(&:id)).
             to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813',
                    target_head])
@@ -115,7 +128,7 @@ describe MergeRequests::ResolveService do
       end
 
       before do
-        described_class.new(project, user, params).execute(merge_request)
+        service.execute(user, params)
       end
 
       it 'creates a commit with the message' do
@@ -154,15 +167,15 @@ describe MergeRequests::ResolveService do
         }
       end
 
-      let(:service) { described_class.new(project, user, invalid_params) }
-
       it 'raises a MissingResolution error' do
-        expect { service.execute(merge_request) }.
+        expect { service.execute(user, invalid_params) }.
           to raise_error(Gitlab::Conflict::File::MissingResolution)
       end
     end
 
     context 'when the content of a file is unchanged' do
+      let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) }
+
       let(:invalid_params) do
         {
           files: [
@@ -173,17 +186,15 @@ describe MergeRequests::ResolveService do
             }, {
               old_path: 'files/ruby/regex.rb',
               new_path: 'files/ruby/regex.rb',
-              content: merge_request.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
+              content: list_service.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
             }
           ],
           commit_message: 'This is a commit message!'
         }
       end
 
-      let(:service) { described_class.new(project, user, invalid_params) }
-
       it 'raises a MissingResolution error' do
-        expect { service.execute(merge_request) }.
+        expect { service.execute(user, invalid_params) }.
           to raise_error(Gitlab::Conflict::File::MissingResolution)
       end
     end
@@ -202,11 +213,9 @@ describe MergeRequests::ResolveService do
         }
       end
 
-      let(:service) { described_class.new(project, user, invalid_params) }
-
       it 'raises a MissingFiles error' do
-        expect { service.execute(merge_request) }.
-          to raise_error(MergeRequests::ResolveService::MissingFiles)
+        expect { service.execute(user, invalid_params) }.
+          to raise_error(described_class::MissingFiles)
       end
     end
   end