Merge branch 'new-diff-notes' into 'master'

New diff notes

Fixes #12732, #14731, #19375, #14783 

Builds on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4110

To do:
- [x] Get it mostly working
- [x] Validate position validity
- [x] Fix: Don’t link to `#`
- [x] Fix: Base ref can be `nil`, potentially, when the MR has an oprhan source branch => Yep, doesn’t work. We need to store a `start_id`
- [x] Optimize: Fewer duplicate `git diff` compares
- [x] Optimize: Pass paths to `PositionTracer#diff` for faster diffs
- [x] Refactor: Use `head_id` in `MergeRequest`/`MergeRequestDiff` instead of `source_sha`
- [x] Refactor: Convert existing array-based diff refs to the DiffRefs model
- [x] Tweak: Use `note_type` in `Autosave` key
- [x] Tweak: Remove `line_code: note.line_code` from `link_to_reply_discussion`
- [x] Update: `SentNotifications` and reply-by-email receiver
- [x] Update: MR diff notification email
- [x] Update: API (MR, Commit note creation and entity)
- [x] Update: GitHub importer
- [x] Address any other TODO comments
- [x] Fix: Suppress "edited 4 minutes ago"
- [x] Write tests
  - [x] `LineMapper`
  - [x] `PositionTracer`
  - [x] `Position`
  - [x] `DiffPositionUpdateService`
  - [x] `DiffNote`
  - [x] `MergeRequests::RefreshService` / `MergeRequest#update_diff_notes_positions`
- [x] Make sure commits with diff notes don't get cleaned up, since this would prevent the diff notes from being rendered (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5062)

Future improvements:
- Display unresolved comments on files outside the diff, if the comment was added when that file _was_ part of the diff
- Allow commenting on sections between hunks, when expanding the diff using `...`
  - (We'd need to generate line code based on Position if we have it, even if it falls outside bounds of diff)
- `diff_hunk` on diff note API entity
- Show diff hunk in notification email
- Resolved line notes would have a boolean, and be inactive through `notes.any? { !active? || resolved? }`
- Multi line notes would store a number of positions, and do the right thing (™) in grouping and then rendering if the first item is multiline? => true
- Image diff notes could store x,y,width,height instead of old_line,new_line for similar grouping. Does it need a reference to say if it's on old or new? These can't have line_codes, clearly. Rendering would be interesting.
- Show commit line comments in the MR diff
- Comment on specific selected words
- Comment on file header
- Unfold top of discussion diff note
- New diff notes API for commits and MRs

/cc @rspeicher

See merge request !4101

app/assets/javascripts/notes.js.coffee

@@ -240,12 +240,16 @@ class @Notes
 
     @note_ids.push(note.id)
     form = $("#new-discussion-note-form-#{note.discussion_id}")
+    if note.original_discussion_id? and form.length is 0
+      form = $("#new-discussion-note-form-#{note.original_discussion_id}")
     row = form.closest("tr")
     note_html = $(note.html)
     note_html.syntaxHighlight()
 
     # is this the first note of discussion?
     discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']")
+    if note.original_discussion_id? and discussionContainer.length is 0
+      discussionContainer = $(".notes[data-discussion-id='" + note.original_discussion_id + "']")
     if discussionContainer.length is 0
       # insert the note and the reply button after the temp row
       row.after note.discussion_html
@@ -318,6 +322,7 @@ class @Notes
     form.addClass "js-main-target-form"
 
     form.find("#note_line_code").remove()
+    form.find("#note_position").remove()
     form.find("#note_type").remove()
 
   ###
@@ -335,10 +340,12 @@ class @Notes
 
     new Autosave textarea, [
       "Note"
-      form.find("#note_commit_id").val()
-      form.find("#note_line_code").val()
       form.find("#note_noteable_type").val()
       form.find("#note_noteable_id").val()
+      form.find("#note_commit_id").val()
+      form.find("#note_type").val()
+      form.find("#note_line_code").val()
+      form.find("#note_position").val()
     ]
 
   ###
@@ -512,10 +519,12 @@ class @Notes
   setupDiscussionNoteForm: (dataHolder, form) =>
     # setup note target
     form.attr 'id', "new-discussion-note-form-#{dataHolder.data("discussionId")}"
+    form.attr "data-line-code", dataHolder.data("lineCode")
     form.find("#note_type").val dataHolder.data("noteType")
     form.find("#line_type").val dataHolder.data("lineType")
     form.find("#note_commit_id").val dataHolder.data("commitId")
     form.find("#note_line_code").val dataHolder.data("lineCode")
+    form.find("#note_position").val dataHolder.attr("data-position")
     form.find("#note_noteable_type").val dataHolder.data("noteableType")
     form.find("#note_noteable_id").val dataHolder.data("noteableId")
     form.find('.js-note-discard')

app/assets/stylesheets/framework/files.scss

@@ -50,7 +50,7 @@
     }
 
     a:not(.btn) {
-      color: $gl-dark-link-color;
+      color: $gl-text-color;
     }
 
     .left-options {

app/assets/stylesheets/pages/diff.scss

@@ -434,13 +434,3 @@
     }
   }
 }
-
-.discussion {
-  .diff-content {
-    .diff-line-num {
-      &:before {
-        content: attr(data-linenumber);
-      }
-    }
-  }
-}

app/controllers/projects/blob_controller.rb

@@ -57,7 +57,7 @@ class Projects::BlobController < Projects::ApplicationController
     diffy = Diffy::Diff.new(@blob.data, @content, diff: '-U 3', include_diff_info: true)
     diff_lines = diffy.diff.scan(/.*\n/)[2..-1]
     diff_lines = Gitlab::Diff::Parser.new.parse(diff_lines)
-    @diff_lines = Gitlab::Diff::Highlight.new(diff_lines).highlight
+    @diff_lines = Gitlab::Diff::Highlight.new(diff_lines, repository: @repository).highlight
 
     render layout: false
   end

app/controllers/projects/commit_controller.rb

@@ -121,7 +121,6 @@ class Projects::CommitController < Projects::ApplicationController
     opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
 
     @diffs = commit.diffs(opts)
-    @diff_refs = [commit.parent || commit, commit]
     @notes_count = commit.notes.count
 
     @statuses = CommitStatus.where(pipeline: pipelines)

app/controllers/projects/compare_controller.rb

@@ -14,14 +14,22 @@ class Projects::CompareController < Projects::ApplicationController
 
   def show
     compare = CompareService.new.
-      execute(@project, @head_ref, @project, @base_ref, diff_options)
+      execute(@project, @head_ref, @project, @start_ref, diff_options)
 
     if compare
       @commits = Commit.decorate(compare.commits, @project)
+
+      @start_commit = @project.commit(@start_ref)
       @commit = @project.commit(@head_ref)
-      @base_commit = @project.merge_base_commit(@base_ref, @head_ref)
+      @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
+
       @diffs = compare.diffs(diff_options)
-      @diff_refs = [@base_commit, @commit]
+      @diff_refs = Gitlab::Diff::DiffRefs.new(
+        base_sha: @base_commit.try(:sha),
+        start_sha: @start_commit.try(:sha),
+        head_sha: @commit.try(:sha)
+      )
+
       @diff_notes_disabled = true
       @grouped_diff_notes = {}
     end
@@ -35,12 +43,12 @@ class Projects::CompareController < Projects::ApplicationController
   private
 
   def assign_ref_vars
-    @base_ref = Addressable::URI.unescape(params[:from])
+    @start_ref = Addressable::URI.unescape(params[:from])
     @ref = @head_ref = Addressable::URI.unescape(params[:to])
   end
 
   def merge_request
     @merge_request ||= @project.merge_requests.opened.
-      find_by(source_project: @project, source_branch: @head_ref, target_branch: @base_ref)
+      find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)
   end
 end

app/controllers/projects/merge_requests_controller.rb

@@ -58,14 +58,17 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html
-      format.json   { render json: @merge_request }
+      
+      format.json do
+        render json: @merge_request
+      end
+
       format.patch  do
-        headers.store(*Gitlab::Workhorse.send_git_patch(@project.repository,
-                                                        @merge_request.diff_base_commit.id,
-                                                        @merge_request.last_commit.id))
-        headers['Content-Disposition'] = 'inline'
-        head :ok
+        return render_404 unless @merge_request.diff_refs
+
+        send_git_patch @project.repository, @merge_request.diff_refs
       end
+
       format.diff do
         return render_404 unless @merge_request.diff_refs
 
@@ -77,18 +80,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diffs
     apply_diff_view_cookie!
 
-    @commit = @merge_request.last_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
+    @commit = @merge_request.diff_head_commit
+    @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
 
     @comments_target = {
       noteable_type: 'MergeRequest',
       noteable_id: @merge_request.id
     }
 
+    @use_legacy_diff_notes = !@merge_request.support_new_diff_notes?
     @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
 
     Banzai::NoteRenderer.render(
@@ -134,7 +134,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @target_project = merge_request.target_project
     @source_project = merge_request.source_project
     @commits = @merge_request.compare_commits.reverse
-    @commit = @merge_request.last_commit
+    @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit
     @diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
     @diff_notes_disabled = true
@@ -212,7 +212,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return
     end
 
-    if params[:sha] != @merge_request.source_sha
+    if params[:sha] != @merge_request.diff_head_sha
       @status = :sha_mismatch
       return
     end
@@ -274,16 +274,16 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       status ||= "preparing"
     else
       ci_service = @merge_request.source_project.ci_service
-      status = ci_service.commit_status(merge_request.last_commit.sha, merge_request.source_branch) if ci_service
+      status = ci_service.commit_status(merge_request.diff_head_sha, merge_request.source_branch) if ci_service
 
       if ci_service.respond_to?(:commit_coverage)
-        coverage = ci_service.commit_coverage(merge_request.last_commit.sha, merge_request.source_branch)
+        coverage = ci_service.commit_coverage(merge_request.diff_head_sha, merge_request.source_branch)
       end
     end
 
     response = {
       title: merge_request.title,
-      sha: merge_request.last_commit_short_sha,
+      sha: merge_request.diff_head_commit.short_id,
       status: status,
       coverage: coverage
     }

app/controllers/projects/notes_controller.rb

@@ -128,7 +128,7 @@ class Projects::NotesController < Projects::ApplicationController
     elsif note.valid?
       Banzai::NoteRenderer.render([note], @project, current_user)
 
-      {
+      attrs = {
         valid: true,
         id: note.id,
         discussion_id: note.discussion_id,
@@ -138,6 +138,23 @@ class Projects::NotesController < Projects::ApplicationController
         discussion_html: note_to_discussion_html(note),
         discussion_with_diff_html: note_to_discussion_with_diff_html(note)
       }
+
+      # The discussion_id is used to add the comment to the correct discussion
+      # element on the merge request page. Among other things, the discussion_id
+      # contains the sha of head commit of the merge request.
+      # When new commits are pushed into the merge request after the initial
+      # load of the merge request page, the discussion elements will still have
+      # the old discussion_ids, with the old head commit sha. The new comment,
+      # however, will have the new discussion_id with the new commit sha.
+      # To ensure that these new comments will still end up in the correct
+      # discussion element, we also send the original discussion_id, with the
+      # old commit sha, along, and fall back on this value when no discussion
+      # element with the new discussion_id could be found.
+      if note.new_diff_note? && note.position != note.original_position
+        attrs[:original_discussion_id] = note.original_discussion_id
+      end
+
+      attrs
     else
       {
         valid: false,
@@ -154,7 +171,7 @@ class Projects::NotesController < Projects::ApplicationController
   def note_params
     params.require(:note).permit(
       :note, :noteable, :noteable_id, :noteable_type, :project_id,
-      :attachment, :line_code, :commit_id, :type
+      :attachment, :line_code, :commit_id, :type, :position
     )
   end
 

app/helpers/application_helper.rb

@@ -306,4 +306,15 @@ module ApplicationHelper
   def truncate_first_line(message, length = 50)
     truncate(message.each_line.first.chomp, length: length) if message
   end
+
+  # While similarly named to Rails's `link_to_if`, this method behaves quite differently.
+  # If `condition` is truthy, a link will be returned with the result of the block
+  # as its body. If `condition` is falsy, only the result of the block will be returned.
+  def conditional_link_to(condition, options, html_options = {}, &block)
+    if condition
+      link_to options, html_options, &block
+    else
+      capture(&block)
+    end
+  end
 end

app/helpers/diff_helper.rb

@@ -30,12 +30,8 @@ module DiffHelper
     options
   end
 
-  def safe_diff_files(diffs, diff_refs)
-    diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs) }
-  end
-
-  def generate_line_code(file_path, line)
-    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
+  def safe_diff_files(diffs, diff_refs: nil, repository: nil)
+    diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
   end
 
   def unfold_bottom_class(bottom)
@@ -93,6 +89,8 @@ module DiffHelper
   end
 
   def commit_for_diff(diff_file)
+    return diff_file.content_commit if diff_file.content_commit
+    
     if diff_file.deleted_file
       @base_commit || @commit.parent || @commit
     else
@@ -100,10 +98,11 @@ module DiffHelper
     end
   end
 
-  def diff_file_html_data(project, diff_commit, diff_file)
+  def diff_file_html_data(project, diff_file)
+    commit = commit_for_diff(diff_file)
     {
       blob_diff_path: namespace_project_blob_diff_path(project.namespace, project,
-                                                       tree_join(diff_commit.id, diff_file.file_path))
+                                                       tree_join(commit.id, diff_file.file_path))
     }
   end
 

app/helpers/merge_requests_helper.rb

@@ -27,7 +27,7 @@ module MergeRequestsHelper
   end
 
   def ci_build_details_path(merge_request)
-    build_url = merge_request.source_project.ci_service.build_page(merge_request.last_commit.sha, merge_request.source_branch)
+    build_url = merge_request.source_project.ci_service.build_page(merge_request.diff_head_sha, merge_request.source_branch)
     return nil unless build_url
 
     parsed_url = URI.parse(build_url)

app/helpers/notes_helper.rb

@@ -24,23 +24,55 @@ module NotesHelper
     }.to_json
   end
 
-  def link_to_new_diff_note(line_code, line_type = nil)
-    discussion_id = LegacyDiffNote.build_discussion_id(
-      @comments_target[:noteable_type],
-      @comments_target[:noteable_id] || @comments_target[:commit_id],
-      line_code
-    )
+  def link_to_new_diff_note(line_code, position, line_type = nil)
+    use_legacy_diff_note = @use_legacy_diff_notes
+    # If the controller doesn't force the use of legacy diff notes, we
+    # determine this on a line-by-line basis by seeing if there already exist
+    # active legacy diff notes at this line, in which case newly created notes
+    # will use the legacy technology as well.
+    # We do this because the discussion_id values of legacy and "new" diff
+    # notes, which are used to group notes on the merge request discussion tab,
+    # are incompatible.
+    # If we didn't, diff notes that would show for the same line on the changes
+    # tab, would show in different discussions on the discussion tab.
+    use_legacy_diff_note ||= begin
+      line_diff_notes = @grouped_diff_notes[line_code]
+      line_diff_notes && line_diff_notes.any?(&:legacy_diff_note?)
+    end
 
     data = {
       noteable_type: @comments_target[:noteable_type],
       noteable_id:   @comments_target[:noteable_id],
       commit_id:     @comments_target[:commit_id],
       line_type:     line_type,
-      line_code:     line_code,
-      note_type:     LegacyDiffNote.name,
-      discussion_id: discussion_id
+      line_code:     line_code
     }
 
+    if use_legacy_diff_note
+      discussion_id = LegacyDiffNote.build_discussion_id(
+        @comments_target[:noteable_type],
+        @comments_target[:noteable_id] || @comments_target[:commit_id],
+        line_code
+      )
+
+      data.merge!(
+        note_type: LegacyDiffNote.name,
+        discussion_id: discussion_id
+      )
+    else
+      discussion_id = DiffNote.build_discussion_id(
+        @comments_target[:noteable_type],
+        @comments_target[:noteable_id] || @comments_target[:commit_id],
+        position
+      )
+
+      data.merge!(
+        position: position.to_json,
+        note_type: DiffNote.name,
+        discussion_id: discussion_id
+      )
+    end
+
     button_tag(class: 'btn add-diff-note js-add-diff-note-button',
                data: data,
                title: 'Add a comment to this line') do
