Merge branch 'gitaly-user-commit-files' into 'master'

Incorporate Gitaly's OperationService.UserCommitFiles RPC

Closes gitaly#890

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

GITALY_SERVER_VERSION

@@ -1 +1 @@
-0.70.0
+0.72.0

Gemfile

@@ -406,7 +406,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.73.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.74.0', require: 'gitaly'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 

Gemfile.lock

@@ -285,7 +285,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.73.0)
+    gitaly-proto (0.74.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -1056,7 +1056,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.73.0)
+  gitaly-proto (~> 0.74.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)

lib/gitlab/git/repository.rb

@@ -1274,32 +1274,15 @@ module Gitlab
         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]
+        gitaly_migrate(:operation_user_commit_files) do |is_enabled|
+          if is_enabled
+            gitaly_operation_client.user_commit_files(user, branch_name,
+              message, actions, author_email, author_name,
+              start_branch_name, start_repository)
+          else
+            rugged_multi_action(user, branch_name, message, actions,
+              author_email, author_name, start_branch_name, start_repository)
           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
@@ -2085,6 +2068,39 @@ module Gitlab
         remove_remote(remote_name)
       end
 
+      def rugged_multi_action(
+        user, branch_name, message, actions, author_email, author_name,
+        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|
+          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
+
       def fetch_remote(remote_name = 'origin', env: nil)
         run_git(['fetch', remote_name], env: env).last.zero?
       end

lib/gitlab/gitaly_client/operation_service.rb

@@ -3,6 +3,8 @@ module Gitlab
     class OperationService
       include Gitlab::EncodingHelper
 
+      MAX_MSG_SIZE = 128.kilobytes.freeze
+
       def initialize(repository)
         @gitaly_repo = repository.gitaly_repository
         @repository = repository
@@ -175,6 +177,49 @@ module Gitlab
         end
       end
 
+      def user_commit_files(
+        user, branch_name, commit_message, actions, author_email, author_name,
+        start_branch_name, start_repository)
+
+        req_enum = Enumerator.new do |y|
+          header = user_commit_files_request_header(user, branch_name,
+          commit_message, actions, author_email, author_name,
+          start_branch_name, start_repository)
+
+          y.yield Gitaly::UserCommitFilesRequest.new(header: header)
+
+          actions.each do |action|
+            action_header = user_commit_files_action_header(action)
+            y.yield Gitaly::UserCommitFilesRequest.new(
+              action: Gitaly::UserCommitFilesAction.new(header: action_header)
+            )
+
+            reader = binary_stringio(action[:content])
+
+            until reader.eof?
+              chunk = reader.read(MAX_MSG_SIZE)
+
+              y.yield Gitaly::UserCommitFilesRequest.new(
+                action: Gitaly::UserCommitFilesAction.new(content: chunk)
+              )
+            end
+          end
+        end
+
+        response = GitalyClient.call(@repository.storage, :operation_service,
+          :user_commit_files, req_enum, remote_storage: start_repository.storage)
+
+        if (pre_receive_error = response.pre_receive_error.presence)
+          raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+        end
+
+        if (index_error = response.index_error.presence)
+          raise Gitlab::Git::Index::IndexError, index_error
+        end
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
+      end
+
       private
 
       def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
@@ -212,6 +257,33 @@ module Gitlab
           Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
         end
       end
+
+      def user_commit_files_request_header(
+        user, branch_name, commit_message, actions, author_email, author_name,
+        start_branch_name, start_repository)
+
+        Gitaly::UserCommitFilesRequestHeader.new(
+          repository: @gitaly_repo,
+          user: Gitlab::Git::User.from_gitlab(user).to_gitaly,
+          branch_name: encode_binary(branch_name),
+          commit_message: encode_binary(commit_message),
+          commit_author_name: encode_binary(author_name),
+          commit_author_email: encode_binary(author_email),
+          start_branch_name: encode_binary(start_branch_name),
+          start_repository: start_repository.gitaly_repository
+        )
+      end
+
+      def user_commit_files_action_header(action)
+        Gitaly::UserCommitFilesActionHeader.new(
+          action: action[:action].upcase.to_sym,
+          file_path: encode_binary(action[:file_path]),
+          previous_path: encode_binary(action[:previous_path]),
+          base64_content: action[:encoding] == 'base64'
+        )
+      rescue RangeError
+        raise ArgumentError, "Unknown action '#{action[:action]}'"
+      end
     end
   end
 end

spec/lib/gitlab/git/gitlab_projects_spec.rb

@@ -219,6 +219,9 @@ describe Gitlab::Git::GitlabProjects do
 
     before do
       FileUtils.mkdir_p(dest_repos_path)
+
+      # Undo spec_helper stub that deletes hooks
+      allow_any_instance_of(described_class).to receive(:fork_repository).and_call_original
     end
 
     after do

spec/services/files/update_service_spec.rb

@@ -72,13 +72,15 @@ describe Files::UpdateService do
       end
     end
 
-    context 'when target branch is different than source branch' do
-      let(:branch_name) { "#{project.default_branch}-new" }
+    context 'with gitaly disabled', :skip_gitaly_mock do
+      context 'when target branch is different than source branch' do
+        let(:branch_name) { "#{project.default_branch}-new" }
 
-      it 'fires hooks only once' do
-        expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original
+        it 'fires hooks only once' do
+          expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original
 
-        subject.execute
+          subject.execute
+        end
       end
     end
   end

spec/spec_helper.rb

@@ -100,6 +100,16 @@ RSpec.configure do |config|
   config.before(:example) do
     # Skip pre-receive hook check so we can use the web editor and merge.
     allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
+
+    allow_any_instance_of(Gitlab::Git::GitlabProjects).to receive(:fork_repository).and_wrap_original do |m, *args|
+      m.call(*args)
+
+      shard_path, repository_relative_path = args
+      # We can't leave the hooks in place after a fork, as those would fail in tests
+      # The "internal" API is not available
+      FileUtils.rm_rf(File.join(shard_path, repository_relative_path, 'hooks'))
+    end
+
     # Enable all features by default for testing
     allow(Feature).to receive(:enabled?) { true }
   end

spec/support/cycle_analytics_helpers.rb

@@ -5,10 +5,16 @@ module CycleAnalyticsHelpers
   end
 
   def create_commit(message, project, user, branch_name, count: 1)
-    oldrev = project.repository.commit(branch_name).sha
+    repository = project.repository
+    oldrev = repository.commit(branch_name).sha
+
+    if Timecop.frozen? && Gitlab::GitalyClient.feature_enabled?(:operation_user_commit_files)
+      mock_gitaly_multi_action_dates(repository.raw)
+    end
+
     commit_shas = Array.new(count) do |index|
-      commit_sha = project.repository.create_file(user, generate(:branch), "content", message: message, branch_name: branch_name)
-      project.repository.commit(commit_sha)
+      commit_sha = repository.create_file(user, generate(:branch), "content", message: message, branch_name: branch_name)
+      repository.commit(commit_sha)
 
       commit_sha
     end
@@ -98,6 +104,25 @@ module CycleAnalyticsHelpers
       pipeline: dummy_pipeline,
       protected: false)
   end
+
+  def mock_gitaly_multi_action_dates(raw_repository)
+    allow(raw_repository).to receive(:multi_action).and_wrap_original do |m, *args|
+      new_date = Time.now
+      branch_update = m.call(*args)
+
+      if branch_update.newrev
+        _, opts = args
+        commit = raw_repository.commit(branch_update.newrev).rugged_commit
+        branch_update.newrev = commit.amend(
+          update_ref: "#{Gitlab::Git::BRANCH_REF_PREFIX}#{opts[:branch_name]}",
+          author: commit.author.merge(time: new_date),
+          committer: commit.committer.merge(time: new_date)
+        )
+      end
+
+      branch_update
+    end
+  end
 end
 
 RSpec.configure do |config|

spec/support/project_forks_helper.rb

@@ -38,10 +38,6 @@ module ProjectForksHelper
       # so we have to explicitely call this method to clear the @exists variable.
       # of the instance we're returning here.
       forked_project.repository.after_import
-
-      # We can't leave the hooks in place after a fork, as those would fail in tests
-      # The "internal" API is not available
-      FileUtils.rm_rf("#{forked_project.repository.path}/hooks")
     end
 
     forked_project