diff options
author | Robert Speicher <robert@gitlab.com> | 2016-03-08 20:11:40 +0000 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2016-03-08 20:11:40 +0000 |
commit | e8cd04e831a2db36c4029f2c193fc40d2568c79e (patch) | |
tree | 62d0449fb89e79db95abb2aa951ad3850470a6b8 | |
parent | fe9a445faaaa5a90c9308b01877aadc1984a111b (diff) | |
parent | 590e1b4b21f2a39bf573800392b04859b563f3e5 (diff) | |
download | gitlab-ce-e8cd04e831a2db36c4029f2c193fc40d2568c79e.tar.gz |
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
-rw-r--r-- | app/models/repository.rb | 49 | ||||
-rw-r--r-- | app/services/git_tag_push_service.rb | 2 | ||||
-rw-r--r-- | app/views/projects/branches/destroy.js.haml | 2 | ||||
-rw-r--r-- | app/views/projects/commits/_head.html.haml | 4 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 86 | ||||
-rw-r--r-- | spec/services/delete_tag_service_spec.rb | 26 |
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 |