Merge branch 'feature/migrate-revert-to-gitaly' into 'master'

Migrate Gitlab::Git::Repository#revert to Gitaly

Closes gitaly#780

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

GITALY_SERVER_VERSION

@@ -1 +1 @@
-0.56.0
+0.57.0

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
 

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)

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|
+        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
+          }
 
-          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])
+          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

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

spec/models/repository_spec.rb

@@ -1372,38 +1372,48 @@ 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
+
+      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)
+        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
+        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
+
+          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 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)
-      end
+    context 'when Gitaly revert feature is enabled' do
+      it_behaves_like 'reverting a commit'
     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
-      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
-
-        repository.revert(user, merge_commit, 'master', message)
-        expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present
-      end
+    context 'when Gitaly revert feature is disabled', :disable_gitaly do
+      it_behaves_like 'reverting a commit'
     end
   end