From c6273ec50c019a115b11a8ef1032a64710f0a46a Mon Sep 17 00:00:00 2001
From: Oswaldo Ferreira <oswaldo@gitlab.com>
Date: Wed, 7 Mar 2018 22:29:12 -0300
Subject: [PATCH] Avoid re-fetching merge-base SHA from Gitaly unnecessarily

---
 app/models/compare.rb                         | 36 ++++++++++---------
 app/services/compare_service.rb               |  9 +++--
 ...recalculating-merge-base-on-mr-loading.yml |  5 +++
 lib/gitlab/diff/diff_refs.rb                  |  6 +++-
 spec/models/compare_spec.rb                   | 30 ++++++++++++++++
 5 files changed, 67 insertions(+), 19 deletions(-)
 create mode 100644 changelogs/unreleased/osw-stop-recalculating-merge-base-on-mr-loading.yml

diff --git a/app/models/compare.rb b/app/models/compare.rb
index 3a8bbcb1acd..19ea2bc3065 100644
--- a/app/models/compare.rb
+++ b/app/models/compare.rb
@@ -1,4 +1,6 @@
 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,41 @@ 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)
+    strong_memoize(:base_commit) do
+      return unless start_commit && head_commit
+      return OpenStruct.new(sha: @base_sha) if @base_sha
 
-    @base_commit = if start_commit && head_commit
-                     project.merge_base_commit(start_commit.id, head_commit.id)
-                   else
-                     nil
-                   end
+      project.merge_base_commit(start_commit.id, head_commit.id)
+    end
   end
 
   def start_commit_sha
-    start_commit.try(:sha)
+    start_commit&.sha
   end
 
   def base_commit_sha
-    base_commit.try(:sha)
+    base_commit&.sha
   end
 
   def head_commit_sha
-    commit.try(:sha)
+    commit&.sha
   end
 
   def raw_diffs(*args)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 1db91c3c90c..2a69a205629 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -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
diff --git a/changelogs/unreleased/osw-stop-recalculating-merge-base-on-mr-loading.yml b/changelogs/unreleased/osw-stop-recalculating-merge-base-on-mr-loading.yml
new file mode 100644
index 00000000000..1673e1d3658
--- /dev/null
+++ b/changelogs/unreleased/osw-stop-recalculating-merge-base-on-mr-loading.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid re-fetching merge-base SHA from Gitaly unnecessarily
+merge_request:
+author:
+type: performance
diff --git a/lib/gitlab/diff/diff_refs.rb b/lib/gitlab/diff/diff_refs.rb
index 88e0db830f6..81df47964be 100644
--- a/lib/gitlab/diff/diff_refs.rb
+++ b/lib/gitlab/diff/diff_refs.rb
@@ -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
diff --git a/spec/models/compare_spec.rb b/spec/models/compare_spec.rb
index 04f3cecae00..4200af0e0f5 100644
--- a/spec/models/compare_spec.rb
+++ b/spec/models/compare_spec.rb
@@ -59,6 +59,36 @@ describe Compare do
     end
   end
 
+  describe '#base_commit_sha' do
+    it 'returns @base_sha if it is present' do
+      expect(project).not_to receive(:merge_base_commit)
+
+      sha = double
+      service = described_class.new(raw_compare, project, base_sha: sha)
+
+      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
+  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