diff options
| -rw-r--r-- | lib/gitlab/git/repository.rb | 21 | ||||
| -rw-r--r-- | lib/gitlab/git/rev_list.rb | 33 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/branch_spec.rb | 8 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/diff_spec.rb | 9 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 121 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/rev_list_spec.rb | 61 | ||||
| -rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 12 | ||||
| -rw-r--r-- | spec/models/repository_spec.rb | 11 | ||||
| -rw-r--r-- | spec/workers/git_garbage_collect_worker_spec.rb | 7 |
9 files changed, 107 insertions, 176 deletions
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index a1a050647b9..d50ac2707f0 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -86,9 +86,6 @@ module Gitlab # Relative path of repo attr_reader :relative_path - # Rugged repo object - attr_reader :rugged - attr_reader :gitlab_projects, :storage, :gl_repository, :relative_path # This initializer method is only used on the client side (gitlab-ce). @@ -112,8 +109,9 @@ module Gitlab [storage, relative_path] == [other.storage, other.relative_path] end + # This method will be removed when Gitaly reaches v1.1. def path - @path ||= File.join( + File.join( Gitlab.config.repositories.storages[@storage].legacy_disk_path, @relative_path ) end @@ -127,8 +125,9 @@ module Gitlab raise Gitlab::Git::CommandError.new(e.message) end + # This method will be removed when Gitaly reaches v1.1. def rugged - @rugged ||= circuit_breaker.perform do + circuit_breaker.perform do Rugged::Repository.new(path, alternates: alternate_object_directories) end rescue Rugged::RepositoryError, Rugged::OSError @@ -713,12 +712,6 @@ module Gitlab Gitlab::Git.committer_hash(email: user.email, name: user.name) end - def create_commit(params = {}) - params[:message].delete!("\r") - - Rugged::Commit.create(rugged, params) - end - # Delete the specified branch from the repository def delete_branch(branch_name) gitaly_migrate(:delete_branch, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled| @@ -1758,6 +1751,12 @@ module Gitlab def sha_from_ref(ref) rev_parse_target(ref).oid end + + def create_commit(params = {}) + params[:message].delete!("\r") + + Rugged::Commit.create(rugged, params) + end end end end diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb index 5fdad077eea..2ba68343aa5 100644 --- a/lib/gitlab/git/rev_list.rb +++ b/lib/gitlab/git/rev_list.rb @@ -12,35 +12,12 @@ module Gitlab end # This method returns an array of new commit references - def new_refs - repository.rev_list(including: newrev, excluding: :all).split("\n") - end - - # Finds newly added objects - # Returns an array of shas + # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233 # - # Can skip objects which do not have a path using required_path: true - # This skips commit objects and root trees, which might not be needed when - # looking for blobs - # - # When given a block it will yield objects as a lazy enumerator so - # the caller can limit work done instead of processing megabytes of data - def new_objects(options: [], require_path: nil, not_in: nil, &lazy_block) - opts = { - including: newrev, - options: options, - excluding: not_in.nil? ? :all : not_in, - require_path: require_path - } - - get_objects(opts, &lazy_block) - end - - def all_objects(options: [], require_path: nil, &lazy_block) - get_objects(including: :all, - options: options, - require_path: require_path, - &lazy_block) + def new_refs + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + repository.rev_list(including: newrev, excluding: :all).split("\n") + end end private diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb index ec1a684cfbc..a8c5627e678 100644 --- a/spec/lib/gitlab/git/branch_spec.rb +++ b/spec/lib/gitlab/git/branch_spec.rb @@ -2,6 +2,11 @@ require "spec_helper" describe Gitlab::Git::Branch, seed_helper: true do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } + let(:rugged) do + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + repository.rugged + end + end subject { repository.branches } @@ -124,6 +129,7 @@ describe Gitlab::Git::Branch, seed_helper: true do it { expect(repository.branches.size).to eq(SeedRepo::Repo::BRANCHES.size) } def create_commit - repository.create_commit(params.merge(committer: committer.merge(time: Time.now))) + params[:message].delete!("\r") + Rugged::Commit.create(rugged, params.merge(committer: committer.merge(time: Time.now))) end end diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 3bb0b5be15b..7c3d2af819b 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -27,6 +27,7 @@ EOT too_large: false } + # TODO use a Gitaly diff object instead @rugged_diff = Gitlab::GitalyClient::StorageSettings.allow_disk_access do repository.rugged.diff("5937ac0a7beb003549fc5fd26fc247adbce4a52e^", "5937ac0a7beb003549fc5fd26fc247adbce4a52e", paths: [".gitmodules"]).patches.first @@ -266,8 +267,12 @@ EOT describe '#submodule?' do before do - commit = repository.lookup('5937ac0a7beb003549fc5fd26fc247adbce4a52e') - @diffs = commit.parents[0].diff(commit).patches + # TODO use a Gitaly diff object instead + rugged_commit = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + repository.lookup('5937ac0a7beb003549fc5fd26fc247adbce4a52e') + end + + @diffs = rugged_commit.parents[0].diff(rugged_commit).patches end it { expect(described_class.new(@diffs[0]).submodule?).to eq(false) } diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 4f8e6f29255..f155ebc9d1a 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -611,21 +611,6 @@ describe Gitlab::Git::Repository, seed_helper: true do end end - describe "#remove_remote" do - before(:all) do - @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') - @repo.remove_remote("expendable") - end - - it "should remove the remote" do - expect(@repo.rugged.remotes).not_to include("expendable") - end - - after(:all) do - ensure_seeds - end - end - describe "#remote_update" do before(:all) do @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') @@ -633,7 +618,9 @@ describe Gitlab::Git::Repository, seed_helper: true do end it "should add the remote" do - expect(@repo.rugged.remotes["expendable"].url).to( + rugged = Gitlab::GitalyClient::StorageSettings.allow_disk_access { @repo.rugged } + + expect(rugged.remotes["expendable"].url).to( eq(TEST_NORMAL_REPO_PATH) ) end @@ -1157,6 +1144,13 @@ describe Gitlab::Git::Repository, seed_helper: true do @repo.rugged.config['core.autocrlf'] = true end + around do |example| + # OK because autocrlf is only used in gitaly-ruby + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + example.run + end + end + it 'return the value of the autocrlf option' do expect(@repo.autocrlf).to be(true) end @@ -1172,6 +1166,13 @@ describe Gitlab::Git::Repository, seed_helper: true do @repo.rugged.config['core.autocrlf'] = false end + around do |example| + # OK because autocrlf= is only used in gitaly-ruby + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + example.run + end + end + it 'should set the autocrlf option to the provided option' do @repo.autocrlf = :input @@ -2042,54 +2043,61 @@ describe Gitlab::Git::Repository, seed_helper: true do let(:repository) do Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') end + let(:rugged) do + Gitlab::GitalyClient::StorageSettings.allow_disk_access { repository.rugged } + end let(:remote_name) { 'my-remote' } + let(:url) { 'http://my-repo.git' } after do ensure_seeds end describe '#add_remote' do - let(:url) { 'http://my-repo.git' } let(:mirror_refmap) { '+refs/*:refs/*' } - it 'creates a new remote via Gitaly' do - expect_any_instance_of(Gitlab::GitalyClient::RemoteService) - .to receive(:add_remote).with(remote_name, url, mirror_refmap) + shared_examples 'add_remote' do + it 'added the remote' do + begin + rugged.remotes.delete(remote_name) + rescue Rugged::ConfigError + end + + repository.add_remote(remote_name, url, mirror_refmap: mirror_refmap) - repository.add_remote(remote_name, url, mirror_refmap: mirror_refmap) + expect(rugged.remotes[remote_name]).not_to be_nil + expect(rugged.config["remote.#{remote_name}.mirror"]).to eq('true') + expect(rugged.config["remote.#{remote_name}.prune"]).to eq('true') + expect(rugged.config["remote.#{remote_name}.fetch"]).to eq(mirror_refmap) + end end - context 'with Gitaly disabled', :skip_gitaly_mock do - it 'creates a new remote via Rugged' do - expect_any_instance_of(Rugged::RemoteCollection).to receive(:create) - .with(remote_name, url) - expect_any_instance_of(Rugged::Config).to receive(:[]=) - .with("remote.#{remote_name}.mirror", true) - expect_any_instance_of(Rugged::Config).to receive(:[]=) - .with("remote.#{remote_name}.prune", true) - expect_any_instance_of(Rugged::Config).to receive(:[]=) - .with("remote.#{remote_name}.fetch", mirror_refmap) + context 'using Gitaly' do + it_behaves_like 'add_remote' + end - repository.add_remote(remote_name, url, mirror_refmap: mirror_refmap) - end + context 'with Gitaly disabled', :disable_gitaly do + it_behaves_like 'add_remote' end end describe '#remove_remote' do - it 'removes the remote via Gitaly' do - expect_any_instance_of(Gitlab::GitalyClient::RemoteService) - .to receive(:remove_remote).with(remote_name) + shared_examples 'remove_remote' do + it 'removes the remote' do + rugged.remotes.create(remote_name, url) - repository.remove_remote(remote_name) + repository.remove_remote(remote_name) + + expect(rugged.remotes[remote_name]).to be_nil + end end - context 'with Gitaly disabled', :skip_gitaly_mock do - it 'removes the remote via Rugged' do - expect_any_instance_of(Rugged::RemoteCollection).to receive(:delete) - .with(remote_name) + context 'using Gitaly' do + it_behaves_like 'remove_remote' + end - repository.remove_remote(remote_name) - end + context 'with Gitaly disabled', :disable_gitaly do + it_behaves_like 'remove_remote' end end end @@ -2281,20 +2289,25 @@ describe Gitlab::Git::Repository, seed_helper: true do let(:worktree_path) { File.join(repository_path, 'worktrees', 'delete-me') } it 'cleans up the files' do - repository.with_worktree(worktree_path, 'master', env: ENV) do - FileUtils.touch(worktree_path, mtime: Time.now - 8.hours) - # git rev-list --all will fail in git 2.16 if HEAD is pointing to a non-existent object, - # but the HEAD must be 40 characters long or git will ignore it. - File.write(File.join(worktree_path, 'HEAD'), Gitlab::Git::BLANK_SHA) + create_worktree = %W[git -C #{repository_path} worktree add --detach #{worktree_path} master] + raise 'preparation failed' unless system(*create_worktree, err: '/dev/null') - # git 2.16 fails with "fatal: bad object HEAD" - expect { repository.rev_list(including: :all) }.to raise_error(Gitlab::Git::Repository::GitError) + FileUtils.touch(worktree_path, mtime: Time.now - 8.hours) + # git rev-list --all will fail in git 2.16 if HEAD is pointing to a non-existent object, + # but the HEAD must be 40 characters long or git will ignore it. + File.write(File.join(worktree_path, 'HEAD'), Gitlab::Git::BLANK_SHA) - repository.clean_stale_repository_files + # git 2.16 fails with "fatal: bad object HEAD" + expect(rev_list_all).to be false - expect { repository.rev_list(including: :all) }.not_to raise_error - expect(File.exist?(worktree_path)).to be_falsey - end + repository.clean_stale_repository_files + + expect(rev_list_all).to be true + expect(File.exist?(worktree_path)).to be_falsey + end + + def rev_list_all + system(*%W[git -C #{repository_path} rev-list --all], out: '/dev/null', err: '/dev/null') end it 'increments a counter upon an error' do diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb index b752c3e8341..d9d405e1ccc 100644 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ b/spec/lib/gitlab/git/rev_list_spec.rb @@ -32,65 +32,4 @@ describe Gitlab::Git::RevList do expect(rev_list.new_refs).to eq(%w[sha1 sha2]) end end - - context '#new_objects' do - it 'fetches list of newly pushed objects using rev-list' do - stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2") - - expect { |b| rev_list.new_objects(&b) }.to yield_with_args(%w[sha1 sha2]) - end - - it 'can skip pathless objects' do - stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2 path/to/file") - - expect { |b| rev_list.new_objects(require_path: true, &b) }.to yield_with_args(%w[sha2]) - end - - it 'can handle non utf-8 paths' do - non_utf_char = [0x89].pack("c*").force_encoding("UTF-8") - stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha2 πå†h/†ø/ƒîlé#{non_utf_char}\nsha1") - - rev_list.new_objects(require_path: true) do |object_ids| - expect(object_ids.force).to eq(%w[sha2]) - end - end - - it 'can yield a lazy enumerator' do - stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2") - - rev_list.new_objects do |object_ids| - expect(object_ids).to be_a Enumerator::Lazy - end - end - - it 'returns the result of the block when given' do - stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2") - - objects = rev_list.new_objects do |object_ids| - object_ids.first - end - - expect(objects).to eq 'sha1' - end - - it 'can accept list of references to exclude' do - stub_popen_rev_list('newrev', '--not', 'master', '--objects', output: "sha1\nsha2") - - expect { |b| rev_list.new_objects(not_in: ['master'], &b) }.to yield_with_args(%w[sha1 sha2]) - end - - it 'handles empty list of references to exclude as listing all known objects' do - stub_popen_rev_list('newrev', '--objects', output: "sha1\nsha2") - - expect { |b| rev_list.new_objects(not_in: [], &b) }.to yield_with_args(%w[sha1 sha2]) - end - end - - context '#all_objects' do - it 'fetches list of all pushed objects using rev-list' do - stub_popen_rev_list('--all', '--objects', output: "sha1\nsha2") - - expect { |b| rev_list.all_objects(&b) }.to yield_with_args(%w[sha1 sha2]) - end - end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 832db2bd906..dbd64c4bec0 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -734,26 +734,18 @@ describe Gitlab::GitAccess do merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/feature" } end - def stub_git_hooks - # Running the `pre-receive` hook is expensive, and not necessary for this test. - allow_any_instance_of(Gitlab::Git::HooksService).to receive(:execute) do |service, &block| - block.call(service) - end - end - def merge_into_protected_branch @protected_branch_merge_commit ||= begin Gitlab::GitalyClient::StorageSettings.allow_disk_access do - stub_git_hooks project.repository.add_branch(user, unprotected_branch, 'feature') - target_branch = project.repository.lookup('feature') + rugged = project.repository.rugged + target_branch = rugged.rev_parse('feature') source_branch = project.repository.create_file( user, 'filename', 'This is the file content', message: 'This is a good commit message', branch_name: unprotected_branch) - rugged = project.repository.rugged author = { email: "email@example.com", time: Time.now, name: "Example Git User" } merge_index = rugged.merge_commits(target_branch, source_branch) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index caf5d829d21..04d00023b8a 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -151,7 +151,9 @@ describe Repository do it { is_expected.to eq(['v1.1.0', 'v1.0.0', annotated_tag_name]) } after do - repository.rugged.tags.delete(annotated_tag_name) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + repository.rugged.tags.delete(annotated_tag_name) + end end end end @@ -2231,8 +2233,11 @@ describe Repository do create_remote_branch('joe', 'remote_branch', masterrev) repository.add_branch(user, 'local_branch', masterrev.id) - expect(repository.remote_branches('joe').any? { |branch| branch.name == 'local_branch' }).to eq(false) - expect(repository.remote_branches('joe').any? { |branch| branch.name == 'remote_branch' }).to eq(true) + # TODO: move this test to gitaly https://gitlab.com/gitlab-org/gitaly/issues/1243 + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + expect(repository.remote_branches('joe').any? { |branch| branch.name == 'local_branch' }).to eq(false) + expect(repository.remote_branches('joe').any? { |branch| branch.name == 'remote_branch' }).to eq(true) + end end end diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb index d5808e21271..30e67e67e0e 100644 --- a/spec/workers/git_garbage_collect_worker_spec.rb +++ b/spec/workers/git_garbage_collect_worker_spec.rb @@ -209,12 +209,7 @@ describe GitGarbageCollectWorker do tree: old_commit.tree, parents: [old_commit] ) - Gitlab::Git::OperationService.new(nil, project.repository.raw_repository).send( - :update_ref, - "refs/heads/#{SecureRandom.hex(6)}", - new_commit_sha, - Gitlab::Git::BLANK_SHA - ) + rugged.references.create("refs/heads/#{SecureRandom.hex(6)}", new_commit_sha) end def packs(project) |