From 887a3739163c32fd5b815dcbba4a59fc69b6db23 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Mon, 4 Dec 2017 14:13:22 +0100 Subject: Migrate Gitlab::Git::Repository#revert to Gitaly Closes gitaly#780 --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +- lib/gitlab/git/repository.rb | 53 +++++++++++++++++--------- lib/gitlab/gitaly_client/operation_service.rb | 32 +++++++++++++++- spec/models/repository_spec.rb | 54 ++++++++++++++++----------- 6 files changed, 102 insertions(+), 45 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 2f4c74eb2dd..46448c71b9d 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.56.0 +0.57.0 diff --git a/Gemfile b/Gemfile index 044778498a2..222e63904a0 100644 --- a/Gemfile +++ b/Gemfile @@ -400,7 +400,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.58.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.59.0', require: 'gitaly' gem 'toml-rb', '~> 0.3.15', require: false diff --git a/Gemfile.lock b/Gemfile.lock index fdb81b101f5..48e46e3fab0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -276,7 +276,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly-proto (0.58.0) + gitaly-proto (0.59.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -1037,7 +1037,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly-proto (~> 0.58.0) + gitaly-proto (~> 0.59.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.6.2) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index eab04bcac65..1468069a991 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -776,24 +776,21 @@ module Gitlab end def revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) - OperationService.new(user, self).with_branch( - branch_name, - start_branch_name: start_branch_name, - start_repository: start_repository - ) do |start_commit| - - Gitlab::Git.check_namespace!(commit, start_repository) - - revert_tree_id = check_revert_content(commit, start_commit.sha) - raise CreateTreeError unless revert_tree_id - - committer = user_to_committer(user) + gitaly_migrate(:revert) do |is_enabled| + args = { + user: user, + commit: commit, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_repository + } - create_commit(message: message, - author: committer, - committer: committer, - tree: revert_tree_id, - parents: [start_commit.sha]) + if is_enabled + gitaly_operations_client.user_revert(args) + else + rugged_revert(args) + end end end @@ -1769,6 +1766,28 @@ module Gitlab end end + def rugged_revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + OperationService.new(user, self).with_branch( + branch_name, + start_branch_name: start_branch_name, + start_repository: start_repository + ) do |start_commit| + + Gitlab::Git.check_namespace!(commit, start_repository) + + revert_tree_id = check_revert_content(commit, start_commit.sha) + raise CreateTreeError unless revert_tree_id + + committer = user_to_committer(user) + + create_commit(message: message, + author: committer, + committer: committer, + tree: revert_tree_id, + parents: [start_commit.sha]) + end + end + def gitaly_add_branch(branch_name, user, target) gitaly_operation_client.user_create_branch(branch_name, user, target) rescue GRPC::FailedPrecondition => ex diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 51de6a9a75d..400a4af363b 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -124,7 +124,31 @@ module Gitlab end def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) - request = Gitaly::UserCherryPickRequest.new( + call_cherry_pick_or_revert(:cherry_pick, + user: user, + commit: commit, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_repository) + end + + def user_revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + call_cherry_pick_or_revert(:revert, + user: user, + commit: commit, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_repository) + end + + private + + def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + request_class = "Gitaly::User#{rpc.to_s.camelcase}Request".constantize + + request = request_class.new( repository: @gitaly_repo, user: Gitlab::Git::User.from_gitlab(user).to_gitaly, commit: commit.to_gitaly_commit, @@ -137,11 +161,15 @@ module Gitlab response = GitalyClient.call( @repository.storage, :operation_service, - :user_cherry_pick, + :"user_#{rpc}", request, remote_storage: start_repository.storage ) + handle_cherry_pick_or_revert_response(response) + end + + def handle_cherry_pick_or_revert_response(response) if response.pre_receive_error.presence raise Gitlab::Git::HooksService::PreReceiveError, response.pre_receive_error elsif response.commit_error.presence diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index af0c86abe86..d37e3d2c527 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1372,39 +1372,49 @@ describe Repository do end describe '#revert' do - let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } - let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } - let(:message) { 'revert message' } + shared_examples 'reverting a commit' do + let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } + let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:message) { 'revert message' } - context 'when there is a conflict' do - it 'raises an error' do - expect { repository.revert(user, new_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + context 'when there is a conflict' do + it 'raises an error' do + expect { repository.revert(user, new_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + end end - end - context 'when commit was already reverted' do - it 'raises an error' do - repository.revert(user, update_image_commit, 'master', message) + context 'when commit was already reverted' do + it 'raises an error' do + repository.revert(user, update_image_commit, 'master', message) - expect { repository.revert(user, update_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + expect { repository.revert(user, update_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + end end - end - context 'when commit can be reverted' do - it 'reverts the changes' do - expect(repository.revert(user, update_image_commit, 'master', message)).to be_truthy + context 'when commit can be reverted' do + it 'reverts the changes' do + expect(repository.revert(user, update_image_commit, 'master', message)).to be_truthy + end end - end - context 'reverting a merge commit' do - it 'reverts the changes' do - merge_commit - expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present + context 'reverting a merge commit' do + it 'reverts the changes' do + merge_commit + expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present - repository.revert(user, merge_commit, 'master', message) - expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present + repository.revert(user, merge_commit, 'master', message) + expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present + end end end + + context 'when Gitaly revert feature is enabled' do + it_behaves_like 'reverting a commit' + end + + context 'when Gitaly revert feature is disabled', :disable_gitaly do + it_behaves_like 'reverting a commit' + end end describe '#cherry_pick' do -- cgit v1.2.1