Refactor conflict resolution to contain git ops within Gitlab::Git

This prepares the codebase for a Gitaly migration. See
https://gitlab.com/gitlab-org/gitaly/issues/553

app/controllers/projects/merge_requests/conflicts_controller.rb

@@ -53,7 +53,7 @@ class Projects::MergeRequests::ConflictsController < Projects::MergeRequests::Ap
       flash[:notice] = 'All merge conflicts were resolved. The merge request can now be merged.'
 
       render json: { redirect_to: project_merge_request_url(@project, @merge_request, resolved_conflicts: true) }
-    rescue Gitlab::Conflict::ResolutionError => e
+    rescue Gitlab::Git::Merge::ResolutionError => e
       render status: :bad_request, json: { message: e.message }
     end
   end

app/models/repository.rb

@@ -906,14 +906,6 @@ class Repository
     end
   end
 
-  def resolve_conflicts(user, branch_name, params)
-    with_branch(user, branch_name) do
-      committer = user_to_committer(user)
-
-      create_commit(params.merge(author: committer, committer: committer))
-    end
-  end
-
   def merged_to_root_ref?(branch_name)
     branch_commit = commit(branch_name)
     root_ref_commit = commit(root_ref)

app/services/merge_requests/conflicts/list_service.rb

@@ -23,13 +23,13 @@ module MergeRequests
           # 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
+        rescue Rugged::OdbError, Gitlab::Git::ConflictParser::UnresolvableError, Gitlab::Git::Merge::ConflictSideMissing
           @conflicts_can_be_resolved_in_ui = false
         end
       end
 
       def conflicts
-        @conflicts ||= Gitlab::Conflict::FileCollection.read_only(merge_request)
+        @conflicts ||= Gitlab::Conflict::FileCollection.new(merge_request)
       end
     end
   end

app/services/merge_requests/conflicts/resolve_service.rb

@@ -2,7 +2,7 @@ module MergeRequests
   module Conflicts
     class ResolveService < MergeRequests::Conflicts::BaseService
       def execute(current_user, params)
-        conflicts = Gitlab::Conflict::FileCollection.for_resolution(merge_request)
+        conflicts = Gitlab::Conflict::FileCollection.new(merge_request)
 
         conflicts.resolve(current_user, params[:commit_message], params[:files])
       end

lib/gitlab/conflict/file.rb

@@ -4,82 +4,26 @@ module Gitlab
       include Gitlab::Routing
       include IconsHelper
 
-      MissingResolution = Class.new(ResolutionError)
-
       CONTEXT_LINES = 3
 
-      attr_reader :merge_file_result, :their_path, :our_path, :our_mode, :merge_request, :repository
+      attr_reader :merge_request, :raw
+
+      delegate :type, :content, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
 
-      def initialize(merge_file_result, conflict, merge_request:)
-        @merge_file_result = merge_file_result
-        @their_path = conflict[:theirs][:path]
-        @our_path = conflict[:ours][:path]
-        @our_mode = conflict[:ours][:mode]
+      def initialize(raw, merge_request:)
+        @raw = raw
         @merge_request = merge_request
-        @repository = merge_request.project.repository
         @match_line_headers = {}
       end
 
-      def content
-        merge_file_result[:data]
-      end
-
-      def our_blob
-        @our_blob ||= repository.blob_at(merge_request.diff_refs.head_sha, our_path)
-      end
-
-      def type
-        lines unless @type
-
-        @type.inquiry
-      end
-
-      # Array of Gitlab::Diff::Line objects
       def lines
         return @lines if defined?(@lines)
 
-        begin
-          @type = 'text'
-          @lines = Gitlab::Conflict::Parser.new.parse(content,
-                                                      our_path: our_path,
-                                                      their_path: their_path,
-                                                      parent_file: self)
-        rescue Gitlab::Conflict::Parser::ParserError
-          @type = 'text-editor'
-          @lines = nil
-        end
+        @lines = raw.lines.nil? ? nil : map_raw_lines(raw.lines)
       end
 
       def resolve_lines(resolution)
-        section_id = nil
-
-        lines.map do |line|
-          unless line.type
-            section_id = nil
-            next line
-          end
-
-          section_id ||= line_code(line)
-
-          case resolution[section_id]
-          when 'head'
-            next unless line.type == 'new'
-          when 'origin'
-            next unless line.type == 'old'
-          else
-            raise MissingResolution, "Missing resolution for section ID: #{section_id}"
-          end
-
-          line
-        end.compact
-      end
-
-      def resolve_content(resolution)
-        if resolution == content
-          raise MissingResolution, "Resolved content has no changes for file #{our_path}"
-        end
-
-        resolution
+        map_raw_lines(raw.resolve_lines(resolution))
       end
 
       def highlight_lines!
@@ -227,9 +171,9 @@ module Gitlab
                                                      new_path: our_path)
       end
 
-      # Don't try to print merge_request or repository.
+      # Don't try to print merge_request.
       def inspect
-        instance_variables = [:merge_file_result, :their_path, :our_path, :our_mode, :type].map do |instance_variable|
+        instance_variables = [:content, :their_path, :our_path, :our_mode, :type].map do |instance_variable|
           value = instance_variable_get("@#{instance_variable}")
 
           "#{instance_variable}=\"#{value}\""
@@ -237,6 +181,16 @@ module Gitlab
 
         "#<#{self.class} #{instance_variables.join(' ')}>"
       end
+
+      private
+
+      def map_raw_lines(raw_lines)
+        raw_lines.map do |raw_line|
+          Gitlab::Diff::Line.new(raw_line[:full_line], raw_line[:type],
+            raw_line[:line_obj_index], raw_line[:line_old],
+            raw_line[:line_new], parent_file: self)
+        end
+      end
     end
   end
 end

lib/gitlab/conflict/file_collection.rb

 module Gitlab
   module Conflict
     class FileCollection
-      ConflictSideMissing = Class.new(StandardError)
-      MissingFiles = Class.new(ResolutionError)
-
-      attr_reader :merge_request, :our_commit, :their_commit, :project, :read_only
-
-      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)
-          new(merge_request, merge_request.source_project, false)
-        end
-
-        # 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, true)
-        end
+      attr_reader :merge_request, :merge
+
+      def initialize(merge_request)
+        source_repo = merge_request.source_project.repository.raw
+        our_commit = merge_request.source_branch_head.raw
+        their_commit = merge_request.target_branch_head.raw
+        target_repo = merge_request.target_project.repository.raw
+        @merge = Gitlab::Git::Merge.new(source_repo, our_commit, target_repo, their_commit)
+        @merge_request = merge_request
       end
 
       def resolve(user, commit_message, files)
-        raise "can't resolve a read-only Conflict File Collection" if read_only
-
-        repository.with_repo_branch_commit(merge_request.target_project.repository.raw, merge_request.target_branch) do
-          rugged = repository.rugged
-
-          files.each do |file_params|
-            conflict_file = 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: commit_message || default_commit_message,
-            parents: [our_commit, their_commit].map(&:oid),
-            tree: merge_index.write_tree(rugged)
-          }
-
-          repository.resolve_conflicts(user, merge_request.source_branch, commit_params)
-        end
-      end
-
-      def merge_index
-        @merge_index ||= repository.rugged.merge_commits(our_commit, their_commit)
+        args = {
+          source_branch: merge_request.source_branch,
+          target_branch: merge_request.target_branch,
+          commit_message: commit_message || default_commit_message
+        }
+        merge.resolve_conflicts(user, files, args)
       end
 
       def files
-        @files ||= merge_index.conflicts.map do |conflict|
-          raise ConflictSideMissing unless conflict[:theirs] && conflict[:ours]
-
-          Gitlab::Conflict::File.new(merge_index.merge_file(conflict[:ours][:path]),
-                                     conflict,
-                                     merge_request: merge_request)
+        @files ||= merge.conflicts.map do |conflict_file|
+          Gitlab::Conflict::File.new(conflict_file, merge_request: merge_request)
         end
       end
 
@@ -81,8 +42,8 @@ module Gitlab
       end
 
       def default_commit_message
-        conflict_filenames = merge_index.conflicts.map do |conflict|
-          "#   #{conflict[:ours][:path]}"
+        conflict_filenames = files.map do |conflict|
+          "#   #{conflict.our_path}"
         end
 
         <<EOM.chomp
@@ -92,31 +53,6 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc
 #{conflict_filenames.join("\n")}
 EOM
       end
