Merge branch 'gitlab-git-dont-memoize-path' into 'master'

Remove Repository#path memoization

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

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)