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

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
parents 7c4da276 57d7ed05
...@@ -15,6 +15,8 @@ class Projects::BranchesController < Projects::ApplicationController ...@@ -15,6 +15,8 @@ class Projects::BranchesController < Projects::ApplicationController
respond_to do |format| respond_to do |format|
format.html do format.html do
@refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name)) @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 # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37429
Gitlab::GitalyClient.allow_n_plus_1_calls do Gitlab::GitalyClient.allow_n_plus_1_calls do
@max_commits = @branches.reduce(0) do |memo, branch| @max_commits = @branches.reduce(0) do |memo, branch|
......
...@@ -902,18 +902,27 @@ class Repository ...@@ -902,18 +902,27 @@ class Repository
end end
end end
def merged_to_root_ref?(branch_name) def merged_to_root_ref?(branch_or_name, pre_loaded_merged_branches = nil)
branch_commit = commit(branch_name) branch = Gitlab::Git::Branch.find(self, branch_or_name)
root_ref_commit = commit(root_ref)
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 && merged
same_head = branch_commit.id == root_ref_commit.id
!same_head && ancestor?(branch_commit.id, root_ref_commit.id)
else else
nil nil
end end
end end
delegate :merged_branch_names, to: :raw_repository
def merge_base(first_commit_id, second_commit_id) def merge_base(first_commit_id, second_commit_id)
first_commit_id = commit(first_commit_id).try(:id) || first_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 second_commit_id = commit(second_commit_id).try(:id) || second_commit_id
......
- merged = local_assigns.fetch(:merged, false)
- commit = @repository.commit(branch.dereferenced_target) - commit = @repository.commit(branch.dereferenced_target)
- bar_graph_width_factor = @max_commits > 0 ? 100.0/@max_commits : 0 - bar_graph_width_factor = @max_commits > 0 ? 100.0/@max_commits : 0
- diverging_commit_counts = @repository.diverging_commit_counts(branch) - diverging_commit_counts = @repository.diverging_commit_counts(branch)
...@@ -12,7 +13,7 @@ ...@@ -12,7 +13,7 @@
&nbsp; &nbsp;
- if branch.name == @repository.root_ref - if branch.name == @repository.root_ref
%span.label.label-primary default %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 } } %span.label.label-info.has-tooltip{ title: s_('Branches|Merged into %{default_branch}') % { default_branch: @repository.root_ref } }
= s_('Branches|merged') = s_('Branches|merged')
...@@ -47,7 +48,7 @@ ...@@ -47,7 +48,7 @@
target: "#modal-delete-branch", target: "#modal-delete-branch",
delete_path: project_branch_path(@project, branch.name), delete_path: project_branch_path(@project, branch.name),
branch_name: 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") = icon("trash-o")
- else - else
%button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled", %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled",
......
...@@ -38,7 +38,7 @@ ...@@ -38,7 +38,7 @@
- if @branches.any? - if @branches.any?
%ul.content-list.all-branches %ul.content-list.all-branches
- @branches.each do |branch| - @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' = paginate @branches, theme: 'gitlab'
- else - else
.nothing-here-block .nothing-here-block
......
---
title: Improve branch listing page performance
merge_request: 14729
author:
type: performance
...@@ -3,6 +3,14 @@ ...@@ -3,6 +3,14 @@
module Gitlab module Gitlab
module Git module Git
class Branch < Ref 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) def initialize(repository, name, target, target_commit)
super(repository, name, target, target_commit) super(repository, name, target, target_commit)
end end
......
...@@ -511,6 +511,10 @@ module Gitlab ...@@ -511,6 +511,10 @@ module Gitlab
gitaly_commit_client.ancestor?(from, to) gitaly_commit_client.ancestor?(from, to)
end 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 # Return an array of Diff objects that represent the diff
# between +from+ and +to+. See Diff::filter_diff_options for the allowed # between +from+ and +to+. See Diff::filter_diff_options for the allowed
# diff options. The +options+ hash can also include :break_rewrites to # diff options. The +options+ hash can also include :break_rewrites to
...@@ -1190,6 +1194,13 @@ module Gitlab ...@@ -1190,6 +1194,13 @@ module Gitlab
sort_branches(branches, sort_by) sort_branches(branches, sort_by)
end 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) def log_using_shell?(options)
options[:path].present? || options[:path].present? ||
options[:disable_walk] || options[:disable_walk] ||
......
...@@ -7,6 +7,38 @@ describe Gitlab::Git::Branch, seed_helper: true do ...@@ -7,6 +7,38 @@ describe Gitlab::Git::Branch, seed_helper: true do
it { is_expected.to be_kind_of Array } 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 describe '#size' do
subject { super().size } subject { super().size }
it { is_expected.to eq(SeedRepo::Repo::BRANCHES.size) } it { is_expected.to eq(SeedRepo::Repo::BRANCHES.size) }
......
...@@ -1135,6 +1135,32 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -1135,6 +1135,32 @@ describe Gitlab::Git::Repository, seed_helper: true do
end end
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 describe "#ls_files" do
let(:master_file_paths) { repository.ls_files("master") } let(:master_file_paths) { repository.ls_files("master") }
let(:not_existed_branch) { repository.ls_files("not_existed_branch") } let(:not_existed_branch) { repository.ls_files("not_existed_branch") }
......
...@@ -299,6 +299,24 @@ describe Repository do ...@@ -299,6 +299,24 @@ describe Repository do
it { is_expected.to be_falsey } it { is_expected.to be_falsey }
end 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 end
describe '#can_be_merged?' do describe '#can_be_merged?' do
......
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