Expire branch/tag git data when needed.18709-branch-tag-collection-caching

When pushing commits to existing branches we don’t
need to flush branch git data (branch names / counts)

When flushes the cache when pushing commits skip to
flush branch and tag git data (names / counts) because
those operations are managed explicitly in each case

Repopulated expired cache as soon as possible

5 files changed, 78 insertions, 14 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4fbffb41436..38bcbbb024f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -21,6 +21,7 @@ v 8.10.0 (unreleased)
   - PipelinesFinder uses git cache data
   - Check for conflicts with existing Project's wiki path when creating a new project.
   - Remove unused front-end variable -> default_issues_tracker
+  - Better caching of git calls on ProjectsController#show.
   - Add API endpoint for a group issues !4520 (mahcsig)
   - Add Bugzilla integration !4930 (iamtjg)
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index f45c3d06abd..eb232ea681b 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -246,24 +246,26 @@ class Repository
     end
   end
 
+  # Keys for data that can be affected for any commit push.
   def cache_keys
-    %i(size branch_names tag_names branch_count tag_count commit_count
+    %i(size commit_count
        readme version contribution_guide changelog
        license_blob license_key gitignore)
   end
 
+  # Keys for data on branch/tag operations.
+  def cache_keys_for_branches_and_tags
+    %i(branch_names tag_names branch_count tag_count)
+  end
+
   def build_cache
-    cache_keys.each do |key|
+    (cache_keys + cache_keys_for_branches_and_tags).each do |key|
       unless cache.exist?(key)
         send(key)
       end
     end
   end
 
-  def expire_gitignore
-    cache.expire(:gitignore)
-  end
-
   def expire_tags_cache
     cache.expire(:tag_names)
     @tags = nil
@@ -286,8 +288,6 @@ class Repository
     # This ensures this particular cache is flushed after the first commit to a
     # new repository.
     expire_emptiness_caches if empty?
-    expire_branch_count_cache
-    expire_tag_count_cache
   end
 
   def expire_branch_cache(branch_name = nil)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index d8350000bf6..8ae083d7132 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -531,8 +531,6 @@ describe Repository, models: true do
   describe '#expire_cache' do
     it 'expires all caches' do
       expect(repository).to receive(:expire_branch_cache)
-      expect(repository).to receive(:expire_branch_count_cache)
-      expect(repository).to receive(:expire_tag_count_cache)
 
       repository.expire_cache
     end
@@ -1055,12 +1053,14 @@ describe Repository, models: true do
     let(:cache) { repository.send(:cache) }
 
     it 'builds the caches if they do not already exist' do
+      cache_keys = repository.cache_keys + repository.cache_keys_for_branches_and_tags
+
       expect(cache).to receive(:exist?).
-        exactly(repository.cache_keys.length).
+        exactly(cache_keys.length).
         times.
         and_return(false)
 
-      repository.cache_keys.each do |key|
+      cache_keys.each do |key|
         expect(repository).to receive(key)
       end
 
@@ -1068,12 +1068,14 @@ describe Repository, models: true do
     end
 
     it 'does not build any caches that already exist' do
+      cache_keys = repository.cache_keys + repository.cache_keys_for_branches_and_tags
+
       expect(cache).to receive(:exist?).
-        exactly(repository.cache_keys.length).
+        exactly(cache_keys.length).
         times.
         and_return(true)
 
-      repository.cache_keys.each do |key|
+      cache_keys.each do |key|
         expect(repository).not_to receive(key)
       end
 
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index f99ad046f0d..1ceb7c3e6ff 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -40,6 +40,18 @@ describe GitPushService, services: true do
 
         subject
       end
+
+      it 'flushes the branches cache' do
+        expect(project.repository).to receive(:expire_branches_cache)
+
+        subject
+      end
+
+      it 'flushes the branch count cache' do
+        expect(project.repository).to receive(:expire_branch_count_cache)
+
+        subject
+      end
     end
 
     context 'existing branch' do
@@ -52,6 +64,18 @@ describe GitPushService, services: true do
 
         subject
       end
+
+      it 'does not flush the branches cache' do
+        expect(project.repository).not_to receive(:expire_branches_cache)
+
+        subject
+      end
+
+      it 'does not flush the branch count cache' do
+        expect(project.repository).not_to receive(:expire_branch_count_cache)
+
+        subject
+      end
     end
 
     context 'rm branch' do
@@ -66,6 +90,18 @@ describe GitPushService, services: true do
         subject
       end
 
+      it 'flushes the branches cache' do
+        expect(project.repository).to receive(:expire_branches_cache)
+
+        subject
+      end
+
+      it 'flushes the branch count cache' do
+        expect(project.repository).to receive(:expire_branch_count_cache)
+
+        subject
+      end
+
       it 'flushes general cached data' do
         expect(project.repository).to receive(:expire_cache).
           with('master', newrev)
diff --git a/spec/services/git_tag_push_service_spec.rb b/spec/services/git_tag_push_service_spec.rb
index a63656e6268..a4fcd44882d 100644
--- a/spec/services/git_tag_push_service_spec.rb
+++ b/spec/services/git_tag_push_service_spec.rb
@@ -11,6 +11,31 @@ describe GitTagPushService, services: true do
   let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
   let(:ref) { 'refs/tags/v1.1.0' }
 
+  describe "Push tags" do
+    subject do
+      service.execute
+      service
+    end
+
+    it 'flushes general cached data' do
+      expect(project.repository).to receive(:expire_cache)
+
+      subject
+    end
+
+    it 'flushes the tags cache' do
+      expect(project.repository).to receive(:expire_tags_cache)
+
+      subject
+    end
+
+    it 'flushes the tag count cache' do
+      expect(project.repository).to receive(:expire_tag_count_cache)
+
+      subject
+    end
+  end
+
   describe "Git Tag Push Data" do
     before do
       service.execute