From 03ac8d5d0b8331a2ecfae05137e02c78428fa899 Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <zegerjan@gitlab.com>
Date: Thu, 7 Dec 2017 15:33:30 +0000
Subject: [PATCH] Remove Rugged::Repository#empty?

---
 app/models/project.rb                         |   5 +-
 app/models/repository.rb                      |  17 ++-
 lib/backup/repository.rb                      |   9 +-
 lib/gitlab/git/operation_service.rb           |   2 +-
 lib/gitlab/git/remote_repository.rb           |   6 +-
 lib/gitlab/git/repository.rb                  |  32 ++---
 lib/gitlab/gitaly_client.rb                   |   2 +-
 spec/lib/{gitlab => }/backup/manager_spec.rb  |   0
 spec/lib/backup/repository_spec.rb            |  69 +++++++++++
 spec/lib/gitlab/backup/repository_spec.rb     | 117 ------------------
 spec/lib/gitlab/git/remote_repository_spec.rb |   4 +-
 spec/lib/gitlab/git/repository_spec.rb        |   2 +-
 spec/models/project_spec.rb                   |  19 ++-
 spec/models/repository_spec.rb                |  12 +-
 spec/tasks/gitlab/backup_rake_spec.rb         |   2 +-
 15 files changed, 127 insertions(+), 171 deletions(-)
 rename spec/lib/{gitlab => }/backup/manager_spec.rb (100%)
 create mode 100644 spec/lib/backup/repository_spec.rb
 delete mode 100644 spec/lib/gitlab/backup/repository_spec.rb

diff --git a/app/models/project.rb b/app/models/project.rb
index 41657c171e2..6ae15a0a50f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -227,7 +227,6 @@ class Project < ActiveRecord::Base
   delegate :members, to: :team, prefix: true
   delegate :add_user, :add_users, to: :team
   delegate :add_guest, :add_reporter, :add_developer, :add_master, to: :team
-  delegate :empty_repo?, to: :repository
 
   # Validations
   validates :creator, presence: true, on: :create
@@ -499,6 +498,10 @@ class Project < ActiveRecord::Base
     auto_devops&.enabled.nil? && !current_application_settings.auto_devops_enabled?
   end
 
+  def empty_repo?
+    repository.empty?
+  end
+
   def repository_storage_path
     Gitlab.config.repositories.storages[repository_storage].try(:[], 'path')
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 82af299ec5e..751306188a0 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -37,7 +37,7 @@ class Repository
                       issue_template_names merge_request_template_names).freeze
 
   # Methods that use cache_method but only memoize the value
-  MEMOIZED_CACHED_METHODS = %i(license empty_repo?).freeze
+  MEMOIZED_CACHED_METHODS = %i(license).freeze
 
   # Certain method caches should be refreshed when certain types of files are
   # changed. This Hash maps file types (as returned by Gitlab::FileDetector) to
@@ -497,7 +497,11 @@ class Repository
   end
   cache_method :exists?
 
-  delegate :empty?, to: :raw_repository
+  def empty?
+    return true unless exists?
+
+    !has_visible_content?
+  end
   cache_method :empty?
 
   # The size of this repository in megabytes.
@@ -944,13 +948,8 @@ class Repository
     end
   end
 
-  def empty_repo?
-    !exists? || !has_visible_content?
-  end
-  cache_method :empty_repo?, memoize_only: true
-
   def search_files_by_content(query, ref)
