Fix conflict resolution from corrupted upstreamfix-conflict-resolution-with-corrupt-repos

I don't know why this happens exactly, but given an upstream and fork repository
from a customer, both of which required GC, resolving conflicts would corrupt
the fork so badly that it couldn't be cloned.

This isn't a perfect fix for that case, because the MR may still need to be
merged manually, but it does ensure that the repository is at least usable.

My best guess is that when we generate the index for the conflict
resolution (which we previously did in the target project), we obtain a
reference to an OID that doesn't exist in the source, even though we already
fetch the refs from the target into the source.

Explicitly setting the source project as the place to get the merge index from
seems to prevent repository corruption in this way.

6 files changed, 114 insertions, 91 deletions

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 ef349530761..67f715ee25e 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1310,71 +1310,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