Merge branch 'gitaly-prepare-cherry-pick-revert' into 'master'

Prepare cherry-pick and revert for migration to Gitaly

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

7 files changed, 136 insertions, 90 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 9d1de4f4306..f11cf1b065d 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -834,10 +834,6 @@ class Repository
     }
   end
 
-  def user_to_committer(user)
-    Gitlab::Git.committer_hash(email: user.email, name: user.name)
-  end
-
   def can_be_merged?(source_sha, target_branch)
     our_commit = rugged.branches[target_branch].target
     their_commit = rugged.lookup(source_sha)
@@ -859,54 +855,34 @@ class Repository
   end
 
   def revert(
-    user, commit, branch_name,
+    user, commit, branch_name, message,
     start_branch_name: nil, start_project: project)
-    with_branch(
-      user,
-      branch_name,
-      start_branch_name: start_branch_name,
-      start_repository: start_project.repository.raw_repository) do |start_commit|
-
-      revert_tree_id = check_revert_content(commit, start_commit.sha)
-      unless revert_tree_id
-        raise Repository::CreateTreeError.new('Failed to revert commit')
-      end
 
-      committer = user_to_committer(user)
-
-      create_commit(message: commit.revert_message(user),
-                    author: committer,
-                    committer: committer,
-                    tree: revert_tree_id,
-                    parents: [start_commit.sha])
+    with_cache_hooks do
+      raw_repository.revert(
+        user: user,
+        commit: commit.raw,
+        branch_name: branch_name,
+        message: message,
+        start_branch_name: start_branch_name,
+        start_repository: start_project.repository.raw_repository
+      )
     end
   end
 
   def cherry_pick(
-    user, commit, branch_name,
+    user, commit, branch_name, message,
     start_branch_name: nil, start_project: project)
-    with_branch(
-      user,
-      branch_name,
-      start_branch_name: start_branch_name,
-      start_repository: start_project.repository.raw_repository) do |start_commit|
 
-      cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha)
-      unless cherry_pick_tree_id
-        raise Repository::CreateTreeError.new('Failed to cherry-pick commit')
-      end
-
-      committer = user_to_committer(user)
-
-      create_commit(message: commit.cherry_pick_message(user),
-                    author: {
-                        email: commit.author_email,
-                        name: commit.author_name,
-                        time: commit.authored_date
-                    },
-                    committer: committer,
-                    tree: cherry_pick_tree_id,
-                    parents: [start_commit.sha])
+    with_cache_hooks do
+      raw_repository.cherry_pick(
+        user: user,
+        commit: commit.raw,
+        branch_name: branch_name,
+        message: message,
+        start_branch_name: start_branch_name,
+        start_repository: start_project.repository.raw_repository
+      )
     end
   end
 
@@ -918,36 +894,6 @@ class Repository
     end
   end
 
-  def check_revert_content(target_commit, source_sha)
-    args = [target_commit.sha, source_sha]
-    args << { mainline: 1 } if target_commit.merge_commit?
-
-    revert_index = rugged.revert_commit(*args)
-    return false if revert_index.conflicts?
-
-    tree_id = revert_index.write_tree(rugged)
-    return false unless diff_exists?(source_sha, tree_id)
-
-    tree_id
-  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
-
   def merged_to_root_ref?(branch_name)
     branch_commit = commit(branch_name)
     root_ref_commit = commit(root_ref)
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 85c2fcf9ea6..b9d0173a2d0 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -12,14 +12,18 @@ module Commits
       raise NotImplementedError unless repository.respond_to?(action)
 
       # rubocop:disable GitlabSecurity/PublicSend
+      message = @commit.public_send(:"#{action}_message", current_user)
+
+      # rubocop:disable GitlabSecurity/PublicSend
       repository.public_send(
         action,
         current_user,
         @commit,
         @branch_name,
+        message,
         start_project: @start_project,
         start_branch_name: @start_branch)
-    rescue Repository::CreateTreeError
+    rescue Gitlab::Git::Repository::CreateTreeError
       error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
                    This #{@commit.change_type_title(current_user)} may already have been #{action.to_s.dasherize}ed, or a more recent commit may have updated some of its content."
       raise ChangeError, error_msg
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index b4b6326cfdd..c78fe63f9b5 100644
--- a/lib/gitlab/git.rb
+++ b/lib/gitlab/git.rb
@@ -57,6 +57,15 @@ module Gitlab
       def version
         Gitlab::VersionInfo.parse(Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --version)).first)
       end
