Commit e27be7b9 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'make-has-visible-commit-more-efficient' into 'master'

Make Repository#has_visible_content more efficient

See merge request gitlab-org/gitlab-ce!14554
parents f43d94a6 403712f0
...@@ -91,12 +91,6 @@ class Repository ...@@ -91,12 +91,6 @@ class Repository
) )
end end
# we need to have this method here because it is not cached in ::Git and
# the method is called multiple times for every request
def has_visible_content?
branch_count > 0
end
def inspect def inspect
"#<#{self.class.name}:#{@disk_path}>" "#<#{self.class.name}:#{@disk_path}>"
end end
...@@ -523,9 +517,10 @@ class Repository ...@@ -523,9 +517,10 @@ class Repository
delegate :tag_names, to: :raw_repository delegate :tag_names, to: :raw_repository
cache_method :tag_names, fallback: [] cache_method :tag_names, fallback: []
delegate :branch_count, :tag_count, to: :raw_repository delegate :branch_count, :tag_count, :has_visible_content?, to: :raw_repository
cache_method :branch_count, fallback: 0 cache_method :branch_count, fallback: 0
cache_method :tag_count, fallback: 0 cache_method :tag_count, fallback: 0
cache_method :has_visible_content?, fallback: false
def avatar def avatar
# n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38327 # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38327
......
...@@ -192,6 +192,28 @@ module Gitlab ...@@ -192,6 +192,28 @@ module Gitlab
end end
end end
def has_local_branches?
gitaly_migrate(:has_local_branches) do |is_enabled|
if is_enabled
gitaly_ref_client.has_local_branches?
else
has_local_branches_rugged?
end
end
end
def has_local_branches_rugged?
rugged.branches.each(:local).any? do |ref|
begin
ref.name && ref.target # ensures the branch is valid
true
rescue Rugged::ReferenceError
false
end
end
end
# Returns the number of valid tags # Returns the number of valid tags
def tag_count def tag_count
gitaly_migrate(:tag_names) do |is_enabled| gitaly_migrate(:tag_names) do |is_enabled|
...@@ -1037,7 +1059,9 @@ module Gitlab ...@@ -1037,7 +1059,9 @@ module Gitlab
# This method return true if repository contains some content visible in project page. # This method return true if repository contains some content visible in project page.
# #
def has_visible_content? def has_visible_content?
branch_count > 0 return @has_visible_content if defined?(@has_visible_content)
@has_visible_content = has_local_branches?
end end
def gitaly_repository def gitaly_repository
......
...@@ -57,6 +57,14 @@ module Gitlab ...@@ -57,6 +57,14 @@ module Gitlab
branch_names.count branch_names.count
end end
# TODO implement a more efficient RPC for this https://gitlab.com/gitlab-org/gitaly/issues/616
def has_local_branches?
request = Gitaly::FindAllBranchNamesRequest.new(repository: @gitaly_repo)
response = GitalyClient.call(@storage, :ref_service, :find_all_branch_names, request).first
response&.names.present?
end
def local_branches(sort_by: nil) def local_branches(sort_by: nil)
request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo) request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo)
request.sort_by = sort_by_param(sort_by) if sort_by request.sort_by = sort_by_param(sort_by) if sort_by
......
...@@ -389,6 +389,40 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -389,6 +389,40 @@ describe Gitlab::Git::Repository, seed_helper: true do
end end
end end
describe '#has_local_branches?' do
shared_examples 'check for local branches' do
it { expect(repository.has_local_branches?).to eq(true) }
context 'mutable' do
let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
after do
ensure_seeds
end
it 'returns false when there are no branches' do
# Sanity check
expect(repository.has_local_branches?).to eq(true)
FileUtils.rm_rf(File.join(repository.path, 'packed-refs'))
heads_dir = File.join(repository.path, 'refs/heads')
FileUtils.rm_rf(heads_dir)
FileUtils.mkdir_p(heads_dir)
expect(repository.has_local_branches?).to eq(false)
end
end
end
context 'with gitaly' do
it_behaves_like 'check for local branches'
end
context 'without gitaly', skip_gitaly_mock: true do
it_behaves_like 'check for local branches'
end
end
describe "#delete_branch" do describe "#delete_branch" do
shared_examples "deleting a branch" do shared_examples "deleting a branch" do
let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') } let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
......
...@@ -1131,21 +1131,31 @@ describe Repository do ...@@ -1131,21 +1131,31 @@ describe Repository do
end end
describe '#has_visible_content?' do describe '#has_visible_content?' do
subject { repository.has_visible_content? }
describe 'when there are no branches' do
before do before do
allow(repository.raw_repository).to receive(:branch_count).and_return(0) # If raw_repository.has_visible_content? gets called more than once then
# caching is broken. We don't want that.
expect(repository.raw_repository).to receive(:has_visible_content?)
.once
.and_return(result)
end end
it { is_expected.to eq(false) } context 'when true' do
let(:result) { true }
it 'returns true and caches it' do
expect(repository.has_visible_content?).to eq(true)
# Second call hits the cache
expect(repository.has_visible_content?).to eq(true)
end
end end
describe 'when there are branches' do context 'when false' do
it 'returns true' do let(:result) { false }
expect(repository.raw_repository).to receive(:branch_count).and_return(3)
expect(subject).to eq(true) it 'returns false and caches it' do
expect(repository.has_visible_content?).to eq(false)
# Second call hits the cache
expect(repository.has_visible_content?).to eq(false)
end end
end end
end end
......
...@@ -32,7 +32,7 @@ describe GitGarbageCollectWorker do ...@@ -32,7 +32,7 @@ describe GitGarbageCollectWorker do
expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
expect_any_instance_of(Repository).to receive(:branch_names).and_call_original expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
subject.perform(project.id, :gc, lease_key, lease_uuid) subject.perform(project.id, :gc, lease_key, lease_uuid)
end end
...@@ -47,7 +47,6 @@ describe GitGarbageCollectWorker do ...@@ -47,7 +47,6 @@ describe GitGarbageCollectWorker do
expect(subject).not_to receive(:command) expect(subject).not_to receive(:command)
expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original
expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
subject.perform(project.id, :gc, lease_key, lease_uuid) subject.perform(project.id, :gc, lease_key, lease_uuid)
...@@ -78,7 +77,7 @@ describe GitGarbageCollectWorker do ...@@ -78,7 +77,7 @@ describe GitGarbageCollectWorker do
expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
expect_any_instance_of(Repository).to receive(:branch_names).and_call_original expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
subject.perform(project.id) subject.perform(project.id)
end end
...@@ -93,7 +92,6 @@ describe GitGarbageCollectWorker do ...@@ -93,7 +92,6 @@ describe GitGarbageCollectWorker do
expect(subject).not_to receive(:command) expect(subject).not_to receive(:command)
expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original
expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
subject.perform(project.id) subject.perform(project.id)
......
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