Merge branch 'gitaly-fetch-ref' into 'master'

Let fetch_ref pull from Gitaly instead of from disk

Closes gitaly#585

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

7 files changed, 80 insertions, 22 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index d47dc9a05cd..d725c65081d 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -989,7 +989,7 @@ class Repository
   end
 
   def create_ref(ref, ref_path)
-    fetch_ref(path_to_repo, ref, ref_path)
+    raw_repository.write_ref(ref_path, ref)
   end
 
   def ls_files(ref)
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index c5c50c216e1..88771c5f5bb 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -499,6 +499,8 @@ production: &base
 
   # Gitaly settings
   gitaly:
+    # Path to the directory containing Gitaly client executables.
+    client_path: /home/git/gitaly
     # Default Gitaly authentication token. Can be overriden per storage. Can
     # be left blank when Gitaly is running locally on a Unix socket, which
     # is the normal way to deploy Gitaly.
@@ -664,7 +666,7 @@ test:
         gitaly_address: unix:tmp/tests/gitaly/gitaly.socket
 
   gitaly:
-    enabled: true
+    client_path: tmp/tests/gitaly
     token: secret
   backup:
     path: tmp/tests/backups
diff --git a/doc/development/testing.md b/doc/development/testing.md
index d856b003353..4d5b90de6fc 100644
--- a/doc/development/testing.md
+++ b/doc/development/testing.md
@@ -302,7 +302,7 @@ range of inputs, might look like this:
 
 ```ruby
 describe "#==" do
-  using Rspec::Parameterized::TableSyntax
+  using RSpec::Parameterized::TableSyntax
 
   let(:project1) { create(:project) }
   let(:project2) { create(:project) }
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index fb3a8516441..89b654253cb 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -53,14 +53,15 @@ module Gitlab
       # Rugged repo object
       attr_reader :rugged
 
-      attr_reader :storage, :gl_repository, :relative_path
+      attr_reader :storage, :gl_repository, :relative_path, :gitaly_resolver
 
-      # 'path' must be the path to a _bare_ git repository, e.g.
-      # /path/to/my-repo.git
+      # This initializer method is only used on the client side (gitlab-ce).
+      # Gitaly-ruby uses a different initializer.
       def initialize(storage, relative_path, gl_repository)
         @storage = storage
         @relative_path = relative_path
         @gl_repository = gl_repository
+        @gitaly_resolver = Gitlab::GitalyClient
 
         storage_path = Gitlab.config.repositories.storages[@storage]['path']
         @path = File.join(storage_path, @relative_path)
@@ -987,9 +988,9 @@ module Gitlab
 
       def with_repo_tmp_commit(start_repository, start_branch_name, sha)
         tmp_ref = fetch_ref(
-          start_repository.path,
-          "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
-          "refs/tmp/#{SecureRandom.hex}/head"
+          start_repository,
+          source_ref: "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
+          target_ref: "refs/tmp/#{SecureRandom.hex}/head"
         )
 
         yield commit(sha)
@@ -1021,13 +1022,27 @@ module Gitlab
         end
       end
 
-      def write_ref(ref_path, sha)
-        rugged.references.create(ref_path, sha, force: true)
+      def write_ref(ref_path, ref)
+        raise ArgumentError, "invalid ref_path #{ref_path.inspect}" if ref_path.include?(' ')
+        raise ArgumentError, "invalid ref #{ref.inspect}" if ref.include?("\x00")
+
+        command = [Gitlab.config.git.bin_path] + %w[update-ref --stdin -z]
+        input = "update #{ref_path}\x00#{ref}\x00\x00"
+        output, status = circuit_breaker.perform do
+          popen(command, path) { |stdin| stdin.write(input) }
+        end
+
+        raise GitError, output unless status.zero?
       end
 
-      def fetch_ref(source_path, source_ref, target_ref)
-        args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
-        message, status = run_git(args)
+      def fetch_ref(source_repository, source_ref:, target_ref:)
+        message, status = GitalyClient.migrate(:fetch_ref) do |is_enabled|
+          if is_enabled
+            gitaly_fetch_ref(source_repository, source_ref: source_ref, target_ref: target_ref)
+          else
+            local_fetch_ref(source_repository.path, source_ref: source_ref, target_ref: target_ref)
+          end
+        end
 
         # Make sure ref was created, and raise Rugged::ReferenceError when not
         raise Rugged::ReferenceError, message if status != 0
@@ -1036,9 +1051,9 @@ module Gitlab
       end
 
       # Refactoring aid; allows us to copy code from app/models/repository.rb
-      def run_git(args)
+      def run_git(args, env: {})
         circuit_breaker.perform do
-          popen([Gitlab.config.git.bin_path, *args], path)
+          popen([Gitlab.config.git.bin_path, *args], path, env)
         end
       end
 
@@ -1498,6 +1513,30 @@ module Gitlab
       rescue Rugged::ReferenceError => ex
         raise InvalidRef, ex
       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)
+      end
+
+      def gitaly_fetch_ref(source_repository, source_ref:, target_ref:)
+        gitaly_ssh = File.absolute_path(File.join(Gitlab.config.gitaly.client_path, 'gitaly-ssh'))
+        gitaly_address = gitaly_resolver.address(source_repository.storage)
+        gitaly_token = gitaly_resolver.token(source_repository.storage)
+
+        request = Gitaly::SSHUploadPackRequest.new(repository: source_repository.gitaly_repository)
+        env = {
+          'GITALY_ADDRESS' => gitaly_address,
+          'GITALY_PAYLOAD' => request.to_json,
+          'GITALY_WD' => Dir.pwd,
+          'GIT_SSH_COMMAND' => "#{gitaly_ssh} upload-pack"
+        }
+        env['GITALY_TOKEN'] = gitaly_token if gitaly_token.present?
+
+        args = %W(fetch --no-tags -f ssh://gitaly/internal.git #{source_ref}:#{target_ref})
+
+        run_git(args, env: env)
+      end
     end
   end
 end
diff --git a/spec/factories/deployments.rb b/spec/factories/deployments.rb
index e5abfd67d60..0dd1238d6e2 100644
--- a/spec/factories/deployments.rb
+++ b/spec/factories/deployments.rb
@@ -12,7 +12,7 @@ FactoryGirl.define do
       deployment.project ||= deployment.environment.project
 
       unless deployment.project.repository_exists?
-        allow(deployment.project.repository).to receive(:fetch_ref)
+        allow(deployment.project.repository).to receive(:create_ref)
       end
     end
   end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index f405b2f2684..5f12125beb2 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1472,6 +1472,23 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#write_ref' do
+    context 'validations' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:ref_path, :ref) do
+        'foo bar' | '123'
+        'foobar'  | "12\x003"
+      end
+
+      with_them do
+        it 'raises ArgumentError' do
+          expect { repository.write_ref(ref_path, ref) }.to raise_error(ArgumentError)
+        end
+      end
+    end
+  end
+
   def create_remote_branch(repository, remote_name, branch_name, source_branch_name)
     source_branch = repository.branches.find { |branch| branch.name == source_branch_name }
     rugged = repository.rugged
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index bdcdad46390..7156c1b7aa8 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -636,18 +636,18 @@ describe Repository do
   describe '#fetch_ref' do
     describe 'when storage is broken', broken_storage: true  do
       it 'should raise a storage error' do
-        path = broken_repository.path_to_repo
-
-        expect_to_raise_storage_error { broken_repository.fetch_ref(path, '1', '2') }
+        expect_to_raise_storage_error do
+          broken_repository.fetch_ref(broken_repository, source_ref: '1', target_ref: '2')
+        end
       end
     end
   end
 
   describe '#create_ref' do
-    it 'redirects the call to fetch_ref' do
+    it 'redirects the call to write_ref' do
       ref, ref_path = '1', '2'
 
-      expect(repository).to receive(:fetch_ref).with(repository.path_to_repo, ref, ref_path)
+      expect(repository.raw_repository).to receive(:write_ref).with(ref_path, ref)
 
       repository.create_ref(ref, ref_path)
     end