Migrate Gitlab::Git::Repository#cherry_pick to Gitalyfeature/migrate-cherry-pick-to-gitaly

Closes gitaly#737

7 files changed, 144 insertions, 63 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 316ba4bd9e6..2f4c74eb2dd 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-0.55.0
+0.56.0
diff --git a/Gemfile b/Gemfile
index ad4b1e73fff..ac082bec100 100644
--- a/Gemfile
+++ b/Gemfile
@@ -400,7 +400,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.54.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.58.0', require: 'gitaly'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 3f4c930c71d..86030981936 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.54.0)
+    gitaly-proto (0.58.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -1036,7 +1036,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.54.0)
+  gitaly-proto (~> 0.58.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index d5518814483..328a54073f9 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -418,6 +418,20 @@ module Gitlab
         parent_ids.size > 1
       end
 
+      def to_gitaly_commit
+        return raw_commit if raw_commit.is_a?(Gitaly::GitCommit)
+
+        message_split = raw_commit.message.split("\n", 2)
+        Gitaly::GitCommit.new(
+          id: raw_commit.oid,
+          subject: message_split[0] ? message_split[0].chomp.b : "",
+          body: raw_commit.message.b,
+          parent_ids: raw_commit.parent_ids,
+          author: gitaly_commit_author_from_rugged(raw_commit.author),
+          committer: gitaly_commit_author_from_rugged(raw_commit.committer)
+        )
+      end
+
       private
 
       def init_from_hash(hash)
@@ -463,6 +477,14 @@ module Gitlab
       def serialize_keys
         SERIALIZE_KEYS
       end
+
+      def gitaly_commit_author_from_rugged(rugged_author)
+        Gitaly::CommitAuthor.new(
+          name: rugged_author[:name].b,
+          email: rugged_author[:email].b,
+          date: Google::Protobuf::Timestamp.new(seconds: rugged_author[:time].to_i)
+        )
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index dbc08747228..05b93a2be84 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -809,44 +809,24 @@ module Gitlab
       end
 
       def cherry_pick(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)
-
-          cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha)
-          raise CreateTreeError unless cherry_pick_tree_id
-
-          committer = user_to_committer(user)
+        gitaly_migrate(:cherry_pick) 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: {
-                            email: commit.author_email,
-                            name: commit.author_name,
-                            time: commit.authored_date
-                        },
-                        committer: committer,
-                        tree: cherry_pick_tree_id,
-                        parents: [start_commit.sha])
+          if is_enabled
+            gitaly_operations_client.user_cherry_pick(args)
+          else
+            rugged_cherry_pick(args)
+          end
         end
       end
 
-      def check_cherry_pick_content(target_commit, source_sha)
-        args = [target_commit.sha, source_sha]
-        args << 1 if target_commit.merge_commit?
-
-        cherry_pick_index = rugged.cherrypick_commit(*args)
-        return false if cherry_pick_index.conflicts?
-
-        tree_id = cherry_pick_index.write_tree(rugged)
-        return false unless diff_exists?(source_sha, tree_id)
-
-        tree_id
-      end
-
       def diff_exists?(sha1, sha2)
         rugged.diff(sha1, sha2).size > 0
       end
@@ -1665,6 +1645,45 @@ module Gitlab
         raise InvalidRef, ex
       end
 
+      def rugged_cherry_pick(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)
+
+          cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha)
+          raise CreateTreeError unless cherry_pick_tree_id
+
+          committer = user_to_committer(user)
+
+          create_commit(message: message,
+                        author: {
+                            email: commit.author_email,
+                            name: commit.author_name,
+                            time: commit.authored_date
+                        },
+                        committer: committer,
+                        tree: cherry_pick_tree_id,
+                        parents: [start_commit.sha])
+        end
+      end
+
+      def check_cherry_pick_content(target_commit, source_sha)
+        args = [target_commit.sha, source_sha]
+        args << 1 if target_commit.merge_commit?
+
+        cherry_pick_index = rugged.cherrypick_commit(*args)
+        return false if cherry_pick_index.conflicts?
+
+        tree_id = cherry_pick_index.write_tree(rugged)
+        return false unless diff_exists?(source_sha, tree_id)
+
+        tree_id
+      end
+
       def local_fetch_ref(source_path, source_ref:, target_ref:)
         args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
         run_git(args)
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 526d44a8b77..1913a28ef25 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -122,6 +122,36 @@ module Gitlab
         ).branch_update
         Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
       end
