Merge branch 'branch-tag-count-methods' into 'master'

Use dedicated methods for counting branches and tags

This started out as "Lets add two methods to count and cache some data" and ended up in a clean-up/fix of some existing code. The two problems were:

1. Different code was used for adding/removing branches/tags via Git and the UI
2. The code used for the UI didn't have any RSpec tests, and I couldn't find any Spinach tests either (though grepping for Spinach stuff is hard)

This MR addresses the following:

1. `Repository#branch_count` and `Repository#tag_count` are used to count and cache the number of branches/tags, these methods are then used on the branches/commits/tags pages.
2. `Repository#add_tag`, `Repository#add_branch`, `Repository#rm_tag` and `Repository#rm_branch` now all the appropriate before/after hook methods instead of calling a random single cache expiration method. This ensures caches are properly flushed when adding/removing tags/branches via the UI.
3. RSpec tests were added for the above methods.

This fixes gitlab-org/gitlab-ce#13459

See merge request !3128

6 files changed, 155 insertions, 14 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index ff48f993d42..6441cd87e87 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -133,18 +133,18 @@ class Repository
       rugged.branches.create(branch_name, target)
     end
 
-    expire_branches_cache
+    after_create_branch
     find_branch(branch_name)
   end
 
   def add_tag(tag_name, ref, message = nil)
-    expire_tags_cache
+    before_push_tag
 
     gitlab_shell.add_tag(path_with_namespace, tag_name, ref, message)
   end
 
   def rm_branch(user, branch_name)
-    expire_branches_cache
+    before_remove_branch
 
     branch = find_branch(branch_name)
     oldrev = branch.try(:target)
@@ -155,12 +155,12 @@ class Repository
       rugged.branches.delete(branch_name)
     end
 
-    expire_branches_cache
+    after_remove_branch
     true
   end
 
   def rm_tag(tag_name)
-    expire_tags_cache
+    before_remove_tag
 
     gitlab_shell.rm_tag(path_with_namespace, tag_name)
   end
@@ -183,6 +183,14 @@ class Repository
     end
   end
 
+  def branch_count
+    @branch_count ||= cache.fetch(:branch_count) { raw_repository.branch_count }
+  end
+
+  def tag_count
+    @tag_count ||= cache.fetch(:tag_count) { raw_repository.rugged.tags.count }
+  end
+
   # Return repo size in megabytes
   # Cached in redis
   def size
@@ -278,6 +286,16 @@ class Repository
     @has_visible_content = nil
   end
 
+  def expire_branch_count_cache
+    cache.expire(:branch_count)
+    @branch_count = nil
+  end
+
+  def expire_tag_count_cache
+    cache.expire(:tag_count)
+    @tag_count = nil
+  end
+
   def rebuild_cache
     cache_keys.each do |key|
       cache.expire(key)
@@ -313,9 +331,17 @@ class Repository
     expire_root_ref_cache
   end
 
-  # Runs code before creating a new tag.
-  def before_create_tag
+  # Runs code before pushing (= creating or removing) a tag.
+  def before_push_tag
     expire_cache
+    expire_tags_cache
+    expire_tag_count_cache
+  end
+
+  # Runs code before removing a tag.
+  def before_remove_tag
+    expire_tags_cache
+    expire_tag_count_cache
   end
 
   # Runs code after a repository has been forked/imported.
@@ -330,12 +356,21 @@ class Repository
 
   # Runs code after a new branch has been created.
   def after_create_branch
+    expire_branches_cache
     expire_has_visible_content_cache
+    expire_branch_count_cache
+  end
+
+  # Runs code before removing an existing branch.
+  def before_remove_branch
+    expire_branches_cache
   end
 
   # Runs code after an existing branch has been removed.
   def after_remove_branch
     expire_has_visible_content_cache
+    expire_branch_count_cache
+    expire_branches_cache
   end
 
   def method_missing(m, *args, &block)
diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb
index a62c5fc4fc4..c88c7672805 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.before_create_tag
+    project.repository.before_push_tag
 
     @project, @user = project, user
     @push_data = build_push_data(oldrev, newrev, ref)
diff --git a/app/views/projects/branches/destroy.js.haml b/app/views/projects/branches/destroy.js.haml
index 882a4d0c5e2..a21ddaf4930 100644
--- a/app/views/projects/branches/destroy.js.haml
+++ b/app/views/projects/branches/destroy.js.haml
@@ -1 +1 @@
-$('.js-totalbranch-count').html("#{@repository.branches.size}")
+$('.js-totalbranch-count').html("#{@repository.branch_count}")
diff --git a/app/views/projects/commits/_head.html.haml b/app/views/projects/commits/_head.html.haml
index 498c5e05b32..7a5b0d993db 100644
--- a/app/views/projects/commits/_head.html.haml
+++ b/app/views/projects/commits/_head.html.haml
@@ -15,9 +15,9 @@
   = nav_link(html_options: {class: branches_tab_class}) do
     = link_to namespace_project_branches_path(@project.namespace, @project) do
       Branches
