diff --git a/app/models/repository.rb b/app/models/repository.rb index 6bdee538172..3a89fa9264b 100644 --- a/app/models/repository.rb +++ b/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 diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb index ab94ba8a73a..e36d5410431 100644 --- a/lib/gitlab/git/operation_service.rb +++ b/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 diff --git a/lib/gitlab/git/remote_repository.rb b/lib/gitlab/git/remote_repository.rb new file mode 100644 index 00000000000..3685aa20669 --- /dev/null +++ b/lib/gitlab/git/remote_repository.rb @@ -0,0 +1,82 @@ +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 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index d236e1b03e6..14a8d551524 100644 --- a/lib/gitlab/git/repository.rb +++ b/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) diff --git a/spec/lib/gitlab/git/remote_repository_spec.rb b/spec/lib/gitlab/git/remote_repository_spec.rb new file mode 100644 index 00000000000..0506210887c --- /dev/null +++ b/spec/lib/gitlab/git/remote_repository_spec.rb @@ -0,0 +1,99 @@ +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 diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index ee14b528ec2..5d990b42c24 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/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,34 +1529,59 @@ describe Gitlab::Git::Repository, seed_helper: true do end describe '#fetch_source_branch!' do - let(:local_ref) { 'refs/merge-requests/1/head' } + 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, '') } - context 'when the branch exists' do - let(:source_branch) { 'master' } - - it 'writes the ref' do - expect(repository).to receive(:write_ref).with(local_ref, /\h{40}/) - - repository.fetch_source_branch!(repository, source_branch, local_ref) + after do + ensure_seeds end - it 'returns true' do - expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(true) + 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 + # Sanity check: the commit should already exist + expect(repository.commit(expected_oid)).not_to be_nil + + 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 + end + end + + context 'when the branch does not exist' do + let(:source_branch) { 'definitely-not-master' } + + it 'does not write the ref' do + expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(false) + expect(repository.commit(local_ref)).to be_nil + end end end - context 'when the branch does not exist' do - let(:source_branch) { 'definitely-not-master' } + it_behaves_like '#fetch_source_branch!' - it 'does not write the ref' do - expect(repository).not_to receive(:write_ref) - - repository.fetch_source_branch!(repository, source_branch, local_ref) - end - - it 'returns false' do - expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(false) - end + 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