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.

2 files changed, 98 insertions, 16 deletions

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