@@ -60,14 +92,15 @@ module NotesHelper
     }
 
     if note.diff_note?
-      data.merge!(
-        line_code: note.line_code,
-        note_type: LegacyDiffNote.name
-      )
+      data[:note_type] = note.type
+
+      data.merge!(note.diff_attributes)
     end
 
-    button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
-                           data: data, title: 'Add a reply'
+    content_tag(:div, class: "discussion-reply-holder") do
+      button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
+                             data: data, title: 'Add a reply'
+    end
   end
 
   def note_max_access_for_user(note)
@@ -79,4 +112,14 @@ module NotesHelper
     full_key = { project: note.project, user_id: note.author_id }
     @max_access_by_user_id[full_key]
   end
+
+  def diff_note_path(note)
+    return unless note.diff_note?
+
+    if note.for_merge_request? && note.active?
+      diffs_namespace_project_merge_request_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code)
+    elsif note.for_commit?
+      namespace_project_commit_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code)
+    end
+  end
 end

app/helpers/workhorse_helper.rb

-# Helpers to send Git blobs, diffs or archives through Workhorse.
+# Helpers to send Git blobs, diffs, patches or archives through Workhorse.
 # Workhorse will also serve files when using `send_file`.
 module WorkhorseHelper
   # Send a Git blob through Workhorse
@@ -16,6 +16,13 @@ module WorkhorseHelper
     head :ok
   end
 
+  # Send a Git patch through Workhorse
+  def send_git_patch(repository, diff_refs)
+    headers.store(*Gitlab::Workhorse.send_git_patch(repository, diff_refs))
+    headers['Content-Disposition'] = 'inline'
+    head :ok
+  end
+
   # Archive a Git repository and send it through Workhorse
   def send_git_archive(repository, ref:, format:)
     headers.store(*Gitlab::Workhorse.send_git_archive(repository, ref: ref, format: format))

app/mailers/emails/projects.rb

@@ -29,8 +29,7 @@ module Emails
       # used in notify layout
       @target_url = @message.target_url
       @project = Project.find(project_id)
-      @diff_notes_disabled = true
-
+      
       add_project_headers
       headers['X-GitLab-Author'] = @message.author_username
 

app/models/commit.rb

@@ -214,6 +214,13 @@ class Commit
     @raw.short_id(7)
   end
 
+  def diff_refs
+    Gitlab::Diff::DiffRefs.new(
+      base_sha: self.parent_id || self.sha,
+      head_sha: self.sha
+    )
+  end
+
   def pipelines
     @pipeline ||= project.pipelines.where(sha: sha)
   end

app/models/concerns/note_on_diff.rb

+module NoteOnDiff
+  extend ActiveSupport::Concern
+
+  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+
+  included do
+    delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true
+  end
+
+  def diff_note?
+    true
+  end
+
+  def diff_file
+    raise NotImplementedError
+  end
+
+  def diff_line
+    raise NotImplementedError
+  end
+
+  def for_line?(line)
+    raise NotImplementedError
+  end
+
+  def diff_attributes
+    raise NotImplementedError
+  end
+
+  def can_be_award_emoji?
+    false
+  end
+
+  # Returns an array of at most 16 highlighted lines above a diff note
+  def truncated_diff_lines
+    prev_lines = []
+
+    highlighted_diff_lines.each do |line|
+      if line.meta?
+        prev_lines.clear
+      else
+        prev_lines << line
+
+        break if for_line?(line)
+
+        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
+      end
+    end
+
+    prev_lines
+  end
+end

app/models/deployment.rb

@@ -11,6 +11,8 @@ class Deployment < ActiveRecord::Base
 
   delegate :name, to: :environment, prefix: true
 
+  after_save :keep_around_commit
+
   def commit
     project.commit(sha)
   end
@@ -26,4 +28,8 @@ class Deployment < ActiveRecord::Base
   def last?
     self == environment.last_deployment
   end
+
+  def keep_around_commit
+    project.repository.keep_around(self.sha)
+  end
 end

app/models/diff_note.rb

+class DiffNote < Note
+  include NoteOnDiff
+
+  serialize :original_position, Gitlab::Diff::Position
+  serialize :position, Gitlab::Diff::Position
+
+  validates :original_position, presence: true
+  validates :position, presence: true
+  validates :diff_line, presence: true
+  validates :line_code, presence: true, line_code: true
+  validates :noteable_type, inclusion: { in: ['Commit', 'MergeRequest'] }
+  validate :positions_complete
+  validate :verify_supported
+
+  before_validation :set_original_position, :update_position, on: :create
+  before_validation :set_line_code
+  after_save :keep_around_commits
+
+  class << self
+    def build_discussion_id(noteable_type, noteable_id, position)
+      [super(noteable_type, noteable_id), *position.key].join("-")
+    end
+  end
+
+  def new_diff_note?
+    true
+  end
+
+  def diff_attributes
+    { position: position.to_json }
+  end
+
+  def discussion_id
+    @discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position)
+  end
+
+  def original_discussion_id
+    @original_discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position)
+  end
+
+  def position=(new_position)
+    if new_position.is_a?(String)
+      new_position = JSON.parse(new_position) rescue nil
+    end
+
+    if new_position.is_a?(Hash)
+      new_position = new_position.with_indifferent_access
+      new_position = Gitlab::Diff::Position.new(new_position)
+    end
+
+    super(new_position)
+  end
+
+  def diff_file
+    @diff_file ||= self.original_position.diff_file(self.project.repository)
+  end
+
+  def diff_line
+    @diff_line ||= diff_file.line_for_position(self.original_position) if diff_file
+  end
+
+  def for_line?(line)
+    diff_file.position(line) == self.original_position
+  end
+
+  def active?(diff_refs = nil)
+    return false unless supported?
+    return true if for_commit?
+
+    diff_refs ||= self.noteable.diff_refs
+
+    self.position.diff_refs == diff_refs
+  end
+
+  private
+
+  def supported?
+    !self.for_merge_request? || self.noteable.support_new_diff_notes?
+  end
+
+  def set_original_position
+    self.original_position = self.position.dup
+  end
+
+  def set_line_code
+    self.line_code = self.position.line_code(self.project.repository)
+  end
+
+  def update_position
+    return unless supported?
+    return if for_commit?
+
+    return if active?
+
+    Notes::DiffPositionUpdateService.new(
+      self.project,
+      nil,
+      old_diff_refs: self.position.diff_refs,
+      new_diff_refs: self.noteable.diff_refs,
+      paths: self.position.paths
+    ).execute(self)
+  end
+
+  def verify_supported
+    return if supported?
+
+    errors.add(:noteable, "doesn't support new-style diff notes")
+  end
+
+  def positions_complete
+    return if self.original_position.complete? && self.position.complete?
+
+    errors.add(:position, "is invalid")
+  end
+
+  def keep_around_commits
+    project.repository.keep_around(self.original_position.base_sha)
+    project.repository.keep_around(self.original_position.start_sha)
+    project.repository.keep_around(self.original_position.head_sha)
+
+    if self.position != self.original_position
+      project.repository.keep_around(self.position.base_sha)
+      project.repository.keep_around(self.position.start_sha)
+      project.repository.keep_around(self.position.head_sha)
+    end
+  end
+end

app/models/legacy_diff_note.rb

 class LegacyDiffNote < Note
+  include NoteOnDiff
+
   serialize :st_diff
 
   validates :line_code, presence: true, line_code: true
@@ -11,12 +13,12 @@ class LegacyDiffNote < Note
     end
   end
 
-  def diff_note?
+  def legacy_diff_note?
     true
   end
 
-  def legacy_diff_note?
-    true
+  def diff_attributes
+    { line_code: line_code }
   end
 
   def discussion_id
@@ -27,61 +29,20 @@ class LegacyDiffNote < Note
     line_code.split('_')[0] if line_code
   end
 
-  def diff_old_line
-    line_code.split('_')[1].to_i if line_code
-  end
-
-  def diff_new_line
-    line_code.split('_')[2].to_i if line_code
-  end
-
   def diff
     @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
   end
 
-  def diff_file_path
-    diff.new_path.presence || diff.old_path
-  end
-
-  def diff_lines
-    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
+  def diff_file
+    @diff_file ||= Gitlab::Diff::File.new(diff, repository: self.project.repository) if diff
   end
 
   def diff_line
-    @diff_line ||= diff_lines.find { |line| generate_line_code(line) == self.line_code }
+    @diff_line ||= diff_file.line_for_line_code(self.line_code)
   end
 
-  def diff_line_text
-    diff_line.try(:text)
-  end
-
-  def diff_line_type
-    diff_line.try(:type)
-  end
-
-  def highlighted_diff_lines
-    Gitlab::Diff::Highlight.new(diff_lines).highlight
-  end
-
-  def truncated_diff_lines
-    max_number_of_lines = 16
-    prev_match_line = nil
-    prev_lines = []
-
-    highlighted_diff_lines.each do |line|
-      if line.type == "match"
-        prev_lines.clear
-        prev_match_line = line
-      else
-        prev_lines << line
-
-        break if generate_line_code(line) == self.line_code
-
-        prev_lines.shift if prev_lines.length >= max_number_of_lines
-      end
-    end
-
-    prev_lines
+  def for_line?(line)
+    !line.meta? && diff_file.line_code(line) == self.line_code
   end
 
   # Check if this note is part of an "active" discussion
@@ -102,7 +63,7 @@ class LegacyDiffNote < Note
     if noteable_diff
       parsed_lines = Gitlab::Diff::Parser.new.parse(noteable_diff.diff.each_line)
 
-      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line_text }
+      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line.text }
     else
       @active = false
     end
@@ -110,10 +71,6 @@ class LegacyDiffNote < Note
     @active
   end
 
-  def award_emoji_supported?
-    false
-  end
-
   private
 
   def find_diff
@@ -149,10 +106,6 @@ class LegacyDiffNote < Note
     self.class.where(attributes).last.try(:diff)
   end
 
-  def generate_line_code(line)
-    Gitlab::Diff::LineCode.generate(diff_file_path, line.new_pos, line.old_pos)
-  end
-
   # Find the diff on noteable that matches our own
   def find_noteable_diff
     diffs = noteable.diffs(Commit.max_diff_options)

app/models/merge_request.rb

@@ -16,7 +16,7 @@ class MergeRequest < ActiveRecord::Base
 
   serialize :merge_params, Hash
 
-  after_create :create_merge_request_diff, unless: :importing
+  after_create :create_merge_request_diff, unless: :importing?
   after_update :update_merge_request_diff
 
   delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil
@@ -29,10 +29,6 @@ class MergeRequest < ActiveRecord::Base
   # when creating new merge request
   attr_accessor :can_be_created, :compare_commits, :compare
 
-  # Temporary fields to store target_sha, and base_sha to
-  # compare when importing pull requests from GitHub
-  attr_accessor :base_target_sha, :head_source_sha
-
   state_machine :state, initial: :opened do
     event :close do
       transition [:reopened, :opened] => :closed
@@ -89,12 +85,7 @@ class MergeRequest < ActiveRecord::Base
     state :cannot_be_merged
 
     around_transition do |merge_request, transition, block|
-      merge_request.record_timestamps = false
-      begin
-        block.call
-      ensure
-        merge_request.record_timestamps = true
-      end
+      Gitlab::Timeless.timeless(merge_request, &block)
     end
   end
 
@@ -169,10 +160,6 @@ class MergeRequest < ActiveRecord::Base
     reference
   end
 
-  def last_commit
-    merge_request_diff ? merge_request_diff.last_commit : compare_commits.last
-  end
-
   def first_commit
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
@@ -182,15 +169,86 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_base_commit
-    if merge_request_diff
+    if persisted?
       merge_request_diff.base_commit
-    elsif source_sha
-      self.target_project.merge_base_commit(self.source_sha, self.target_branch)
+    elsif diff_start_commit && diff_head_commit
+      self.target_project.merge_base_commit(diff_start_sha, diff_head_sha)
+    end
+  end
+
+  # MRs created before 8.4 don't store a MergeRequestDiff#base_commit_sha,
+  # but we need to get a commit for the "View file @ ..." link by deleted files,
+  # so we find the likely one if we can't get the actual one.
+  # This will not be the actual base commit if the target branch was merged into
+  # the source branch after the merge request was created, but it is good enough
+  # for the specific purpose of linking to a commit.
+  # It is not good enough for use in `Gitlab::Git::DiffRefs`, which needs the
+  # true base commit, so we can't simply have `#diff_base_commit` fall back on
+  # this method.
+  def likely_diff_base_commit
+    first_commit.parent || first_commit
+  end
+
+  def diff_start_commit
+    if persisted?
+      merge_request_diff.start_commit
+    else
+      target_branch_head
+    end
+  end
+
+  def diff_head_commit
+    if persisted?
+      merge_request_diff.head_commit
+    else
+      source_branch_head
     end
   end
 
-  def last_commit_short_sha
-    last_commit.short_id
+  def diff_start_sha
+    diff_start_commit.try(:sha)
+  end
+
+  def diff_base_sha
+    diff_base_commit.try(:sha)
+  end
+
+  def diff_head_sha
+    diff_head_commit.try(:sha)
+  end
+
+  # When importing a pull request from GitHub, the old and new branches may no
+  # longer actually exist by those names, but we need to recreate the merge
+  # request diff with the right source and target shas.
+  # We use these attributes to force these to the intended values.
+  attr_writer :target_branch_sha, :source_branch_sha
+
+  def source_branch_head
+    source_branch_ref = @source_branch_sha || source_branch
+    source_project.repository.commit(source_branch) if source_branch_ref
+  end
+
+  def target_branch_head
+    target_branch_ref = @target_branch_sha || target_branch
+    target_project.repository.commit(target_branch) if target_branch_ref
+  end
+
+  def target_branch_sha
+    target_branch_head.try(:sha)
+  end
+
+  def source_branch_sha
+    source_branch_head.try(:sha)
+  end
+
+  def diff_refs
+    return unless diff_start_commit || diff_base_commit
+
+    Gitlab::Diff::DiffRefs.new(
+      base_sha:  diff_base_sha,
+      start_sha: diff_start_sha,
+      head_sha:  diff_head_sha
+    )
   end
 
   def validate_branches
@@ -227,21 +285,30 @@ class MergeRequest < ActiveRecord::Base
 
   def update_merge_request_diff
     if source_branch_changed? || target_branch_changed?
-      reload_code
+      reload_diff
     end
   end
 
-  def reload_code
-    if merge_request_diff && open?
-      merge_request_diff.reload_content
-    end
+  def reload_diff
+    return unless merge_request_diff && open?
+
+    old_diff_refs = self.diff_refs
+
+    merge_request_diff.reload_content
+
+    new_diff_refs = self.diff_refs
+
+    update_diff_notes_positions(
+      old_diff_refs: old_diff_refs,
+      new_diff_refs: new_diff_refs
+    )
   end
 
   def check_if_can_be_merged
     return unless unchecked?
 
     can_be_merged =
-      !broken? && project.repository.can_be_merged?(source_sha, target_branch)
+      !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch)
 
     if can_be_merged
       mark_as_mergeable
@@ -293,7 +360,7 @@ class MergeRequest < ActiveRecord::Base
     !source_project.protected_branch?(source_branch) &&
       !source_project.root_ref?(source_branch) &&
       Ability.abilities.allowed?(current_user, :push_code, source_project) &&
-      last_commit == source_project.commit(source_branch)
+      diff_head_commit == source_branch_head
   end
 
   def should_remove_source_branch?
@@ -331,8 +398,8 @@ class MergeRequest < ActiveRecord::Base
       work_in_progress: work_in_progress?
     }
 
