Merge branch 'gitlab-git-remote-repository' into 'master'

Prepare Repository#fetch_source_branch for migration

Closes gitaly#681

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

app/models/repository.rb

@@ -1062,6 +1062,10 @@ class Repository
     blob_data_at(sha, path)
   end
 
+  def fetch_ref(source_repository, source_ref:, target_ref:)
+    raw_repository.fetch_ref(source_repository.raw_repository, source_ref: source_ref, target_ref: target_ref)
+  end
+
   private
 
   # TODO Generice finder, later split this on finders by Ref or Oid

lib/gitlab/git/operation_service.rb

@@ -72,7 +72,7 @@ module Gitlab
 
       # Whenever `start_branch_name` is passed, if `branch_name` doesn't exist,
       # it would be created from `start_branch_name`.
-      # If `start_project` is passed, and the branch doesn't exist,
+      # If `start_repository` is passed, and the branch doesn't exist,
       # it would try to find the commits from it instead of current repository.
       def with_branch(
         branch_name,
@@ -80,15 +80,13 @@ module Gitlab
         start_repository: repository,
         &block)
 
-        # Refactoring aid
-        unless start_repository.is_a?(Gitlab::Git::Repository)
-          raise "expected a Gitlab::Git::Repository, got #{start_repository}"
-        end
+        Gitlab::Git.check_namespace!(start_repository)
+        start_repository = RemoteRepository.new(start_repository) unless start_repository.is_a?(RemoteRepository)
 
         start_branch_name = nil if start_repository.empty_repo?
 
         if start_branch_name && !start_repository.branch_exists?(start_branch_name)
-          raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.full_path}"
+          raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.relative_path}"
         end
 
         update_branch_with_hooks(branch_name) do

lib/gitlab/git/remote_repository.rb

+module Gitlab
+  module Git
+    #
+    # When a Gitaly call involves two repositories instead of one we cannot
+    # assume that both repositories are on the same Gitaly server. In this
+    # case we need to make a distinction between the repository that the
+    # call is being made on (a Repository instance), and the "other"
+    # repository (a RemoteRepository instance). This is the reason why we
+    # have the RemoteRepository class in Gitlab::Git.
+    #
+    # When you make changes, be aware that gitaly-ruby sub-classes this
+    # class.
+    #
+    class RemoteRepository
+      attr_reader :path, :relative_path, :gitaly_repository
+
+      def initialize(repository)
+        @relative_path = repository.relative_path
+        @gitaly_repository = repository.gitaly_repository
+
+        # These instance variables will not be available in gitaly-ruby, where
+        # we have no disk access to this repository.
+        @repository = repository
+        @path = repository.path
+      end
+
+      def empty_repo?
+        # We will override this implementation in gitaly-ruby because we cannot
+        # use '@repository' there.
+        @repository.empty_repo?
+      end
+
+      def commit_id(revision)
+        # We will override this implementation in gitaly-ruby because we cannot
+        # use '@repository' there.
+        @repository.commit(revision)&.sha
+      end
+
+      def branch_exists?(name)
+        # We will override this implementation in gitaly-ruby because we cannot
+        # use '@repository' there.
+        @repository.branch_exists?(name)
+      end
+
+      # Compares self to a Gitlab::Git::Repository. This implementation uses
+      # 'self.gitaly_repository' so that it will also work in the
+      # GitalyRemoteRepository subclass defined in gitaly-ruby.
+      def same_repository?(other_repository)
+        gitaly_repository.storage_name == other_repository.storage &&
+          gitaly_repository.relative_path == other_repository.relative_path
+      end
+
+      def fetch_env
+        gitaly_ssh = File.absolute_path(File.join(Gitlab.config.gitaly.client_path, 'gitaly-ssh'))
+        gitaly_address = gitaly_client.address(storage)
+        gitaly_token = gitaly_client.token(storage)
+
+        request = Gitaly::SSHUploadPackRequest.new(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?
+
+        env
+      end
+
+      private
+
+      # Must return an object that responds to 'address' and 'storage'.
+      def gitaly_client
+        Gitlab::GitalyClient
+      end
+
+      def storage
+        gitaly_repository.storage_name
+      end
+    end
+  end
+end

lib/gitlab/git/repository.rb

@@ -58,7 +58,7 @@ module Gitlab
       # Rugged repo object
       attr_reader :rugged
 
-      attr_reader :storage, :gl_repository, :relative_path, :gitaly_resolver
+      attr_reader :storage, :gl_repository, :relative_path
 
       # This initializer method is only used on the client side (gitlab-ce).
       # Gitaly-ruby uses a different initializer.
@@ -66,7 +66,6 @@ module Gitlab
         @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)
