Save and use actual diff base commit for MR diff highlighting

10 files changed, 75 insertions, 32 deletions

diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 60360c8e5c8..f8ec76cd4e5 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -21,8 +21,8 @@ class Projects::CompareController < Projects::ApplicationController
       @commits = Commit.decorate(compare_result.commits, @project)
       @diffs = compare_result.diffs
       @commit = @project.commit(head_ref)
-      @first_commit = @project.commit(base_ref)
-      @diff_refs = [@first_commit, @commit]
+      @base_commit = @project.commit(base_ref)
+      @diff_refs = [@base_commit, @commit]
       @line_notes = []
     end
   end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index a6284a24223..ed3050d59aa 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -58,7 +58,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def diffs
     @commit = @merge_request.last_commit
-    @first_commit = @merge_request.first_commit
+    @base_commit = @merge_request.diff_base_commit
+
+    # MRs created before 8.4 don't have a diff_base_commit,
+    # but we need it for the "View file @ ..." link by deleted files
+    @base_commit ||= @merge_request.first_commit.parent || @merge_request.first_commit
 
     @comments_allowed = @reply_allowed = true
     @comments_target = {
@@ -102,7 +106,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @source_project = merge_request.source_project
     @commits = @merge_request.compare_commits.reverse
     @commit = @merge_request.last_commit
-    @first_commit = @merge_request.first_commit
+    @base_commit = @merge_request.diff_base_commit
     @diffs = @merge_request.compare_diffs
 
     @ci_commit = @merge_request.ci_commit
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index f500d3572e0..62971d1e14b 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -102,8 +102,7 @@ module DiffHelper
 
   def commit_for_diff(diff)
     if diff.deleted_file
-      first_commit = @first_commit || @commit
-      first_commit.parent || @first_commit
+      @base_commit || @commit.parent || @commit
     else
       @commit
     end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 0a890bbf6db..41dd248d80a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -180,6 +180,14 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
 
+  def diff_base_commit
+    if merge_request_diff
+      merge_request_diff.base_commit
+    else
+      self.target_project.commit(self.target_branch)
+    end
+  end
+
   def last_commit_short_sha
     last_commit.short_id
   end
@@ -477,8 +485,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def target_sha
-    @target_sha ||= target_project.
-      repository.commit(target_branch).sha
+    @target_sha ||= target_project.repository.commit(target_branch).sha
   end
 
   def source_sha
@@ -519,6 +526,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_refs
-    [first_commit.parent || first_commit, last_commit]
+    return nil unless diff_base_commit
+
+    [diff_base_commit, last_commit]
   end
 end
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index c499a4b5b4c..ba0194cd0a6 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -73,6 +73,12 @@ class MergeRequestDiff < ActiveRecord::Base
     commits.last
   end
 
+  def base_commit
+    return nil unless self.base_commit_sha
+
+    merge_request.target_project.commit(self.base_commit_sha)
+  end
+
   def last_commit_short_sha
     @last_commit_short_sha ||= last_commit.short_id
   end
@@ -156,6 +162,9 @@ class MergeRequestDiff < ActiveRecord::Base
     end
 
     self.st_diffs = new_diffs
+
+    self.base_commit_sha = merge_request.target_project.commit(target_branch).try(:sha)
+
     self.save
   end
 
diff --git a/db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb b/db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb
new file mode 100644
index 00000000000..d6c6aa4a4e8
--- /dev/null
+++ b/db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb
@@ -0,0 +1,5 @@
+class AddBaseCommitShaToMergeRequestDiffs < ActiveRecord::Migration
+  def change
+    add_column :merge_request_diffs, :base_commit_sha, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index dc7cb9f667f..7eb99a47c71 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160119145451) do
+ActiveRecord::Schema.define(version: 20160120172143) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -490,6 +490,7 @@ ActiveRecord::Schema.define(version: 20160119145451) do
     t.integer  "merge_request_id", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string   "base_commit_sha"
   end
 
   add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree
@@ -725,19 +726,19 @@ ActiveRecord::Schema.define(version: 20160119145451) do
     t.string   "type"
     t.string   "title"
     t.integer  "project_id"
-    t.datetime "created_at",                                           null: false
-    t.datetime "updated_at",                                           null: false
-    t.boolean  "active",                                               null: false
+    t.datetime "created_at",                               null: false
+    t.datetime "updated_at",                               null: false
+    t.boolean  "active",                                   null: false
     t.text     "properties"
-    t.boolean  "template",                          default: false
-    t.boolean  "push_events",                       default: true
-    t.boolean  "issues_events",                     default: true
-    t.boolean  "merge_requests_events",             default: true
-    t.boolean  "tag_push_events",                   default: true
-    t.boolean  "note_events",                       default: true,     null: false
-    t.boolean  "build_events",                      default: false,    null: false
-    t.string   "category",                          default: "common", null: false
-    t.boolean  "default",                           default: false
+    t.boolean  "template",              default: false
+    t.boolean  "push_events",           default: true
+    t.boolean  "issues_events",         default: true
+    t.boolean  "merge_requests_events", default: true
+    t.boolean  "tag_push_events",       default: true
+    t.boolean  "note_events",           default: true,     null: false
+    t.boolean  "build_events",          default: false,    null: false
+    t.string   "category",              default: "common", null: false
+    t.boolean  "default",               default: false
   end
 
   add_index "services", ["category"], name: "index_services_on_category", using: :btree
@@ -854,7 +855,7 @@ ActiveRecord::Schema.define(version: 20160119145451) do
     t.boolean  "hide_project_limit",          default: false
     t.string   "unlock_token"
     t.datetime "otp_grace_period_started_at"
-    t.boolean  "ldap_email",                              default: false, null: false
+    t.boolean  "ldap_email",                  default: false, null: false
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 74b1c117129..a484177ae33 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -1,14 +1,22 @@
 module Gitlab
   module Diff
     class File
-      attr_reader :diff, :new_ref, :old_ref
+      attr_reader :diff, :diff_refs
 
       delegate :new_file, :deleted_file, :renamed_file,
         :old_path, :new_path, to: :diff, prefix: false
 
       def initialize(diff, diff_refs)
         @diff = diff
-        @old_ref, @new_ref = diff_refs
+        @diff_refs = diff_refs
+      end
+
+      def old_ref
+        diff_refs[0] if diff_refs
+      end
+
+      def new_ref
+        diff_refs[1] if diff_refs
       end
 
       # Array of Gitlab::DIff::Line objects
diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb
index b6875f07279..fe084a7399a 100644
--- a/lib/gitlab/diff/highlight.rb
+++ b/lib/gitlab/diff/highlight.rb
@@ -31,6 +31,8 @@ module Gitlab
       private
 
       def highlight_line(diff_line, index)
+        return html_escape(diff_line.text) unless diff_file.diff_refs
+
         line_prefix = diff_line.text.match(/\A([+-])/) ? $1 : ' '
 
         case diff_line.type
@@ -42,7 +44,7 @@ module Gitlab
 
         # Only update text if line is found. This will prevent
         # issues with submodules given the line only exists in diff content.
-        rich_line ? line_prefix + rich_line : diff_line.text
+        rich_line ? line_prefix + rich_line : html_escape(diff_line.text)
       end
 
       def inline_diffs
@@ -63,6 +65,11 @@ module Gitlab
 
         [ref.project.repository, ref.id, path]
       end
+
+      def html_escape(str)
+        replacements = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;', "'" => '&#39;' }
+        str.gsub(/[&"'><]/, replacements)
+      end
     end
   end
 end
diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb
index c31149d374e..7ca198eeb86 100644
--- a/lib/gitlab/diff/inline_diff_marker.rb
+++ b/lib/gitlab/diff/inline_diff_marker.rb
@@ -9,17 +9,18 @@ module Gitlab
       end
 
       def mark(line_inline_diffs)
-        offset = 0
+        marker_ranges = []
         line_inline_diffs.each do |inline_diff_range|
           # Map the inline-diff range based on the raw line to character positions in the rich line
           inline_diff_positions = position_mapping[inline_diff_range].flatten
           # Turn the array of character positions into ranges
-          marker_ranges = collapse_ranges(inline_diff_positions)
+          marker_ranges.concat(collapse_ranges(inline_diff_positions))
+        end
 
-          # Mark each range
-          marker_ranges.each do |range|
-            offset = insert_around_range(rich_line, range, "<span class='idiff'>", "</span>", offset)
-          end
+        offset = 0
+        # Mark each range
+        marker_ranges.each do |range|
+          offset = insert_around_range(rich_line, range, "<span class='idiff'>", "</span>", offset)
         end
 
         rich_line