-      %span.badge.js-totalbranch-count= @repository.branches.size
+      %span.badge.js-totalbranch-count= @repository.branch_count
 
   = nav_link(controller: [:tags, :releases]) do
     = link_to namespace_project_tags_path(@project.namespace, @project) do
       Tags
-      %span.badge.js-totaltags-count= @repository.tags.length
+      %span.badge.js-totaltags-count= @repository.tag_count
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 150422ac349..34866be3395 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -148,6 +148,12 @@ describe Repository, models: true do
 
         expect(branch.name).to eq('new_feature')
       end
+
+      it 'calls the after_create_branch hook' do
+        expect(repository).to receive(:after_create_branch)
+
+        repository.add_branch(user, 'new_feature', 'master')
+      end
     end
 
     context 'when pre hooks failed' do
@@ -405,7 +411,7 @@ describe Repository, models: true do
     end
   end
 
-  describe '#expire_branch_ache' do
+  describe '#expire_branch_cache' 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) }
@@ -556,11 +562,12 @@ describe Repository, models: true do
     end
   end
 
-  describe '#before_create_tag' do
+  describe '#before_push_tag' do
     it 'flushes the cache' do
       expect(repository).to receive(:expire_cache)
+      expect(repository).to receive(:expire_tag_count_cache)
 
-      repository.before_create_tag
+      repository.before_push_tag
     end
   end
 
@@ -607,4 +614,77 @@ describe Repository, models: true do
       expect(repository.main_language).to be_nil
     end
   end
+
+  describe '#before_remove_tag' do
+    it 'flushes the tag cache' do
+      expect(repository).to receive(:expire_tag_count_cache)
+
+      repository.before_remove_tag
+    end
+  end
+
+  describe '#branch_count' do
+    it 'returns the number of branches' do
+      expect(repository.branch_count).to be_an_instance_of(Fixnum)
+    end
+  end
+
+  describe '#tag_count' do
+    it 'returns the number of tags' do
+      expect(repository.tag_count).to be_an_instance_of(Fixnum)
+    end
+  end
+
+  describe '#expire_branch_count_cache' do
+    let(:cache) { repository.send(:cache) }
+
+    it 'expires the cache' do
+      expect(cache).to receive(:expire).with(:branch_count)
+
+      repository.expire_branch_count_cache
+    end
+  end
+
+  describe '#expire_tag_count_cache' do
+    let(:cache) { repository.send(:cache) }
+
+    it 'expires the cache' do
+      expect(cache).to receive(:expire).with(:tag_count)
+
+      repository.expire_tag_count_cache
+    end
+  end
+
+  describe '#add_tag' do
+    it 'adds a tag' do
+      expect(repository).to receive(:before_push_tag)
+
+      expect_any_instance_of(Gitlab::Shell).to receive(:add_tag).
+        with(repository.path_with_namespace, '8.5', 'master', 'foo')
+
+      repository.add_tag('8.5', 'master', 'foo')
+    end
+  end
+
+  describe '#rm_branch' do
+    let(:user) { create(:user) }
+
+    it 'removes a branch' do
+      expect(repository).to receive(:before_remove_branch)
+      expect(repository).to receive(:after_remove_branch)
+
+      repository.rm_branch(user, 'feature')
+    end
+  end
+
+  describe '#rm_tag' do
+    it 'removes a tag' do
+      expect(repository).to receive(:before_remove_tag)
+
+      expect_any_instance_of(Gitlab::Shell).to receive(:rm_tag).
+        with(repository.path_with_namespace, '8.5')
+
+      repository.rm_tag('8.5')
+    end
+  end
 end
diff --git a/spec/services/delete_tag_service_spec.rb b/spec/services/delete_tag_service_spec.rb
new file mode 100644
index 00000000000..5b7ba521812
--- /dev/null
+++ b/spec/services/delete_tag_service_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe DeleteTagService, services: true do
+  let(:project) { create(:project) }
+  let(:repository) { project.repository }
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(project, user) }
+
+  let(:tag) { double(:tag, name: '8.5', target: 'abc123') }
+
+  describe '#execute' do
+    before do
+      allow(repository).to receive(:find_tag).and_return(tag)
+    end
+
+    it 'removes the tag' do
+      expect_any_instance_of(Gitlab::Shell).to receive(:rm_tag).
+        and_return(true)
+
+      expect(repository).to receive(:before_remove_tag)
+      expect(service).to receive(:success)
+
+      service.execute('8.5')
+    end
+  end
+end