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

2 files changed, 109 insertions, 3 deletions

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