Merge branch 'gitaly-multi-action-prep' into 'master'

Move git operations for multi_action into Gitlab::Git

Closes gitaly#872

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

app/models/repository.rb

@@ -783,34 +783,30 @@ class Repository
   end
 
   def create_dir(user, path, **options)
-    options[:user] = user
     options[:actions] = [{ action: :create_dir, file_path: path }]
 
-    multi_action(**options)
+    multi_action(user, **options)
   end
 
   def create_file(user, path, content, **options)
-    options[:user] = user
     options[:actions] = [{ action: :create, file_path: path, content: content }]
 
-    multi_action(**options)
+    multi_action(user, **options)
   end
 
   def update_file(user, path, content, **options)
     previous_path = options.delete(:previous_path)
     action = previous_path && previous_path != path ? :move : :update
 
-    options[:user] = user
     options[:actions] = [{ action: action, file_path: path, previous_path: previous_path, content: content }]
 
-    multi_action(**options)
+    multi_action(user, **options)
   end
 
   def delete_file(user, path, **options)
-    options[:user] = user
     options[:actions] = [{ action: :delete, file_path: path }]
 
-    multi_action(**options)
+    multi_action(user, **options)
   end
 
   def with_cache_hooks
@@ -824,59 +820,14 @@ class Repository
     result.newrev
   end
 
-  def with_branch(user, *args)
-    with_cache_hooks do
-      Gitlab::Git::OperationService.new(user, raw_repository).with_branch(*args) do |start_commit|
-        yield start_commit
-      end
-    end
-  end
-
-  # rubocop:disable Metrics/ParameterLists
-  def multi_action(
-    user:, branch_name:, message:, actions:,
-    author_email: nil, author_name: nil,
-    start_branch_name: nil, start_project: project)
+  def multi_action(user, **options)
+    start_project = options.delete(:start_project)
 
-    with_branch(
-      user,
-      branch_name,
-      start_branch_name: start_branch_name,
-      start_repository: start_project.repository.raw_repository) do |start_commit|
-
-      index = Gitlab::Git::Index.new(raw_repository)
-
-      if start_commit
-        index.read_tree(start_commit.rugged_commit.tree)
-        parents = [start_commit.sha]
-      else
-        parents = []
+    if start_project
+      options[:start_repository] = start_project.repository.raw_repository
     end
 
-      actions.each do |options|
-        index.public_send(options.delete(:action), options) # rubocop:disable GitlabSecurity/PublicSend
-      end
-
-      options = {
-        tree: index.write_tree,
-        message: message,
-        parents: parents
-      }
-      options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
-
-      create_commit(options)
-    end
-  end
-  # rubocop:enable Metrics/ParameterLists
-
-  def get_committer_and_author(user, email: nil, name: nil)
-    committer = user_to_committer(user)
-    author = Gitlab::Git.committer_hash(email: email, name: name) || committer
-
-    {
-      author: author,
-      committer: committer
-    }
+    with_cache_hooks { raw.multi_action(user, **options) }
   end
 
   def can_be_merged?(source_sha, target_branch)

app/services/files/multi_service.rb

@@ -4,7 +4,7 @@ module Files
 
     def create_commit!
       repository.multi_action(
-        user: current_user,
+        current_user,
         message: @commit_message,
         branch_name: @branch_name,
         actions: params[:actions],
@@ -13,6 +13,8 @@ module Files
         start_project: @start_project,
         start_branch_name: @start_branch
       )
+    rescue ArgumentError => e
+      raise_error(e)
     end
 
     private
@@ -20,16 +22,7 @@ module Files
     def validate!
       super
 
-      params[:actions].each do |action|
-        validate_action!(action)
-        validate_file_status!(action)
-      end
-    end
-
-    def validate_action!(action)
-      unless Gitlab::Git::Index::ACTIONS.include?(action[:action].to_s)
-        raise_error("Unknown action '#{action[:action]}'")
-      end
+      params[:actions].each { |action| validate_file_status!(action) }
     end
 
     def validate_file_status!(action)

lib/gitlab/git/index.rb

@@ -10,6 +10,7 @@ module Gitlab
       DEFAULT_MODE = 0o100644
 
       ACTIONS = %w(create create_dir update move delete).freeze
+      ACTION_OPTIONS = %i(file_path previous_path content encoding).freeze
 
       attr_reader :repository, :raw_index
 
@@ -20,6 +21,11 @@ module Gitlab
 
       delegate :read_tree, :get, to: :raw_index
 