-    if last_commit
-      attrs.merge!(last_commit: last_commit.hook_attrs)
+    if diff_head_commit
+      attrs.merge!(last_commit: diff_head_commit.hook_attrs)
     end
 
     attributes.merge!(attrs)
@@ -510,22 +577,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def target_sha
-    return @base_target_sha if defined?(@base_target_sha)
-
-    target_project.repository.commit(target_branch).try(:sha)
-  end
-
-  def source_sha
-    return @head_source_sha if defined?(@head_source_sha)
-
-    last_commit.try(:sha) || source_tip.try(:sha)
-  end
-
-  def source_tip
-    source_branch && source_project.repository.commit(source_branch)
-  end
-
   def fetch_ref
     target_project.repository.fetch_ref(
       source_project.repository.path_to_repo,
@@ -558,10 +609,10 @@ class MergeRequest < ActiveRecord::Base
   def diverged_commits_count
     cache = Rails.cache.read(:"merge_request_#{id}_diverged_commits")
 
-    if cache.blank? || cache[:source_sha] != source_sha || cache[:target_sha] != target_sha
+    if cache.blank? || cache[:source_sha] != source_branch_sha || cache[:target_sha] != target_branch_sha
       cache = {
-        source_sha: source_sha,
-        target_sha: target_sha,
+        source_sha: source_branch_sha,
+        target_sha: target_branch_sha,
         diverged_commits_count: compute_diverged_commits_count
       }
       Rails.cache.write(:"merge_request_#{id}_diverged_commits", cache)
@@ -571,9 +622,9 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def compute_diverged_commits_count
-    return 0 unless source_sha && target_sha
+    return 0 unless source_branch_sha && target_branch_sha
 
-    Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_sha, target_sha).size
+    Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_branch_sha, target_branch_sha).size
   end
   private :compute_diverged_commits_count
 
@@ -582,13 +633,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def pipeline
-    @pipeline ||= source_project.pipeline(last_commit.id, source_branch) if last_commit && source_project
-  end
-
-  def diff_refs
-    return nil unless diff_base_commit
-
-    [diff_base_commit, last_commit]
+    @pipeline ||= source_project.pipeline(diff_head_sha, source_branch) if diff_head_sha && source_project
   end
 
   def merge_commit
@@ -603,6 +648,36 @@ class MergeRequest < ActiveRecord::Base
     merge_commit
   end
 
+  def support_new_diff_notes?
+    diff_refs && diff_refs.complete?
+  end
+
+  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)
+    return unless support_new_diff_notes?
+    return if new_diff_refs == old_diff_refs
+
+    active_diff_notes = self.notes.diff_notes.select do |note|
+      note.new_diff_note? && note.active?(old_diff_refs)
+    end
+
+    return if active_diff_notes.empty?
+
+    paths = active_diff_notes.flat_map { |n| n.diff_file.paths }.uniq
+
+    service = Notes::DiffPositionUpdateService.new(
+      self.project,
+      nil,
+      old_diff_refs: old_diff_refs,
+      new_diff_refs: new_diff_refs,
+      paths: paths
+    )
+
+    active_diff_notes.each do |note|
+      service.execute(note)
+      Gitlab::Timeless.timeless(note, &:save)
+    end
+  end
+
   def keep_around_commit
     project.repository.keep_around(self.merge_commit_sha)
   end

app/models/merge_request_diff.rb

@@ -7,7 +7,7 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
 
-  delegate :head_source_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
+  delegate :source_branch_sha, :target_branch_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
 
   state_machine :state, initial: :empty do
     state :collected
@@ -24,7 +24,7 @@ class MergeRequestDiff < ActiveRecord::Base
   serialize :st_diffs
 
   after_create :reload_content, unless: :importing?
-  after_save :keep_around_commit
+  after_save :keep_around_commits, unless: :importing?
 
   def reload_content
     reload_commits
@@ -39,9 +39,9 @@ class MergeRequestDiff < ActiveRecord::Base
     if options[:ignore_whitespace_change]
       @diffs_no_whitespace ||= begin
         compare = Gitlab::Git::Compare.new(
-          self.repository.raw_repository,
-          self.base,
-          self.head,
+          repository.raw_repository,
+          self.start_commit_sha || self.target_branch_sha,
+          self.head_commit_sha || self.source_branch_sha,
         )
         compare.diffs(options)
       end
@@ -63,37 +63,39 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def base_commit
-    return nil unless self.base_commit_sha
+    return unless self.base_commit_sha
 
-    merge_request.target_project.commit(self.base_commit_sha)
+    project.commit(self.base_commit_sha)
   end
 
-  def last_commit_short_sha
-    @last_commit_short_sha ||= last_commit.short_id
-  end
+  def start_commit
+    return unless self.start_commit_sha
 
-  def dump_commits(commits)
-    commits.map(&:to_hash)
+    project.commit(self.start_commit_sha)
   end
 
-  def load_commits(array)
-    array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash), merge_request.source_project) }
-  end
+  def head_commit
+    return last_commit unless self.head_commit_sha
 
-  def dump_diffs(diffs)
-    if diffs.respond_to?(:map)
-      diffs.map(&:to_hash)
-    end
+    project.commit(self.head_commit_sha)
   end
 
-  def load_diffs(raw, options)
-    if raw.respond_to?(:each)
-      Gitlab::Git::DiffCollection.new(raw, options)
-    else
-      Gitlab::Git::DiffCollection.new([])
-    end
+  def compare
+    @compare ||=
+      begin
+        # Update ref for merge request
+        merge_request.fetch_ref
+
+        Gitlab::Git::Compare.new(
+          repository.raw_repository,
+          self.target_branch_sha,
+          self.source_branch_sha
+        )
+      end
   end
 
+  private
+
   # Collect array of Git::Commit objects
   # between target and source branches
   def unmerged_commits
@@ -106,6 +108,14 @@ class MergeRequestDiff < ActiveRecord::Base
     commits
   end
 
+  def dump_commits(commits)
+    commits.map(&:to_hash)
+  end
+
+  def load_commits(array)
+    array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash), merge_request.source_project) }
+  end
+
   # Reload all commits related to current merge request from repo
   # and save it as array of hashes in st_commits db field
   def reload_commits
@@ -120,6 +130,26 @@ class MergeRequestDiff < ActiveRecord::Base
     update_columns_serialized(new_attributes)
   end
 
+  # Collect array of Git::Diff objects
+  # between target and source branches
+  def unmerged_diffs
+    compare.diffs(Commit.max_diff_options)
+  end
+
+  def dump_diffs(diffs)
+    if diffs.respond_to?(:map)
+      diffs.map(&:to_hash)
+    end
+  end
+
+  def load_diffs(raw, options)
+    if raw.respond_to?(:each)
+      Gitlab::Git::DiffCollection.new(raw, options)
+    else
+      Gitlab::Git::DiffCollection.new([])
+    end
+  end
+
   # Reload diffs between branches related to current merge request from repo
   # and save it as array of hashes in st_diffs db field
   def reload_diffs
@@ -147,59 +177,33 @@ class MergeRequestDiff < ActiveRecord::Base
 
     new_attributes[:st_diffs] = new_diffs
 
-    base_commit_sha = self.repository.merge_base(self.head, self.base)
-    new_attributes[:base_commit_sha] = base_commit_sha
-
-    self.repository.keep_around(base_commit_sha)
+    new_attributes[:start_commit_sha] = self.target_branch_sha
+    new_attributes[:head_commit_sha] = self.source_branch_sha
+    new_attributes[:base_commit_sha] = branch_base_sha
 
     update_columns_serialized(new_attributes)
-  end
 
-  # Collect array of Git::Diff objects
-  # between target and source branches
-  def unmerged_diffs
-    compare.diffs(Commit.max_diff_options)
+    keep_around_commits
   end
 
-  def repository
-    merge_request.target_project.repository
+  def project
+    merge_request.target_project
   end
 
-  def source_sha
-    return head_source_sha if head_source_sha.present?
-
-    source_commit = merge_request.source_project.commit(source_branch)
-    source_commit.try(:sha)
+  def repository
+    project.repository
   end
 
-  def target_sha
-    merge_request.target_sha
-  end
+  def branch_base_commit
+    return unless self.source_branch_sha && self.target_branch_sha
 
-  def base
-    self.target_sha || self.target_branch
+    project.merge_base_commit(self.source_branch_sha, self.target_branch_sha)
   end
 
-  def head
-    self.source_sha
+  def branch_base_sha
+    branch_base_commit.try(:sha)
   end
 
-  def compare
-    @compare ||=
-      begin
-        # Update ref for merge request
-        merge_request.fetch_ref
-
-        Gitlab::Git::Compare.new(
-          self.repository.raw_repository,
-          self.base,
-          self.head
-        )
-      end
-  end
-
-  private
-
   #
   # #save or #update_attributes providing changes on serialized attributes do a lot of
   # serialization and deserialization calls resulting in bad performance.
@@ -223,7 +227,9 @@ class MergeRequestDiff < ActiveRecord::Base
     reload
   end
 
-  def keep_around_commit
-    self.repository.keep_around(self.base_commit_sha)
+  def keep_around_commits
+    repository.keep_around(target_branch_sha)
+    repository.keep_around(source_branch_sha)
+    repository.keep_around(branch_base_sha)
   end
 end

app/models/note.rb

@@ -56,7 +56,7 @@ class Note < ActiveRecord::Base
   scope :inc_author, ->{ includes(:author) }
   scope :inc_author_project_award_emoji, ->{ includes(:project, :author, :award_emoji) }
 
-  scope :legacy_diff_notes, ->{ where(type: 'LegacyDiffNote') }
+  scope :diff_notes, ->{ where(type: ['LegacyDiffNote', 'DiffNote']) }
   scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
 
   scope :with_associations, -> do
@@ -82,7 +82,7 @@ class Note < ActiveRecord::Base
     end
 
     def grouped_diff_notes
-      legacy_diff_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code)
+      diff_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code)
     end
 
     # Searches for notes matching the given query.
@@ -115,6 +115,10 @@ class Note < ActiveRecord::Base
     false
   end
 
+  def new_diff_note?
+    false
+  end
+
   def active?
     true
   end
@@ -193,7 +197,7 @@ class Note < ActiveRecord::Base
   end
 
   def award_emoji?
-    award_emoji_supported? && contains_emoji_only?
+    can_be_award_emoji? && contains_emoji_only?
   end
 
   def emoji_awardable?
@@ -204,7 +208,7 @@ class Note < ActiveRecord::Base
     self.line_code = nil if self.line_code.blank?
   end
 
-  def award_emoji_supported?
+  def can_be_award_emoji?
     noteable.is_a?(Awardable)
   end
 

app/models/project_services/jira_service.rb

@@ -144,7 +144,7 @@ class JiraService < IssueTrackerService
     commit_id = if entity.is_a?(Commit)
                   entity.id
                 elsif entity.is_a?(MergeRequest)
-                  entity.last_commit.id
+                  entity.diff_head_sha
                 end
     commit_url = build_entity_url(:commit, commit_id)
 

app/models/repository.rb

@@ -653,16 +653,6 @@ class Repository
     end
   end
 
-  def blob_for_diff(commit, diff)
-    blob_at(commit.id, diff.file_path)
-  end
-
-  def prev_blob_for_diff(commit, diff)
-    if commit.parent_id
-      blob_at(commit.parent_id, diff.old_path)
-    end
-  end
-
   def refs_contains_sha(ref_type, sha)
     args = %W(#{Gitlab.config.git.bin_path} #{ref_type} --contains #{sha})
     names = Gitlab::Popen.popen(args, path_to_repo).first

app/models/sent_notification.rb

 class SentNotification < ActiveRecord::Base
+  serialize :position, Gitlab::Diff::Position
+
   belongs_to :project
   belongs_to :noteable, polymorphic: true
   belongs_to :recipient, class_name: "User"
@@ -7,7 +9,7 @@ class SentNotification < ActiveRecord::Base
   validates :reply_key, uniqueness: true
   validates :noteable_id, presence: true, unless: :for_commit?
   validates :commit_id, presence: true, if: :for_commit?
-  validates :line_code, line_code: true, allow_blank: true
+  validate :note_valid
 
   after_save :keep_around_commit
 
@@ -20,7 +22,7 @@ class SentNotification < ActiveRecord::Base
       find_by(reply_key: reply_key)
     end
 
-    def record(noteable, recipient_id, reply_key, params = {})
+    def record(noteable, recipient_id, reply_key, attrs = {})
       return unless reply_key
 
       noteable_id = nil
@@ -31,7 +33,7 @@ class SentNotification < ActiveRecord::Base
         noteable_id = noteable.id
       end
 
-      params.reverse_merge!(
+      attrs.reverse_merge!(
         project:        noteable.project,
         noteable_type:  noteable.class.name,
         noteable_id:    noteable_id,
@@ -40,13 +42,17 @@ class SentNotification < ActiveRecord::Base
         reply_key:      reply_key
       )
 
-      create(params)
+      create(attrs)
     end
 
-    def record_note(note, recipient_id, reply_key, params = {})
-      params[:line_code] = note.line_code
+    def record_note(note, recipient_id, reply_key, attrs = {})
+      if note.diff_note?
+        attrs[:note_type] = note.type
+
+        attrs.merge!(note.diff_attributes)
+      end
 
-      record(note.noteable, recipient_id, reply_key, params)
+      record(note.noteable, recipient_id, reply_key, attrs)
     end
   end
 
@@ -70,8 +76,33 @@ class SentNotification < ActiveRecord::Base
     self.reply_key
   end
 
+  def note_attributes
+    {
+      project:        self.project,
+      author:         self.recipient,
+      type:           self.note_type,
+      noteable_type:  self.noteable_type,
+      noteable_id:    self.noteable_id,
+      commit_id:      self.commit_id,
+      line_code:      self.line_code,
+      position:       self.position.to_json
+    }
+  end
+
+  def create_note(note)
+    Notes::CreateService.new(
+      self.project,
+      self.recipient,
+      self.note_attributes.merge(note: note)
+    ).execute
+  end
+
   private
 
+  def note_valid
+    Note.new(note_attributes.merge(note: "Test")).valid?
+  end
+
   def keep_around_commit
     project.repository.keep_around(self.commit_id)
   end

app/services/merge_requests/merge_service.rb

@@ -34,7 +34,7 @@ module MergeRequests
         committer: committer
       }
 
-      commit_id = repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options)
+      commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, options)
       merge_request.update(merge_commit_sha: commit_id)
     rescue GitHooksService::PreReceiveError => e
       merge_request.update(merge_error: e.message)

app/services/merge_requests/merge_when_build_succeeds_service.rb

@@ -12,7 +12,7 @@ module MergeRequests
         merge_request.merge_when_build_succeeds = true
         merge_request.merge_user                = @current_user
 
-        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.last_commit)
+        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
       end
 
       merge_request.save

app/services/merge_requests/refresh_service.rb

@@ -34,10 +34,10 @@ module MergeRequests
     def close_merge_requests
       commit_ids = @commits.map(&:id)
       merge_requests = @project.merge_requests.opened.where(target_branch: @branch_name).to_a
-      merge_requests = merge_requests.select(&:last_commit)
+      merge_requests = merge_requests.select(&:diff_head_commit)
 
       merge_requests = merge_requests.select do |merge_request|
-        commit_ids.include?(merge_request.last_commit.id)
+        commit_ids.include?(merge_request.diff_head_sha)
       end
 
       merge_requests.uniq.select(&:source_project).each do |merge_request|
@@ -60,7 +60,7 @@ module MergeRequests
 
       merge_requests.each do |merge_request|
         if merge_request.source_branch == @branch_name || force_push?
-          merge_request.reload_code
+          merge_request.reload_diff
           merge_request.mark_as_unchecked
         else
           mr_commit_ids = merge_request.commits.map(&:id)