-
-      private
-
-      def write_resolved_file_to_index(merge_index, rugged, file, params)
-        if params[:sections]
-          new_file = file.resolve_lines(params[:sections]).map(&:text).join("\n")
-
-          new_file << "\n" if file.our_blob.data.ends_with?("\n")
-        elsif params[:content]
-          new_file = 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
-
-      def initialize(merge_request, project, read_only)
-        @merge_request = merge_request
-        @our_commit = merge_request.source_branch_head.raw.rugged_commit
-        @their_commit = merge_request.target_branch_head.raw.rugged_commit
-        @project = project
-        @read_only = read_only
-      end
     end
   end
 end

lib/gitlab/conflict/parser.rb

-module Gitlab
-  module Conflict
-    class Parser
-      UnresolvableError = Class.new(StandardError)
-      UnmergeableFile = Class.new(UnresolvableError)
-      UnsupportedEncoding = Class.new(UnresolvableError)
-
-      # Recoverable errors - the conflict can be resolved in an editor, but not with
-      # sections.
-      ParserError = Class.new(StandardError)
-      UnexpectedDelimiter = Class.new(ParserError)
-      MissingEndDelimiter = Class.new(ParserError)
-
-      def parse(text, our_path:, their_path:, parent_file: nil)
-        validate_text!(text)
-
-        line_obj_index = 0
-        line_old = 1
-        line_new = 1
-        type = nil
-        lines = []
-        conflict_start = "<<<<<<< #{our_path}"
-        conflict_middle = '======='
-        conflict_end = ">>>>>>> #{their_path}"
-
-        text.each_line.map do |line|
-          full_line = line.delete("\n")
-
-          if full_line == conflict_start
-            validate_delimiter!(type.nil?)
-
-            type = 'new'
-          elsif full_line == conflict_middle
-            validate_delimiter!(type == 'new')
-
-            type = 'old'
-          elsif full_line == conflict_end
-            validate_delimiter!(type == 'old')
-
-            type = nil
-          elsif line[0] == '\\'
-            type = 'nonewline'
-            lines << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: parent_file)
-          else
-            lines << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: parent_file)
-            line_old += 1 if type != 'new'
-            line_new += 1 if type != 'old'
-
-            line_obj_index += 1
-          end
-        end
-
-        raise MissingEndDelimiter unless type.nil?
-
-        lines
-      end
-
-      private
-
-      def validate_text!(text)
-        raise UnmergeableFile if text.blank? # Typically a binary file
-        raise UnmergeableFile if text.length > 200.kilobytes
-
-        text.force_encoding('UTF-8')
-
-        raise UnsupportedEncoding unless text.valid_encoding?
-      end
-
-      def validate_delimiter!(condition)
-        raise UnexpectedDelimiter unless condition
-      end
-    end
-  end
-end

lib/gitlab/conflict/resolution_error.rb

-module Gitlab
-  module Conflict
-    ResolutionError = Class.new(StandardError)
-  end
-end

lib/gitlab/git/conflict_file.rb

+module Gitlab
+  module Git
+    class ConflictFile
+      attr_reader :content, :their_path, :our_path, :our_mode, :repository
+
+      def initialize(repository, commit_oid, conflict, content)
+        @repository = repository
+        @commit_oid = commit_oid
+        @their_path = conflict[:theirs][:path]
+        @our_path = conflict[:ours][:path]
+        @our_mode = conflict[:ours][:mode]
+        @content = content
+      end
+
+      def lines
+        return @lines if defined?(@lines)
+
+        begin
+          @type = 'text'
+          @lines = Gitlab::Git::ConflictParser.parse(content,
+                                                     our_path: our_path,
+                                                     their_path: their_path)
+        rescue Gitlab::Git::ConflictParser::ParserError
+          @type = 'text-editor'
+          @lines = nil
+        end
+      end
+
+      def type
+        lines unless @type
+
+        @type.inquiry
+      end
+
+      def our_blob
+        # REFACTOR NOTE: the source of `commit_oid` used to be
+        # `merge_request.diff_refs.head_sha`. Instead of passing this value
+        # around the new lib structure, I decided to use `@commit_oid` which is
+        # equivalent to `merge_request.source_branch_head.raw.rugged_commit.oid`.
+        # That is what `merge_request.diff_refs.head_sha` is equivalent to when
+        # `merge_request` is not persisted (see `MergeRequest#diff_head_commit`).
+        # I think using the same oid is more consistent anyways, but if Conflicts
+        # start breaking, the change described above is a good place to look at.
+        @our_blob ||= repository.blob_at(@commit_oid, our_path)
+      end
+
+      def line_code(line)
+        Gitlab::Git::DiffLineCode.generate(our_path, line[:line_new], line[:line_old])
+      end
+
+      def resolve_lines(resolution)
+        section_id = nil
+
+        lines.map do |line|
+          unless line[:type]
+            section_id = nil
+            next line
+          end
+
+          section_id ||= line_code(line)
+
+          case resolution[section_id]
+          when 'head'
+            next unless line[:type] == 'new'
+          when 'origin'
+            next unless line[:type] == 'old'
+          else
+            raise Gitlab::Git::Merge::ResolutionError, "Missing resolution for section ID: #{section_id}"
+          end
+
+          line
+        end.compact
+      end
+
+      def resolve_content(resolution)
+        if resolution == content
+          raise Gitlab::Git::Merge::ResolutionError, "Resolved content has no changes for file #{our_path}"
+        end
+
+        resolution
+      end
+    end
+  end
+end