+      def apply(action, options)
+        validate_action!(action)
+        public_send(action, options.slice(*ACTION_OPTIONS)) # rubocop:disable GitlabSecurity/PublicSend
+      end
+
       def write_tree
         raw_index.write_tree(repository.rugged)
       end
@@ -140,6 +146,12 @@ module Gitlab
       rescue Rugged::IndexError => e
         raise IndexError, e.message
       end
+
+      def validate_action!(action)
+        unless ACTIONS.include?(action.to_s)
+          raise ArgumentError, "Unknown action '#{action}'"
+        end
+      end
     end
   end
 end

lib/gitlab/git/repository.rb

@@ -1300,6 +1300,42 @@ module Gitlab
         success || gitlab_projects_error
       end
 
+      # rubocop:disable Metrics/ParameterLists
+      def multi_action(
+        user, branch_name:, message:, actions:,
+        author_email: nil, author_name: nil,
+        start_branch_name: nil, start_repository: self)
+
+        OperationService.new(user, self).with_branch(
+          branch_name,
+          start_branch_name: start_branch_name,
+          start_repository: start_repository
+        ) do |start_commit|
+          index = Gitlab::Git::Index.new(self)
+          parents = []
+
+          if start_commit
+            index.read_tree(start_commit.rugged_commit.tree)
+            parents = [start_commit.sha]
+          end
+
+          actions.each { |opts| index.apply(opts.delete(:action), opts) }
+
+          committer = user_to_committer(user)
+          author = Gitlab::Git.committer_hash(email: author_email, name: author_name) || committer
+          options = {
+            tree: index.write_tree,
+            message: message,
+            parents: parents,
+            author: author,
+            committer: committer
+          }
+
+          create_commit(options)
+        end
+      end
+      # rubocop:enable Metrics/ParameterLists
+
       def gitaly_repository
         Gitlab::GitalyClient::Util.repository(@storage, @relative_path, @gl_repository)
       end

spec/models/repository_spec.rb

@@ -582,38 +582,6 @@ describe Repository do
     end
   end
 
-  describe '#get_committer_and_author' do
-    it 'returns the committer and author data' do
-      options = repository.get_committer_and_author(user)
-      expect(options[:committer][:email]).to eq(user.email)
-      expect(options[:author][:email]).to eq(user.email)
-    end
-
-    context 'when the email/name are given' do
-      it 'returns an object containing the email/name' do
-        options = repository.get_committer_and_author(user, email: author_email, name: author_name)
-        expect(options[:author][:email]).to eq(author_email)
-        expect(options[:author][:name]).to eq(author_name)
-      end
-    end
-
-    context 'when the email is given but the name is not' do
-      it 'returns the committer as the author' do
-        options = repository.get_committer_and_author(user, email: author_email)
-        expect(options[:author][:email]).to eq(user.email)
-        expect(options[:author][:name]).to eq(user.name)
-      end
-    end
-
-    context 'when the name is given but the email is not' do
-      it 'returns nil' do
-        options = repository.get_committer_and_author(user, name: author_name)
-        expect(options[:author][:email]).to eq(user.email)
-        expect(options[:author][:name]).to eq(user.name)
-      end
-    end
-  end
-
   describe "search_files_by_content" do
     let(:results) { repository.search_files_by_content('feature', 'master') }
     subject { results }
@@ -1112,16 +1080,16 @@ describe Repository do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, ''])
       end
 
-      it 'expires branch cache' do
-        expect(repository).not_to receive(:expire_exists_cache)
-        expect(repository).not_to receive(:expire_root_ref_cache)
-        expect(repository).not_to receive(:expire_emptiness_caches)
-        expect(repository).to     receive(:expire_branches_cache)
-
-        repository.with_branch(user, 'new-feature') do
+      subject do
+        Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch('new-feature') do
           new_rev
         end
       end
+
+      it 'returns branch_created as true' do
+        expect(subject).not_to be_repo_created
+        expect(subject).to     be_branch_created
+      end
     end
 
     context 'when repository is empty' do

spec/services/files/multi_service_spec.rb

@@ -41,7 +41,7 @@ describe Files::MultiService do
 
   describe '#execute' do
     context 'with a valid action' do
-      it 'returns a hash with the :success status ' do
+      it 'returns a hash with the :success status' do
         results = subject.execute
 
         expect(results[:status]).to eq(:success)
@@ -51,7 +51,7 @@ describe Files::MultiService do
     context 'with an invalid action' do
       let(:action) { 'rename' }
 
-      it 'returns a hash with the :error status ' do
+      it 'returns a hash with the :error status' do
         results = subject.execute
 
         expect(results[:status]).to eq(:error)