Handle multiple merge conflict files in collection

app/controllers/projects/merge_requests_controller.rb

@@ -135,33 +135,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html { render 'show' }
-      format.json do
-        rugged = @merge_request.project.repository.rugged
-        their_commit = rugged.branches[@merge_request.target_branch].target
-        our_commit = rugged.lookup(@merge_request.diff_head_sha)
-        merge = rugged.merge_commits(our_commit, their_commit)
-        commit_message = "Merge branch '#{@merge_request.target_branch}' into '#{@merge_request.source_branch}'\n\n# Conflicts:"
-
-        files = merge.conflicts.map do |conflict|
-          their_path = conflict[:theirs][:path]
-          our_path = conflict[:ours][:path]
-
-          raise 'path mismatch!' unless their_path == our_path
-
-          commit_message << "\n#   #{our_path}"
-          merge_file = merge.merge_file(our_path)
-
-          Gitlab::Conflict::File.new(merge_file, conflict, @merge_request.target_branch, @merge_request.source_branch, @merge_request.project.repository)
-        end
-
-        render json: {
-          target_branch: @merge_request.target_branch,
-          source_branch: @merge_request.source_branch,
-          commit_sha: @merge_request.diff_head_sha,
-          commit_message: commit_message,
-          files: files
-        }
-      end
+      format.json { render json: Gitlab::Conflict::FileCollection.new(@merge_request) }
     end
   end
 

lib/gitlab/conflict/file.rb

@@ -3,20 +3,22 @@ module Gitlab
     class File
       CONTEXT_LINES = 3
 
-      attr_reader :merge_file, :their_path, :their_ref, :our_path, :our_ref, :repository
+      attr_reader :merge_file_result, :their_path, :their_ref, :our_path, :our_ref, :repository
 
-      def initialize(merge_file, conflict, their_ref, our_ref, repository)
-        @merge_file = merge_file
+      def initialize(merge_file_result, conflict, diff_refs:, repository:)
+        @merge_file_result = merge_file_result
         @their_path = conflict[:theirs][:path]
         @our_path = conflict[:ours][:path]
-        @their_ref = their_ref
-        @our_ref = our_ref
+        @their_ref = diff_refs.start_sha
+        @our_ref = diff_refs.head_sha
         @repository = repository
       end
 
       # Array of Gitlab::Diff::Line objects
       def lines
-        @lines ||= Gitlab::Conflict::Parser.new.parse(merge_file[:data], their_path, our_path)
+        @lines ||= Gitlab::Conflict::Parser.new.parse(merge_file_result[:data],
+                                                      our_path: our_path,
+                                                      their_path: their_path)
       end
 
       def highlighted_lines
@@ -28,9 +30,9 @@ module Gitlab
         @highlighted_lines = lines.map do |line|
           line = line.dup
           if line.type == 'old'
-            line.rich_text = their_highlight[line.old_line - 1].delete("\n")
+            line.rich_text = their_highlight[line.old_line - 1]
           else
-            line.rich_text = our_highlight[line.new_line - 1].delete("\n")
+            line.rich_text = our_highlight[line.new_line - 1]
           end
           line
         end

lib/gitlab/conflict/file_collection.rb

