Merge branch 'conflict-resolution-refactor' into 'master'

Conflict resolution refactor

See merge request gitlab-org/gitlab-ce!14747

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::Conflict::Parser).to receive(:parse)
+          .and_raise(Gitlab::Git::Conflict::Parser::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::Conflict::Parser).to receive(:parse)
+          .and_raise(Gitlab::Git::Conflict::Parser::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::Conflict::File.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::Conflict::Resolver::ResolutionError)
       expect { conflict_file.resolve_lines(empty_hash) }
-        .to raise_error(Gitlab::Conflict::File::MissingResolution)
+        .to raise_error(Gitlab::Git::Conflict::Resolver::ResolutionError)
       expect { conflict_file.resolve_lines(invalid_hash) }
-        .to raise_error(Gitlab::Conflict::File::MissingResolution)
+        .to raise_error(Gitlab::Git::Conflict::Resolver::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/diff/position_spec.rb

@@ -40,7 +40,7 @@ describe Gitlab::Diff::Position do
     describe "#line_code" do
       it "returns the correct line code" do
-        line_code = Gitlab::Diff::LineCode.generate(subject.file_path, subject.new_line, 0)
+        line_code = Gitlab::Git.diff_line_code(subject.file_path, subject.new_line, 0)
         expect(subject.line_code(project.repository)).to eq(line_code)
       end
@@ -108,7 +108,7 @@ describe Gitlab::Diff::Position do
       describe "#line_code" do
         it "returns the correct line code" do
-          line_code = Gitlab::Diff::LineCode.generate(subject.file_path, subject.new_line, 15)
+          line_code = Gitlab::Git.diff_line_code(subject.file_path, subject.new_line, 15)
           expect(subject.line_code(project.repository)).to eq(line_code)
         end
@@ -149,7 +149,7 @@ describe Gitlab::Diff::Position do
       describe "#line_code" do
         it "returns the correct line code" do
-          line_code = Gitlab::Diff::LineCode.generate(subject.file_path, subject.new_line, subject.old_line)
+          line_code = Gitlab::Git.diff_line_code(subject.file_path, subject.new_line, subject.old_line)
           expect(subject.line_code(project.repository)).to eq(line_code)
         end
@@ -189,7 +189,7 @@ describe Gitlab::Diff::Position do
       describe "#line_code" do
         it "returns the correct line code" do
-          line_code = Gitlab::Diff::LineCode.generate(subject.file_path, 13, subject.old_line)
+          line_code = Gitlab::Git.diff_line_code(subject.file_path, 13, subject.old_line)
           expect(subject.line_code(project.repository)).to eq(line_code)
         end
@@ -233,7 +233,7 @@ describe Gitlab::Diff::Position do
       describe "#line_code" do
         it "returns the correct line code" do
-          line_code = Gitlab::Diff::LineCode.generate(subject.file_path, subject.new_line, 5)
+          line_code = Gitlab::Git.diff_line_code(subject.file_path, subject.new_line, 5)
           expect(subject.line_code(project.repository)).to eq(line_code)
         end
@@ -274,7 +274,7 @@ describe Gitlab::Diff::Position do
       describe "#line_code" do
         it "returns the correct line code" do
-          line_code = Gitlab::Diff::LineCode.generate(subject.file_path, subject.new_line, subject.old_line)
+          line_code = Gitlab::Git.diff_line_code(subject.file_path, subject.new_line, subject.old_line)
           expect(subject.line_code(project.repository)).to eq(line_code)
         end
@@ -314,7 +314,7 @@ describe Gitlab::Diff::Position do
       describe "#line_code" do
         it "returns the correct line code" do
-          line_code = Gitlab::Diff::LineCode.generate(subject.file_path, 4, subject.old_line)
+          line_code = Gitlab::Git.diff_line_code(subject.file_path, 4, subject.old_line)
           expect(subject.line_code(project.repository)).to eq(line_code)
         end
@@ -357,7 +357,7 @@ describe Gitlab::Diff::Position do
     describe "#line_code" do
       it "returns the correct line code" do
-        line_code = Gitlab::Diff::LineCode.generate(subject.file_path, 0, subject.old_line)
+        line_code = Gitlab::Git.diff_line_code(subject.file_path, 0, subject.old_line)
         expect(subject.line_code(project.repository)).to eq(line_code)
       end
@@ -399,7 +399,7 @@ describe Gitlab::Diff::Position do
     describe "#line_code" do
       it "returns the correct line code" do
-        line_code = Gitlab::Diff::LineCode.generate(subject.file_path, subject.new_line, 0)
+        line_code = Gitlab::Git.diff_line_code(subject.file_path, subject.new_line, 0)
         expect(subject.line_code(project.repository)).to eq(line_code)
       end
@@ -447,7 +447,7 @@ describe Gitlab::Diff::Position do
     describe "#line_code" do
       it "returns the correct line code" do
-        line_code = Gitlab::Diff::LineCode.generate(subject.file_path, 0, subject.old_line)
+        line_code = Gitlab::Git.diff_line_code(subject.file_path, 0, subject.old_line)
         expect(subject.line_code(project.repository)).to eq(line_code)
       end

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::Conflict::Parser 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::Conflict::Parser::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::Conflict::Parser::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::Conflict::Parser::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::Conflict::Parser::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::Conflict::Parser::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::Conflict::Parser::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::Conflict::Parser::MissingEndDelimiter)
         expect { parse_text(start_text + '>>>>>>> some-other-path.md') }
-          .to raise_error(Gitlab::Conflict::Parser::MissingEndDelimiter)
+          .to raise_error(Gitlab::Git::Conflict::Parser::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::Conflict::Parser::UnmergeableFile)
         expect { parse_text(nil) }
-          .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile)
+          .to raise_error(Gitlab::Git::Conflict::Parser::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::Conflict::Parser::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::Conflict::Parser::UnsupportedEncoding)
         end
       end
     end

spec/models/diff_note_spec.rb

@@ -105,7 +105,7 @@ describe DiffNote do
   describe "#line_code" do
     it "returns the correct line code" do
-      line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.formatter.new_line, 15)
+      line_code = Gitlab::Git.diff_line_code(position.file_path, position.formatter.new_line, 15)
       expect(subject.line_code).to eq(line_code)
     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::Conflict::Resolver::ResolutionError)
       end
     end
     context 'when the content of a file is unchanged' do
-      let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) }
+      let(:resolver) do
+        MergeRequests::Conflicts::ListService.new(merge_request).conflicts.resolver
+      end
+      let(:regex_conflict) do
+        resolver.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::Conflict::Resolver::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(described_class::MissingFiles)
+          .to raise_error(Gitlab::Git::Conflict::Resolver::ResolutionError)
       end
     end
   end