diff options
author | Rémy Coutable <remy@rymai.me> | 2017-09-29 16:08:48 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2017-09-29 19:42:58 +0300 |
commit | 52b3667e5e6755fe128d06dd0c754a4bd6222229 (patch) | |
tree | 1e015c0e95d6dd7678a30e097c1a7fa242fe21b7 | |
parent | 7c34c3c0945a75f0252ce4b2bd5a4996f55d16a0 (diff) |
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
-rw-r--r-- | app/models/repository.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 26 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/ref_service.rb | 8 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 34 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 30 | ||||
-rw-r--r-- | spec/workers/git_garbage_collect_worker_spec.rb | 6 |
6 files changed, 91 insertions, 22 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb index 0315fed24c5..4cb9f1baffd 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -90,12 +90,6 @@ class Repository ) 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 "#<#{self.class.name}:#{@disk_path}>" end @@ -522,9 +516,10 @@ class Repository delegate :tag_names, to: :raw_repository 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 :tag_count, fallback: 0 + cache_method :has_visible_content?, fallback: false def avatar if tree = file_on_head(:avatar) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 6c639f286ee..984e34a9ef1 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -190,6 +190,28 @@ module Gitlab 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 def tag_count gitaly_migrate(:tag_names) do |is_enabled| @@ -909,7 +931,9 @@ module Gitlab # This method return true if repository contains some content visible in project page. # def has_visible_content? - branch_count > 0 + return @has_visible_content if defined?(@has_visible_content) + + @has_visible_content = has_local_branches? end def gitaly_repository diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 8ef873d5848..52c1f1ab3d0 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -57,6 +57,14 @@ module Gitlab branch_names.count 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) request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo) request.sort_by = sort_by_param(sort_by) if sort_by diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 6c014fac2b0..236265a9090 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -389,6 +389,40 @@ describe Gitlab::Git::Repository, seed_helper: true do 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 shared_examples "deleting a branch" do let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 7065d467ec0..d76ab421553 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1134,21 +1134,31 @@ describe Repository, models: true do end describe '#has_visible_content?' do - subject { repository.has_visible_content? } + before do + # 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 - describe 'when there are no branches' do - before do - allow(repository.raw_repository).to receive(:branch_count).and_return(0) - end + context 'when true' do + let(:result) { true } - it { is_expected.to eq(false) } + 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 - describe 'when there are branches' do - it 'returns true' do - expect(repository.raw_repository).to receive(:branch_count).and_return(3) + context 'when false' do + let(:result) { false } - 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 diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb index f7b67b8efc6..ab656d619f4 100644 --- a/spec/workers/git_garbage_collect_worker_spec.rb +++ b/spec/workers/git_garbage_collect_worker_spec.rb @@ -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(:branch_names).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) end @@ -47,7 +47,6 @@ describe GitGarbageCollectWorker do 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(: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 subject.perform(project.id, :gc, lease_key, lease_uuid) @@ -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(:branch_names).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) end @@ -93,7 +92,6 @@ describe GitGarbageCollectWorker do 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(: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 subject.perform(project.id) |