Merge branch 'make-has-visible-commit-more-efficient' into 'master'

Make Repository#has_visible_content more efficient

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

6 files changed, 91 insertions, 22 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 0315fed24c5..4cb9f1baffd 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -90,12 +90,6 @@ class Repository
     )
   end
 
-  # we need to have this method here because it is not cached in ::Git and
-  # the method is called multiple times for every request
-  def has_visible_content?
-    branch_count > 0
-  end
-
   def inspect
     "#<#{self.class.name}:#{@disk_path}>"
   end
@@ -522,9 +516,10 @@ class Repository
   delegate :tag_names, to: :raw_repository
   cache_method :tag_names, fallback: []
 
-  delegate :branch_count, :tag_count, to: :raw_repository
+  delegate :branch_count, :tag_count, :has_visible_content?, to: :raw_repository
   cache_method :branch_count, fallback: 0
   cache_method :tag_count, fallback: 0
+  cache_method :has_visible_content?, fallback: false
 
   def avatar
     if tree = file_on_head(:avatar)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 6c639f286ee..984e34a9ef1 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -190,6 +190,28 @@ module Gitlab
         end
       end
 
+      def has_local_branches?
+        gitaly_migrate(:has_local_branches) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.has_local_branches?
+          else
+            has_local_branches_rugged?
+          end
+        end
+      end
+
+      def has_local_branches_rugged?
+        rugged.branches.each(:local).any? do |ref|
+          begin
+            ref.name && ref.target # ensures the branch is valid
+
+            true
+          rescue Rugged::ReferenceError
+            false
+          end
+        end
+      end
+
       # Returns the number of valid tags
       def tag_count
         gitaly_migrate(:tag_names) do |is_enabled|
@@ -909,7 +931,9 @@ module Gitlab
       # This method return true if repository contains some content visible in project page.
       #
       def has_visible_content?
-        branch_count > 0
+        return @has_visible_content if defined?(@has_visible_content)
+
+        @has_visible_content = has_local_branches?
       end
 
       def gitaly_repository
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index 8ef873d5848..52c1f1ab3d0 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -57,6 +57,14 @@ module Gitlab
         branch_names.count
       end
 
+      # TODO implement a more efficient RPC for this https://gitlab.com/gitlab-org/gitaly/issues/616
+      def has_local_branches?
+        request = Gitaly::FindAllBranchNamesRequest.new(repository: @gitaly_repo)
+        response = GitalyClient.call(@storage, :ref_service, :find_all_branch_names, request).first
+
+        response&.names.present?
+      end
+
       def local_branches(sort_by: nil)
         request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo)
         request.sort_by = sort_by_param(sort_by) if sort_by
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 6c014fac2b0..236265a9090 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -389,6 +389,40 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#has_local_branches?' do
+    shared_examples 'check for local branches' do
+      it { expect(repository.has_local_branches?).to eq(true) }
+
+      context 'mutable' do
+        let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
+
+        after do
+          ensure_seeds
+        end
+
+        it 'returns false when there are no branches' do
+          # Sanity check
+          expect(repository.has_local_branches?).to eq(true)
+
+          FileUtils.rm_rf(File.join(repository.path, 'packed-refs'))
+          heads_dir = File.join(repository.path, 'refs/heads')
+          FileUtils.rm_rf(heads_dir)
+          FileUtils.mkdir_p(heads_dir)
+
+          expect(repository.has_local_branches?).to eq(false)
+        end
+      end
+    end
+
+    context 'with gitaly' do
+      it_behaves_like 'check for local branches'
+    end
+
+    context 'without gitaly', skip_gitaly_mock: true do
+      it_behaves_like 'check for local branches'
+    end
+  end
+
   describe "#delete_branch" do
     shared_examples "deleting a branch" do
       let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 7065d467ec0..d76ab421553 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1134,21 +1134,31 @@ describe Repository, models: true do
   end
 
   describe '#has_visible_content?' do
-    subject { repository.has_visible_content? }
+    before do
+      # If raw_repository.has_visible_content? gets called more than once then
+      # caching is broken. We don't want that.
+      expect(repository.raw_repository).to receive(:has_visible_content?)
+        .once
+        .and_return(result)
+    end
 
-    describe 'when there are no branches' do
-      before do
-        allow(repository.raw_repository).to receive(:branch_count).and_return(0)
-      end
+    context 'when true' do
+      let(:result) { true }
 
-      it { is_expected.to eq(false) }
+      it 'returns true and caches it' do
+        expect(repository.has_visible_content?).to eq(true)
+        # Second call hits the cache
+        expect(repository.has_visible_content?).to eq(true)
+      end
     end
 
-    describe 'when there are branches' do
-      it 'returns true' do
-        expect(repository.raw_repository).to receive(:branch_count).and_return(3)
+    context 'when false' do
+      let(:result) { false }
 
-        expect(subject).to eq(true)
+      it 'returns false and caches it' do
+        expect(repository.has_visible_content?).to eq(false)
+        # Second call hits the cache
+        expect(repository.has_visible_content?).to eq(false)
       end
     end
   end
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index f7b67b8efc6..ab656d619f4 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/spec/workers/git_garbage_collect_worker_spec.rb
@@ -32,7 +32,7 @@ describe GitGarbageCollectWorker do
           expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
           expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
           expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
-          expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original
+          expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
 
           subject.perform(project.id, :gc, lease_key, lease_uuid)
         end
@@ -47,7 +47,6 @@ describe GitGarbageCollectWorker do
           expect(subject).not_to receive(:command)
           expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
           expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
-          expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original
           expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
 
           subject.perform(project.id, :gc, lease_key, lease_uuid)
@@ -78,7 +77,7 @@ describe GitGarbageCollectWorker do
             expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
             expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
             expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
-            expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original
+            expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
 
             subject.perform(project.id)
           end
@@ -93,7 +92,6 @@ describe GitGarbageCollectWorker do
             expect(subject).not_to receive(:command)
             expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
             expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
-            expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original
             expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
 
             subject.perform(project.id)