@@ -68,7 +68,7 @@ module MergeRequests
           matches = mr_commit_ids & push_commit_ids
 
           if matches.any?
-            merge_request.reload_code
+            merge_request.reload_diff
             merge_request.mark_as_unchecked
           else
             merge_request.mark_as_unchecked
@@ -94,12 +94,10 @@ module MergeRequests
         merge_request = merge_requests_for_source_branch.first
         return unless merge_request
 
-        last_commit = merge_request.last_commit
-
         begin
           # Since any number of commits could have been made to the restored branch,
           # find the common root to see what has been added.
-          common_ref = @project.repository.merge_base(last_commit.id, @newrev)
+          common_ref = @project.repository.merge_base(merge_request.diff_head_sha, @newrev)
           # If the a commit no longer exists in this repo, gitlab_git throws
           # a Rugged::OdbError. This is fixed in https://gitlab.com/gitlab-org/gitlab_git/merge_requests/52
           @commits = @project.repository.commits_between(common_ref, @newrev) if common_ref

app/services/merge_requests/reopen_service.rb

@@ -6,7 +6,7 @@ module MergeRequests
         create_note(merge_request)
         notification_service.reopen_mr(merge_request, current_user)
         execute_hooks(merge_request, 'reopen')
-        merge_request.reload_code
+        merge_request.reload_diff
         merge_request.mark_as_unchecked
       end
 

app/services/notes/diff_position_update_service.rb

+module Notes
+  class DiffPositionUpdateService < BaseService
+    def execute(note)
+      new_position = tracer.trace(note.position)
+
+      # Don't update the position if the type doesn't match, since that means
+      # the diff line commented on was changed, and the comment is now outdated
+      old_position = note.position
+      if new_position &&
+          new_position != old_position &&
+          new_position.type == old_position.type
+
+        note.position = new_position
+      end
+
+      note
+    end
+
+    private
+
+    def tracer
+      @tracer ||= Gitlab::Diff::PositionTracer.new(
+        repository: project.repository,
+        old_diff_refs: params[:old_diff_refs],
+        new_diff_refs: params[:new_diff_refs],
+        paths: params[:paths]
+      )
+    end
+  end
+end

app/views/notify/note_merge_request_email.html.haml

-- if @note.legacy_diff_note?
+- if @note.diff_note?
   %p.details
     New comment on diff for
-    = link_to @note.diff_file_path, @target_url
+    = link_to @note.diff_file.file_path, @target_url
     \:
 
 = render 'note_message'

app/views/notify/repository_push_email.html.haml

@@ -72,12 +72,11 @@
             The diff for this file was not included because it is too large.
           - else
             %hr
-            - diff_commit = diff_file.deleted_file ? @message.diff_refs.first : @message.diff_refs.last
-            - blob = @message.project.repository.blob_for_diff(diff_commit, diff_file)
+            - blob = diff_file.blob
             - if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob)
               %table.code.white
                 - diff_file.highlighted_diff_lines.each do |line|
-                  = render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: nil, plain: true}
+                  = render "projects/diffs/line", line: line, diff_file: diff_file, plain: true
             - else
               No preview for this file type
           %br

app/views/projects/commit/show.html.haml

@@ -7,8 +7,7 @@
   = render "ci_menu"
 - else
   %div.block-connector
-= render "projects/diffs/diffs", diffs: @diffs, project: @project,
-         diff_refs: @diff_refs
+= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @commit.diff_refs
 = render "projects/notes/notes_with_form"
 - if can_collaborate_with_project?
   - %w(revert cherry-pick).each do |type|

app/views/projects/diffs/_diffs.html.haml

@@ -2,7 +2,7 @@
 - if diff_view == 'parallel'
   - fluid_layout true
 
-- diff_files = safe_diff_files(diffs, diff_refs)
+- diff_files = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository)
 
 .content-block.oneline-block.files-changed
   .inline-parallel-buttons
@@ -24,7 +24,7 @@
 .files
   - diff_files.each_with_index do |diff_file, index|
     - diff_commit = commit_for_diff(diff_file)
-    - blob = project.repository.blob_for_diff(diff_commit, diff_file)
+    - blob = diff_file.blob(diff_commit)
     - next unless blob
     - blob.load_all_data!(project.repository) unless blob.only_display_raw?
 

app/views/projects/diffs/_file.html.haml

-.diff-file.file-holder{id: "diff-#{i}", data: diff_file_html_data(project, diff_commit, diff_file)}
+.diff-file.file-holder{id: "diff-#{i}", data: diff_file_html_data(project, diff_file)}
   .file-title{id: "file-path-#{hexdigest(diff_file.file_path)}"}
-    - if diff_file.diff.submodule?
-      %span
-        = icon('archive fw')
-        %span
-          = submodule_link(blob, @commit.id, project.repository)
-    - else
-      = blob_icon blob.mode, blob.name
-
-      = link_to "#diff-#{i}" do
-        - if diff_file.renamed_file
-          - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
-          = old_path
-          →
-          = new_path
-        - else
-          %span
-            = diff_file.new_path
-          - if diff_file.deleted_file
-            deleted
-
-      - if diff_file.mode_changed?
-        %small
-          = "#{diff_file.diff.a_mode} → #{diff_file.diff.b_mode}"
+    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, project: project, url: "#diff-#{i}"
 
+    - unless diff_file.submodule?
       .file-actions.hidden-xs
         - if blob_text_viewable?(blob)
           = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip btn-file-option', title: "Toggle comments for this file" do
@@ -42,17 +21,23 @@
     - return unless blob.respond_to?(:text?)
     - if diff_file.too_large?
       .nothing-here-block This diff could not be displayed because it is too large.
-    - elsif blob_text_viewable?(blob) && !project.repository.diffable?(blob)
-      .nothing-here-block This diff was suppressed by a .gitattributes entry.
-    - elsif blob_text_viewable?(blob)
-      - if diff_view == 'parallel'
-        = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i
-      - else
-        = render "projects/diffs/text_file", diff_file: diff_file, index: i
     - elsif blob.only_display_raw?
       .nothing-here-block This file is too large to display.
+    - elsif blob_text_viewable?(blob)
+      - if !project.repository.diffable?(blob)
+        .nothing-here-block This diff was suppressed by a .gitattributes entry.
+      - elsif diff_file.diff_lines.length > 0
+        - if diff_view == 'parallel'
+          = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i
+        - else
+          = render "projects/diffs/text_file", diff_file: diff_file, index: i
+      - else
+        - if diff_file.mode_changed?
+          .nothing-here-block File mode changed
+        - elsif diff_file.renamed_file
+          .nothing-here-block File moved
     - elsif blob.image?
-      - old_file = project.repository.prev_blob_for_diff(diff_commit, diff_file)
-      = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i, diff_refs: diff_refs
+      - old_blob = diff_file.old_blob(diff_commit)
+      = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob, index: i
     - else
       .nothing-here-block No preview for this file type

app/views/projects/diffs/_file_header.html.haml

+- if defined?(blob) && blob && diff_file.submodule?
+  %span
+    = icon('archive fw')
+    %span
+      = submodule_link(blob, diff_commit.id, project.repository)
+- else
+  = conditional_link_to url.present?, url do
+    = blob_icon diff_file.b_mode, diff_file.file_path
+
+    - if diff_file.renamed_file
+      - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
+      %strong
+        = old_path
+      →
+      %strong
+        = new_path
+    - else
+      %strong
+        = diff_file.new_path
+      - if diff_file.deleted_file
+        deleted
+
+  - if diff_file.mode_changed?
+    %small
+      = "#{diff_file.a_mode} → #{diff_file.b_mode}"

app/views/projects/diffs/_image.html.haml

 - diff = diff_file.diff
-- file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(@commit.id, diff.new_path))
+- file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.new_ref, diff.new_path))
 // diff_refs will be nil for orphaned commits (e.g. first commit in repo)
-- if diff_refs
-  - old_commit_id = diff_refs.first.id
-  - old_file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(old_commit_id, diff.old_path))
+- if diff_file.old_ref
+  - old_file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.old_ref, diff.old_path))
 
 - if diff.renamed_file || diff.new_file || diff.deleted_file
   .image
@@ -16,7 +15,7 @@
     %div.two-up.view
       %span.wrap
         .frame.deleted
-          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(old_commit_id, diff.old_path))}
+          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.old_ref, diff.old_path))}
             %img{src: old_file_raw_path}
         %p.image-info.hide
           %span.meta-filesize= "#{number_to_human_size old_file.size}"
@@ -28,7 +27,7 @@
           %span.meta-height
       %span.wrap
         .frame.added
-          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(@commit.id, diff.new_path))}
+          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.new_ref, diff.new_path))}
             %img{src: file_raw_path}
         %p.image-info.hide
           %span.meta-filesize= "#{number_to_human_size file.size}"

app/views/projects/diffs/_line.html.haml

+- plain = local_assigns.fetch(:plain, false)
+- line_code = diff_file.line_code(line)
+- position = diff_file.position(line)
 - type = line.type
 %tr.line_holder{ id: line_code, class: type }
   - case type
@@ -9,18 +12,19 @@
     %td.new_line.diff-line-num
     %td.line_content.match= line.text
   - else
-    %td.old_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
+    %td.old_line.diff-line-num{ class: type, data: { linenumber: line.old_pos } }
       - link_text = type == "new" ? " ".html_safe : line.old_pos
-      - if defined?(plain) && plain
+      - if plain
         = link_text
       - else
         = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
-      - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
-        = link_to_new_diff_note(line_code)
+
+      - if !plain && !@diff_notes_disabled && can?(current_user, :create_note, @project)
+        = link_to_new_diff_note(line_code, position)
     %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
       - link_text = type == "old" ? " ".html_safe : line.new_pos
-      - if defined?(plain) && plain
+      - if plain
         = link_text
       - else
         = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
-    %td.line_content{ class: ['noteable_line', type, line_code], data: { line_code: line_code } }= diff_line_content(line.text, type)
+    %td.line_content.noteable_line{ class: [type, line_code], data: { line_code: line_code, position: position.to_json } }= diff_line_content(line.text, type)

app/views/projects/diffs/_parallel_view.html.haml

@@ -14,30 +14,28 @@
           %td.new_line.diff-line-num
           %td.line_content.parallel.match= left[:text]
         - else
-          %td.old_line.diff-line-num{id: left[:line_code], class: "#{left[:type]} #{'empty-cell' if !left[:number]}"}
+          %td.old_line.diff-line-num{id: left[:line_code], class: [left[:type], ('empty-cell' unless left[:number])]}
             = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code]
             - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
-              = link_to_new_diff_note(left[:line_code], 'old')
-          %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]} #{'empty-cell' if left[:text].empty?}", data: { line_code: left[:line_code] }}= diff_line_content(left[:text])
+              = link_to_new_diff_note(left[:line_code], left[:position], 'old')
+          %td.line_content.parallel.noteable_line{class: [left[:type], left[:line_code], ('empty-cell' if left[:text].empty?)], data: { line_code: left[:line_code], position: left[:position].to_json }}= diff_line_content(left[:text])
 
           - if right[:type] == 'new'
             - new_line_class = 'new'
             - new_line_code = right[:line_code]
+            - new_position = right[:position]
           - else
             - new_line_class = nil
             - new_line_code = left[:line_code]
+            - new_position = left[:position]
 
-          %td.new_line.diff-line-num{id: new_line_code, class: "#{new_line_class} #{'empty-cell' if !right[:number]}", data: { linenumber: right[:number] }}
+          %td.new_line.diff-line-num{id: new_line_code, class: [new_line_class, ('empty-cell' unless right[:number])], data: { linenumber: right[:number] }}
             = link_to raw(right[:number]), "##{new_line_code}", id: new_line_code
             - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
-              = link_to_new_diff_note(new_line_code, 'new')
-          %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code} #{'empty-cell' if right[:text].empty?}", data: { line_code: new_line_code }}= diff_line_content(right[:text])
+              = link_to_new_diff_note(new_line_code, new_position, 'new')
+          %td.line_content.parallel.noteable_line{class: [new_line_class, new_line_code, ('empty-cell' if right[:text].empty?)], data: { line_code: new_line_code, position: new_position.to_json }}= diff_line_content(right[:text])
 
       - unless @diff_notes_disabled
         - notes_left, notes_right = organize_comments(left, right)
         - if notes_left.present? || notes_right.present?
           = render "projects/notes/diff_notes_with_reply_parallel", notes_left: notes_left, notes_right: notes_right
-
-- if diff_file.diff.diff.blank? && diff_file.mode_changed?
-  .file-mode-changed
-    File mode changed

app/views/projects/diffs/_text_file.html.haml

@@ -4,22 +4,17 @@
     %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show.
 
 %table.text-file.code.js-syntax-highlight{ class: too_big ? 'hide' : '' }
-
   - last_line = 0
-  - diff_file.highlighted_diff_lines.each_with_index do |line, index|
-    - line_code = generate_line_code(diff_file.file_path, line)
+  - diff_file.highlighted_diff_lines.each do |line|
     - last_line = line.new_pos
-    = render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: line_code}
+    = render "projects/diffs/line", line: line, diff_file: diff_file
 
     - unless @diff_notes_disabled
-      - diff_notes = @grouped_diff_notes[line_code]
+      - line_code = diff_file.line_code(line)
+      - diff_notes = @grouped_diff_notes[line_code] if line_code
       - if diff_notes
         = render "projects/notes/diff_notes_with_reply", notes: diff_notes
 
   - if last_line > 0
     = render "projects/diffs/match_line", { line: "",
       line_old: last_line, line_new: last_line, bottom: true, new_file: diff_file.new_file }
-
-- if diff_file.diff.blank? && diff_file.mode_changed?
-  .file-mode-changed
-    File mode changed

app/views/projects/merge_requests/widget/_heading.html.haml

@@ -7,7 +7,7 @@
           CI build
           = ci_label_for_status(status)
         for
-        - commit = @merge_request.last_commit
+        - commit = @merge_request.diff_head_commit
         = succeed "." do
           = link_to @pipeline.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @pipeline.sha), class: "monospace"
         %span.ci-coverage
@@ -24,7 +24,7 @@
           CI build
           = ci_label_for_status(status)
         for
-        - commit = @merge_request.last_commit
+        - commit = @merge_request.diff_head_commit
         = succeed "." do
           = link_to commit.short_id, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, commit), class: "monospace"
         %span.ci-coverage
@@ -33,12 +33,12 @@
 
     .ci_widget
       = icon("spinner spin")
-      Checking CI status for #{@merge_request.last_commit_short_sha}…
+      Checking CI status for #{@merge_request.diff_head_commit.short_id}…
 
     .ci_widget.ci-not_found{style: "display:none"}
       = icon("times-circle")
-      Could not find CI status for #{@merge_request.last_commit_short_sha}.
+      Could not find CI status for #{@merge_request.diff_head_commit.short_id}.
 
     .ci_widget.ci-error{style: "display:none"}
       = icon("times-circle")
-      Could not connect to the CI server. Please check your settings and try again.
+      Could not connect to the CI server. Please check your settings and try again.
\ No newline at end of file

app/views/projects/merge_requests/widget/open/_accept.html.haml

@@ -2,7 +2,7 @@
 
 = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f|
   = hidden_field_tag :authenticity_token, form_authenticity_token
-  = hidden_field_tag :sha, @merge_request.source_sha
+  = hidden_field_tag :sha, @merge_request.diff_head_sha
   .accept-merge-holder.clearfix.js-toggle-container
     .clearfix
       .accept-action

app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml

@@ -16,7 +16,7 @@
   - if remove_source_branch_button || user_can_cancel_automatic_merge
     .clearfix.prepend-top-10
       - if remove_source_branch_button
-        = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.source_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
+        = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.diff_head_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
           = icon('times')
           Remove Source Branch When Merged
 

app/views/projects/notes/_diff_notes_with_reply.html.haml

@@ -4,5 +4,4 @@
   %td.notes_content
     %ul.notes{ data: { discussion_id: note.discussion_id } }
       = render partial: "projects/notes/note", collection: notes, as: :note
