Merge branch 'smarter-diverging-commit-cache-flushing' into 'master'

Smarter flushing of branch statistics caches

This basically ensures we only flush caches of branches whenever we really have to. See commit c514f8b850219cd3e5526e73e1d00e6729e2b466 for the details.

cc @joshfng @rspeicher 

See merge request !2769

4 files changed, 54 insertions, 16 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 27bdbac3e52..0e17d2b669a 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -205,12 +205,6 @@ class Repository
        readme version contribution_guide changelog license)
   end
 
-  def branch_cache_keys
-    branches.map do |branch|
-      :"diverging_commit_counts_#{branch.name}"
-    end
-  end
-
   def build_cache
     cache_keys.each do |key|
       unless cache.exist?(key)
@@ -235,17 +229,26 @@ class Repository
     @branches = nil
   end
 
-  def expire_cache
+  def expire_cache(branch_name = nil)
     cache_keys.each do |key|
       cache.expire(key)
     end
 
-    expire_branch_cache
+    expire_branch_cache(branch_name)
   end
 
-  def expire_branch_cache
-    branches.each do |branch|
-      cache.expire(:"diverging_commit_counts_#{branch.name}")
+  def expire_branch_cache(branch_name = nil)
+    # When we push to the root branch we have to flush the cache for all other
+    # branches as their statistics are based on the commits relative to the
+    # root branch.
+    if !branch_name || branch_name == root_ref
+      branches.each do |branch|
+        cache.expire(:"diverging_commit_counts_#{branch.name}")
+      end
+    # In case a commit is pushed to a non-root branch we only have to flush the
+    # cache for said branch.
+    else
+      cache.expire(:"diverging_commit_counts_#{branch_name}")
     end
   end
 
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 0f31756797d..e3bf14966c8 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -18,7 +18,9 @@ class GitPushService
   def execute(project, user, oldrev, newrev, ref)
     @project, @user = project, user
 
-    project.repository.expire_cache
+    branch_name = Gitlab::Git.ref_name(ref)
+
+    project.repository.expire_cache(branch_name)
 
     if push_remove_branch?(ref, newrev)
       project.repository.expire_has_visible_content_cache
@@ -33,7 +35,6 @@ class GitPushService
         @push_commits = project.repository.commits(newrev)
 
         # Ensure HEAD points to the default branch in case it is not master
-        branch_name = Gitlab::Git.ref_name(ref)
         project.change_head(branch_name)
 
         # Set protection on the default branch if configured
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 753012be578..72b4ac6d660 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -291,6 +291,12 @@ describe Repository, models: true do
 
       repository.expire_cache
     end
+
+    it 'expires the caches for a specific branch' do
+      expect(repository).to receive(:expire_branch_cache).with('master')
+
+      repository.expire_cache('master')
+    end
   end
 
   describe '#expire_root_ref_cache' do
@@ -320,4 +326,32 @@ describe Repository, models: true do
       expect(repository.has_visible_content?).to eq(false)
     end
   end
+
+  describe '#expire_branch_ache' do
+    # This method is private but we need it for testing purposes. Sadly there's
+    # no other proper way of testing caching operations.
+    let(:cache) { repository.send(:cache) }
+
+    it 'expires the cache for all branches' do
+      expect(cache).to receive(:expire).
+        at_least(repository.branches.length).
+        times
+
+      repository.expire_branch_cache
+    end
+
+    it 'expires the cache for all branches when the root branch is given' do
+      expect(cache).to receive(:expire).
+        at_least(repository.branches.length).
+        times
+
+      repository.expire_branch_cache(repository.root_ref)
+    end
+
+    it 'expires the cache for a specific branch' do
+      expect(cache).to receive(:expire).once
+
+      repository.expire_branch_cache('foo')
+    end
+  end
 end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 349809743c7..eb3a5fe43f5 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -23,7 +23,7 @@ describe GitPushService, services: true do
       it { is_expected.to be_truthy }
 
       it 'flushes general cached data' do
-        expect(project.repository).to receive(:expire_cache)
+        expect(project.repository).to receive(:expire_cache).with('master')
 
         subject
       end
@@ -43,7 +43,7 @@ describe GitPushService, services: true do
       it { is_expected.to be_truthy }
 
       it 'flushes general cached data' do
-        expect(project.repository).to receive(:expire_cache)
+        expect(project.repository).to receive(:expire_cache).with('master')
 
         subject
       end
@@ -63,7 +63,7 @@ describe GitPushService, services: true do
       end
 
       it 'flushes general cached data' do
-        expect(project.repository).to receive(:expire_cache)
+        expect(project.repository).to receive(:expire_cache).with('master')
 
         subject
       end