lib/gitlab/git/conflict_parser.rb

+module Gitlab
+  module Git
+    class ConflictParser
+      UnresolvableError = Class.new(StandardError)
+      UnmergeableFile = Class.new(UnresolvableError)
+      UnsupportedEncoding = Class.new(UnresolvableError)
+
+      # Recoverable errors - the conflict can be resolved in an editor, but not with
+      # sections.
+      ParserError = Class.new(StandardError)
+      UnexpectedDelimiter = Class.new(ParserError)
+      MissingEndDelimiter = Class.new(ParserError)
+
+      class << self
+        def parse(text, our_path:, their_path:, parent_file: nil)
+          validate_text!(text)
+
+          line_obj_index = 0
+          line_old = 1
+          line_new = 1
+          type = nil
+          lines = []
+          conflict_start = "<<<<<<< #{our_path}"
+          conflict_middle = '======='
+          conflict_end = ">>>>>>> #{their_path}"
+
+          text.each_line.map do |line|
+            full_line = line.delete("\n")
+
+            if full_line == conflict_start
+              validate_delimiter!(type.nil?)
+
+              type = 'new'
+            elsif full_line == conflict_middle
+              validate_delimiter!(type == 'new')
+
+              type = 'old'
+            elsif full_line == conflict_end
+              validate_delimiter!(type == 'old')
+
+              type = nil
+            elsif line[0] == '\\'
+              type = 'nonewline'
+              lines << {
+                full_line: full_line,
+                type: type,
+                line_obj_index: line_obj_index,
+                line_old: line_old,
+                line_new: line_new
+              }
+            else
+              lines << {
+                full_line: full_line,
+                type: type,
+                line_obj_index: line_obj_index,
+                line_old: line_old,
+                line_new: line_new
+              }
+
+              line_old += 1 if type != 'new'
+              line_new += 1 if type != 'old'
+
+              line_obj_index += 1
+            end
+          end
+
+          raise MissingEndDelimiter unless type.nil?
+
+          lines
+        end
+
+        private
+
+        def validate_text!(text)
+          raise UnmergeableFile if text.blank? # Typically a binary file
+          raise UnmergeableFile if text.length > 200.kilobytes
+
+          text.force_encoding('UTF-8')
+
+          raise UnsupportedEncoding unless text.valid_encoding?
+        end
+
+        def validate_delimiter!(condition)
+          raise UnexpectedDelimiter unless condition
+        end
+      end
+    end
+  end
+end

lib/gitlab/git/merge.rb