-    .discussion-reply-holder
-      = link_to_reply_discussion(note)
+    = link_to_reply_discussion(note)

app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml

@@ -8,8 +8,7 @@
       %ul.notes{ data: { discussion_id: note_left.discussion_id } }
         = render partial: "projects/notes/note", collection: notes_left, as: :note
 
-      .discussion-reply-holder
-        = link_to_reply_discussion(note_left, 'old')
+      = link_to_reply_discussion(note_left, 'old')
   - else
     %td.notes_line.old= ""
     %td.notes_content.parallel.old= ""
@@ -20,8 +19,7 @@
       %ul.notes{ data: { discussion_id: note_right.discussion_id } }
         = render partial: "projects/notes/note", collection: notes_right, as: :note
 
-      .discussion-reply-holder
-        = link_to_reply_discussion(note_right, 'new')
+      = link_to_reply_discussion(note_right, 'new')
   - else
     %td.notes_line.new= ""
     %td.notes_content.parallel.new= ""

app/views/projects/notes/_form.html.haml

@@ -7,6 +7,7 @@
   = f.hidden_field :noteable_id
   = f.hidden_field :noteable_type
   = f.hidden_field :type
+  = f.hidden_field :position
 
   = render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do
     = render 'projects/zen', f: f, attr: :note, classes: 'note-textarea js-note-text', placeholder: "Write a comment or drag your files here..."

app/views/projects/notes/discussions/_diff_with_notes.html.haml

 - note = discussion_notes.first
-- diff = note.diff
-- return unless diff
+- diff_file = note.diff_file
+- return unless diff_file
+
+- blob = note.blob
+
+.diff-file.file-holder
+  .file-title
+    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: note.project, url: diff_note_path(note)
 
-.diff-file
-  .diff-header
-    %span
-      - if diff.deleted_file
-        = diff.old_path
-      - else
-        = diff.new_path
-        - if diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode
-          %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}"
   .diff-content.code.js-syntax-highlight
     %table
       - note.truncated_diff_lines.each do |line|
-        - type = line.type
-        - line_code = generate_line_code(note.diff_file_path, line)
-        %tr.line_holder{ id: line_code, class: "#{type}" }
-          - if type == "match"
-            %td.old_line.diff-line-num= "..."
-            %td.new_line.diff-line-num= "..."
-            %td.line_content.match= line.text
-          - else
-            %td.old_line.diff-line-num{ data: { linenumber: type == "new" ? " ".html_safe : line.old_pos } }
-            %td.new_line.diff-line-num{ data: { linenumber: type == "old" ? " ".html_safe : line.new_pos } }
-            %td.line_content{ class: ['noteable_line', type, line_code], line_code: line_code }= diff_line_content(line.text, type)
+        = render "projects/diffs/line", line: line, diff_file: diff_file, plain: true
 
-            - if line_code == note.line_code
-              = render "projects/notes/diff_notes_with_reply", notes: discussion_notes
+        - if note.for_line?(line)
+          = render "projects/notes/diff_notes_with_reply", notes: discussion_notes

app/views/projects/notes/discussions/_notes.html.haml

@@ -3,5 +3,4 @@
   .notes{ data: { discussion_id: note.discussion_id } }
     %ul.notes.timeline
       = render partial: "projects/notes/note", collection: discussion_notes, as: :note
-  .discussion-reply-holder
-    = link_to_reply_discussion(note)
+  = link_to_reply_discussion(note)

app/workers/emails_on_push_worker.rb

@@ -28,18 +28,30 @@ class EmailsOnPushWorker
         :push
       end
 
+    merge_base_sha = project.merge_base_commit(before_sha, after_sha).try(:sha)
+
     diff_refs = nil
     compare = nil
     reverse_compare = false
     if action == :push
       compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
-      diff_refs = [project.merge_base_commit(before_sha, after_sha), project.commit(after_sha)]
+
+      diff_refs = Gitlab::Diff::DiffRefs.new(
+        base_sha: merge_base_sha,
+        start_sha: before_sha,
+        head_sha: after_sha
+      )
 
       return false if compare.same
 
       if compare.commits.empty?
         compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
-        diff_refs = [project.merge_base_commit(after_sha, before_sha), project.commit(before_sha)]
+
+        diff_refs = Gitlab::Diff::DiffRefs.new(
+          base_sha: merge_base_sha,
+          start_sha: after_sha,
+          head_sha: before_sha
+        )
 
         reverse_compare = true
 

db/migrate/20160508202603_add_head_commit_id_to_merge_request_diffs.rb

+class AddHeadCommitIdToMergeRequestDiffs < ActiveRecord::Migration
+  def change
+    add_column :merge_request_diffs, :head_commit_sha, :string
+  end
+end

db/migrate/20160508215920_add_positions_to_diff_notes.rb

+class AddPositionsToDiffNotes < ActiveRecord::Migration
+  def change
+    add_column :notes, :position, :text
+    add_column :notes, :original_position, :text
+  end
+end

db/migrate/20160516224534_add_start_commit_id_to_merge_request_diffs.rb

+class AddStartCommitIdToMergeRequestDiffs < ActiveRecord::Migration
+  def change
+    add_column :merge_request_diffs, :start_commit_sha, :string
+  end
+end

db/migrate/20160522215720_add_note_type_and_position_to_sent_notification.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddNoteTypeAndPositionToSentNotification < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :sent_notifications, :note_type, :string
+    add_column :sent_notifications, :position, :text
+  end
+end

db/schema.rb

@@ -593,6 +593,8 @@ ActiveRecord::Schema.define(version: 20160705163108) do
     t.datetime "updated_at"
     t.string   "base_commit_sha"
     t.string   "real_size"
+    t.string   "head_commit_sha"
+    t.string   "start_commit_sha"
   end
 
   add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree
@@ -689,10 +691,12 @@ ActiveRecord::Schema.define(version: 20160705163108) do
     t.string   "line_code"
     t.string   "commit_id"
     t.integer  "noteable_id"
-    t.boolean  "system",        default: false, null: false
+    t.boolean  "system",            default: false, null: false
     t.text     "st_diff"
     t.integer  "updated_by_id"
     t.string   "type"
+    t.text     "position"
+    t.text     "original_position"
   end
 
   add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree
@@ -881,6 +885,8 @@ ActiveRecord::Schema.define(version: 20160705163108) do
     t.string  "commit_id"
     t.string  "reply_key",     null: false
     t.string  "line_code"
+    t.string  "note_type"
+    t.text    "position"
   end
 
   add_index "sent_notifications", ["reply_key"], name: "index_sent_notifications_on_reply_key", unique: true, using: :btree

features/steps/project/merge_requests.rb

@@ -272,10 +272,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
 
   step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
     mr = MergeRequest.find_by(title: "Bug NS-05")
-    create(:note_on_merge_request_diff, project: project,
+    create(:diff_note_on_merge_request, project: project,
                                         noteable: mr,
                                         author: user_exists("John Doe"),
-                                        line_code: sample_commit.line_code,
                                         note: 'Line is wrong')
   end
 
@@ -519,7 +518,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   step '"Bug NS-05" has CI status' do
     project = merge_request.source_project
     project.enable_ci
-    pipeline = create :ci_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch
+    pipeline = create :ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch
     create :ci_build, pipeline: pipeline
   end
 

features/steps/shared/diff_note.rb

@@ -23,7 +23,7 @@ module SharedDiffNote
     page.within(diff_file_selector) do
       click_diff_line(sample_commit.line_code)
 
-      page.within("form[id$='#{sample_commit.line_code}-true']") do
+      page.within("form[data-line-code='#{sample_commit.line_code}']") do
         fill_in "note[note]", with: "Typo, please fix"
         find(".js-comment-button").trigger("click")
         sleep 0.05
@@ -33,7 +33,7 @@ module SharedDiffNote
 
   step 'I leave a diff comment in a parallel view on the left side like "Old comment"' do
     click_parallel_diff_line(sample_commit.line_code, 'old')
-    page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}-true']") do
+    page.within("#{diff_file_selector} form[data-line-code='#{sample_commit.line_code}']") do
       fill_in "note[note]", with: "Old comment"
       find(".js-comment-button").trigger("click")
     end
@@ -41,7 +41,7 @@ module SharedDiffNote
 
   step 'I leave a diff comment in a parallel view on the right side like "New comment"' do
     click_parallel_diff_line(sample_commit.line_code, 'new')
-    page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}-true']") do
+    page.within("#{diff_file_selector} form[data-line-code='#{sample_commit.line_code}']") do
       fill_in "note[note]", with: "New comment"
       find(".js-comment-button").trigger("click")
     end
@@ -51,7 +51,7 @@ module SharedDiffNote
     page.within(diff_file_selector) do
       click_diff_line(sample_commit.line_code)
 
-      page.within("form[id$='#{sample_commit.line_code}-true']") do
+      page.within("form[data-line-code='#{sample_commit.line_code}']") do
         fill_in "note[note]", with: "Should fix it :smile:"
         find('.js-md-preview-button').click
       end
@@ -62,7 +62,7 @@ module SharedDiffNote
     page.within(diff_file_selector) do
       click_diff_line(sample_commit.del_line_code)
 
-      page.within("form[id$='#{sample_commit.del_line_code}-true']") do
+      page.within("form[data-line-code='#{sample_commit.del_line_code}']") do
         fill_in "note[note]", with: "DRY this up"
         find('.js-md-preview-button').click
       end
@@ -91,7 +91,7 @@ module SharedDiffNote
     page.within(diff_file_selector) do
       click_diff_line(sample_commit.line_code)
 
-      page.within("form[id$='#{sample_commit.line_code}-true']") do
+      page.within("form[data-line-code='#{sample_commit.line_code}']") do
         fill_in 'note[note]', with: ':smile:'
         click_button('Comment')
       end

lib/api/entities.rb

@@ -240,9 +240,9 @@ module API
 
     class CommitNote < Grape::Entity
       expose :note
-      expose(:path) { |note| note.diff_file_path if note.legacy_diff_note? }
-      expose(:line) { |note| note.diff_new_line if note.legacy_diff_note? }
-      expose(:line_type) { |note| note.diff_line_type if note.legacy_diff_note? }
+      expose(:path) { |note| note.diff_file.try(:file_path) if note.diff_note? }
+      expose(:line) { |note| note.diff_line.try(:new_line) if note.diff_note? }
+      expose(:line_type) { |note| note.diff_line.try(:type) if note.diff_note? }
       expose :author, using: Entities::UserBasic
       expose :created_at
     end

lib/api/merge_requests.rb

@@ -233,8 +233,8 @@ module API
 
           render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?
 
-          if params[:sha] && merge_request.source_sha != params[:sha]
-            render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409)
+          if params[:sha] && merge_request.diff_head_sha != params[:sha]
+            render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
           end
 
           merge_params = {

lib/gitlab/diff/diff_refs.rb

+module Gitlab
+  module Diff
+    class DiffRefs
+      attr_reader :base_sha
+      attr_reader :start_sha
+      attr_reader :head_sha
+
+      def initialize(base_sha:, start_sha: base_sha, head_sha:)
+        @base_sha = base_sha
+        @start_sha = start_sha
+        @head_sha = head_sha
+      end
+
+      def ==(other)
+        other.is_a?(self.class) &&
+          base_sha == other.base_sha &&
+          start_sha == other.start_sha &&
+          head_sha == other.head_sha
+      end
+
+      # There is only one case in which we will have `start_sha` and `head_sha`,
+      # but not `base_sha`, which is when a diff is generated between an
+      # orphaned branch and another branch, which means there _is_ no base, but
+      # we're still able to highlight it, and to create diff notes, which are
+      # the primary things `DiffRefs` are used for.
+      # `DiffRefs` are "complete" when they have `start_sha` and `head_sha`,
+      # because `base_sha` can always be derived from this, to return an actual
+      # sha, or `nil`.
+      # We have `base_sha` directly available on `DiffRefs` because it's faster#
+      # than having to look it up in the repo every time.
+      def complete?
+        start_sha && head_sha
+      end
+    end
+  end
+end

lib/gitlab/diff/file.rb

 module Gitlab
   module Diff
     class File
-      attr_reader :diff, :diff_refs
+      attr_reader :diff, :repository, :diff_refs
 
       delegate :new_file, :deleted_file, :renamed_file,
-        :old_path, :new_path, to: :diff, prefix: false
+        :old_path, :new_path, :a_mode, :b_mode,
+        :submodule?, :too_large?, to: :diff, prefix: false
 
-      def initialize(diff, diff_refs)
+      def initialize(diff, repository:, diff_refs: nil)
         @diff = diff
+        @repository = repository
         @diff_refs = diff_refs
       end
 
+      def position(line)
+        return unless diff_refs
+
+        Position.new(
+          old_path: old_path,
+          new_path: new_path,
+          old_line: line.old_line,
+          new_line: line.new_line,
+          diff_refs: diff_refs
+        )
+      end
+
+      def line_code(line)
+        return if line.meta?
+
+        Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
+      end
+
+      def line_for_line_code(code)
+        diff_lines.find { |line| line_code(line) == code }
+      end
+
+      def line_for_position(pos)
+        diff_lines.find { |line| position(line) == pos }
+      end
+
+      def position_for_line_code(code)
+        line = line_for_line_code(code)
+        position(line) if line
+      end
+
+      def line_code_for_position(pos)
+        line = line_for_position(pos)
+        line_code(line) if line
+      end
+
+      def content_commit
+        return unless diff_refs
+
+        repository.commit(deleted_file ? old_ref : new_ref)
+      end
+
       def old_ref
-        diff_refs[0] if diff_refs
+        diff_refs.try(:base_sha)
       end
 
       def new_ref
-        diff_refs[1] if diff_refs
+        diff_refs.try(:head_sha)
       end
 
-      # Array of Gitlab::DIff::Line objects
+      # Array of Gitlab::Diff::Line objects
       def diff_lines
-        @lines ||= parser.parse(raw_diff.each_line).to_a
-      end
-
-      def too_large?
-        diff.too_large?
+        @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
       end
 
       def highlighted_diff_lines
-        Gitlab::Diff::Highlight.new(self).highlight
+        @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
       end
 
       def parallel_diff_lines
-        Gitlab::Diff::ParallelDiff.new(self).parallelize
+        @parallel_diff_lines ||= Gitlab::Diff::ParallelDiff.new(self).parallelize
       end
 
       def mode_changed?
-        !!(diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode)
-      end
-
-      def parser
-        Gitlab::Diff::Parser.new
+        a_mode && b_mode && a_mode != b_mode
       end
 
       def raw_diff
@@ -53,17 +89,15 @@ module Gitlab
       end
 
       def prev_line(index)
-        if index > 0
-          diff_lines[index - 1]
-        end
+        diff_lines[index - 1] if index > 0
+      end
+
+      def paths
+        [old_path, new_path].compact
       end
 
       def file_path
-        if diff.new_path.present?
-          diff.new_path
-        elsif diff.old_path.present?
-          diff.old_path
-        end
+        new_path.presence || old_path
       end
 
       def added_lines
@@ -73,6 +107,21 @@ module Gitlab
       def removed_lines
         diff_lines.count(&:removed?)
       end
+
+      def old_blob(commit = content_commit)
+        return unless commit
+
+        parent_id = commit.parent_id
+        return unless parent_id
+
+        repository.blob_at(parent_id, old_path)
+      end
+
+      def blob(commit = content_commit)
+        return unless commit
+
+        repository.blob_at(commit.id, file_path)
+      end
     end
   end
 end

lib/gitlab/diff/highlight.rb

 module Gitlab
   module Diff
     class Highlight
-      attr_reader :diff_file, :diff_lines, :raw_lines
+      attr_reader :diff_file, :diff_lines, :raw_lines, :repository
 
       delegate :old_path, :new_path, :old_ref, :new_ref, to: :diff_file, prefix: :diff
 
-      def initialize(diff_lines)
+      def initialize(diff_lines, repository: nil)
+        @repository = repository
+
         if diff_lines.is_a?(Gitlab::Diff::File)
           @diff_file = diff_lines
           @diff_lines = @diff_file.diff_lines
@@ -19,7 +21,7 @@ module Gitlab
         @diff_lines.map.with_index do |diff_line, i|
           diff_line = diff_line.dup
           # ignore highlighting for "match" lines
-          next diff_line if diff_line.type == 'match' || diff_line.type == 'nonewline'
+          next diff_line if diff_line.meta?
 
           rich_line = highlight_line(diff_line) || diff_line.text
 
@@ -40,12 +42,12 @@ module Gitlab
 
         line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
 
-        case diff_line.type
-        when 'new', nil
-          rich_line = new_lines[diff_line.new_pos - 1]
-        when 'old'
-          rich_line = old_lines[diff_line.old_pos - 1]
-        end
+        rich_line =
+          if diff_line.unchanged? || diff_line.added?
+            new_lines[diff_line.new_pos - 1]
+          elsif diff_line.removed?
+            old_lines[diff_line.old_pos - 1]
+          end
 
         # Only update text if line is found. This will prevent
         # issues with submodules given the line only exists in diff content.
@@ -58,19 +60,12 @@ module Gitlab
 
       def old_lines
         return unless diff_file
-        @old_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:old))
+        @old_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_old_ref, diff_old_path)
       end
 
       def new_lines
         return unless diff_file
-        @new_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:new))
-      end
-
-      def processing_args(version)
-        ref  = send("diff_#{version}_ref")
-        path = send("diff_#{version}_path")
-
-        [ref.project.repository, ref.id, path]
+        @new_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_new_ref, diff_new_path)
       end
     end
   end

lib/gitlab/diff/line.rb

@@ -9,6 +9,18 @@ module Gitlab
         @old_pos, @new_pos = old_pos, new_pos
       end
 
+      def old_line
+        old_pos unless added? || meta?
+      end
+
+      def new_line
+        new_pos unless removed? || meta?
+      end
+
+      def unchanged?
+        type.nil?
+      end
+
       def added?
         type == 'new'
       end
@@ -16,6 +28,10 @@ module Gitlab
       def removed?
         type == 'old'
       end
+
+      def meta?
+        type == 'match' || type == 'nonewline'
+      end
     end
   end
 end

lib/gitlab/diff/line_mapper.rb

+# When provided a diff for a specific file, maps old line numbers to new line
+# numbers and back, to find out where a specific line in a file was moved by the
+# changes.
+module Gitlab
+  module Diff
+    class LineMapper
+      attr_accessor :diff_file
+
+      def initialize(diff_file)
+        @diff_file = diff_file
+      end
+
+      # Find new line number for old line number.
+      def old_to_new(old_line)
+        map_line_number(old_line, from: :old_line, to: :new_line)
+      end
+
+      # Find old line number for new line number.
+      def new_to_old(new_line)
+        map_line_number(new_line, from: :new_line, to: :old_line)
+      end
+
+      private
+
+      def diff_lines
+        @diff_lines ||= @diff_file.diff_lines
+      end
+
+      # Find old/new line number based on its old/new counterpart line number.
+      def map_line_number(from_line, from:, to:)
+        # If no diff file could be found, the file wasn't changed, and the
+        # mapped line number is the same as the specified line number.
+        return from_line unless diff_file
+
+        # To find the mapped line number for the specified line number,
+        # we need to find:
+        # - The diff line with that exact line number, if it is in the diff context
+        # - The first diff line with a higher line number, if it falls between diff contexts
+        # - The last known diff line, if it falls after the last diff context
+        diff_line = diff_lines.find do |diff_line|
+          diff_from_line = diff_line.send(from)
+          diff_from_line && diff_from_line >= from_line
+        end
+        diff_line ||= diff_lines.last
+
+        # If no diff line could be found, the file wasn't changed, and the
+        # mapped line number is the same as the specified line number.
+        return from_line unless diff_line
+
+        diff_from_line = diff_line.send(from)
+        diff_to_line = diff_line.send(to)
+
+        # If the line was removed, there is no mapped line number.
+        return unless diff_to_line
+
+        # Because we may not have the diff line with the exact line number
+        # we were looking for, we need to adjust the mapped line number.
+        distance = diff_from_line - from_line
+
+        diff_to_line - distance
+      end
+    end
+  end
+end

lib/gitlab/diff/parallel_diff.rb

@@ -15,17 +15,19 @@ module Gitlab
         highlighted_diff_lines.each do |line|
           full_line = line.text
           type = line.type
-          line_code = generate_line_code(diff_file.file_path, line)
+          line_code = diff_file.line_code(line)
           line_new = line.new_pos
           line_old = line.old_pos
+          position = diff_file.position(line)
 
           next_line = diff_file.next_line(line.index)
 
           if next_line
             next_line = highlighted_diff_lines[next_line.index]
-            next_line_code = generate_line_code(diff_file.file_path, next_line)
+            full_next_line = next_line.text
+            next_line_code = diff_file.line_code(next_line)
             next_type = next_line.type
-            next_line = next_line.text
+            next_position = diff_file.position(next_line)
           end
 
           case type
@@ -37,12 +39,14 @@ module Gitlab
                 number:     line_old,
                 text:       full_line,
                 line_code:  line_code,
+                position:   position
               },
               right: {
                 type:       type,
                 number:     line_new,
                 text:       full_line,
-                line_code:  line_code
+                line_code:  line_code,
+                position:   position
               }
             }
           when 'old'
@@ -55,12 +59,14 @@ module Gitlab
                   number:     line_old,
                   text:       full_line,
                   line_code:  line_code,
+                  position:   position
                 },
                 right: {
                   type:       next_type,
                   number:     line_new,
-                  text:       next_line,
-                  line_code:  next_line_code
+                  text:       full_next_line,
+                  line_code:  next_line_code,
+                  position:   next_position,
                 }
               }
               skip_next = true
@@ -73,12 +79,14 @@ module Gitlab
                   number:     line_old,
                   text:       full_line,
                   line_code:  line_code,
+                  position:   position
                 },
                 right: {
                   type:       next_type,
                   number:     nil,
                   text:       "",
-                  line_code:  nil
+                  line_code:  nil,
+                  position:   nil
                 }
               }
             end
@@ -95,12 +103,14 @@ module Gitlab
                   number:     nil,
                   text:       "",
                   line_code:  line_code,
+                  position:   position
                 },
                 right: {
                   type:       type,
                   number:     line_new,
                   text:       full_line,
-                  line_code:  line_code
+                  line_code:  line_code,
+                  position:   position
                 }
               }
             end
@@ -108,12 +118,6 @@ module Gitlab
         end
         lines
       end
-
-      private
-
-      def generate_line_code(file_path, line)
-        Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
-      end
     end
   end
 end

lib/gitlab/diff/position.rb

+# Defines a specific location, identified by paths and line numbers,
+# within a specific diff, identified by start, head and base commit ids.
+module Gitlab
+  module Diff
+    class Position
+      attr_reader :old_path
+      attr_reader :new_path
+      attr_reader :old_line
+      attr_reader :new_line
+      attr_reader :base_sha
+      attr_reader :start_sha
+      attr_reader :head_sha
+
+      def initialize(attrs = {})
+        @old_path = attrs[:old_path]
+        @new_path = attrs[:new_path]
+        @old_line = attrs[:old_line]
+        @new_line = attrs[:new_line]
+
+        if attrs[:diff_refs]
+          @base_sha  = attrs[:diff_refs].base_sha
+          @start_sha = attrs[:diff_refs].start_sha
+          @head_sha  = attrs[:diff_refs].head_sha
+        else
+          @base_sha  = attrs[:base_sha]
+          @start_sha = attrs[:start_sha]
+          @head_sha  = attrs[:head_sha]
+        end
+      end
+
+      # `Gitlab::Diff::Position` objects are stored as serialized attributes in
+      # `DiffNote`, which use YAML to encode and decode objects.
+      # `#init_with` and `#encode_with` can be used to customize the en/decoding
+      # behavior. In this case, we override these to prevent memoized instance
+      # variables like `@diff_file` and `@diff_line` from being serialized.
+      def init_with(coder)
+        initialize(coder['attributes'])
+
+        self
+      end
+
+      def encode_with(coder)
+        coder['attributes'] = self.to_h
+      end
+
+      def key
+        @key ||= [base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || ""), old_line, new_line]
+      end
+
+      def ==(other)
+        other.is_a?(self.class) && key == other.key
+      end
+
+      def to_h
+        {
+          old_path: old_path,
+          new_path: new_path,
+          old_line: old_line,
+          new_line: new_line,
+          base_sha:  base_sha,
+          start_sha: start_sha,
+          head_sha:  head_sha
+        }
+      end
+
+      def inspect
+        %(#<#{self.class}:#{object_id} #{to_h}>)
+      end
+
+      def complete?
+        file_path.present? &&
+          (old_line || new_line) &&
+          diff_refs.complete?
+      end
+
+      def to_json
+        JSON.generate(self.to_h)
+      end
+
+      def type
+        if old_line && new_line
+          nil
+        elsif new_line
+          'new'
+        else
+          'old'
+        end
+      end
+
+      def unchanged?
+        type.nil?
+      end
+
+      def added?
+        type == 'new'
+      end
+
+      def removed?
+        type == 'old'
+      end
+
+      def paths
+        [old_path, new_path].compact.uniq
+      end
+
+      def file_path
+        new_path.presence || old_path
+      end
+
+      def diff_refs
+        @diff_refs ||= DiffRefs.new(base_sha: base_sha, start_sha: start_sha, head_sha: head_sha)
+      end
+
+      def diff_file(repository)
+        @diff_file ||= begin
+          if RequestStore.active?
+            key = {
+              project_id: repository.project.id,
+              start_sha: start_sha,
+              head_sha: head_sha,
+              path: file_path
+            }
+
+            RequestStore.fetch(key) { find_diff_file(repository) }
+          else
+            find_diff_file(repository)
+          end
+        end
+      end
+
+      def diff_line(repository)
+        @diff_line ||= diff_file(repository).line_for_position(self)
+      end
+
+      def line_code(repository)
+        @line_code ||= diff_file(repository).line_code_for_position(self)
+      end
+
+      private
+
+      def find_diff_file(repository)
+        diffs = Gitlab::Git::Compare.new(
+          repository.raw_repository,
+          start_sha,
+          head_sha
+        ).diffs(paths: paths)
+
+        diff = diffs.first
+        return unless diff
+
+        Gitlab::Diff::File.new(diff, repository: repository, diff_refs: diff_refs)
+      end
+    end
+  end
+end

lib/gitlab/diff/position_tracer.rb

+# Finds the diff position in the new diff that corresponds to the same location
+# specified by the provided position in the old diff.
+module Gitlab
+  module Diff
+    class PositionTracer
+      attr_accessor :repository
+      attr_accessor :old_diff_refs
+      attr_accessor :new_diff_refs
+      attr_accessor :paths
+
+      def initialize(repository:, old_diff_refs:, new_diff_refs:, paths: nil)
+        @repository = repository
+        @old_diff_refs = old_diff_refs
+        @new_diff_refs = new_diff_refs
+        @paths = paths
+      end
+
+      def trace(old_position)
+        return unless old_diff_refs.complete? && new_diff_refs.complete?
+        return unless old_position.diff_refs == old_diff_refs
+
+        # Suppose we have an MR with source branch `feature` and target branch `master`.
+        # When the MR was created, the head of `master` was commit A, and the
+        # head of `feature` was commit B, resulting in the original diff A->B.
+        # Since creation, `master` was updated to C.
+        # Now `feature` is being updated to D, and the newly generated MR diff is C->D.
+        # It is possible that C and D are direct decendants of A and B respectively,
+        # but this isn't necessarily the case as rebases and merges come into play.
+        #
+        # Suppose we have a diff note on the original diff A->B. Now that the MR
+        # is updated, we need to find out what line in C->D corresponds to the
+        # line the note was originally created on, so that we can update the diff note's
+        # records and continue to display it in the right place in the diffs.
+        # If we cannot find this line in the new diff, this means the diff note is now
+        # outdated, and we will display that fact to the user.
+        #
+        # In the new diff, the file the diff note was originally created on may
+        # have been renamed, deleted or even created, if the file existed in A and B,
+        # but was removed in C, and restored in D.
+        #
+        # Every diff note stores a Position object that defines a specific location,
+        # identified by paths and line numbers, within a specific diff, identified
+        # by start, head and base commit ids.
+        #
+        # For diff notes for diff A->B, the position looks like this:
+        # Position
+        #   base_sha - ID of commit A
+        #   head_sha - ID of commit B
+        #   old_path - path as of A (nil if file was newly created)
+        #   new_path - path as of B (nil if file was deleted)
+        #   old_line - line number as of A (nil if file was newly created)
+        #   new_line - line number as of B (nil if file was deleted)
+        #
+        # We can easily update `base_sha` and `head_sha` to hold the IDs of commits C and D,
+        # but need to find the paths and line numbers as of C and D.
+        #
+        # If the file was unchanged or newly created in A->B, the path as of D can be found
+        # by generating diff B->D ("head to head"), finding the diff file with
+        # `diff_file.old_path == position.new_path`, and taking `diff_file.new_path`.
+        # The path as of C can be found by taking diff C->D, finding the diff file
+        # with that same `new_path` and taking `diff_file.old_path`.
+        # The line number as of D can be found by using the LineMapper on diff B->D
+        # and providing the line number as of B.
+        # The line number as of C can be found by using the LineMapper on diff C->D
+        # and providing the line number as of D.
+        #
+        # If the file was deleted in A->B, the path as of C can be found
+        # by generating diff A->C ("base to base"), finding the diff file with
+        # `diff_file.old_path == position.old_path`, and taking `diff_file.new_path`.
+        # The path as of D can be found by taking diff C->D, finding the diff file
+        # with that same `old_path` and taking `diff_file.new_path`.
+        # The line number as of C can be found by using the LineMapper on diff A->C
+        # and providing the line number as of A.
+        # The line number as of D can be found by using the LineMapper on diff C->D
+        # and providing the line number as of C.
+
+        results = nil
+        results ||= trace_added_line(old_position)   if old_position.added?   || old_position.unchanged?
+        results ||= trace_removed_line(old_position) if old_position.removed? || old_position.unchanged?
+
+        return unless results
+
+        file_diff, old_line, new_line = results
+
+        Position.new(
+          old_path: file_diff.old_path,
+          new_path: file_diff.new_path,
+          head_sha: new_diff_refs.head_sha,
+          start_sha: new_diff_refs.start_sha,
+          base_sha: new_diff_refs.base_sha,
+          old_line: old_line,
+          new_line: new_line
+        )
+      end
+
+      private
+
+      def trace_added_line(old_position)
+        file_path = old_position.new_path
+
+        return unless diff_head_to_head
+
+        file_head_to_head = diff_head_to_head.find { |diff_file| diff_file.old_path == file_path }
+
+        file_path = file_head_to_head.new_path if file_head_to_head
+
+        new_line = LineMapper.new(file_head_to_head).old_to_new(old_position.new_line)
+
+        return unless new_line
+
+        file_diff = new_diffs.find { |diff_file| diff_file.new_path == file_path }
+        return unless file_diff
+
+        old_line = LineMapper.new(file_diff).new_to_old(new_line)
+
+        [file_diff, old_line, new_line]
+      end
+
+      def trace_removed_line(old_position)
+        file_path = old_position.old_path
+
+        return unless diff_base_to_base
+
+        file_base_to_base = diff_base_to_base.find { |diff_file| diff_file.old_path == file_path }
+
+        file_path = file_base_to_base.old_path if file_base_to_base
+
+        old_line = LineMapper.new(file_base_to_base).old_to_new(old_position.old_line)
+
+        return unless old_line
+
+        file_diff = new_diffs.find { |diff_file| diff_file.old_path == file_path }
+        return unless file_diff
+
+        new_line = LineMapper.new(file_diff).old_to_new(old_line)
+
+        [file_diff, old_line, new_line]
+      end
+
+      def diff_base_to_base
+        @diff_base_to_base ||= diff_files(old_diff_refs.base_sha || old_diff_refs.start_sha, new_diff_refs.base_sha || new_diff_refs.start_sha)
+      end
+
+      def diff_head_to_head
+        @diff_head_to_head ||= diff_files(old_diff_refs.head_sha, new_diff_refs.head_sha)
+      end
+
+      def new_diffs
+        @new_diffs ||= diff_files(new_diff_refs.start_sha, new_diff_refs.head_sha, use_base: true)
+      end
+
+      def diff_files(start_sha, head_sha, use_base: false)
+        base_sha = self.repository.merge_base(start_sha, head_sha) || start_sha
+
+        diffs = self.repository.raw_repository.diff(
+          use_base ? base_sha : start_sha,
+          head_sha,
+          {},
+          *paths
+        )
+
+        diffs.decorate! do |diff|
+          Gitlab::Diff::File.new(diff, repository: self.repository)
+        end
+      end
+    end
+  end
+end

lib/gitlab/email/message/repository_push.rb

@@ -33,11 +33,15 @@ module Gitlab
         end
 
         def commits
-          @commits ||= (Commit.decorate(compare.commits, project) if compare)
+          return unless compare
+
+          @commits ||= Commit.decorate(compare.commits, project)
         end
 
         def diffs
-          @diffs ||= (safe_diff_files(compare.diffs(max_files: 30), diff_refs) if compare)
+          return unless compare
+          
+          @diffs ||= safe_diff_files(compare.diffs(max_files: 30), diff_refs: diff_refs, repository: project.repository)
         end
 
         def diffs_count

lib/gitlab/email/receiver.rb

@@ -104,15 +104,7 @@ module Gitlab
       end
 
       def create_note(reply)
-        Notes::CreateService.new(
-          sent_notification.project,
-          sent_notification.recipient,
-          note:           reply,
-          noteable_type:  sent_notification.noteable_type,
-          noteable_id:    sent_notification.noteable_id,
-          commit_id:      sent_notification.commit_id,
-          line_code:      sent_notification.line_code
-        ).execute
+        sent_notification.create_note(reply)
       end
     end
   end

lib/gitlab/github_import/pull_request_formatter.rb

@@ -11,10 +11,10 @@ module Gitlab
           description: description,
           source_project: source_branch_project,
           source_branch: source_branch_name,
-          head_source_sha: source_branch_sha,
+          source_branch_sha: source_branch_sha,
           target_project: target_branch_project,
           target_branch: target_branch_name,
-          base_target_sha: target_branch_sha,
+          target_branch_sha: target_branch_sha,
           state: state,
           milestone: milestone,
           author_id: author_id,

lib/gitlab/timeless.rb

+module Gitlab
+  module Timeless
+    def self.timeless(model, &block)
+      original_record_timestamps = model.record_timestamps
+      model.record_timestamps = false
+
+      if block.arity.abs == 1
+        block.call(model)
+      else
+        block.call
+      end
+    ensure
+      model.record_timestamps = original_record_timestamps
+    end
+  end
+end

lib/gitlab/workhorse.rb

@@ -38,12 +38,10 @@ module Gitlab
       end
 
       def send_git_diff(repository, diff_refs)
-        from, to = diff_refs
-
         params = {
           'RepoPath'  => repository.path_to_repo,
-          'ShaFrom'   => from.sha,
-          'ShaTo'     => to.sha
+          'ShaFrom'   => diff_refs.start_sha,
+          'ShaTo'     => diff_refs.head_sha
         }
 
         [
@@ -52,11 +50,11 @@ module Gitlab
         ]
       end
 
-      def send_git_patch(repository, from, to)
+      def send_git_patch(repository, diff_refs)
         params = {
           'RepoPath'  => repository.path_to_repo,
-          'ShaFrom'   => from,
-          'ShaTo'     => to
+          'ShaFrom'   => diff_refs.start_sha,
+          'ShaTo'     => diff_refs.head_sha
         }
 
         [

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -103,7 +103,7 @@ describe Projects::MergeRequestsController do
             id: merge_request.iid,
             format: :patch)
 
-        expect(response.headers['Gitlab-Workhorse-Send-Data']).to start_with("git-format-patch:")
+        expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-format-patch:")
       end
     end
   end
@@ -212,7 +212,7 @@ describe Projects::MergeRequestsController do
 
     context 'when the sha parameter matches the source SHA' do
       def merge_with_sha
-        post :merge, base_params.merge(sha: merge_request.source_sha)
+        post :merge, base_params.merge(sha: merge_request.diff_head_sha)
       end
 
       it 'returns :success' do
@@ -229,11 +229,11 @@ describe Projects::MergeRequestsController do
 
       context 'when merge_when_build_succeeds is passed' do
         def merge_when_build_succeeds
-          post :merge, base_params.merge(sha: merge_request.source_sha, merge_when_build_succeeds: '1')
+          post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_build_succeeds: '1')
         end
 
         before do
