diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2018-01-03 08:39:16 +0000 |
---|---|---|
committer | Grzegorz Bizon <grzegorz@gitlab.com> | 2018-01-03 08:39:16 +0000 |
commit | 57906a532227c86acac0e9da0ae918967b592f06 (patch) | |
tree | 2bfc7720c0b38fa21aea0f58a2f5aa3a055398b1 | |
parent | 305038e53aa4366ac6a27013787ec0e51d1f3b67 (diff) | |
parent | 65e3a1e9e9a316a2bbbcd49e22b858b8b1bd9890 (diff) | |
download | gitlab-ce-57906a532227c86acac0e9da0ae918967b592f06.tar.gz |
Merge branch 'gitaly-conflict-resolver' into 'master'
Gitaly conflict resolver
Closes gitaly#813
See merge request gitlab-org/gitlab-ce!15755
-rw-r--r-- | app/services/merge_requests/conflicts/list_service.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/conflict/file_collection.rb | 7 | ||||
-rw-r--r-- | lib/gitlab/git/conflict/file.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/git/conflict/resolution.rb | 15 | ||||
-rw-r--r-- | lib/gitlab/git/conflict/resolver.rb | 82 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/conflicts_service.rb | 93 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/util.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb | 90 | ||||
-rw-r--r-- | spec/services/merge_requests/conflicts/list_service_spec.rb | 26 | ||||
-rw-r--r-- | spec/services/merge_requests/conflicts/resolve_service_spec.rb | 20 |
11 files changed, 300 insertions, 49 deletions
diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb index 0f677a996f7..ca9a33678e4 100644 --- a/app/services/merge_requests/conflicts/list_service.rb +++ b/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 diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index 76aee5a3deb..0a3ae2c3760 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/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 diff --git a/lib/gitlab/git/conflict/file.rb b/lib/gitlab/git/conflict/file.rb index b2a625e08fa..2a9cf10a068 100644 --- a/lib/gitlab/git/conflict/file.rb +++ b/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 diff --git a/lib/gitlab/git/conflict/resolution.rb b/lib/gitlab/git/conflict/resolution.rb new file mode 100644 index 00000000000..ab9be683e15 --- /dev/null +++ b/lib/gitlab/git/conflict/resolution.rb @@ -0,0 +1,15 @@ +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 diff --git a/lib/gitlab/git/conflict/resolver.rb b/lib/gitlab/git/conflict/resolver.rb index 03e5c0fcd6f..74c9874d590 100644 --- a/lib/gitlab/git/conflict/resolver.rb +++ b/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 end + 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 - 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(', ')}" + 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 - - commit_params = { - message: commit_message, - parents: [@our_commit_oid, @their_commit_oid] - } - - source_repository.commit_index(user, source_branch, index, commit_params) 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 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 490bd753eda..37d67b6052c 100644 --- a/lib/gitlab/git/repository.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client/conflicts_service.rb b/lib/gitlab/gitaly_client/conflicts_service.rb new file mode 100644 index 00000000000..1e631e4bd3d --- /dev/null +++ b/lib/gitlab/gitaly_client/conflicts_service.rb @@ -0,0 +1,93 @@ +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 diff --git a/lib/gitlab/gitaly_client/util.rb b/lib/gitlab/gitaly_client/util.rb index b1a033280b4..7a3fffae9b6 100644 --- a/lib/gitlab/gitaly_client/util.rb +++ b/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) diff --git a/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb b/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb new file mode 100644 index 00000000000..b9641de7eda --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb @@ -0,0 +1,90 @@ +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 diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb index 0b32c51a16f..6cadcd438c3 100644 --- a/spec/services/merge_requests/conflicts/list_service_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/conflicts/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb index e28d8d7ae5c..cff09237005 100644 --- a/spec/services/merge_requests/conflicts/resolve_service_spec.rb +++ b/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 |