+
+      def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
+        request = Gitaly::UserCherryPickRequest.new(
+          repository: @gitaly_repo,
+          user: Gitlab::Git::User.from_gitlab(user).to_gitaly,
+          commit: commit.to_gitaly_commit,
+          branch_name: GitalyClient.encode(branch_name),
+          message: GitalyClient.encode(message),
+          start_repository: start_repository.gitaly_repository,
+          start_branch_name: GitalyClient.encode(start_branch_name.to_s)
+        )
+
+        response = GitalyClient.call(
+          @repository.storage,
+          :operation_service,
+          :user_cherry_pick,
+          request,
+          remote_storage: start_repository.storage
+        )
+
+        if pre_receive_error = response.pre_receive_error.presence
+          raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+        elsif commit_error = response.commit_error.presence
+          raise Gitlab::Git::CommitError, commit_error
+        elsif create_tree_error = response.create_tree_error.presence
+          raise Gitlab::Git::Repository::CreateTreeError, create_tree_error
+        else
+          Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
+        end
+      end
     end
   end
 end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 27f0a99b2fa..af0c86abe86 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1408,42 +1408,52 @@ describe Repository do
   end
 
   describe '#cherry_pick' do
-    let(:conflict_commit) { repository.commit('c642fe9b8b9f28f9225d7ea953fe14e74748d53b') }
-    let(:pickable_commit) { repository.commit('7d3b0f7cff5f37573aea97cebfd5692ea1689924') }
-    let(:pickable_merge) { repository.commit('e56497bb5f03a90a51293fc6d516788730953899') }
-    let(:message) { 'cherry-pick message' }
-
-    context 'when there is a conflict' do
-      it 'raises an error' do
-        expect { repository.cherry_pick(user, conflict_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError)
+    shared_examples 'cherry-picking a commit' do
+      let(:conflict_commit) { repository.commit('c642fe9b8b9f28f9225d7ea953fe14e74748d53b') }
+      let(:pickable_commit) { repository.commit('7d3b0f7cff5f37573aea97cebfd5692ea1689924') }
+      let(:pickable_merge) { repository.commit('e56497bb5f03a90a51293fc6d516788730953899') }
+      let(:message) { 'cherry-pick message' }
+
+      context 'when there is a conflict' do
+        it 'raises an error' do
+          expect { repository.cherry_pick(user, conflict_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError)
+        end
       end
-    end
 
-    context 'when commit was already cherry-picked' do
-      it 'raises an error' do
-        repository.cherry_pick(user, pickable_commit, 'master', message)
+      context 'when commit was already cherry-picked' do
+        it 'raises an error' do
+          repository.cherry_pick(user, pickable_commit, 'master', message)
 
-        expect { repository.cherry_pick(user, pickable_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError)
+          expect { repository.cherry_pick(user, pickable_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError)
+        end
       end
-    end
 
-    context 'when commit can be cherry-picked' do
-      it 'cherry-picks the changes' do
-        expect(repository.cherry_pick(user, pickable_commit, 'master', message)).to be_truthy
+      context 'when commit can be cherry-picked' do
+        it 'cherry-picks the changes' do
+          expect(repository.cherry_pick(user, pickable_commit, 'master', message)).to be_truthy
+        end
       end
-    end
 
-    context 'cherry-picking a merge commit' do
-      it 'cherry-picks the changes' do
-        expect(repository.blob_at_branch('improve/awesome', 'foo/bar/.gitkeep')).to be_nil
+      context 'cherry-picking a merge commit' do
+        it 'cherry-picks the changes' do
+          expect(repository.blob_at_branch('improve/awesome', 'foo/bar/.gitkeep')).to be_nil
 
-        cherry_pick_commit_sha = repository.cherry_pick(user, pickable_merge, 'improve/awesome', message)
-        cherry_pick_commit_message = project.commit(cherry_pick_commit_sha).message
+          cherry_pick_commit_sha = repository.cherry_pick(user, pickable_merge, 'improve/awesome', message)
+          cherry_pick_commit_message = project.commit(cherry_pick_commit_sha).message
 
-        expect(repository.blob_at_branch('improve/awesome', 'foo/bar/.gitkeep')).not_to be_nil
-        expect(cherry_pick_commit_message).to eq(message)
+          expect(repository.blob_at_branch('improve/awesome', 'foo/bar/.gitkeep')).not_to be_nil
+          expect(cherry_pick_commit_message).to eq(message)
+        end
       end
     end
+
+    context 'when Gitaly cherry_pick feature is enabled' do
+      it_behaves_like 'cherry-picking a commit'
+    end
+
+    context 'when Gitaly cherry_pick feature is disabled', :disable_gitaly do
+      it_behaves_like 'cherry-picking a commit'
+    end
   end
 
   describe '#before_delete' do