+module Gitlab
+  module Conflict
+    class FileCollection
+      attr_reader :merge_request, :our_commit, :their_commit
+
+      def initialize(merge_request)
+        @merge_request = merge_request
+        @our_commit = merge_request.diff_head_commit.raw.raw_commit
+        @their_commit = merge_request.target_branch_head.raw.raw_commit
+      end
+
+      def repository
+        merge_request.project.repository
+      end
+
+      def merge_index
+        @merge_index ||= repository.rugged.merge_commits(our_commit, their_commit)
+      end
+
+      def files
+        @files ||= merge_index.conflicts.map do |conflict|
+          their_path = conflict[:theirs][:path]
+          our_path = conflict[:ours][:path]
+
+          # TODO remove this
+          raise 'path mismatch!' unless their_path == our_path
+
+          Gitlab::Conflict::File.new(merge_index.merge_file(our_path),
+                                     conflict,
+                                     diff_refs: merge_request.diff_refs,
+                                     repository: repository)
+        end
+      end
+
+      def as_json(opts = nil)
+        {
+          target_branch: merge_request.target_branch,
+          source_branch: merge_request.source_branch,
+          commit_sha: merge_request.diff_head_sha,
+          commit_message: default_commit_message,
+          files: files
+        }
+      end
+
+      def default_commit_message
+        conflict_filenames = merge_index.conflicts.map do |conflict|
+          "#   #{conflict[:ours][:path]}"
+        end
+
+        <<EOM.chomp
+Merge branch '#{merge_request.source_branch}' into '#{merge_request.target_branch}'
+
+# Conflicts:
+#{conflict_filenames.join("\n")}
+EOM
+      end
+    end
+  end
+end

lib/gitlab/conflict/parser.rb

@@ -7,7 +7,7 @@ module Gitlab
       class MissingEndDelimiter < StandardError
       end
 
-      def parse(text, their_path, our_path)
+      def parse(text, our_path:, their_path:)
         return [] if text.blank?
 
         line_obj_index = 0
@@ -46,14 +46,10 @@ module Gitlab
           end
         end
 
-        raise MissingEndDelimiter unless type == nil
+        raise MissingEndDelimiter unless type.nil?
 
         lines
       end
-
-      def empty?
-        @lines.empty?
-      end
     end
   end
 end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -523,4 +523,69 @@ describe Projects::MergeRequestsController do
       end
     end
   end
+
+  describe 'GET conflicts' do
+    let(:merge_request_with_conflicts) do
+      create(:merge_request, source_branch: 'conflict-a', target_branch: 'conflict-b', source_project: project) do |mr|
+        mr.mark_as_unmergeable
+      end
+    end
+
+    context 'as JSON' do
+      before do
+        get :conflicts,
+            namespace_id: merge_request_with_conflicts.project.namespace.to_param,
+            project_id: merge_request_with_conflicts.project.to_param,
+            id: merge_request_with_conflicts.iid,
+            format: 'json'
+      end
+
+      let(:json_response) { JSON.parse(response.body) }
+
+      it 'includes meta info about the MR' do
+        expect(json_response['commit_message']).to include('Merge branch')
+        expect(json_response['commit_sha']).to match(/\h{40}/)
+        expect(json_response['source_branch']).to eq(merge_request_with_conflicts.source_branch)
+        expect(json_response['target_branch']).to eq(merge_request_with_conflicts.target_branch)
+      end
+
+      it 'includes each file that has conflicts' do
+        filenames = json_response['files'].map { |file| file['new_path'] }
+
+        expect(filenames).to contain_exactly('files/ruby/popen.rb', 'files/ruby/regex.rb')
+      end
+
+      it 'splits files into sections with lines' do
+        json_response['files'].each do |file|
+          file['sections'].each do |section|
+            expect(section).to include('conflict', 'lines')
+
+            section['lines'].each do |line|
+              if section['conflict']
+                expect(line['type']).to be_in(['old', 'new'])
+                expect(line.values_at('old_line', 'new_line')).to contain_exactly(nil, a_kind_of(Integer))
+              else
+                if line['type'].nil?
+                  expect(line['old_line']).not_to eq(nil)
+                  expect(line['new_line']).not_to eq(nil)
+                else
+                  expect(line['type']).to eq('match')
+                  expect(line['old_line']).to eq(nil)
+                  expect(line['new_line']).to eq(nil)
+                end
+              end
+            end
+          end
+        end
+      end
+
+      it 'has unique section IDs across files' do
+        section_ids = json_response['files'].flat_map do |file|
+          file['sections'].map { |section| section['id'] }.compact
+        end
+
+        expect(section_ids.uniq).to eq(section_ids)
+      end
+    end
+  end
 end