-          create(:ci_empty_pipeline, project: project, sha: merge_request.source_sha, ref: merge_request.source_branch)
+          create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch)
         end
 
         it 'returns :merge_when_build_succeeds' do

spec/factories/notes.rb

@@ -10,21 +10,46 @@ FactoryGirl.define do
     on_issue
 
     factory :note_on_commit,             traits: [:on_commit]
-    factory :note_on_commit_diff,        traits: [:on_commit, :on_diff], class: LegacyDiffNote
     factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
     factory :note_on_merge_request,      traits: [:on_merge_request]
-    factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff], class: LegacyDiffNote
     factory :note_on_project_snippet,    traits: [:on_project_snippet]
     factory :system_note,                traits: [:system]
 
+    factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
+    factory :legacy_diff_note_on_merge_request, traits: [:on_merge_request, :legacy_diff_note], class: LegacyDiffNote
+
+    factory :diff_note_on_merge_request, traits: [:on_merge_request], class: DiffNote do
+      position do
+        Gitlab::Diff::Position.new(
+          old_path: "files/ruby/popen.rb",
+          new_path: "files/ruby/popen.rb",
+          old_line: nil,
+          new_line: 14,
+          diff_refs: noteable.diff_refs
+        )
+      end
+    end
+
+    factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
+      position do
+        Gitlab::Diff::Position.new(
+          old_path: "files/ruby/popen.rb",
+          new_path: "files/ruby/popen.rb",
+          old_line: nil,
+          new_line: 14,
+          diff_refs: project.commit(commit_id).try(:diff_refs)
+        )
+      end
+    end
+
     trait :on_commit do
       noteable nil
-      noteable_id nil
       noteable_type 'Commit'
+      noteable_id nil
       commit_id RepoHelpers.sample_commit.id
     end
 
-    trait :on_diff do
+    trait :legacy_diff_note do
       line_code "0_184_184"
     end
 

spec/features/merge_requests/created_from_fork_spec.rb

@@ -30,7 +30,7 @@ feature 'Merge request created from fork' do
 
     given(:pipeline) do
       create(:ci_pipeline_with_two_job, project: fork_project,
-                                        sha: merge_request.last_commit.id,
+                                        sha: merge_request.diff_head_sha,
                                         ref: merge_request.source_branch)
     end
 

spec/features/merge_requests/merge_when_build_succeeds_spec.rb

@@ -12,7 +12,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
   end
 
   context "Active build for Merge Request" do
-    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
     let!(:ci_build) { create(:ci_build, pipeline: pipeline) }
 
     before do
@@ -47,7 +47,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
                                                   merge_user: user, title: "MepMep", merge_when_build_succeeds: true)
     end
 
-    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
     let!(:ci_build) { create(:ci_build, pipeline: pipeline) }
 
     before do

spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb

@@ -19,7 +19,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
   end
 
   context 'when project has CI enabled' do
-    let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
 
     context 'when merge requests can only be merged if the build succeeds' do
       before do

spec/features/notes_on_merge_requests_spec.rb

@@ -166,12 +166,14 @@ describe 'Comments', feature: true do
           click_diff_line
 
           is_expected.
-            to have_css("tr[id='#{line_code}'] + .js-temp-notes-holder form",
+            to have_css("form[data-line-code='#{line_code}']",
                         count: 1)
         end
 
         it 'should be removed when canceled' do
-          page.within(".diff-file form[id$='#{line_code}-true']") do
+          is_expected.to have_css('.js-temp-notes-holder')
+
+          page.within("form[data-line-code='#{line_code}']") do
             find('.js-close-discussion-note-form').trigger('click')
           end
 

spec/helpers/diff_helper_spec.rb

@@ -9,7 +9,7 @@ describe DiffHelper do
   let(:diffs) { commit.diffs }
   let(:diff) { diffs.first }
   let(:diff_refs) { [commit.parent, commit] }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
 
   describe 'diff_view' do
     it 'returns a valid value when cookie is set' do

spec/lib/gitlab/diff/file_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Diff::File, lib: true do
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe :diff_lines do
     let(:diff_lines) { diff_file.diff_lines }

spec/lib/gitlab/diff/highlight_spec.rb

@@ -6,11 +6,11 @@ describe Gitlab::Diff::Highlight, lib: true do
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe '#highlight' do
     context "with a diff file" do
-      let(:subject) { Gitlab::Diff::Highlight.new(diff_file).highlight }
+      let(:subject) { Gitlab::Diff::Highlight.new(diff_file, repository: project.repository).highlight }
 
       it 'should return Gitlab::Diff::Line elements' do
         expect(subject.first).to be_an_instance_of(Gitlab::Diff::Line)
@@ -41,7 +41,7 @@ describe Gitlab::Diff::Highlight, lib: true do
     end
 
     context "with diff lines" do
-      let(:subject) { Gitlab::Diff::Highlight.new(diff_file.diff_lines).highlight }
+      let(:subject) { Gitlab::Diff::Highlight.new(diff_file.diff_lines, repository: project.repository).highlight }
 
       it 'should return Gitlab::Diff::Line elements' do
         expect(subject.first).to be_an_instance_of(Gitlab::Diff::Line)

spec/lib/gitlab/diff/line_mapper_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Diff::LineMapper, lib: true do
+  include RepoHelpers
+
+  let(:project) { create(:project) }
+  let(:repository) { project.repository }
+  let(:commit) { project.commit(sample_commit.id) }
+  let(:diffs) { commit.diffs }
+  let(:diff) { diffs.first }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
+  subject { described_class.new(diff_file) }
+
+  describe '#old_to_new' do
+    context "with a diff file" do
+      let(:mapping) do
+        {
+          1 => 1,
+          2 => 2,
+          3 => 3,
+          4 => 4,
+          5 => 5,
+          6 => 6,
+          7 => 7,
+          8 => 8,
+          9 => nil,
+          # nil => 9,
+          10 => 10,
+          11 => 11,
+          12 => 12,
+          13 => nil,
+          14 => nil,
+          # nil => 15,
+          # nil => 16,
+          # nil => 17,
+          # nil => 18,
+          # nil => 19,
+          # nil => 20,
+          15 => 21,
+          16 => 22,
+          17 => 23,
+          18 => 24,
+          19 => 25,
+          20 => 26,
+          21 => 27,
+          # nil => 28,
+          22 => 29,
+          23 => 30,
+          24 => 31,
+          25 => 32,
+          26 => 33,
+          27 => 34,
+          28 => 35,
+          29 => 36,
+          30 => 37
+        }
+      end
+
+      it 'returns the new line number for the old line number' do
+        mapping.each do |old_line, new_line|
+          expect(subject.old_to_new(old_line)).to eq(new_line)
+        end
+      end
+    end
+
+    context "without a diff file" do
+      let(:diff_file) { nil }
+
+      it "returns the same line number" do
+        expect(subject.old_to_new(100)).to eq(100)
+      end
+    end
+  end
+
+  describe '#new_to_old' do
+    context "with a diff file" do
+      let(:mapping) do
+        {
+          1 => 1,
+          2 => 2,
+          3 => 3,
+          4 => 4,
+          5 => 5,
+          6 => 6,
+          7 => 7,
+          8 => 8,
+          # nil => 9,
+          9 => nil,
+          10 => 10,
+          11 => 11,
+          12 => 12,
+          # nil => 13,
+          # nil => 14,
+          13 => nil,
+          14 => nil,
+          15 => nil,
+          16 => nil,
+          17 => nil,
+          18 => nil,
+          19 => nil,
+          20 => nil,
+          21 => 15,
+          22 => 16,
+          23 => 17,
+          24 => 18,
+          25 => 19,
+          26 => 20,
+          27 => 21,
+          28 => nil,
+          29 => 22,
+          30 => 23,
+          31 => 24,
+          32 => 25,
+          33 => 26,
+          34 => 27,
+          35 => 28,
+          36 => 29,
+          37 => 30
+        }
+      end
+
+      it 'returns the old line number for the new line number' do
+        mapping.each do |new_line, old_line|
+          expect(subject.new_to_old(new_line)).to eq(old_line)
+        end
+      end
+    end
+
+    context "without a diff file" do
+      let(:diff_file) { nil }
+
+      it "returns the same line number" do
+        expect(subject.new_to_old(100)).to eq(100)
+      end
+    end
+  end
+end

spec/lib/gitlab/diff/parallel_diff_spec.rb

@@ -8,8 +8,7 @@ describe Gitlab::Diff::ParallelDiff, lib: true do
   let(:commit) { project.commit(sample_commit.id) }
   let(:diffs) { commit.diffs }
   let(:diff) { diffs.first }
-  let(:diff_refs) { [commit.parent, commit] }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
   subject { described_class.new(diff_file) }
 
   let(:parallel_diff_result_array) { YAML.load_file("#{Rails.root}/spec/fixtures/parallel_diff_result.yml") }

spec/lib/gitlab/github_import/pull_request_formatter_spec.rb

@@ -43,10 +43,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: source_sha,
+          source_branch_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: target_sha,
+          target_branch_sha: target_sha,
           state: 'opened',
           milestone: nil,
           author_id: project.creator_id,
@@ -70,10 +70,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: source_sha,
+          source_branch_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: target_sha,
+          target_branch_sha: target_sha,
           state: 'closed',
           milestone: nil,
           author_id: project.creator_id,
@@ -97,10 +97,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: source_sha,
+          source_branch_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: target_sha,
+          target_branch_sha: target_sha,
           state: 'merged',
           milestone: nil,
           author_id: project.creator_id,

spec/lib/gitlab/import_export/project_tree_saver_spec.rb

@@ -125,7 +125,7 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
 
     ci_pipeline = create(:ci_pipeline,
                        project: project,
-                       sha: merge_request.last_commit.id,
+                       sha: merge_request.diff_head_sha,
                        ref: merge_request.source_branch,
                        statuses: [commit_status])
 

spec/lib/gitlab/note_data_builder_spec.rb

@@ -27,7 +27,7 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   end
 
   describe 'When asking for a note on commit diff' do
-    let(:note) { create(:note_on_commit_diff, project: project) }
+    let(:note) { create(:diff_note_on_commit, project: project) }
 
     it 'returns the note and commit-specific data' do
       expect(data).to have_key(:commit)
@@ -90,7 +90,7 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
     end
 
     let(:note) do
-      create(:note_on_merge_request_diff, noteable: merge_request,
+      create(:diff_note_on_merge_request, noteable: merge_request,
                                           project: project)
     end
 

spec/lib/gitlab/url_builder_spec.rb

@@ -43,9 +43,9 @@ describe Gitlab::UrlBuilder, lib: true do
         end
       end
 
-      context 'on a CommitDiff' do
+      context 'on a Commit Diff' do
         it 'returns a proper URL' do
-          note = build_stubbed(:note_on_commit_diff)
+          note = build_stubbed(:diff_note_on_commit)
 
           url = described_class.build(note)
 
@@ -75,10 +75,10 @@ describe Gitlab::UrlBuilder, lib: true do
         end
       end
 
-      context 'on a MergeRequestDiff' do
+      context 'on a MergeRequest Diff' do
         it 'returns a proper URL' do
           merge_request = create(:merge_request, iid: 42)
-          note = build_stubbed(:note_on_merge_request_diff, noteable: merge_request)
+          note = build_stubbed(:diff_note_on_merge_request, noteable: merge_request)
 
           url = described_class.build(note)
 

spec/mailers/notify_spec.rb

@@ -948,7 +948,7 @@ describe Notify do
     let(:commits) { Commit.decorate(compare.commits, nil) }
     let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
     let(:send_from_committer_email) { false }
-    let(:diff_refs) { [project.merge_base_commit(sample_image_commit.id, sample_commit.id), project.commit(sample_commit.id)] }
+    let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: project.merge_base_commit(sample_image_commit.id, sample_commit.id).id, head_sha: sample_commit.id) }
 
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, diff_refs: diff_refs, send_from_committer_email: send_from_committer_email) }
 