+module Gitlab
+  module Git
+    class Merge
+      ConflictSideMissing = Class.new(StandardError)
+      ResolutionError = Class.new(StandardError)
+
+      def initialize(repository, our_commit, target_repository, their_commit)
+        @repository = repository
+        @our_commit = our_commit.rugged_commit
+        @target_repository = target_repository
+        @their_commit = their_commit.rugged_commit
+      end
+
+      def conflicts
+        @conflicts ||= begin
+          target_index = @target_repository.rugged.merge_commits(@our_commit, @their_commit)
+
+          # We don't need to do `with_repo_branch_commit` here, because the target
+          # project always fetches source refs when creating merge request diffs.
+          target_index.conflicts.map do |conflict|
+            raise ConflictSideMissing unless conflict[:theirs] && conflict[:ours]
+
+            Gitlab::Git::ConflictFile.new(
+              @target_repository,
+              @our_commit.oid,
+              conflict,
+              target_index.merge_file(conflict[:ours][:path])[:data]
+            )
+          end
+        end
+      end
+
+      def resolve_conflicts(user, files, source_branch:, target_branch:, commit_message:)
+        @repository.with_repo_branch_commit(@target_repository, target_branch) do
+          files.each do |file_params|
+            conflict_file = conflict_for_path(file_params[:old_path], file_params[:new_path])
+
+            write_resolved_file_to_index(conflict_file, file_params)
+          end
+
+          unless index.conflicts.empty?
+            missing_files = index.conflicts.map { |file| file[:ours][:path] }
+
+            raise ResolutionError, "Missing resolutions for the following files: #{missing_files.join(', ')}"
+          end
+
+          commit_params = {
+            message: commit_message,
+            parents: [@our_commit, @their_commit].map(&:oid)
+          }
+
+          @repository.commit_index(user, source_branch, index, commit_params)
+        end
+      end
+
+      def conflict_for_path(old_path, new_path)
+        conflicts.find do |conflict|
+          conflict.their_path == old_path && conflict.our_path == new_path
+        end
+      end
+
+      private
+
+      # 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 index
+        @index ||= @repository.rugged.merge_commits(@our_commit, @their_commit)
+      end
+
+      def write_resolved_file_to_index(file, params)
+        if params[:sections]
+          resolved_lines = file.resolve_lines(params[:sections])
+          new_file = resolved_lines.map { |line| line[:full_line] }.join("\n")
+
+          new_file << "\n" if file.our_blob.data.ends_with?("\n")
+        elsif params[:content]
+          new_file = file.resolve_content(params[:content])
+        end
+
+        our_path = file.our_path
+
+        index.add(path: our_path, oid: @repository.rugged.write(new_file, :blob), mode: file.our_mode)
+        index.conflict_remove(our_path)
+      end
+    end
+  end
+end

lib/gitlab/git/repository.rb

@@ -1092,6 +1092,20 @@ module Gitlab
         Gitlab::Git::Blob.find(self, sha, path) unless Gitlab::Git.blank_ref?(sha)
       end
 
+      def commit_index(user, branch_name, index, options)
+        committer = user_to_committer(user)
+
+        OperationService.new(user, self).with_branch(branch_name) do
+          commit_params = options.merge(
+            tree: index.write_tree(rugged),
+            author: committer,
+            committer: committer
+          )
+
+          create_commit(commit_params)
+        end
+      end
+
       def gitaly_repository
         Gitlab::GitalyClient::Util.repository(@storage, @relative_path, @gl_repository)
       end

spec/controllers/projects/merge_requests/conflicts_controller_spec.rb

@@ -17,8 +17,8 @@ describe Projects::MergeRequests::ConflictsController do
   describe 'GET show' do
     context 'when the conflicts cannot be resolved in the UI' do
       before do
-        allow_any_instance_of(Gitlab::Conflict::Parser)
-          .to receive(:parse).and_raise(Gitlab::Conflict::Parser::UnmergeableFile)
+        allow(Gitlab::Git::ConflictParser).to receive(:parse)
+          .and_raise(Gitlab::Git::ConflictParser::UnmergeableFile)
 
         get :show,
             namespace_id: merge_request_with_conflicts.project.namespace.to_param,
@@ -109,8 +109,8 @@ describe Projects::MergeRequests::ConflictsController do
 
     context 'when the conflicts cannot be resolved in the UI' do
       before do
-        allow_any_instance_of(Gitlab::Conflict::Parser)
-          .to receive(:parse).and_raise(Gitlab::Conflict::Parser::UnmergeableFile)
+        allow(Gitlab::Git::ConflictParser).to receive(:parse)
+          .and_raise(Gitlab::Git::ConflictParser::UnmergeableFile)
 
         conflict_for_path('files/ruby/regex.rb')
       end

spec/lib/gitlab/conflict/file_collection_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::Conflict::FileCollection do
   let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') }