@@ -1014,23 +1013,22 @@ module Gitlab
 
       def with_repo_branch_commit(start_repository, start_branch_name)
         Gitlab::Git.check_namespace!(start_repository)
+        start_repository = RemoteRepository.new(start_repository) unless start_repository.is_a?(RemoteRepository)
 
         return yield nil if start_repository.empty_repo?
 
-        if start_repository == self
+        if start_repository.same_repository?(self)
           yield commit(start_branch_name)
         else
-          start_commit = start_repository.commit(start_branch_name)
+          start_commit_id = start_repository.commit_id(start_branch_name)
 
-          return yield nil unless start_commit
+          return yield nil unless start_commit_id
 
-          sha = start_commit.sha
-
-          if branch_commit = commit(sha)
+          if branch_commit = commit(start_commit_id)
             yield branch_commit
           else
             with_repo_tmp_commit(
-              start_repository, start_branch_name, sha) do |tmp_commit|
+              start_repository, start_branch_name, start_commit_id) do |tmp_commit|
               yield tmp_commit
             end
           end
@@ -1087,6 +1085,9 @@ module Gitlab
       end
 
       def fetch_ref(source_repository, source_ref:, target_ref:)
+        Gitlab::Git.check_namespace!(source_repository)
+        source_repository = RemoteRepository.new(source_repository) unless source_repository.is_a?(RemoteRepository)
+
         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)
@@ -1620,22 +1621,9 @@ module Gitlab
       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)
+        run_git(args, env: source_repository.fetch_env)
       end
 
       def gitaly_ff_merge(user, source_sha, target_branch)

spec/lib/gitlab/git/remote_repository_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Git::RemoteRepository, seed_helper: true do
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
+  subject { described_class.new(repository) }
+
+  describe '#empty_repo?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:repository, :result) do
+      Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '')       | false
+      Gitlab::Git::Repository.new('default', 'does-not-exist.git', '') | true
+    end
+
+    with_them do
+      it { expect(subject.empty_repo?).to eq(result) }
+    end
+  end
+
+  describe '#commit_id' do
+    it 'returns an OID if the revision exists' do
+      expect(subject.commit_id('v1.0.0')).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+    end
+
+    it 'is nil when the revision does not exist' do
+      expect(subject.commit_id('does-not-exist')).to be_nil
+    end
+  end
+
+  describe '#branch_exists?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:branch, :result) do
+      'master'         | true
+      'does-not-exist' | false
+    end
+
+    with_them do
+      it { expect(subject.branch_exists?(branch)).to eq(result) }
+    end
+  end
+
+  describe '#same_repository?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:other_repository, :result) do
+      repository                                                                     | true
+      Gitlab::Git::Repository.new(repository.storage, repository.relative_path, '')  | true
+      Gitlab::Git::Repository.new('broken', TEST_REPO_PATH, '')                      | false
+      Gitlab::Git::Repository.new(repository.storage, 'wrong/relative-path.git', '') | false
+      Gitlab::Git::Repository.new('broken', 'wrong/relative-path.git', '')           | false
+    end
+
+    with_them do
+      it { expect(subject.same_repository?(other_repository)).to eq(result) }
+    end
+  end
+
+  describe '#fetch_env' do
+    let(:remote_repository) { described_class.new(repository) }
+
+    let(:gitaly_client) { double(:gitaly_client) }
+    let(:address) { 'fake-address' }
+    let(:token) { 'fake-token' }
+
+    subject { remote_repository.fetch_env }
+
+    before do
+      allow(remote_repository).to receive(:gitaly_client).and_return(gitaly_client)
+
+      expect(gitaly_client).to receive(:address).with(repository.storage).and_return(address)
+      expect(gitaly_client).to receive(:token).with(repository.storage).and_return(token)
+    end
+
+    it { expect(subject).to be_a(Hash) }
+    it { expect(subject['GITALY_ADDRESS']).to eq(address) }
+    it { expect(subject['GITALY_TOKEN']).to eq(token) }
+    it { expect(subject['GITALY_WD']).to eq(Dir.pwd) }
+
+    it 'creates a plausible GIT_SSH_COMMAND' do
+      git_ssh_command = subject['GIT_SSH_COMMAND']
+
+      expect(git_ssh_command).to start_with('/')
+      expect(git_ssh_command).to end_with('/gitaly-ssh upload-pack')
+    end
+
+    it 'creates a plausible GITALY_PAYLOAD' do
+      req = Gitaly::SSHUploadPackRequest.decode_json(subject['GITALY_PAYLOAD'])
+
+      expect(remote_repository.gitaly_repository).to eq(req.repository)
+    end
+
+    context 'when the token is blank' do
+      let(:token) { '' }
+
+      it { expect(subject.keys).not_to include('GITALY_TOKEN') }
+    end
+  end
+end