@@ -1049,7 +1049,7 @@ describe Notify do
     let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) }
     let(:commits) { Commit.decorate(compare.commits, nil) }
     let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
-    let(:diff_refs) { [project.merge_base_commit(sample_commit.parent_id, sample_commit.id), project.commit(sample_commit.id)] }
+    let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: project.merge_base_commit(sample_image_commit.id, sample_commit.id).id, head_sha: sample_commit.id) }
 
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, diff_refs: diff_refs) }
 

spec/models/diff_note_spec.rb

+require 'spec_helper'
+
+describe DiffNote, models: true do
+  include RepoHelpers
+
+  let(:project) { create(:project) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  let(:commit) { project.commit(sample_commit.id) }
+
+  let(:path) { "files/ruby/popen.rb" }
+
+  let!(:position) do
+    Gitlab::Diff::Position.new(
+      old_path: path,
+      new_path: path,
+      old_line: nil,
+      new_line: 14,
+      diff_refs: merge_request.diff_refs
+    )
+  end
+
+  let!(:new_position) do
+    Gitlab::Diff::Position.new(
+      old_path: path,
+      new_path: path,
+      old_line: 16,
+      new_line: 22,
+      diff_refs: merge_request.diff_refs
+    )
+  end
+
+  subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
+
+  describe "#position=" do
+    context "when provided a string" do
+      it "sets the position" do
+        subject.position = new_position.to_json
+
+        expect(subject.position).to eq(new_position)
+      end
+    end
+
+    context "when provided a hash" do
+      it "sets the position" do
+        subject.position = new_position.to_h
+
+        expect(subject.position).to eq(new_position)
+      end
+    end
+
+    context "when provided a position object" do
+      it "sets the position" do
+        subject.position = new_position
+
+        expect(subject.position).to eq(new_position)
+      end
+    end
+  end
+
+  describe "#diff_file" do
+    it "returns the correct diff file" do
+      diff_file = subject.diff_file
+
+      expect(diff_file.old_path).to eq(position.old_path)
+      expect(diff_file.new_path).to eq(position.new_path)
+      expect(diff_file.diff_refs).to eq(position.diff_refs)
+    end
+  end
+
+  describe "#diff_line" do
+    it "returns the correct diff line" do
+      diff_line = subject.diff_line
+
+      expect(diff_line.added?).to be true
+      expect(diff_line.new_line).to eq(position.new_line)
+      expect(diff_line.text).to eq("+    vars = {")
+    end
+  end
+
+  describe "#line_code" do
+    it "returns the correct line code" do
+      line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.new_line, 15)
+
+      expect(subject.line_code).to eq(line_code)
+    end
+  end
+
+  describe "#for_line?" do
+    context "when provided the correct diff line" do
+      it "returns true" do
+        expect(subject.for_line?(subject.diff_line)).to be true
+      end
+    end
+
+    context "when provided a different diff line" do
+      it "returns false" do
+        some_line = subject.diff_file.diff_lines.first
+
+        expect(subject.for_line?(some_line)).to be false
+      end
+    end
+  end
+
+  describe "#active?" do
+    context "when noteable is a commit" do
+      subject { create(:diff_note_on_commit, project: project, position: position) }
+
+      it "returns true" do
+        expect(subject.active?).to be true
+      end
+    end
+
+    context "when noteable is a merge request" do
+      context "when the merge request's diff refs match that of the diff note" do
+        it "returns true" do
+          expect(subject.active?).to be true
+        end
+      end
+
+      context "when the merge request's diff refs don't match that of the diff note" do
+        before do
+          allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs)
+        end
+
+        it "returns false" do
+          expect(subject.active?).to be false
+        end
+      end
+    end
+  end
+
+  describe "creation" do
+    describe "updating of position" do
+      context "when noteable is a commit" do
+        let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) }
+
+        it "doesn't use the DiffPositionUpdateService" do
+          expect(Notes::DiffPositionUpdateService).not_to receive(:new)
+
+          diff_note
+        end
+
+        it "doesn't update the position" do
+          diff_note
+
+          expect(diff_note.original_position).to eq(position)
+          expect(diff_note.position).to eq(position)
+        end
+      end
+
+      context "when noteable is a merge request" do
+        let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
+
+        context "when the note is active" do
+          it "doesn't use the DiffPositionUpdateService" do
+            expect(Notes::DiffPositionUpdateService).not_to receive(:new)
+
+            diff_note
+          end
+
+          it "doesn't update the position" do
+            diff_note
+
+            expect(diff_note.original_position).to eq(position)
+            expect(diff_note.position).to eq(position)
+          end
+        end
+
+        context "when the note is outdated" do
+          before do
+            allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
+          end
+
+          it "uses the DiffPositionUpdateService" do
+            service = instance_double("Notes::DiffPositionUpdateService")
+            expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+              project,
+              nil,
+              old_diff_refs: position.diff_refs,
+              new_diff_refs: commit.diff_refs,
+              paths: [path]
+            ).and_return(service)
+            expect(service).to receive(:execute)
+
+            diff_note
+          end
+        end
+      end
+    end
+  end
+end

spec/models/event_spec.rb

@@ -56,7 +56,7 @@ describe Event, models: true do
     end
 
     context 'merge request diff note event' do
-      let(:target) { create(:note_on_merge_request_diff) }
+      let(:target) { create(:legacy_diff_note_on_merge_request) }
 
       it { is_expected.to be_note }
     end
@@ -132,7 +132,7 @@ describe Event, models: true do
     context 'merge request diff note event' do
       let(:project) { create(:project, :public) }
       let(:merge_request) { create(:merge_request, source_project: project, author: author, assignee: assignee) }
-      let(:note_on_merge_request) { create(:note_on_merge_request_diff, noteable: merge_request, project: project) }
+      let(:note_on_merge_request) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project) }
       let(:target) { note_on_merge_request }
 
       it { expect(event.visible_to_user?(non_member)).to eq true }

spec/models/legacy_diff_note_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe LegacyDiffNote, models: true do
   describe "Commit diff line notes" do
-    let!(:note) { create(:note_on_commit_diff, note: "+1 from me") }
+    let!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") }
     let!(:commit) { note.noteable }
 
     it "should save a valid note" do
@@ -17,7 +17,7 @@ describe LegacyDiffNote, models: true do
 
   describe '#active?' do
     it 'is always true when the note has no associated diff' do
-      note = build(:note_on_merge_request_diff)
+      note = build(:legacy_diff_note_on_merge_request)
 
       expect(note).to receive(:diff).and_return(nil)
 
@@ -25,7 +25,7 @@ describe LegacyDiffNote, models: true do
     end
 
     it 'is never true when the note has no noteable associated' do
-      note = build(:note_on_merge_request_diff)
+      note = build(:legacy_diff_note_on_merge_request)
 
       expect(note).to receive(:diff).and_return(double)
       expect(note).to receive(:noteable).and_return(nil)
@@ -34,7 +34,7 @@ describe LegacyDiffNote, models: true do
     end
 
     it 'returns the memoized value if defined' do
-      note = build(:note_on_merge_request_diff)
+      note = build(:legacy_diff_note_on_merge_request)
 
       note.instance_variable_set(:@active, 'foo')
       expect(note).not_to receive(:find_noteable_diff)
@@ -45,7 +45,7 @@ describe LegacyDiffNote, models: true do
     context 'for a merge request noteable' do
       it 'is false when noteable has no matching diff' do
         merge = build_stubbed(:merge_request, :simple)
-        note = build(:note_on_merge_request_diff, noteable: merge)
+        note = build(:legacy_diff_note_on_merge_request, noteable: merge)
 
         allow(note).to receive(:diff).and_return(double)
         expect(note).to receive(:find_noteable_diff).and_return(nil)
@@ -63,9 +63,9 @@ describe LegacyDiffNote, models: true do
         code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
 
         # We're persisting in order to trigger the set_diff callback
-        note = create(:note_on_merge_request_diff, noteable: merge,
-                                                   line_code: code,
-                                                   project: merge.source_project)
+        note = create(:legacy_diff_note_on_merge_request,  noteable: merge,
+                                                           line_code: code,
+                                                           project: merge.source_project)
 
         # Make sure we don't get a false positive from a guard clause
         expect(note).to receive(:find_noteable_diff).and_call_original

spec/models/merge_request_spec.rb

 require 'spec_helper'
 
 describe MergeRequest, models: true do
+  include RepoHelpers
+
   subject { create(:merge_request) }
 
   describe 'associations' do
@@ -62,7 +64,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#target_sha' do
+  describe '#target_branch_sha' do
     context 'when the target branch does not exist anymore' do
       let(:project) { create(:project) }
 
@@ -73,32 +75,32 @@ describe MergeRequest, models: true do
       end
 
       it 'returns nil' do
-        expect(subject.target_sha).to be_nil
+        expect(subject.target_branch_sha).to be_nil
       end
     end
   end
 
-  describe '#source_sha' do
+  describe '#source_branch_sha' do
     let(:last_branch_commit) { subject.source_project.repository.commit(subject.source_branch) }
 
     context 'with diffs' do
       subject { create(:merge_request, :with_diffs) }
       it 'returns the sha of the source branch last commit' do
-        expect(subject.source_sha).to eq(last_branch_commit.sha)
+        expect(subject.source_branch_sha).to eq(last_branch_commit.sha)
       end
     end
 
     context 'without diffs' do
       subject { create(:merge_request, :without_diffs) }
       it 'returns the sha of the source branch last commit' do
-        expect(subject.source_sha).to eq(last_branch_commit.sha)
+        expect(subject.source_branch_sha).to eq(last_branch_commit.sha)
       end
     end
 
     context 'when the merge request is being created' do
       subject { build(:merge_request, source_branch: nil, compare_commits: []) }
       it 'returns nil' do
-        expect(subject.source_sha).to be_nil
+        expect(subject.source_branch_sha).to be_nil
       end
     end
   end
@@ -252,12 +254,14 @@ describe MergeRequest, models: true do
     end
 
     it "can be removed if the last commit is the head of the source branch" do
-      allow(subject.source_project).to receive(:commit).and_return(subject.last_commit)
+      allow(subject.source_project).to receive(:commit).and_return(subject.diff_head_commit)
 
       expect(subject.can_remove_source_branch?(user)).to be_truthy
     end
 
     it "cannot be removed if the last commit is not also the head of the source branch" do
+      subject.source_branch = "lfs"
+
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
   end
@@ -363,7 +367,7 @@ describe MergeRequest, models: true do
           and_return(2)
 
         subject.diverged_commits_count
-        allow(subject).to receive(:source_sha).and_return('123abc')
+        allow(subject).to receive(:source_branch_sha).and_return('123abc')
         subject.diverged_commits_count
       end
 
@@ -373,7 +377,7 @@ describe MergeRequest, models: true do
           and_return(2)
 
         subject.diverged_commits_count
-        allow(subject).to receive(:target_sha).and_return('123abc')
+        allow(subject).to receive(:target_branch_sha).and_return('123abc')
         subject.diverged_commits_count
       end
     end
@@ -392,11 +396,10 @@ describe MergeRequest, models: true do
 
   describe '#pipeline' do
     describe 'when the source project exists' do
-      it 'returns the latest commit' do
-        commit   = double(:commit, id: '123abc')
+      it 'returns the latest pipeline' do
         pipeline = double(:ci_pipeline, ref: 'master')
 
-        allow(subject).to receive(:last_commit).and_return(commit)
+        allow(subject).to receive(:diff_head_sha).and_return('123abc')
 
         expect(subject.source_project).to receive(:pipeline).
           with('123abc', 'master').
@@ -464,7 +467,7 @@ describe MergeRequest, models: true do
     context 'when it is not broken and has no conflicts' do
       it 'is marked as mergeable' do
         allow(subject).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?) { true }
+        allow(project.repository).to receive(:can_be_merged?).and_return(true)
 
         expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
       end
@@ -481,7 +484,7 @@ describe MergeRequest, models: true do
     context 'when it has conflicts' do
       before do
         allow(subject).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?) { false }
+        allow(project.repository).to receive(:can_be_merged?).and_return(false)
       end
 
       it 'becomes unmergeable' do
@@ -608,4 +611,42 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe "#reload_diff" do
+    let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
+
+    let(:commit) { subject.project.commit(sample_commit.id) }
+
+    it "reloads the diff content" do
+      expect(subject.merge_request_diff).to receive(:reload_content)
+
+      subject.reload_diff
+    end
+
+    it "updates diff note positions" do
+      old_diff_refs = subject.diff_refs
+
+      merge_request_diff = subject.merge_request_diff
+
+      # Update merge_request_diff so that #diff_refs will return commit.diff_refs
+      allow(merge_request_diff).to receive(:reload_content) do
+        merge_request_diff.base_commit_sha = commit.parent_id
+        merge_request_diff.start_commit_sha = commit.parent_id
+        merge_request_diff.head_commit_sha = commit.sha
+      end
+
+      expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+        subject.project,
+        nil,
+        old_diff_refs: old_diff_refs,
+        new_diff_refs: commit.diff_refs,
+        paths: note.position.paths
+      ).and_call_original
+      expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
+
+      expect_any_instance_of(DiffNote).to receive(:save).once
+
+      subject.reload_diff
+    end
+  end
 end

spec/models/project_spec.rb

@@ -312,7 +312,7 @@ describe Project, models: true do
     it 'should update merge request commits with new one if pushed to source branch' do
       project.update_merge_requests(prev_commit_id, commit_id, "refs/heads/#{merge_request.source_branch}", key.user)
       merge_request.reload
-      expect(merge_request.last_commit.id).to eq(commit_id)
+      expect(merge_request.diff_head_sha).to eq(commit_id)
     end
   end
 

spec/models/repository_spec.rb

@@ -12,8 +12,8 @@ describe Repository, models: true do
   end
   let(:merge_commit) do
     source_sha = repository.find_branch('feature').target
-    merge_commit_id = repository.merge(user, source_sha, 'master', commit_options)
-    repository.commit(merge_commit_id)
+    merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options)
+    repository.commit(merge_commit_sha)
   end
 
   describe :branch_names_contains do

spec/requests/api/merge_requests_spec.rb

@@ -439,14 +439,14 @@ describe API::API, api: true  do
     end
 
     it "returns 409 if the SHA parameter doesn't match" do
-      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha.succ
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.diff_head_sha.reverse
 
       expect(response).to have_http_status(409)
       expect(json_response['message']).to start_with('SHA does not match HEAD of source branch')
     end
 
     it "succeeds if the SHA parameter matches" do
-      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.diff_head_sha
 
       expect(response).to have_http_status(200)
     end

spec/services/system_note_service_spec.rb

@@ -213,7 +213,7 @@ describe SystemNoteService, services: true do
       create(:merge_request, source_project: project, target_project: project)
     end
 
-    subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) }
+    subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.diff_head_commit) }
 
     it_behaves_like 'a system note'