-  let(:file_collection) { described_class.read_only(merge_request) }
+  let(:file_collection) { described_class.new(merge_request) }
 
   describe '#files' do
     it 'returns an array of Conflict::Files' do

spec/lib/gitlab/conflict/file_spec.rb

@@ -8,9 +8,10 @@ describe Gitlab::Conflict::File do
   let(:our_commit) { rugged.branches['conflict-resolvable'].target }
   let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) }
   let(:index) { rugged.merge_commits(our_commit, their_commit) }
-  let(:conflict) { index.conflicts.last }
-  let(:merge_file_result) { index.merge_file('files/ruby/regex.rb') }
-  let(:conflict_file) { described_class.new(merge_file_result, conflict, merge_request: merge_request) }
+  let(:rugged_conflict) { index.conflicts.last }
+  let(:raw_conflict_content) { index.merge_file('files/ruby/regex.rb')[:data] }
+  let(:raw_conflict_file) { Gitlab::Git::ConflictFile.new(repository, our_commit.oid, rugged_conflict, raw_conflict_content) }
+  let(:conflict_file) { described_class.new(raw_conflict_file, merge_request: merge_request) }
 
   describe '#resolve_lines' do
     let(:section_keys) { conflict_file.sections.map { |section| section[:id] }.compact }
@@ -48,18 +49,18 @@ describe Gitlab::Conflict::File do
       end
     end
 
-    it 'raises MissingResolution when passed a hash without resolutions for all sections' do
+    it 'raises ResolutionError when passed a hash without resolutions for all sections' do
       empty_hash = section_keys.map { |key| [key, nil] }.to_h
       invalid_hash = section_keys.map { |key| [key, 'invalid'] }.to_h
 
       expect { conflict_file.resolve_lines({}) }
-        .to raise_error(Gitlab::Conflict::File::MissingResolution)
+        .to raise_error(Gitlab::Git::Merge::ResolutionError)
 
       expect { conflict_file.resolve_lines(empty_hash) }
-        .to raise_error(Gitlab::Conflict::File::MissingResolution)
+        .to raise_error(Gitlab::Git::Merge::ResolutionError)
 
       expect { conflict_file.resolve_lines(invalid_hash) }
-        .to raise_error(Gitlab::Conflict::File::MissingResolution)
+        .to raise_error(Gitlab::Git::Merge::ResolutionError)
     end
   end
 
@@ -144,7 +145,7 @@ describe Gitlab::Conflict::File do
     end
 
     context 'with an example file' do
-      let(:file) do
+      let(:raw_conflict_content) do
         <<FILE
   # Ensure there is no match line header here
   def username_regexp
@@ -220,7 +221,6 @@ end
 FILE
       end
 
-      let(:conflict_file) { described_class.new({ data: file }, conflict, merge_request: merge_request) }
       let(:sections) { conflict_file.sections }
 
       it 'sets the correct match line headers' do

spec/lib/gitlab/conflict/parser_spec.rb → spec/lib/gitlab/git/conflict_parser_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Conflict::Parser do
-  let(:parser) { described_class.new }
-
-  describe '#parse' do
+describe Gitlab::Git::ConflictParser do
+  describe '.parse' do
     def parse_text(text)
-      parser.parse(text, our_path: 'README.md', their_path: 'README.md')
+      described_class.parse(text, our_path: 'README.md', their_path: 'README.md')
     end
 
     context 'when the file has valid conflicts' do
@@ -87,33 +85,37 @@ CONFLICT
       end
 
       let(:lines) do
-        parser.parse(text, our_path: 'files/ruby/regex.rb', their_path: 'files/ruby/regex.rb')
+        described_class.parse(text, our_path: 'files/ruby/regex.rb', their_path: 'files/ruby/regex.rb')
+      end
+      let(:old_line_numbers) do
+        lines.select { |line| line[:type] != 'new' }.map { |line| line[:line_old] }
       end
+      let(:new_line_numbers) do
+        lines.select { |line| line[:type] != 'old' }.map { |line| line[:line_new] }
+      end
+      let(:line_indexes) { lines.map { |line| line[:line_obj_index] } }
 
       it 'sets our lines as new lines' do
