Commit e8cd04e8 authored by Robert Speicher's avatar Robert Speicher

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
parents fe9a445f 590e1b4b
...@@ -133,18 +133,18 @@ class Repository ...@@ -133,18 +133,18 @@ class Repository
rugged.branches.create(branch_name, target) rugged.branches.create(branch_name, target)
end end
expire_branches_cache after_create_branch
find_branch(branch_name) find_branch(branch_name)
end end
def add_tag(tag_name, ref, message = nil) 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) gitlab_shell.add_tag(path_with_namespace, tag_name, ref, message)
end end
def rm_branch(user, branch_name) def rm_branch(user, branch_name)
expire_branches_cache before_remove_branch
branch = find_branch(branch_name) branch = find_branch(branch_name)
oldrev = branch.try(:target) oldrev = branch.try(:target)
...@@ -155,12 +155,12 @@ class Repository ...@@ -155,12 +155,12 @@ class Repository
rugged.branches.delete(branch_name) rugged.branches.delete(branch_name)
end end
expire_branches_cache after_remove_branch
true true
end end
def rm_tag(tag_name) def rm_tag(tag_name)
expire_tags_cache before_remove_tag
gitlab_shell.rm_tag(path_with_namespace, tag_name) gitlab_shell.rm_tag(path_with_namespace, tag_name)
end end
...@@ -183,6 +183,14 @@ class Repository ...@@ -183,6 +183,14 @@ class Repository
end end
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 # Return repo size in megabytes
# Cached in redis # Cached in redis
def size def size
...@@ -278,6 +286,16 @@ class Repository ...@@ -278,6 +286,16 @@ class Repository
@has_visible_content = nil @has_visible_content = nil
end 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 def rebuild_cache
cache_keys.each do |key| cache_keys.each do |key|
cache.expire(key) cache.expire(key)
...@@ -313,9 +331,17 @@ class Repository ...@@ -313,9 +331,17 @@ class Repository
expire_root_ref_cache expire_root_ref_cache
end end
# Runs code before creating a new tag. # Runs code before pushing (= creating or removing) a tag.
def before_create_tag def before_push_tag
expire_cache 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 end
# Runs code after a repository has been forked/imported. # Runs code after a repository has been forked/imported.
...@@ -330,12 +356,21 @@ class Repository ...@@ -330,12 +356,21 @@ class Repository
# Runs code after a new branch has been created. # Runs code after a new branch has been created.
def after_create_branch def after_create_branch
expire_branches_cache
expire_has_visible_content_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 end
# Runs code after an existing branch has been removed. # Runs code after an existing branch has been removed.
def after_remove_branch def after_remove_branch
expire_has_visible_content_cache expire_has_visible_content_cache
expire_branch_count_cache
expire_branches_cache
end end
def method_missing(m, *args, &block) def method_missing(m, *args, &block)
......
...@@ -2,7 +2,7 @@ class GitTagPushService ...@@ -2,7 +2,7 @@ class GitTagPushService
attr_accessor :project, :user, :push_data attr_accessor :project, :user, :push_data
def execute(project, user, oldrev, newrev, ref) def execute(project, user, oldrev, newrev, ref)
project.repository.before_create_tag project.repository.before_push_tag
@project, @user = project, user @project, @user = project, user
@push_data = build_push_data(oldrev, newrev, ref) @push_data = build_push_data(oldrev, newrev, ref)
......
$('.js-totalbranch-count').html("#{@repository.branches.size}") $('.js-totalbranch-count').html("#{@repository.branch_count}")
...@@ -15,9 +15,9 @@ ...@@ -15,9 +15,9 @@
= nav_link(html_options: {class: branches_tab_class}) do = nav_link(html_options: {class: branches_tab_class}) do
= link_to namespace_project_branches_path(@project.namespace, @project) do = link_to namespace_project_branches_path(@project.namespace, @project) do
Branches Branches
%span.badge.js-totalbranch-count= @repository.branches.size %span.badge.js-totalbranch-count= @repository.branch_count
= nav_link(controller: [:tags, :releases]) do = nav_link(controller: [:tags, :releases]) do
= link_to namespace_project_tags_path(@project.namespace, @project) do = link_to namespace_project_tags_path(@project.namespace, @project) do
Tags Tags
%span.badge.js-totaltags-count= @repository.tags.length %span.badge.js-totaltags-count= @repository.tag_count
...@@ -148,6 +148,12 @@ describe Repository, models: true do ...@@ -148,6 +148,12 @@ describe Repository, models: true do
expect(branch.name).to eq('new_feature') expect(branch.name).to eq('new_feature')
end 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 end
context 'when pre hooks failed' do context 'when pre hooks failed' do
...@@ -405,7 +411,7 @@ describe Repository, models: true do ...@@ -405,7 +411,7 @@ describe Repository, models: true do
end end
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 # This method is private but we need it for testing purposes. Sadly there's
# no other proper way of testing caching operations. # no other proper way of testing caching operations.
let(:cache) { repository.send(:cache) } let(:cache) { repository.send(:cache) }
...@@ -556,11 +562,12 @@ describe Repository, models: true do ...@@ -556,11 +562,12 @@ describe Repository, models: true do
end end
end end
describe '#before_create_tag' do describe '#before_push_tag' do
it 'flushes the cache' do it 'flushes the cache' do
expect(repository).to receive(:expire_cache) expect(repository).to receive(:expire_cache)
expect(repository).to receive(:expire_tag_count_cache)
repository.before_create_tag repository.before_push_tag
end end
end end
...@@ -607,4 +614,77 @@ describe Repository, models: true do ...@@ -607,4 +614,77 @@ describe Repository, models: true do
expect(repository.main_language).to be_nil expect(repository.main_language).to be_nil
end end
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 end
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment