Merge branch 'mr-merge-base' into 'master'

Correctly determine MR diff base when MR has merge conflicts

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/12779

See merge request !2632

7 files changed, 24 insertions, 22 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 322765c6d08..fe0504ec996 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -27,6 +27,7 @@ v 8.4.2
     improvement when checking if a repository was empty
   - Add instrumentation for Gitlab::Git::Repository instance methods so we can
     track them in Performance Monitoring.
+  - Correctly highlight MR diff when MR has merge conflicts
   - Increase contrast between highlighted code comments and inline diff marker
   - Fix method undefined when using external commit status in builds
   - Fix highlighting in blame view.
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index f8ec76cd4e5..7bbe75b3974 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController
       @commits = Commit.decorate(compare_result.commits, @project)
       @diffs = compare_result.diffs
       @commit = @project.commit(head_ref)
-      @base_commit = @project.commit(base_ref)
+      @base_commit = @project.merge_base_commit(base_ref, head_ref)
       @diff_refs = [@base_commit, @commit]
       @line_notes = []
     end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 41dd248d80a..0af60645545 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -183,8 +183,8 @@ class MergeRequest < ActiveRecord::Base
   def diff_base_commit
     if merge_request_diff
       merge_request_diff.base_commit
-    else
-      self.target_project.commit(self.target_branch)
+    elsif source_sha
+      self.target_project.merge_base_commit(self.source_sha, self.target_branch)
     end
   end
 
@@ -489,7 +489,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def source_sha
-    commits.first.sha
+    last_commit.try(:sha)
   end
 
   def fetch_ref
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index ba0194cd0a6..c95179d6046 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -48,14 +48,11 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def diffs_no_whitespace
-    # Get latest sha of branch from source project
-    source_sha = merge_request.source_project.commit(source_branch).sha
-
     compare_result = Gitlab::CompareResult.new(
       Gitlab::Git::Compare.new(
-        merge_request.target_project.repository.raw_repository,
-        merge_request.target_branch,
-        source_sha,
+        self.repository.raw_repository,
+        self.target_branch,
+        self.source_sha,
       ), { ignore_whitespace_change: true }
     )
     @diffs_no_whitespace ||= load_diffs(dump_commits(compare_result.diffs))
@@ -83,8 +80,6 @@ class MergeRequestDiff < ActiveRecord::Base
     @last_commit_short_sha ||= last_commit.short_id
   end
 
-  private
-
   def dump_commits(commits)
     commits.map(&:to_hash)
   end
@@ -163,7 +158,7 @@ class MergeRequestDiff < ActiveRecord::Base
 
     self.st_diffs = new_diffs
 
-    self.base_commit_sha = merge_request.target_project.commit(target_branch).try(:sha)
+    self.base_commit_sha = self.repository.merge_base(self.source_sha, self.target_branch)
 
     self.save
   end
@@ -181,7 +176,10 @@ class MergeRequestDiff < ActiveRecord::Base
     merge_request.target_project.repository
   end
 
-  private
+  def source_sha
+    source_commit = merge_request.source_project.commit(source_branch)
+    source_commit.try(:sha)
+  end
 
   def compare_result
     @compare_result ||=
@@ -189,15 +187,11 @@ class MergeRequestDiff < ActiveRecord::Base
         # Update ref for merge request
         merge_request.fetch_ref
 
-        # Get latest sha of branch from source project
-        source_commit = merge_request.source_project.commit(source_branch)
-        source_sha = source_commit.try(:sha)
-
         Gitlab::CompareResult.new(
           Gitlab::Git::Compare.new(
-            merge_request.target_project.repository.raw_repository,
-            merge_request.target_branch,
-            source_sha,
+            self.repository.raw_repository,
+            self.target_branch,
+            self.source_sha
           )
         )
       end
diff --git a/app/models/project.rb b/app/models/project.rb
index 4bd51449c25..238932f59a7 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -348,6 +348,11 @@ class Project < ActiveRecord::Base
     repository.commit(id)
   end
 
+  def merge_base_commit(first_commit_id, second_commit_id)
+    sha = repository.merge_base(first_commit_id, second_commit_id)
+    repository.commit(sha) if sha
+  end
+
   def saved?
     id && persisted?
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6c1ee4b29cd..130daddd9d1 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -589,6 +589,8 @@ class Repository
 
   def merge_base(first_commit_id, second_commit_id)
     rugged.merge_base(first_commit_id, second_commit_id)
+  rescue Rugged::ReferenceError
+    nil
   end
 
   def is_ancestor?(ancestor_id, descendant_id)
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index d30bc7d0554..606340d87e4 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Ci::Build, models: true do
-  let(:project) { FactoryGirl.create :empty_project }
+  let(:project) { FactoryGirl.create :project }
   let(:commit) { FactoryGirl.create :ci_commit, project: project }
   let(:build) { FactoryGirl.create :ci_build, commit: commit }