-        expect(lines[8..13]).to all(have_attributes(type: 'new'))
-        expect(lines[26..27]).to all(have_attributes(type: 'new'))
-        expect(lines[56..57]).to all(have_attributes(type: 'new'))
+        expect(lines[8..13]).to all(include(type: 'new'))
+        expect(lines[26..27]).to all(include(type: 'new'))
+        expect(lines[56..57]).to all(include(type: 'new'))
       end
 
       it 'sets their lines as old lines' do
-        expect(lines[14..19]).to all(have_attributes(type: 'old'))
-        expect(lines[28..29]).to all(have_attributes(type: 'old'))
-        expect(lines[58..59]).to all(have_attributes(type: 'old'))
+        expect(lines[14..19]).to all(include(type: 'old'))
+        expect(lines[28..29]).to all(include(type: 'old'))
+        expect(lines[58..59]).to all(include(type: 'old'))
       end
 
       it 'sets non-conflicted lines as both' do
-        expect(lines[0..7]).to all(have_attributes(type: nil))
-        expect(lines[20..25]).to all(have_attributes(type: nil))
-        expect(lines[30..55]).to all(have_attributes(type: nil))
-        expect(lines[60..62]).to all(have_attributes(type: nil))
+        expect(lines[0..7]).to all(include(type: nil))
+        expect(lines[20..25]).to all(include(type: nil))
+        expect(lines[30..55]).to all(include(type: nil))
+        expect(lines[60..62]).to all(include(type: nil))
       end
 
-      it 'sets consecutive line numbers for index, old_pos, and new_pos' do
-        old_line_numbers = lines.select { |line| line.type != 'new' }.map(&:old_pos)
-        new_line_numbers = lines.select { |line| line.type != 'old' }.map(&:new_pos)
-
-        expect(lines.map(&:index)).to eq(0.upto(62).to_a)
+      it 'sets consecutive line numbers for line_obj_index, line_old, and line_new' do
+        expect(line_indexes).to eq(0.upto(62).to_a)
         expect(old_line_numbers).to eq(1.upto(53).to_a)
         expect(new_line_numbers).to eq(1.upto(53).to_a)
       end
@@ -123,12 +125,12 @@ CONFLICT
       context 'when there is a non-start delimiter first' do
         it 'raises UnexpectedDelimiter when there is a middle delimiter first' do
           expect { parse_text('=======') }
-            .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter)
+            .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter)
         end
 
         it 'raises UnexpectedDelimiter when there is an end delimiter first' do
           expect { parse_text('>>>>>>> README.md') }
-            .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter)
+            .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter)
         end
 
         it 'does not raise when there is an end delimiter for a different path first' do
@@ -143,12 +145,12 @@ CONFLICT
 
         it 'raises UnexpectedDelimiter when it is followed by an end delimiter' do
           expect { parse_text(start_text + '>>>>>>> README.md' + end_text) }
-            .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter)
+            .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter)
         end
 
         it 'raises UnexpectedDelimiter when it is followed by another start delimiter' do
           expect { parse_text(start_text + start_text + end_text) }
-            .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter)
+            .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter)
         end
 
         it 'does not raise when it is followed by a start delimiter for a different path' do
@@ -163,12 +165,12 @@ CONFLICT
 
         it 'raises UnexpectedDelimiter when it is followed by another middle delimiter' do
           expect { parse_text(start_text + '=======' + end_text) }
-            .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter)
+            .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter)
         end
 
         it 'raises UnexpectedDelimiter when it is followed by a start delimiter' do
           expect { parse_text(start_text + start_text + end_text) }
-            .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter)
+            .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter)
         end
 
         it 'does not raise when it is followed by a start delimiter for another path' do
@@ -181,25 +183,25 @@ CONFLICT
         start_text = "<<<<<<< README.md\n=======\n"
 
         expect { parse_text(start_text) }
-          .to raise_error(Gitlab::Conflict::Parser::MissingEndDelimiter)
+          .to raise_error(Gitlab::Git::ConflictParser::MissingEndDelimiter)
 
         expect { parse_text(start_text + '>>>>>>> some-other-path.md') }
-          .to raise_error(Gitlab::Conflict::Parser::MissingEndDelimiter)
+          .to raise_error(Gitlab::Git::ConflictParser::MissingEndDelimiter)
       end
     end
 
     context 'other file types' do
       it 'raises UnmergeableFile when lines is blank, indicating a binary file' do
         expect { parse_text('') }