spec/lib/gitlab/conflict/file_collection_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Conflict::FileCollection, lib: true do
+  let(:merge_request) { create(:merge_request, source_branch: 'conflict-a', target_branch: 'conflict-b') }
+  let(:file_collection) { Gitlab::Conflict::FileCollection.new(merge_request) }
+
+  describe '#files' do
+    it 'returns an array of Conflict::Files' do
+      expect(file_collection.files).to all(be_an_instance_of(Gitlab::Conflict::File))
+    end
+  end
+
+  describe '#default_commit_message' do
+    it 'matches the format of the git CLI commit message' do
+      expect(file_collection.default_commit_message).to eq(<<EOM.chomp)
+Merge branch 'conflict-a' into 'conflict-b'
+
+# Conflicts:
+#   files/ruby/popen.rb
+#   files/ruby/regex.rb
+EOM
+    end
+  end
+end

spec/lib/gitlab/conflict/file_spec.rb

@@ -4,24 +4,21 @@ describe Gitlab::Conflict::File, lib: true do
   let(:project) { create(:project) }
   let(:repository) { project.repository }
   let(:rugged) { repository.rugged }
-  let(:their_ref) { their_commit.oid }
   let(:their_commit) { rugged.branches['conflict-a'].target }
-  let(:our_ref) { our_commit.oid }
+  let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: their_commit.oid, head_sha: our_commit.oid) }
   let(:our_commit) { rugged.branches['conflict-b'].target }
   let(:index) { rugged.merge_commits(our_commit, their_commit) }
   let(:conflict) { index.conflicts.last }
-  let(:merge_file) { index.merge_file('files/ruby/regex.rb') }
-  let(:conflict_file) { Gitlab::Conflict::File.new(merge_file, conflict, their_ref, our_ref, repository) }
+  let(:merge_file_result) { index.merge_file('files/ruby/regex.rb') }
+  let(:conflict_file) { Gitlab::Conflict::File.new(merge_file_result, conflict, diff_refs: diff_refs, repository: repository) }
 
   describe '#highlighted_lines' do
     def html_to_text(html)
-      CGI.unescapeHTML(ActionView::Base.full_sanitizer.sanitize(html))
+      CGI.unescapeHTML(ActionView::Base.full_sanitizer.sanitize(html)).delete("\n")
     end
 
     it 'returns lines with rich_text' do
-      conflict_file.highlighted_lines.each do |line|
-        expect(line).to have_attributes(rich_text: an_instance_of(String))
-      end
+      expect(conflict_file.highlighted_lines).to all(have_attributes(rich_text: a_kind_of(String)))
     end
 
     it 'returns lines with rich_text matching the text content of the line' do

spec/lib/gitlab/conflict/parser_spec.rb

@@ -82,7 +82,9 @@ end
 CONFLICT
       end
 
-      let(:lines) { parser.parse(text, 'files/ruby/regex.rb', 'files/ruby/regex.rb') }
+      let(:lines) do
+        parser.parse(text, our_path: 'files/ruby/regex.rb', their_path: 'files/ruby/regex.rb')
+      end
 
       it 'sets our lines as new lines' do
         expect(lines[8..13]).to all(have_attributes(type: 'new'))
@@ -117,7 +119,7 @@ CONFLICT
       let(:path) { 'README.md' }
 
       def parse_text(text)
-        parser.parse(text, path, path)
+        parser.parse(text, our_path: path, their_path: path)
       end
 
       it 'raises UnexpectedDelimiter when there is a non-start delimiter first' do
@@ -171,8 +173,8 @@ CONFLICT
     end
 
     context 'when lines is blank' do
-      it { expect(parser.parse('', 'README.md', 'README.md')).to eq([]) }
-      it { expect(parser.parse(nil, 'README.md', 'README.md')).to eq([]) }
+      it { expect(parser.parse('', our_path: 'README.md', their_path: 'README.md')).to eq([]) }
+      it { expect(parser.parse(nil, our_path: 'README.md', their_path: 'README.md')).to eq([]) }
     end
   end
 end