Save and use actual diff base commit for MR diff highlighting

Author: DouweM

app/controllers/projects/compare_controller.rb

@@ -21,8 +21,8 @@ def show
       @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

app/controllers/projects/merge_requests_controller.rb

@@ -58,7 +58,11 @@ def show
 
   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 @@ def new
     @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

app/helpers/diff_helper.rb

@@ -102,8 +102,7 @@ def submodule_link(blob, ref, repository = @repository)
 
   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

app/models/merge_request.rb

@@ -180,6 +180,14 @@ def first_commit
     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 @@ def state_human_name
   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 @@ def ci_commit
   end
 
   def diff_refs
-    [first_commit.parent || first_commit, last_commit]
+    return nil unless diff_base_commit
+
+    [diff_base_commit, last_commit]
   end
 end

app/models/merge_request_diff.rb

@@ -73,6 +73,12 @@ def first_commit
     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 @@ def reload_diffs
     end
 
     self.st_diffs = new_diffs
+
+    self.base_commit_sha = merge_request.target_project.commit(target_branch).try(:sha)
+
     self.save
   end
 

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

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 @@
     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 @@
     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 @@
     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

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

lib/gitlab/diff/highlight.rb

@@ -31,6 +31,8 @@ def highlight
       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 @@ def highlight_line(diff_line, index)
 
         # 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 @@ def processing_args(version)
 
         [ref.project.repository, ref.id, path]
       end
+
+      def html_escape(str)
+        replacements = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;', "'" => '&#39;' }
+        str.gsub(/[&"'><]/, replacements)
+      end
     end
   end
 end

lib/gitlab/diff/inline_diff_marker.rb

@@ -9,17 +9,18 @@ def initialize(raw_line, rich_line = raw_line)
       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