-    return [] if empty_repo? || query.blank?
+    return [] if empty? || query.blank?
 
     offset = 2
     args = %W(grep -i -I -n --before-context #{offset} --after-context #{offset} -E -e #{Regexp.escape(query)} #{ref || root_ref})
@@ -959,7 +958,7 @@ class Repository
   end
 
   def search_files_by_name(query, ref)
-    return [] if empty_repo? || query.blank?
+    return [] if empty? || query.blank?
 
     args = %W(ls-tree --full-tree -r #{ref || root_ref} --name-status | #{Regexp.escape(query)})
 
diff --git a/lib/backup/repository.rb b/lib/backup/repository.rb
index b6d273b98c2..2a04c03919d 100644
--- a/lib/backup/repository.rb
+++ b/lib/backup/repository.rb
@@ -193,12 +193,9 @@ module Backup
     end
 
     def empty_repo?(project_or_wiki)
-      project_or_wiki.repository.expire_exists_cache # protect backups from stale cache
-      project_or_wiki.repository.empty_repo?
-    rescue => e
-      progress.puts "Ignoring repository error and continuing backing up project: #{display_repo_path(project_or_wiki)} - #{e.message}".color(:orange)
-
-      false
+      # Protect against stale caches
+      project_or_wiki.repository.expire_emptiness_caches
+      project_or_wiki.repository.empty?
     end
 
     def repository_storage_paths_args
diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb
index e36d5410431..7e8fe173056 100644
--- a/lib/gitlab/git/operation_service.rb
+++ b/lib/gitlab/git/operation_service.rb
@@ -83,7 +83,7 @@ module Gitlab
         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?
+        start_branch_name = nil if start_repository.empty?
 
         if start_branch_name && !start_repository.branch_exists?(start_branch_name)
           raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.relative_path}"
diff --git a/lib/gitlab/git/remote_repository.rb b/lib/gitlab/git/remote_repository.rb
index 3685aa20669..6bd6e58feeb 100644
--- a/lib/gitlab/git/remote_repository.rb
+++ b/lib/gitlab/git/remote_repository.rb
@@ -24,10 +24,12 @@ module Gitlab
         @path = repository.path
       end
 
-      def empty_repo?
+      def empty?
         # We will override this implementation in gitaly-ruby because we cannot
         # use '@repository' there.
-        @repository.empty_repo?
+        #
+        # Caches and memoization used on the Rails side
+        !@repository.exists? || @repository.empty?
       end
 
       def commit_id(revision)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 1468069a991..91dd2fbbdbc 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -75,9 +75,6 @@ module Gitlab
         @attributes = Gitlab::Git::Attributes.new(path)
       end
 
-      delegate  :empty?,
-                to: :rugged
-
       def ==(other)
         path == other.path
       end
@@ -206,6 +203,13 @@ module Gitlab
         end
       end
 
+      # Git repository can contains some hidden refs like:
+      #   /refs/notes/*
+      #   /refs/git-as-svn/*
+      #   /refs/pulls/*
+      # This refs by default not visible in project page and not cloned to client side.
+      alias_method :has_visible_content?, :has_local_branches?
+
       def has_local_branches_rugged?
         rugged.branches.each(:local).any? do |ref|
           begin
@@ -1004,7 +1008,7 @@ module Gitlab
         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?
+        return yield nil if start_repository.empty?
 
         if start_repository.same_repository?(self)
           yield commit(start_branch_name)
@@ -1120,24 +1124,8 @@ module Gitlab
         Gitlab::Git::Commit.find(self, ref)
       end
 
-      # Refactoring aid; allows us to copy code from app/models/repository.rb
-      def empty_repo?
-        !exists? || !has_visible_content?
-      end
-
-      #
-      # Git repository can contains some hidden refs like:
-      #   /refs/notes/*
-      #   /refs/git-as-svn/*
-      #   /refs/pulls/*
-      # This refs by default not visible in project page and not cloned to client side.
-      #
-      # This method return true if repository contains some content visible in project page.
-      #
-      def has_visible_content?
-        return @has_visible_content if defined?(@has_visible_content)
-
-        @has_visible_content = has_local_branches?
+      def empty?
+        !has_visible_content?
       end
 
       # Like all public `Gitlab::Git::Repository` methods, this method is part
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 1fe938a39a8..b753ac46291 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -27,7 +27,7 @@ module Gitlab
     end
 
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
-    MAXIMUM_GITALY_CALLS = 30
+    MAXIMUM_GITALY_CALLS = 35
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
 
     MUTEX = Mutex.new
diff --git a/spec/lib/gitlab/backup/manager_spec.rb b/spec/lib/backup/manager_spec.rb
similarity index 100%
rename from spec/lib/gitlab/backup/manager_spec.rb
rename to spec/lib/backup/manager_spec.rb
diff --git a/spec/lib/backup/repository_spec.rb b/spec/lib/backup/repository_spec.rb
new file mode 100644
index 00000000000..6ee3d531d6e
--- /dev/null
+++ b/spec/lib/backup/repository_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe Backup::Repository do
+  let(:progress) { StringIO.new }
+  let!(:project) { create(:project) }
+
+  before do
+    allow(progress).to receive(:puts)
+    allow(progress).to receive(:print)
+
+    allow_any_instance_of(String).to receive(:color) do |string, _color|
+      string
+    end
+
+    allow_any_instance_of(described_class).to receive(:progress).and_return(progress)
+  end
+
+  describe '#dump' do
+    describe 'repo failure' do
+      before do
+        allow(Gitlab::Popen).to receive(:popen).and_return(['normal output', 0])
+      end
+
+      it 'does not raise error' do
+        expect { described_class.new.dump }.not_to raise_error
+      end
+    end
+  end
+
+  describe '#restore' do
+    describe 'command failure' do
+      before do
+        allow(Gitlab::Popen).to receive(:popen).and_return(['error', 1])
+      end
+
+      it 'shows the appropriate error' do
+        described_class.new.restore
+
+        expect(progress).to have_received(:puts).with("Ignoring error on #{project.full_path} - error")
+      end
+    end
+  end
+
+  describe '#empty_repo?' do
+    context 'for a wiki' do
+      let(:wiki) { create(:project_wiki) }
+
+      it 'invalidates the emptiness cache' do
+        expect(wiki.repository).to receive(:expire_emptiness_caches).once
+
+        wiki.empty?
+      end
+
+      context 'wiki repo has content' do
+        let!(:wiki_page) { create(:wiki_page, wiki: wiki) }
+
+        it 'returns true, regardless of bad cache value' do
+          expect(described_class.new.send(:empty_repo?, wiki)).to be(false)
+        end
+      end
+
+      context 'wiki repo does not have content' do
+        it 'returns true, regardless of bad cache value' do
+          expect(described_class.new.send(:empty_repo?, wiki)).to be_truthy
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/backup/repository_spec.rb b/spec/lib/gitlab/backup/repository_spec.rb
deleted file mode 100644
index 535cce12780..00000000000
--- a/spec/lib/gitlab/backup/repository_spec.rb
+++ /dev/null
@@ -1,117 +0,0 @@
-require 'spec_helper'
-
-describe Backup::Repository do
-  let(:progress) { StringIO.new }
-  let!(:project) { create(:project) }
-
-  before do
-    allow(progress).to receive(:puts)
-    allow(progress).to receive(:print)
-
-    allow_any_instance_of(String).to receive(:color) do |string, _color|
-      string
-    end
-
-    allow_any_instance_of(described_class).to receive(:progress).and_return(progress)
-  end
-
-  describe '#dump' do
-    describe 'repo failure' do
-      before do
-        allow_any_instance_of(Repository).to receive(:empty_repo?).and_raise(Rugged::OdbError)
-        allow(Gitlab::Popen).to receive(:popen).and_return(['normal output', 0])
-      end
-
-      it 'does not raise error' do
-        expect { described_class.new.dump }.not_to raise_error
-      end
-
-      it 'shows the appropriate error' do
-        described_class.new.dump
-
-        expect(progress).to have_received(:puts).with("Ignoring repository error and continuing backing up project: #{project.full_path} - Rugged::OdbError")
-      end
-    end
-
-    describe 'command failure' do
-      before do
-        allow_any_instance_of(Repository).to receive(:empty_repo?).and_return(false)
-        allow(Gitlab::Popen).to receive(:popen).and_return(['error', 1])
-      end
-
-      it 'shows the appropriate error' do
-        described_class.new.dump
-
-        expect(progress).to have_received(:puts).with("Ignoring error on #{project.full_path} - error")
-      end
-    end
-  end
-
-  describe '#restore' do
-    describe 'command failure' do
-      before do
-        allow(Gitlab::Popen).to receive(:popen).and_return(['error', 1])
-      end
-
-      it 'shows the appropriate error' do
-        described_class.new.restore
-
-        expect(progress).to have_received(:puts).with("Ignoring error on #{project.full_path} - error")
-      end
-    end
-  end
-
-  describe '#empty_repo?' do
-    context 'for a wiki' do
-      let(:wiki) { create(:project_wiki) }
-
-      context 'wiki repo has content' do
-        let!(:wiki_page) { create(:wiki_page, wiki: wiki) }
-
-        before do
-          wiki.repository.exists? # initial cache
-        end
-
-        context '`repository.exists?` is incorrectly cached as false' do
-          before do
-            repo = wiki.repository
-            repo.send(:cache).expire(:exists?)
-            repo.send(:cache).fetch(:exists?) { false }
-            repo.send(:instance_variable_set, :@exists, false)
-          end
-
-          it 'returns false, regardless of bad cache value' do
-            expect(described_class.new.send(:empty_repo?, wiki)).to be_falsey
-          end
-        end
-
-        context '`repository.exists?` is correctly cached as true' do
-          it 'returns false' do
-            expect(described_class.new.send(:empty_repo?, wiki)).to be_falsey
-          end
-        end
-      end
-
-      context 'wiki repo does not have content' do
-        context '`repository.exists?` is incorrectly cached as true' do
-          before do
-            repo = wiki.repository
-            repo.send(:cache).expire(:exists?)
-            repo.send(:cache).fetch(:exists?) { true }
-            repo.send(:instance_variable_set, :@exists, true)
-          end
-
-          it 'returns true, regardless of bad cache value' do
-            expect(described_class.new.send(:empty_repo?, wiki)).to be_truthy
-          end
-        end
-
-        context '`repository.exists?` is correctly cached as false' do
-          it 'returns true' do
-            expect(described_class.new.send(:empty_repo?, wiki)).to be_truthy
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/git/remote_repository_spec.rb b/spec/lib/gitlab/git/remote_repository_spec.rb
index 0506210887c..eb148cc3804 100644
--- a/spec/lib/gitlab/git/remote_repository_spec.rb
+++ b/spec/lib/gitlab/git/remote_repository_spec.rb
@@ -4,7 +4,7 @@ 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
+  describe '#empty?' do
     using RSpec::Parameterized::TableSyntax
 
     where(:repository, :result) do
@@ -13,7 +13,7 @@ describe Gitlab::Git::RemoteRepository, seed_helper: true do
     end
 
     with_them do
-      it { expect(subject.empty_repo?).to eq(result) }
+      it { expect(subject.empty?).to eq(result) }
     end
   end
 
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 08dd6ea80ff..f19b65a5f71 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -257,7 +257,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#empty?' do
-    it { expect(repository.empty?).to be_falsey }
+    it { expect(repository).not_to be_empty }
   end
 
   describe '#ref_names' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index bda1d1cb612..f4699fd243d 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -313,7 +313,6 @@ describe Project do
       it { is_expected.to delegate_method(method).to(:team) }
     end
 
-    it { is_expected.to delegate_method(:empty_repo?).to(:repository) }
     it { is_expected.to delegate_method(:members).to(:team).with_prefix(true) }
     it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).with_arguments(allow_nil: true) }
   end
