Merge branch 'repository-caching-refactor' into 'master'

Refactor Repository Caching

Related issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/13515

See merge request !2936

8 files changed, 155 insertions, 25 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 95ad88c76ae..6f5d592755a 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -801,10 +801,7 @@ class Project < ActiveRecord::Base
   end
 
   def change_head(branch)
-    # Cached divergent commit counts are based on repository head
-    repository.expire_branch_cache
-    repository.expire_root_ref_cache
-
+    repository.before_change_head
     gitlab_shell.update_repository_head(self.path_with_namespace, branch)
     reload_default_branch
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index e050bd45254..a214a69d749 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -245,15 +245,6 @@ class Repository
     expire_emptiness_caches if empty?
   end
 
-  # Expires _all_ caches, including those that would normally only be expired
-  # under specific conditions.
-  def expire_all_caches!
-    expire_cache
-    expire_root_ref_cache
-    expire_emptiness_caches
-    expire_has_visible_content_cache
-  end
-
   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
@@ -307,6 +298,46 @@ class Repository
     cache.expire(:branch_names)
   end
 
+  # Runs code just before a repository is deleted.
+  def before_delete
+    expire_cache if exists?
+
+    expire_root_ref_cache
+    expire_emptiness_caches
+  end
+
+  # Runs code just before the HEAD of a repository is changed.
+  def before_change_head
+    # Cached divergent commit counts are based on repository head
+    expire_branch_cache
+    expire_root_ref_cache
+  end
+
+  # Runs code before creating a new tag.
+  def before_create_tag
+    expire_cache
+  end
+
+  # Runs code after a repository has been forked/imported.
+  def after_import
+    expire_emptiness_caches
+  end
+
+  # Runs code after a new commit has been pushed.
+  def after_push_commit(branch_name)
+    expire_cache(branch_name)
+  end
+
+  # Runs code after a new branch has been created.
+  def after_create_branch
+    expire_has_visible_content_cache
+  end
+
+  # Runs code after an existing branch has been removed.
+  def after_remove_branch
+    expire_has_visible_content_cache
+  end
+
   def method_missing(m, *args, &block)
     if m == :lookup && !block_given?
       lookup_cache[m] ||= {}
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index a1711d234ff..9ba200f7bde 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -16,13 +16,13 @@ class GitPushService < BaseService
   #  5. Executes the project's services
   #
   def execute
-    @project.repository.expire_cache(branch_name)
+    @project.repository.after_push_commit(branch_name)
 
     if push_remove_branch?
-      @project.repository.expire_has_visible_content_cache
+      @project.repository.after_remove_branch
       @push_commits = []
     elsif push_to_new_branch?
-      @project.repository.expire_has_visible_content_cache
+      @project.repository.after_create_branch
 
       # Re-find the pushed commits.
       if is_default_branch?
diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb
index 4144c7111d0..a62c5fc4fc4 100644
--- a/app/services/git_tag_push_service.rb
+++ b/app/services/git_tag_push_service.rb
@@ -2,7 +2,7 @@ class GitTagPushService
   attr_accessor :project, :user, :push_data
 
   def execute(project, user, oldrev, newrev, ref)
-    project.repository.expire_cache
+    project.repository.before_create_tag
 
     @project, @user = project, user
     @push_data = build_push_data(oldrev, newrev, ref)
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index f4dcb142850..df5054f08d7 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -76,11 +76,9 @@ module Projects
     end
 
     def flush_caches(project, wiki_path)
-      project.repository.expire_all_caches! if project.repository.exists?
+      project.repository.before_delete
 
-      wiki_repo = Repository.new(wiki_path, project)
-
-      wiki_repo.expire_all_caches! if wiki_repo.exists?
+      Repository.new(wiki_path, project).before_delete
     end
   end
 end
diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb
index 2572b9d6d98..21d311579e3 100644
--- a/app/workers/repository_fork_worker.rb
+++ b/app/workers/repository_fork_worker.rb
@@ -27,7 +27,7 @@ class RepositoryForkWorker
       return
     end
 
