Merge branch 'gitaly-conflict-resolver' into 'master'

Gitaly conflict resolver

Closes gitaly#813

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

app/services/merge_requests/conflicts/list_service.rb

@@ -23,7 +23,7 @@ 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::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing
+        rescue Gitlab::Git::CommandError, Gitlab::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing
           @conflicts_can_be_resolved_in_ui = false
         end
       end

lib/gitlab/conflict/file_collection.rb

@@ -13,12 +13,13 @@ module Gitlab
       end
 
       def resolve(user, commit_message, files)
+        msg = commit_message || default_commit_message
+        resolution = Gitlab::Git::Conflict::Resolution.new(user, files, msg)
         args = {
           source_branch: merge_request.source_branch,
-          target_branch: merge_request.target_branch,
-          commit_message: commit_message || default_commit_message
+          target_branch: merge_request.target_branch
         }
-        resolver.resolve_conflicts(@source_repo, user, files, args)
+        resolver.resolve_conflicts(@source_repo, resolution, args)
       ensure
         @merge_request.clear_memoized_shas
       end

lib/gitlab/git/conflict/file.rb

@@ -2,7 +2,9 @@ module Gitlab
   module Git
     module Conflict
       class File
-        attr_reader :content, :their_path, :our_path, :our_mode, :repository, :commit_oid
+        attr_reader :their_path, :our_path, :our_mode, :repository, :commit_oid
+
+        attr_accessor :content
 
         def initialize(repository, commit_oid, conflict, content)
           @repository = repository

lib/gitlab/git/conflict/resolution.rb

+module Gitlab
+  module Git
+    module Conflict
+      class Resolution
+        attr_reader :user, :files, :commit_message
+
+        def initialize(user, files, commit_message)
+          @user = user
+          @files = files
+          @commit_message = commit_message
+        end
+      end
+    end
+  end
+end

lib/gitlab/git/conflict/resolver.rb

@@ -13,37 +13,27 @@ module Gitlab
 
         def conflicts
           @conflicts ||= begin
-            target_index = @target_repository.rugged.merge_commits(@our_commit_oid, @their_commit_oid)
-
-            # We don't need to do `with_repo_branch_commit` here, because the target
-            # project always fetches source refs when creating merge request diffs.
-            conflict_files(@target_repository, target_index)
+            @target_repository.gitaly_migrate(:conflicts_list_conflict_files) do |is_enabled|
+              if is_enabled
+                gitaly_conflicts_client(@target_repository).list_conflict_files
+              else
+                rugged_list_conflict_files
               end
             end
-
-        def resolve_conflicts(source_repository, user, files, source_branch:, target_branch:, commit_message:)
-          source_repository.with_repo_branch_commit(@target_repository, target_branch) do
-            index = source_repository.rugged.merge_commits(@our_commit_oid, @their_commit_oid)
-            conflicts = conflict_files(source_repository, index)
-
-            files.each do |file_params|
-              conflict_file = conflict_for_path(conflicts, file_params[:old_path], file_params[:new_path])
-
-              write_resolved_file_to_index(source_repository, 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(', ')}"
+        rescue GRPC::FailedPrecondition => e
+          raise Gitlab::Git::Conflict::Resolver::ConflictSideMissing.new(e.message)
+        rescue Rugged::OdbError, GRPC::BadStatus => e
+          raise Gitlab::Git::CommandError.new(e)
         end
 
-            commit_params = {
-              message: commit_message,
-              parents: [@our_commit_oid, @their_commit_oid]
-            }
-
-            source_repository.commit_index(user, source_branch, index, commit_params)
+        def resolve_conflicts(source_repository, resolution, source_branch:, target_branch:)
+          source_repository.gitaly_migrate(:conflicts_resolve_conflicts) do |is_enabled|
+            if is_enabled
+              gitaly_conflicts_client(source_repository).resolve_conflicts(@target_repository, resolution, source_branch, target_branch)
+            else
+              rugged_resolve_conflicts(source_repository, resolution, source_branch, target_branch)
+            end
           end
         end
 
@@ -68,6 +58,10 @@ module Gitlab
           end
         end
 
+        def gitaly_conflicts_client(repository)
+          repository.gitaly_conflicts_client(@our_commit_oid, @their_commit_oid)
+        end
+
         def write_resolved_file_to_index(repository, index, file, params)
           if params[:sections]
             resolved_lines = file.resolve_lines(params[:sections])
@@ -84,6 +78,40 @@ module Gitlab
           index.add(path: our_path, oid: oid, mode: file.our_mode)
           index.conflict_remove(our_path)
         end
