Commit 4bc58ca2 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'osw-stop-recalculating-merge-base-on-mr-loading' into 'master'

Avoid re-fetching merge-base SHA from Gitaly unnecessarily

See merge request gitlab-org/gitlab-ce!17630
parents cd7b75dd 5174e99a
class Compare
include Gitlab::Utils::StrongMemoize
delegate :same, :head, :base, to: :@compare
attr_reader :project
......@@ -11,9 +13,10 @@ class Compare
end
end
def initialize(compare, project, straight: false)
def initialize(compare, project, base_sha: nil, straight: false)
@compare = compare
@project = project
@base_sha = base_sha
@straight = straight
end
......@@ -22,40 +25,36 @@ class Compare
end
def start_commit
return @start_commit if defined?(@start_commit)
strong_memoize(:start_commit) do
commit = @compare.base
commit = @compare.base
@start_commit = commit ? ::Commit.new(commit, project) : nil
::Commit.new(commit, project) if commit
end
end
def head_commit
return @head_commit if defined?(@head_commit)
strong_memoize(:head_commit) do
commit = @compare.head
commit = @compare.head
@head_commit = commit ? ::Commit.new(commit, project) : nil
::Commit.new(commit, project) if commit
end
end
alias_method :commit, :head_commit
def base_commit
return @base_commit if defined?(@base_commit)
@base_commit = if start_commit && head_commit
project.merge_base_commit(start_commit.id, head_commit.id)
else
nil
end
end
def start_commit_sha
start_commit.try(:sha)
start_commit&.sha
end
def base_commit_sha
base_commit.try(:sha)
strong_memoize(:base_commit) do
next unless start_commit && head_commit
@base_sha || project.merge_base_commit(start_commit.id, head_commit.id)&.sha
end
end
def head_commit_sha
commit.try(:sha)
commit&.sha
end
def raw_diffs(*args)
......
......@@ -10,9 +10,14 @@ class CompareService
@start_ref_name = new_start_ref_name
end
def execute(target_project, target_ref, straight: false)
def execute(target_project, target_ref, base_sha: nil, straight: false)
raw_compare = target_project.repository.compare_source_branch(target_ref, start_project.repository, start_ref_name, straight: straight)
Compare.new(raw_compare, target_project, straight: straight) if raw_compare
return unless raw_compare
Compare.new(raw_compare,
target_project,
base_sha: base_sha,
straight: straight)
end
end
---
title: Avoid re-fetching merge-base SHA from Gitaly unnecessarily
merge_request:
author:
type: performance
......@@ -44,7 +44,11 @@ module Gitlab
project.commit(head_sha)
else
straight = start_sha == base_sha
CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
CompareService.new(project, head_sha).execute(project,
start_sha,
base_sha: base_sha,
straight: straight)
end
end
end
......
......@@ -37,33 +37,51 @@ describe Compare do
end
end
describe '#base_commit' do
let(:base_commit) { Commit.new(another_sample_commit, project) }
describe '#base_commit_sha' do
it 'returns @base_sha if it is present' do
expect(project).not_to receive(:merge_base_commit)
it 'returns project merge base commit' do
expect(project).to receive(:merge_base_commit).with(start_commit.id, head_commit.id).and_return(base_commit)
sha = double
service = described_class.new(raw_compare, project, base_sha: sha)
expect(subject.base_commit).to eq(base_commit)
expect(service.base_commit_sha).to eq(sha)
end
it 'fetches merge base SHA from repo when @base_sha is nil' do
expect(project).to receive(:merge_base_commit)
.with(start_commit.id, head_commit.id)
.once
.and_call_original
expect(subject.base_commit_sha)
.to eq(project.repository.merge_base(start_commit.id, head_commit.id))
end
it 'is memoized on first call' do
expect(project).to receive(:merge_base_commit)
.with(start_commit.id, head_commit.id)
.once
.and_call_original
3.times { subject.base_commit_sha }
end
it 'returns nil if there is no start_commit' do
expect(subject).to receive(:start_commit).and_return(nil)
expect(subject.base_commit).to eq(nil)
expect(subject.base_commit_sha).to eq(nil)
end
it 'returns nil if there is no head commit' do
expect(subject).to receive(:head_commit).and_return(nil)
expect(subject.base_commit).to eq(nil)
expect(subject.base_commit_sha).to eq(nil)
end
end
describe '#diff_refs' do
it 'uses base_commit sha as base_sha' do
expect(subject).to receive(:base_commit).at_least(:once).and_call_original
expect(subject.diff_refs.base_sha).to eq(subject.base_commit.id)
it 'uses base_commit_sha sha as base_sha' do
expect(subject.diff_refs.base_sha).to eq(subject.base_commit_sha)
end
it 'uses start_commit sha as start_sha' 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