summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2017-10-27 15:55:19 +0000
committerRémy Coutable <remy@rymai.me>2017-10-27 15:55:19 +0000
commitdfd6c3f824ada5558546b809fd32468325788b94 (patch)
treecc5f65076fc0acc1b5ff191d502a706e797544af
parent7c4da276d34e008b5aa86971cfd627a813d84df7 (diff)
parent57d7ed05d96928f7e33135e7397bdd6b3b0d25e0 (diff)
downloadgitlab-ce-dfd6c3f824ada5558546b809fd32468325788b94.tar.gz
Merge branch 'use-git-branch-merged' into 'master'
Fetch the merged branches at once. Checking it one by one in the view. We don't cache this yet because this would already much improve the performance. A naive test against a particularly large repository: ``` ruby begin now = Time.now branches.map{ |b| r.merged_to_root_ref?(b.name) } Time.now - now end # 8.265830782 ``` Around 10 times faster: ``` ruby begin now = Time.now r.merged_branches(branches.map(&:name)) Time.now - now end # 0.807405397 ``` This should make the branches page usable. See merge request gitlab-org/gitlab-ce!14729
-rw-r--r--app/controllers/projects/branches_controller.rb2
-rw-r--r--app/models/repository.rb21
-rw-r--r--app/views/projects/branches/_branch.html.haml5
-rw-r--r--app/views/projects/branches/index.html.haml2
-rw-r--r--changelogs/unreleased/use-git-branch-merged.yml5
-rw-r--r--lib/gitlab/git/branch.rb8
-rw-r--r--lib/gitlab/git/repository.rb11
-rw-r--r--spec/lib/gitlab/git/branch_spec.rb32
-rw-r--r--spec/lib/gitlab/git/repository_spec.rb26
-rw-r--r--spec/models/repository_spec.rb18
10 files changed, 121 insertions, 9 deletions
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 7f03ce07dec..f28df83d5a5 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -15,6 +15,8 @@ class Projects::BranchesController < Projects::ApplicationController
respond_to do |format|
format.html do
@refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name))
+ @merged_branch_names =
+ repository.merged_branch_names(@branches.map(&:name))
# n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37429
Gitlab::GitalyClient.allow_n_plus_1_calls do
@max_commits = @branches.reduce(0) do |memo, branch|
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 54388d1c8b4..44a1e9ce529 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -902,18 +902,27 @@ class Repository
end
end
- def merged_to_root_ref?(branch_name)
- branch_commit = commit(branch_name)
- root_ref_commit = commit(root_ref)
+ def merged_to_root_ref?(branch_or_name, pre_loaded_merged_branches = nil)
+ branch = Gitlab::Git::Branch.find(self, branch_or_name)
+
+ if branch
+ root_ref_sha = commit(root_ref).sha
+ same_head = branch.target == root_ref_sha
+ merged =
+ if pre_loaded_merged_branches
+ pre_loaded_merged_branches.include?(branch.name)
+ else
+ ancestor?(branch.target, root_ref_sha)
+ end
- if branch_commit
- same_head = branch_commit.id == root_ref_commit.id
- !same_head && ancestor?(branch_commit.id, root_ref_commit.id)
+ !same_head && merged
else
nil
end
end
+ delegate :merged_branch_names, to: :raw_repository
+
def merge_base(first_commit_id, second_commit_id)
first_commit_id = commit(first_commit_id).try(:id) || first_commit_id
second_commit_id = commit(second_commit_id).try(:id) || second_commit_id
diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml
index 49101d1efa4..6e02ae6c9cc 100644
--- a/app/views/projects/branches/_branch.html.haml
+++ b/app/views/projects/branches/_branch.html.haml
@@ -1,3 +1,4 @@
+- merged = local_assigns.fetch(:merged, false)
- commit = @repository.commit(branch.dereferenced_target)
- bar_graph_width_factor = @max_commits > 0 ? 100.0/@max_commits : 0
- diverging_commit_counts = @repository.diverging_commit_counts(branch)
@@ -12,7 +13,7 @@
&nbsp;
- if branch.name == @repository.root_ref
%span.label.label-primary default
- - elsif @repository.merged_to_root_ref? branch.name
+ - elsif merged
%span.label.label-info.has-tooltip{ title: s_('Branches|Merged into %{default_branch}') % { default_branch: @repository.root_ref } }
= s_('Branches|merged')
@@ -47,7 +48,7 @@
target: "#modal-delete-branch",
delete_path: project_branch_path(@project, branch.name),
branch_name: branch.name,
- is_merged: ("true" if @repository.merged_to_root_ref?(branch.name)) } }
+ is_merged: ("true" if merged) } }
= icon("trash-o")
- else
%button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled",
diff --git a/app/views/projects/branches/index.html.haml b/app/views/projects/branches/index.html.haml
index 7d9645d79e6..aade310236e 100644
--- a/app/views/projects/branches/index.html.haml
+++ b/app/views/projects/branches/index.html.haml
@@ -38,7 +38,7 @@
- if @branches.any?
%ul.content-list.all-branches
- @branches.each do |branch|
- = render "projects/branches/branch", branch: branch
+ = render "projects/branches/branch", branch: branch, merged: @repository.merged_to_root_ref?(branch, @merged_branch_names)
= paginate @branches, theme: 'gitlab'
- else
.nothing-here-block
diff --git a/changelogs/unreleased/use-git-branch-merged.yml b/changelogs/unreleased/use-git-branch-merged.yml
new file mode 100644
index 00000000000..24ec226250c
--- /dev/null
+++ b/changelogs/unreleased/use-git-branch-merged.yml
@@ -0,0 +1,5 @@
+---
+title: Improve branch listing page performance
+merge_request: 14729
+author:
+type: performance
diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb
index c53882787f1..3487e099381 100644
--- a/lib/gitlab/git/branch.rb
+++ b/lib/gitlab/git/branch.rb
@@ -3,6 +3,14 @@
module Gitlab
module Git
class Branch < Ref
+ def self.find(repo, branch_name)
+ if branch_name.is_a?(Gitlab::Git::Branch)
+ branch_name
+ else
+ repo.find_branch(branch_name)
+ end
+ end
+
def initialize(repository, name, target, target_commit)
super(repository, name, target, target_commit)
end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 408616d174b..fc8af38d4d9 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -511,6 +511,10 @@ module Gitlab
gitaly_commit_client.ancestor?(from, to)
end
+ def merged_branch_names(branch_names = [])
+ Set.new(git_merged_branch_names(branch_names))
+ end
+
# Return an array of Diff objects that represent the diff
# between +from+ and +to+. See Diff::filter_diff_options for the allowed
# diff options. The +options+ hash can also include :break_rewrites to
@@ -1190,6 +1194,13 @@ module Gitlab
sort_branches(branches, sort_by)
end
+ def git_merged_branch_names(branch_names = [])
+ lines = run_git(['branch', '--merged', root_ref] + branch_names)
+ .first.lines
+
+ lines.map(&:strip)
+ end
+
def log_using_shell?(options)
options[:path].present? ||
options[:disable_walk] ||
diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb
index 318a7b7a332..708870060e7 100644
--- a/spec/lib/gitlab/git/branch_spec.rb
+++ b/spec/lib/gitlab/git/branch_spec.rb
@@ -7,6 +7,38 @@ describe Gitlab::Git::Branch, seed_helper: true do
it { is_expected.to be_kind_of Array }
+ describe '.find' do
+ subject { described_class.find(repository, branch) }
+
+ before do
+ allow(repository).to receive(:find_branch).with(branch)
+ .and_call_original
+ end
+
+ context 'when finding branch via branch name' do
+ let(:branch) { 'master' }
+
+ it 'returns a branch object' do
+ expect(subject).to be_a(described_class)
+ expect(subject.name).to eq(branch)
+
+ expect(repository).to have_received(:find_branch).with(branch)
+ end
+ end
+
+ context 'when the branch is already a branch' do
+ let(:commit) { repository.commit('master') }
+ let(:branch) { described_class.new(repository, 'master', commit.sha, commit) }
+
+ it 'returns a branch object' do
+ expect(subject).to be_a(described_class)
+ expect(subject).to eq(branch)
+
+ expect(repository).not_to have_received(:find_branch).with(branch)
+ end
+ end
+ end
+
describe '#size' do
subject { super().size }
it { is_expected.to eq(SeedRepo::Repo::BRANCHES.size) }
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index e3d320631bc..b161d25b96c 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1135,6 +1135,32 @@ describe Gitlab::Git::Repository, seed_helper: true do
end
end
+ describe '#merged_branch_names' do
+ context 'when branch names are passed' do
+ it 'only returns the names we are asking' do
+ names = repository.merged_branch_names(%w[merge-test])
+
+ expect(names).to contain_exactly('merge-test')
+ end
+
+ it 'does not return unmerged branch names' do
+ names = repository.merged_branch_names(%w[feature])
+
+ expect(names).to be_empty
+ end
+ end
+
+ context 'when no branch names are specified' do
+ it 'returns all merged branch names' do
+ names = repository.merged_branch_names
+
+ expect(names).to include('merge-test')
+ expect(names).to include('fix-mode')
+ expect(names).not_to include('feature')
+ end
+ end
+ end
+
describe "#ls_files" do
let(:master_file_paths) { repository.ls_files("master") }
let(:not_existed_branch) { repository.ls_files("not_existed_branch") }
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 455d5e8a656..d7c07676911 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -299,6 +299,24 @@ describe Repository do
it { is_expected.to be_falsey }
end
+
+ context 'when pre-loaded merged branches are provided' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:branch, :pre_loaded, :expected) do
+ 'not-merged-branch' | ['branch-merged'] | false
+ 'branch-merged' | ['not-merged-branch'] | false
+ 'branch-merged' | ['branch-merged'] | true
+ 'not-merged-branch' | ['not-merged-branch'] | false
+ 'master' | ['master'] | false
+ end
+
+ with_them do
+ subject { repository.merged_to_root_ref?(branch, pre_loaded) }
+
+ it { is_expected.to eq(expected) }
+ end
+ end
end
describe '#can_be_merged?' do