-          .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile)
+          .to raise_error(Gitlab::Git::ConflictParser::UnmergeableFile)
 
         expect { parse_text(nil) }
-          .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile)
+          .to raise_error(Gitlab::Git::ConflictParser::UnmergeableFile)
       end
 
       it 'raises UnmergeableFile when the file is over 200 KB' do
         expect { parse_text('a' * 204801) }
-          .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile)
+          .to raise_error(Gitlab::Git::ConflictParser::UnmergeableFile)
       end
 
       # All text from Rugged has an encoding of ASCII_8BIT, so force that in
@@ -214,7 +216,7 @@ CONFLICT
       context 'when the file contains non-UTF-8 characters' do
         it 'raises UnsupportedEncoding' do
           expect { parse_text("a\xC4\xFC".force_encoding(Encoding::ASCII_8BIT)) }
-            .to raise_error(Gitlab::Conflict::Parser::UnsupportedEncoding)
+            .to raise_error(Gitlab::Git::ConflictParser::UnsupportedEncoding)
         end
       end
     end

spec/services/merge_requests/conflicts/list_service_spec.rb

@@ -35,7 +35,7 @@ describe MergeRequests::Conflicts::ListService do
     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)
+      allow_any_instance_of(Rugged::Repository).to receive(:merge_commits).and_raise(Rugged::OdbError)
 
       expect(service.can_be_resolved_in_ui?).to be_falsey
     end

spec/services/merge_requests/conflicts/resolve_service_spec.rb

@@ -107,25 +107,27 @@ describe MergeRequests::Conflicts::ResolveService do
             branch_name: 'conflict-start')
         end
 
-        def resolve_conflicts
+        subject do
           described_class.new(merge_request_from_fork).execute(user, params)
         end
 
         it 'gets conflicts from the source project' do
+          # REFACTOR NOTE: We used to test that `project.repository.rugged` wasn't
+          # used in this case, but since the refactor, for simplification,
+          # we always use that repository for read only operations.
           expect(forked_project.repository.rugged).to receive(:merge_commits).and_call_original
-          expect(project.repository.rugged).not_to receive(:merge_commits)
 
-          resolve_conflicts
+          subject
         end
 
         it 'creates a commit with the message' do
-          resolve_conflicts
+          subject
 
           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
+          subject
 
           expect(merge_request_from_fork.source_branch_head.parents.map(&:id))
             .to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813', target_head])
@@ -200,14 +202,19 @@ describe MergeRequests::Conflicts::ResolveService do
         }
       end
 
-      it 'raises a MissingResolution error' do
+      it 'raises a ResolutionError error' do
         expect { service.execute(user, invalid_params) }
-          .to raise_error(Gitlab::Conflict::File::MissingResolution)
+          .to raise_error(Gitlab::Git::Merge::ResolutionError)
       end
     end
 
     context 'when the content of a file is unchanged' do
-      let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) }
+      let(:merge) do
+        MergeRequests::Conflicts::ListService.new(merge_request).conflicts.merge
+      end
+      let(:regex_conflict) do
+        merge.conflict_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb')
+      end
 
       let(:invalid_params) do
         {
@@ -219,16 +226,16 @@ describe MergeRequests::Conflicts::ResolveService do
             }, {
               old_path: 'files/ruby/regex.rb',
               new_path: 'files/ruby/regex.rb',
-              content: list_service.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
+              content: regex_conflict.content
             }
           ],
           commit_message: 'This is a commit message!'
         }
       end
 
-      it 'raises a MissingResolution error' do
+      it 'raises a ResolutionError error' do
         expect { service.execute(user, invalid_params) }
-          .to raise_error(Gitlab::Conflict::File::MissingResolution)
+          .to raise_error(Gitlab::Git::Merge::ResolutionError)
       end
     end
 
@@ -246,9 +253,9 @@ describe MergeRequests::Conflicts::ResolveService do
         }
       end
 
-      it 'raises a MissingFiles error' do
+      it 'raises a ResolutionError error' do
         expect { service.execute(user, invalid_params) }
-          .to raise_error(Gitlab::Conflict::FileCollection::MissingFiles)
+          .to raise_error(Gitlab::Git::Merge::ResolutionError)
       end
     end
   end