spec/lib/gitlab/git/repository_spec.rb

@@ -449,7 +449,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
       let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
 
       after do
-        FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
         ensure_seeds
       end
 
@@ -484,7 +483,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
       let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
 
       after do
-        FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
         ensure_seeds
       end
 
@@ -544,7 +542,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     after(:all) do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
       ensure_seeds
     end
   end
@@ -570,7 +567,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     after(:all) do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
       ensure_seeds
     end
   end
@@ -588,7 +584,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     after(:all) do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
       ensure_seeds
     end
   end
@@ -1122,7 +1117,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
 
       after do
-        FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
         ensure_seeds
       end
 
@@ -1169,7 +1163,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
 
       after do
-        FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
         ensure_seeds
       end
 
@@ -1419,7 +1412,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     after(:all) do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
       ensure_seeds
     end
 
@@ -1537,19 +1529,42 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#fetch_source_branch!' do
+    shared_examples '#fetch_source_branch!' do
       let(:local_ref) { 'refs/merge-requests/1/head' }
+      let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
+      let(:source_repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
+
+      after do
+        ensure_seeds
+      end
 
       context 'when the branch exists' do
+        context 'when the commit does not exist locally' do
+          let(:source_branch) { 'new-branch-for-fetch-source-branch' }
+          let(:source_rugged) { source_repository.rugged }
+          let(:new_oid) { new_commit_edit_old_file(source_rugged).oid }
+
+          before do
+            source_rugged.branches.create(source_branch, new_oid)
+          end
+
+          it 'writes the ref' do
+            expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(true)
+            expect(repository.commit(local_ref).sha).to eq(new_oid)
+          end
+        end
+
+        context 'when the commit exists locally' do
           let(:source_branch) { 'master' }
+          let(:expected_oid) { SeedRepo::LastCommit::ID }
 
           it 'writes the ref' do
-        expect(repository).to receive(:write_ref).with(local_ref, /\h{40}/)
+            # Sanity check: the commit should already exist
+            expect(repository.commit(expected_oid)).not_to be_nil
 
-        repository.fetch_source_branch!(repository, source_branch, local_ref)
+            expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(true)
+            expect(repository.commit(local_ref).sha).to eq(expected_oid)
           end
-
-      it 'returns true' do
-        expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(true)
         end
       end
 
@@ -1557,14 +1572,16 @@ describe Gitlab::Git::Repository, seed_helper: true do
         let(:source_branch) { 'definitely-not-master' }
 
         it 'does not write the ref' do
-        expect(repository).not_to receive(:write_ref)
-
-        repository.fetch_source_branch!(repository, source_branch, local_ref)
+          expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(false)
+          expect(repository.commit(local_ref)).to be_nil
+        end
       end
-
-      it 'returns false' do
-        expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(false)
     end
+
+    it_behaves_like '#fetch_source_branch!'
+
+    context 'without gitaly', :skip_gitaly_mock do
+      it_behaves_like '#fetch_source_branch!'
     end
   end
 
@@ -1641,7 +1658,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     after do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
       ensure_seeds
     end