From b4b267b7395ca524f4251f6eefe91e502b086ab0 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Tue, 9 Jan 2018 13:53:35 +0100 Subject: [PATCH] Migrate Repository#can_be_merged? to Gitaly --- app/models/repository.rb | 21 +++++--- lib/gitlab/git/conflict/resolver.rb | 2 +- .../gitaly_client/conflict_files_stitcher.rb | 47 ++++++++++++++++ lib/gitlab/gitaly_client/conflicts_service.rb | 38 ++----------- .../conflict_files_stitcher_spec.rb | 54 +++++++++++++++++++ .../gitaly_client/conflicts_service_spec.rb | 33 +----------- spec/models/repository_spec.rb | 44 +++++++++------ 7 files changed, 150 insertions(+), 89 deletions(-) create mode 100644 lib/gitlab/gitaly_client/conflict_files_stitcher.rb create mode 100644 spec/lib/gitlab/gitaly_client/conflict_files_stitcher_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index 9c879e2006b..b5d118f1c4c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -831,13 +831,12 @@ class Repository end def can_be_merged?(source_sha, target_branch) - our_commit = rugged.branches[target_branch].target - their_commit = rugged.lookup(source_sha) - - if our_commit && their_commit - !rugged.merge_commits(our_commit, their_commit).conflicts? - else - false + raw_repository.gitaly_migrate(:can_be_merged) do |is_enabled| + if is_enabled + gitaly_can_be_merged?(source_sha, find_branch(target_branch).target) + else + rugged_can_be_merged?(source_sha, target_branch) + end end end @@ -1128,6 +1127,14 @@ class Repository Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', Gitlab::GlRepository.gl_repository(project, is_wiki)) end + def gitaly_can_be_merged?(their_commit, our_commit) + !raw_repository.gitaly_conflicts_client(our_commit, their_commit).conflicts? + end + + def rugged_can_be_merged?(their_commit, our_commit) + !rugged.merge_commits(our_commit, their_commit).conflicts? + end + def find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) ref ||= root_ref diff --git a/lib/gitlab/git/conflict/resolver.rb b/lib/gitlab/git/conflict/resolver.rb index 74c9874d590..07b7e811a34 100644 --- a/lib/gitlab/git/conflict/resolver.rb +++ b/lib/gitlab/git/conflict/resolver.rb @@ -15,7 +15,7 @@ module Gitlab @conflicts ||= begin @target_repository.gitaly_migrate(:conflicts_list_conflict_files) do |is_enabled| if is_enabled - gitaly_conflicts_client(@target_repository).list_conflict_files + gitaly_conflicts_client(@target_repository).list_conflict_files.to_a else rugged_list_conflict_files end diff --git a/lib/gitlab/gitaly_client/conflict_files_stitcher.rb b/lib/gitlab/gitaly_client/conflict_files_stitcher.rb new file mode 100644 index 00000000000..97c13d1fdb0 --- /dev/null +++ b/lib/gitlab/gitaly_client/conflict_files_stitcher.rb @@ -0,0 +1,47 @@ +module Gitlab + module GitalyClient + class ConflictFilesStitcher + include Enumerable + + def initialize(rpc_response) + @rpc_response = rpc_response + end + + def each + current_file = nil + + @rpc_response.each do |msg| + msg.files.each do |gitaly_file| + if gitaly_file.header + yield current_file if current_file + + current_file = file_from_gitaly_header(gitaly_file.header) + else + current_file.content << gitaly_file.content + end + end + end + + yield current_file if current_file + end + + private + + 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/conflicts_service.rb b/lib/gitlab/gitaly_client/conflicts_service.rb index 40f032cf873..2565d537aff 100644 --- a/lib/gitlab/gitaly_client/conflicts_service.rb +++ b/lib/gitlab/gitaly_client/conflicts_service.rb @@ -20,7 +20,11 @@ module Gitlab ) response = GitalyClient.call(@repository.storage, :conflicts_service, :list_conflict_files, request) - files_from_response(response).to_a + GitalyClient::ConflictFilesStitcher.new(response) + end + + def conflicts? + list_conflict_files.any? end def resolve_conflicts(target_repository, resolution, source_branch, target_branch) @@ -58,38 +62,6 @@ module Gitlab 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/spec/lib/gitlab/gitaly_client/conflict_files_stitcher_spec.rb b/spec/lib/gitlab/gitaly_client/conflict_files_stitcher_spec.rb new file mode 100644 index 00000000000..1c933410bd5 --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/conflict_files_stitcher_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient::ConflictFilesStitcher do + describe 'enumeration' do + it 'combines segregated ConflictFile messages together' do + target_project = create(:project, :repository) + target_repository = target_project.repository.raw + target_gitaly_repository = target_repository.gitaly_repository + + our_path_1 = 'our/path/1' + their_path_1 = 'their/path/1' + our_mode_1 = 0744 + commit_oid_1 = 'f00' + content_1 = 'content of the first file' + + our_path_2 = 'our/path/2' + their_path_2 = 'their/path/2' + our_mode_2 = 0600 + commit_oid_2 = 'ba7' + content_2 = 'content of the second file' + + header_1 = double(repository: target_gitaly_repository, commit_oid: commit_oid_1, + our_path: our_path_1, their_path: their_path_1, our_mode: our_mode_1) + header_2 = double(repository: target_gitaly_repository, commit_oid: commit_oid_2, + our_path: our_path_2, their_path: their_path_2, our_mode: our_mode_2) + + messages = [ + double(files: [double(header: header_1), double(header: nil, content: content_1[0..5])]), + double(files: [double(header: nil, content: content_1[6..-1])]), + double(files: [double(header: header_2)]), + double(files: [double(header: nil, content: content_2[0..5]), double(header: nil, content: content_2[6..10])]), + double(files: [double(header: nil, content: content_2[11..-1])]) + ] + + conflict_files = described_class.new(messages).to_a + + expect(conflict_files.size).to be(2) + + expect(conflict_files[0].content).to eq(content_1) + expect(conflict_files[0].their_path).to eq(their_path_1) + expect(conflict_files[0].our_path).to eq(our_path_1) + expect(conflict_files[0].our_mode).to be(our_mode_1) + expect(conflict_files[0].repository).to eq(target_repository) + expect(conflict_files[0].commit_oid).to eq(commit_oid_1) + + expect(conflict_files[1].content).to eq(content_2) + expect(conflict_files[1].their_path).to eq(their_path_2) + expect(conflict_files[1].our_path).to eq(our_path_2) + expect(conflict_files[1].our_mode).to be(our_mode_2) + expect(conflict_files[1].repository).to eq(target_repository) + expect(conflict_files[1].commit_oid).to eq(commit_oid_2) + end + end +end diff --git a/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb b/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb index b9641de7eda..e4fe01a671f 100644 --- a/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb @@ -19,41 +19,12 @@ describe Gitlab::GitalyClient::ConflictsService do 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([]) + .with(request, kind_of(Hash)).and_return([].to_enum) - 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) + client.list_conflict_files end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index c0db2c1b386..859b53ac560 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -358,28 +358,38 @@ describe Repository do end describe '#can_be_merged?' do - context 'mergeable branches' do - subject { repository.can_be_merged?('0b4bc9a49b562e85de7cc9e834518ea6828729b9', 'master') } + shared_examples 'can be merged' do + context 'mergeable branches' do + subject { repository.can_be_merged?('0b4bc9a49b562e85de7cc9e834518ea6828729b9', 'master') } - it { is_expected.to be_truthy } + it { is_expected.to be_truthy } + end + + context 'non-mergeable branches' do + subject { repository.can_be_merged?('bb5206fee213d983da88c47f9cf4cc6caf9c66dc', 'feature') } + + it { is_expected.to be_falsey } + end + + context 'non merged branch' do + subject { repository.merged_to_root_ref?('fix') } + + it { is_expected.to be_falsey } + end + + context 'non existent branch' do + subject { repository.merged_to_root_ref?('non_existent_branch') } + + it { is_expected.to be_nil } + end end - context 'non-mergeable branches' do - subject { repository.can_be_merged?('bb5206fee213d983da88c47f9cf4cc6caf9c66dc', 'feature') } - - it { is_expected.to be_falsey } + context 'when Gitaly can_be_merged feature is enabled' do + it_behaves_like 'can be merged' end - context 'non merged branch' do - subject { repository.merged_to_root_ref?('fix') } - - it { is_expected.to be_falsey } - end - - context 'non existent branch' do - subject { repository.merged_to_root_ref?('non_existent_branch') } - - it { is_expected.to be_nil } + context 'when Gitaly can_be_merged feature is disabled', :disable_gitaly do + it_behaves_like 'can be merged' end end