+
+      def check_namespace!(*objects)
+        expected_namespace = self.name + '::'
+        objects.each do |object|
+          unless object.class.name.start_with?(expected_namespace)
+            raise ArgumentError, "expected object in #{expected_namespace}, got #{object}"
+          end
+        end
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 1f370686186..1957c254c28 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -413,6 +413,10 @@ module Gitlab
                            end
       end
 
+      def merge_commit?
+        parent_ids.size > 1
+      end
+
       private
 
       def init_from_hash(hash)
diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb
index dcdec818f5e..6f054ed3c6c 100644
--- a/lib/gitlab/git/operation_service.rb
+++ b/lib/gitlab/git/operation_service.rb
@@ -15,9 +15,7 @@ module Gitlab
         end
 
         # Refactoring aid
-        unless new_repository.is_a?(Gitlab::Git::Repository)
-          raise "expected a Gitlab::Git::Repository, got #{new_repository}"
-        end
+        Gitlab::Git.check_namespace!(new_repository)
 
         @repository = new_repository
       end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 18210bcab4e..4b000bd31e2 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -19,6 +19,7 @@ module Gitlab
       InvalidRef = Class.new(StandardError)
       GitError = Class.new(StandardError)
       DeleteBranchError = Class.new(StandardError)
+      CreateTreeError = Class.new(StandardError)
 
       class << self
         # Unlike `new`, `create` takes the storage path, not the storage name
@@ -684,6 +685,88 @@ module Gitlab
         nil
       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)
+
+          create_commit(message: message,
+                        author: committer,
+                        committer: committer,
+                        tree: revert_tree_id,
+                        parents: [start_commit.sha])
+        end
+      end
+
+      def check_revert_content(target_commit, source_sha)
+        args = [target_commit.sha, source_sha]
+        args << { mainline: 1 } if target_commit.merge_commit?
+
+        revert_index = rugged.revert_commit(*args)
+        return false if revert_index.conflicts?
+
+        tree_id = revert_index.write_tree(rugged)
+        return false unless diff_exists?(source_sha, tree_id)
+
+        tree_id
+      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)
+
+          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 diff_exists?(sha1, sha2)
+        rugged.diff(sha1, sha2).size > 0
+      end
+
+      def user_to_committer(user)
+        Gitlab::Git.committer_hash(email: user.email, name: user.name)
+      end
+
       def create_commit(params = {})
         params[:message].delete!("\r")
 
@@ -835,7 +918,7 @@ module Gitlab
       end
 
       def with_repo_branch_commit(start_repository, start_branch_name)
-        raise "expected Gitlab::Git::Repository, got #{start_repository}" unless start_repository.is_a?(Gitlab::Git::Repository)
+        Gitlab::Git.check_namespace!(start_repository)
 
         return yield nil if start_repository.empty_repo?
 
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 60cd7e70055..76bb658b10d 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1311,24 +1311,25 @@ describe Repository, models: true do
   describe '#revert' 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') }.to raise_error(/Failed to/)
+        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')
+        repository.revert(user, update_image_commit, 'master', message)
 
-        expect { repository.revert(user, update_image_commit, 'master') }.to raise_error(/Failed to/)
+        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')).to be_truthy
+        expect(repository.revert(user, update_image_commit, 'master', message)).to be_truthy
       end
     end
 
@@ -1337,7 +1338,7 @@ describe Repository, models: true do
         merge_commit
         expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present
 
-        repository.revert(user, merge_commit, 'master')
+        repository.revert(user, merge_commit, 'master', message)
         expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present
       end
     end
@@ -1347,24 +1348,25 @@ describe Repository, models: true 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') }.to raise_error(/Failed to/)
+        expect { repository.cherry_pick(user, conflict_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError)
       end
     end
 
     context 'when commit was already cherry-picked' do
       it 'raises an error' do
-        repository.cherry_pick(user, pickable_commit, 'master')
+        repository.cherry_pick(user, pickable_commit, 'master', message)
 
-        expect { repository.cherry_pick(user, pickable_commit, 'master') }.to raise_error(/Failed to/)
+        expect { repository.cherry_pick(user, pickable_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError)
       end
     end
 
     context 'when commit can be cherry-picked' do
       it 'cherry-picks the changes' do
-        expect(repository.cherry_pick(user, pickable_commit, 'master')).to be_truthy
+        expect(repository.cherry_pick(user, pickable_commit, 'master', message)).to be_truthy
       end
     end
 
@@ -1372,11 +1374,11 @@ describe Repository, models: true 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')
+        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 include('cherry picked from')
+        expect(cherry_pick_commit_message).to eq(message)
       end
     end
   end