@@ -656,6 +655,24 @@ describe Project do
     end
   end
 
+  describe '#empty_repo?' do
+    context 'when the repo does not exist' do
+      let(:project) { build_stubbed(:project) }
+
+      it 'returns true' do
+        expect(project.empty_repo?).to be(true)
+      end
+    end
+
+    context 'when the repo exists' do
+      let(:project) { create(:project, :repository) }
+      let(:empty_project) { create(:project, :empty_repo) }
+
+      it { expect(empty_project.empty_repo?).to be(true) }
+      it { expect(project.empty_repo?).to be(false) }
+    end
+  end
+
   describe '#external_issue_tracker' do
     let(:project) { create(:project) }
     let(:ext_project) { create(:redmine_project) }
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index d37e3d2c527..82ed1ecee33 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -583,7 +583,7 @@ describe Repository do
     end
 
     it 'properly handles query when repo is empty' do
-      repository = create(:project).repository
+      repository = create(:project, :empty_repo).repository
       results = repository.search_files_by_content('test', 'master')
 
       expect(results).to match_array([])
@@ -619,7 +619,7 @@ describe Repository do
     end
 
     it 'properly handles query when repo is empty' do
-      repository = create(:project).repository
+      repository = create(:project, :empty_repo).repository
 
       results = repository.search_files_by_name('test', 'master')
 
@@ -1204,17 +1204,15 @@ describe Repository do
     let(:empty_repository) { create(:project_empty_repo).repository }
 
     it 'returns true for an empty repository' do
-      expect(empty_repository.empty?).to eq(true)
+      expect(empty_repository).to be_empty
     end
 
     it 'returns false for a non-empty repository' do
-      expect(repository.empty?).to eq(false)
+      expect(repository).not_to be_empty
     end
 
     it 'caches the output' do
-      expect(repository.raw_repository).to receive(:empty?)
-        .once
-        .and_return(false)
+      expect(repository.raw_repository).to receive(:has_visible_content?).once
 
       repository.empty?
       repository.empty?
diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb
index bf2e11bc360..b41c3b3958a 100644
--- a/spec/tasks/gitlab/backup_rake_spec.rb
+++ b/spec/tasks/gitlab/backup_rake_spec.rb
@@ -212,7 +212,7 @@ describe 'gitlab:app namespace rake task' do
 
         # Avoid asking gitaly about the root ref (which will fail beacuse of the
         # mocked storages)
-        allow_any_instance_of(Repository).to receive(:empty_repo?).and_return(false)
+        allow_any_instance_of(Repository).to receive(:empty?).and_return(false)
       end
 
       after do