+
+        def rugged_list_conflict_files
+          target_index = @target_repository.rugged.merge_commits(@our_commit_oid, @their_commit_oid)
+
+          # We don't need to do `with_repo_branch_commit` here, because the target
+          # project always fetches source refs when creating merge request diffs.
+          conflict_files(@target_repository, target_index)
+        end
+
+        def rugged_resolve_conflicts(source_repository, resolution, source_branch, target_branch)
+          source_repository.with_repo_branch_commit(@target_repository, target_branch) do
+            index = source_repository.rugged.merge_commits(@our_commit_oid, @their_commit_oid)
+            conflicts = conflict_files(source_repository, index)
+
+            resolution.files.each do |file_params|
+              conflict_file = conflict_for_path(conflicts, file_params[:old_path], file_params[:new_path])
+
+              write_resolved_file_to_index(source_repository, 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: resolution.commit_message,
+              parents: [@our_commit_oid, @their_commit_oid]
+            }
+
+            source_repository.commit_index(resolution.user, source_branch, index, commit_params)
+          end
+        end
       end
     end
   end

lib/gitlab/git/repository.rb

@@ -1294,6 +1294,10 @@ module Gitlab
         @gitaly_remote_client ||= Gitlab::GitalyClient::RemoteService.new(self)
       end
 
+      def gitaly_conflicts_client(our_commit_oid, their_commit_oid)
+        Gitlab::GitalyClient::ConflictsService.new(self, our_commit_oid, their_commit_oid)
+      end
+
       def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
         Gitlab::GitalyClient.migrate(method, status: status, &block)
       rescue GRPC::NotFound => e

lib/gitlab/gitaly_client/conflicts_service.rb

+module Gitlab
+  module GitalyClient
+    class ConflictsService
+      MAX_MSG_SIZE = 128.kilobytes.freeze
+
+      def initialize(repository, our_commit_oid, their_commit_oid)
+        @gitaly_repo = repository.gitaly_repository
+        @repository = repository
+        @our_commit_oid = our_commit_oid
+        @their_commit_oid = their_commit_oid
+      end
+
+      def list_conflict_files
+        request = Gitaly::ListConflictFilesRequest.new(
+          repository: @gitaly_repo,
+          our_commit_oid: @our_commit_oid,
+          their_commit_oid: @their_commit_oid
+        )
+        response = GitalyClient.call(@repository.storage, :conflicts_service, :list_conflict_files, request)
+
+        files_from_response(response).to_a
+      end
+
+      def resolve_conflicts(target_repository, resolution, source_branch, target_branch)
+        reader = GitalyClient.binary_stringio(resolution.files.to_json)
+
+        req_enum = Enumerator.new do |y|
+          header = resolve_conflicts_request_header(target_repository, resolution, source_branch, target_branch)
+          y.yield Gitaly::ResolveConflictsRequest.new(header: header)
+
+          until reader.eof?
+            chunk = reader.read(MAX_MSG_SIZE)
+
+            y.yield Gitaly::ResolveConflictsRequest.new(files_json: chunk)
+          end
+        end
+
+        response = GitalyClient.call(@repository.storage, :conflicts_service, :resolve_conflicts, req_enum, remote_storage: target_repository.storage)
+
+        if response.resolution_error.present?
+          raise Gitlab::Git::Conflict::Resolver::ResolutionError, response.resolution_error
+        end
+      end
+
+      private
+
+      def resolve_conflicts_request_header(target_repository, resolution, source_branch, target_branch)
+        Gitaly::ResolveConflictsRequestHeader.new(
+          repository: @gitaly_repo,
+          our_commit_oid: @our_commit_oid,
+          target_repository: target_repository.gitaly_repository,
+          their_commit_oid: @their_commit_oid,
+          source_branch: source_branch,
+          target_branch: target_branch,
+          commit_message: resolution.commit_message,
+          user: Gitlab::Git::User.from_gitlab(resolution.user).to_gitaly
+        )
+      end
+
+      def files_from_response(response)
+        files = []
+
+        response.each do |msg|
+          msg.files.each do |gitaly_file|
+            if gitaly_file.header
+              files << file_from_gitaly_header(gitaly_file.header)
+            else
+              files.last.content << gitaly_file.content
+            end
+          end
+        end
+
+        files
+      end
+
+      def file_from_gitaly_header(header)
+        Gitlab::Git::Conflict::File.new(
+          Gitlab::GitalyClient::Util.git_repository(header.repository),
+          header.commit_oid,
+          conflict_from_gitaly_file_header(header),
+          ''
+        )
+      end
+
+      def conflict_from_gitaly_file_header(header)
+        {
+          ours: { path: header.our_path, mode: header.our_mode },
+          theirs: { path: header.their_path }
+        }
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client/util.rb

@@ -18,6 +18,12 @@ module Gitlab
           )
         end
 
+        def git_repository(gitaly_repository)
+          Gitlab::Git::Repository.new(gitaly_repository.storage_name,
+                                      gitaly_repository.relative_path,
+                                      gitaly_repository.gl_repository)
+        end
+
         def gitlab_tag_from_gitaly_tag(repository, gitaly_tag)
           if gitaly_tag.target_commit.present?
             commit = Gitlab::Git::Commit.decorate(repository, gitaly_tag.target_commit)

spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb

+require 'spec_helper'
+
+describe Gitlab::GitalyClient::ConflictsService do
+  let(:project) { create(:project, :repository) }
+  let(:target_project) { create(:project, :repository) }
+  let(:source_repository) { project.repository.raw }
+  let(:target_repository) { target_project.repository.raw }
+  let(:target_gitaly_repository) { target_repository.gitaly_repository }
+  let(:our_commit_oid) { 'f00' }
+  let(:their_commit_oid) { 'f44' }
+  let(:client) do
+    described_class.new(target_repository, our_commit_oid, their_commit_oid)
+  end
+
+  describe '#list_conflict_files' do
+    let(:request) do
+      Gitaly::ListConflictFilesRequest.new(
+        repository: target_gitaly_repository, our_commit_oid: our_commit_oid,
+        their_commit_oid: their_commit_oid
+      )
+    end
+    let(:our_path) { 'our/path' }
+    let(:their_path) { 'their/path' }
+    let(:our_mode) { 0744 }
+    let(:header) do
+      double(repository: target_gitaly_repository, commit_oid: our_commit_oid,
+             our_path: our_path, our_mode: 0744, their_path: their_path)
+    end
+    let(:response) do
+      [
+        double(files: [double(header: header), double(content: 'foo', header: nil)]),
+        double(files: [double(content: 'bar', header: nil)])
+      ]
+    end
+    let(:file) { subject[0] }
+
+    subject { client.list_conflict_files }
+
+    it 'sends an RPC request' do
+      expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
+        .with(request, kind_of(Hash)).and_return([])
+
+      subject
+    end
+
+    it 'forms a Gitlab::Git::ConflictFile collection from the response' do
+      allow_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
+        .with(request, kind_of(Hash)).and_return(response)
+
+      expect(subject.size).to be(1)
+      expect(file.content).to eq('foobar')
+      expect(file.their_path).to eq(their_path)
+      expect(file.our_path).to eq(our_path)
+      expect(file.our_mode).to be(our_mode)
+      expect(file.repository).to eq(target_repository)
+      expect(file.commit_oid).to eq(our_commit_oid)
+    end
+  end
+
+  describe '#resolve_conflicts' do
+    let(:user) { create(:user) }
+    let(:files) do
+      [{ old_path: 'some/path', new_path: 'some/path', content: '' }]
+    end
+    let(:source_branch) { 'master' }
+    let(:target_branch) { 'feature' }
+    let(:commit_message) { 'Solving conflicts' }
+    let(:resolution) do
+      Gitlab::Git::Conflict::Resolution.new(user, files, commit_message)
+    end
+
+    subject do
+      client.resolve_conflicts(source_repository, resolution, source_branch, target_branch)
+    end
+
+    it 'sends an RPC request' do
+      expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:resolve_conflicts)
+        .with(kind_of(Enumerator), kind_of(Hash)).and_return(double(resolution_error: ""))
+
+      subject
+    end
+
+    it 'raises a relevant exception if resolution_error is present' do
+      expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:resolve_conflicts)
+        .with(kind_of(Enumerator), kind_of(Hash)).and_return(double(resolution_error: "something happened"))
+
+      expect { subject }.to raise_error(Gitlab::Git::Conflict::Resolver::ResolutionError)
+    end
+  end
+end

spec/services/merge_requests/conflicts/list_service_spec.rb

@@ -32,14 +32,6 @@ describe MergeRequests::Conflicts::ListService do
       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_any_instance_of(Rugged::Repository).to receive(:merge_commits).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)
@@ -76,5 +68,23 @@ describe MergeRequests::Conflicts::ListService do
 
       expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
     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_any_instance_of(Gitlab::GitalyClient::ConflictsService).to receive(:list_conflict_files).and_raise(GRPC::Unknown)
+
+      expect(service.can_be_resolved_in_ui?).to be_falsey
+    end
+
+    context 'with gitaly disabled', :skip_gitaly_mock 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_any_instance_of(Rugged::Repository).to receive(:merge_commits).and_raise(Rugged::OdbError)
+
+        expect(service.can_be_resolved_in_ui?).to be_falsey
+      end
+    end
   end
 end

spec/services/merge_requests/conflicts/resolve_service_spec.rb

@@ -111,15 +111,6 @@ describe MergeRequests::Conflicts::ResolveService 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
-
-          subject
-        end
-
         it 'creates a commit with the message' do
           subject
 
@@ -132,6 +123,17 @@ describe MergeRequests::Conflicts::ResolveService do
           expect(merge_request_from_fork.source_branch_head.parents.map(&:id))
             .to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813', target_head])
         end
+
+        context 'when gitaly is disabled', :skip_gitaly_mock do
+          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
+
+            subject
+          end
+        end
       end
     end