-    project.repository.expire_emptiness_caches
+    project.repository.after_import
     project.import_finish
   end
 end
diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb
index 0b6f746e118..2937493c614 100644
--- a/app/workers/repository_import_worker.rb
+++ b/app/workers/repository_import_worker.rb
@@ -18,7 +18,7 @@ class RepositoryImportWorker
       return
     end
 
-    project.repository.expire_emptiness_caches
+    project.repository.after_import
     project.import_finish
   end
 end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index b596782f4e1..51ae2c04ed0 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -362,14 +362,14 @@ describe Repository, models: true do
       repository.expire_cache('master')
     end
 
-    it 'expires the emptiness cache for an empty repository' do
+    it 'expires the emptiness caches for an empty repository' do
       expect(repository).to receive(:empty?).and_return(true)
       expect(repository).to receive(:expire_emptiness_caches)
 
       repository.expire_cache
     end
 
-    it 'does not expire the emptiness cache for a non-empty repository' do
+    it 'does not expire the emptiness caches for a non-empty repository' do
       expect(repository).to receive(:empty?).and_return(false)
       expect(repository).to_not receive(:expire_emptiness_caches)
 
@@ -464,4 +464,108 @@ describe Repository, models: true do
       expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present
     end
   end
+
+  describe '#before_delete' do
+    describe 'when a repository does not exist' do
+      before do
+        allow(repository).to receive(:exists?).and_return(false)
+      end
+
+      it 'does not flush caches that depend on repository data' do
+        expect(repository).to_not receive(:expire_cache)
+
+        repository.before_delete
+      end
+
+      it 'flushes the root ref cache' do
+        expect(repository).to receive(:expire_root_ref_cache)
+
+        repository.before_delete
+      end
+
+      it 'flushes the emptiness caches' do
+        expect(repository).to receive(:expire_emptiness_caches)
+
+        repository.before_delete
+      end
+    end
+
+    describe 'when a repository exists' do
+      before do
+        allow(repository).to receive(:exists?).and_return(true)
+      end
+
+      it 'flushes the caches that depend on repository data' do
+        expect(repository).to receive(:expire_cache)
+
+        repository.before_delete
+      end
+
+      it 'flushes the root ref cache' do
+        expect(repository).to receive(:expire_root_ref_cache)
+
+        repository.before_delete
+      end
+
+      it 'flushes the emptiness caches' do
+        expect(repository).to receive(:expire_emptiness_caches)
+
+        repository.before_delete
+      end
+    end
+  end
+
+  describe '#before_change_head' do
+    it 'flushes the branch cache' do
+      expect(repository).to receive(:expire_branch_cache)
+
+      repository.before_change_head
+    end
+
+    it 'flushes the root ref cache' do
+      expect(repository).to receive(:expire_root_ref_cache)
+
+      repository.before_change_head
+    end
+  end
+
+  describe '#before_create_tag' do
+    it 'flushes the cache' do
+      expect(repository).to receive(:expire_cache)
+
+      repository.before_create_tag
+    end
+  end
+
+  describe '#after_import' do
+    it 'flushes the emptiness cachess' do
+      expect(repository).to receive(:expire_emptiness_caches)
+
+      repository.after_import
+    end
+  end
+
+  describe '#after_push_commit' do
+    it 'flushes the cache' do
+      expect(repository).to receive(:expire_cache).with('master')
+
+      repository.after_push_commit('master')
+    end
+  end
+
+  describe '#after_create_branch' do
+    it 'flushes the visible content cache' do
+      expect(repository).to receive(:expire_has_visible_content_cache)
+
+      repository.after_create_branch
+    end
+  end
+
+  describe '#after_remove_branch' do
+    it 'flushes the visible content cache' do
+      expect(repository).to receive(:expire_has_visible_content_cache)
+
+      repository.after_remove_branch
+    end
+  end
 end