Move diff parsing to own class. Correctly identify note diff line

app/helpers/commits_helper.rb

@@ -15,63 +15,9 @@ module CommitsHelper
     commit_person_link(commit, options.merge(source: :committer))
   end
 
-  def identification_type(line)
-    if line[0] == "+"
-      "new"
-    elsif line[0] == "-"
-      "old"
-    else
-      nil
-    end
-  end
-
-  def build_line_anchor(diff, line_new, line_old)
-    "#{hexdigest(diff.new_path)}_#{line_old}_#{line_new}"
-  end
-
   def each_diff_line(diff, index)
-    diff_arr = diff.diff.lines.to_a
-
-    line_old = 1
-    line_new = 1
-    type = nil
-
-    lines_arr = ::Gitlab::InlineDiff.processing diff_arr
-    lines_arr.each do |line|
-      raw_line = line.dup
-
-      next if line.match(/^\-\-\- \/dev\/null/)
-      next if line.match(/^\+\+\+ \/dev\/null/)
-      next if line.match(/^\-\-\- a/)
-      next if line.match(/^\+\+\+ b/)
-
-      full_line = html_escape(line.gsub(/\n/, ''))
-      full_line = ::Gitlab::InlineDiff.replace_markers full_line
-
-      if line.match(/^@@ -/)
-        type = "match"
-
-        line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0
-        line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0
-
-        next if line_old == 1 && line_new == 1 #top of file
-        yield(full_line, type, nil, nil, nil)
-        next
-      else
-        type = identification_type(line)
-        line_code = build_line_anchor(diff, line_new, line_old)
-        yield(full_line, type, line_code, line_new, line_old, raw_line)
-      end
-
-
-      if line[0] == "+"
-        line_new += 1
-      elsif line[0] == "-"
-        line_old += 1
-      else
-        line_new += 1
-        line_old += 1
-      end
+    Gitlab::DiffParser.new(diff).each do |full_line, type, line_code, line_new, line_old|
+      yield(full_line, type, line_code, line_new, line_old)
     end
   end
 

app/models/note.rb

@@ -51,7 +51,7 @@ class Note < ActiveRecord::Base
   scope :inc_author, ->{ includes(:author) }
 
   serialize :st_diff
-  before_create :set_diff, if: ->(n) { n.noteable_type == 'MergeRequest' && n.line_code.present? }
+  before_create :set_diff, if: ->(n) { n.line_code.present? }
 
   def self.create_status_change_note(noteable, author, status)
     create({
@@ -81,7 +81,7 @@ class Note < ActiveRecord::Base
   def set_diff
     # First lets find notes with same diff
     # before iterating over all mr diffs
-    diff = self.noteable.notes.where(line_code: self.line_code).last.try(:diff)
+    diff = Note.where(noteable_id: self.noteable_id, noteable_type: self.noteable_type, line_code: self.line_code).last.try(:diff)
     diff ||= find_diff
 
     self.st_diff = diff.to_hash if diff
@@ -91,6 +91,12 @@ class Note < ActiveRecord::Base
     @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
   end
 
+  def active?
+    # TODO: determine if discussion is outdated
+    # according to recent MR diff or not
+    true
+  end
+
   def diff_file_index
     line_code.split('_')[0]
   end
@@ -108,12 +114,17 @@ class Note < ActiveRecord::Base
   end
 
   def diff_line
+    return @diff_line if @diff_line
+
     if diff
-      @diff_line ||= diff.diff.lines.select { |line| line =~ /\A\+/ }[diff_new_line] ||
-        diff.diff.lines.select { |line| line =~ /\A\-/ }[diff_old_line]
+      Gitlab::DiffParser.new(diff).each do |full_line, type, line_code, line_new, line_old|
+        @diff_line = full_line if line_code == self.line_code
       end
     end
 
+    @diff_line
+  end
+
   def discussion_id
     @discussion_id ||= [:discussion, noteable_type.try(:underscore), noteable_id || commit_id, line_code].join("-").to_sym
   end

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

@@ -20,4 +20,4 @@
     - if @reply_allowed
       - comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at)
       - unless comments.empty?
-        = render "projects/notes/diff_notes_with_reply", notes: comments, raw_line: raw_line
+        = render "projects/notes/diff_notes_with_reply", notes: comments, line: line

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

 - note = notes.first # example note
 -# Check if line want not changed since comment was left
-- if !defined?(raw_line) || raw_line == note.diff_line
+- if !defined?(line) || line == note.diff_line
   %tr.notes_holder
     %td.notes_line{ colspan: 2 }
       %span.btn.disabled

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

@@ -36,7 +36,7 @@
         ago
   .discussion-body
     - if note.for_diff_line?
-      - if note.diff
+      - if note.active?
         .content
           .file= render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note
       - else

doc/update/5.4-to-6.0.md

@@ -61,6 +61,7 @@ sudo -u git -H bundle exec rake db:migrate RAILS_ENV=production
 sudo -u git -H bundle exec rake migrate_groups RAILS_ENV=production
 sudo -u git -H bundle exec rake migrate_global_projects RAILS_ENV=production
 sudo -u git -H bundle exec rake migrate_keys RAILS_ENV=production
+sudo -u git -H bundle exec rake migrate_inline_notes RAILS_ENV=production
 
 ```
 

lib/gitlab/diff_parser.rb

+module Gitlab
+  class DiffParser
+    include Enumerable
+
+    attr_reader :lines, :new_path
+
+    def initialize(diff)
+      @lines = diff.diff.lines.to_a
+      @new_path = diff.new_path
+    end
+
+    def each
+      line_old = 1
+      line_new = 1
+      type = nil
+
+      lines_arr = ::Gitlab::InlineDiff.processing lines
+      lines_arr.each do |line|
+        raw_line = line.dup
+
+        next if line.match(/^\-\-\- \/dev\/null/)
+        next if line.match(/^\+\+\+ \/dev\/null/)
+        next if line.match(/^\-\-\- a/)
+        next if line.match(/^\+\+\+ b/)
+
+        full_line = html_escape(line.gsub(/\n/, ''))
+        full_line = ::Gitlab::InlineDiff.replace_markers full_line
+
+        if line.match(/^@@ -/)
+          type = "match"
+
+          line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0
+          line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0
+
+          next if line_old == 1 && line_new == 1 #top of file
+          yield(full_line, type, nil, nil, nil)
+          next
+        else
+          type = identification_type(line)
+          line_code = generate_line_code(new_path, line_new, line_old)
+          yield(full_line, type, line_code, line_new, line_old, raw_line)
+        end
+
+
+        if line[0] == "+"
+          line_new += 1
+        elsif line[0] == "-"
+          line_old += 1
+        else
+          line_new += 1
+          line_old += 1
+        end
+      end
+    end
+
+    private
+
+    def identification_type(line)
+      if line[0] == "+"
+        "new"
+      elsif line[0] == "-"
+        "old"
+      else
+        nil
+      end
+    end
+
+    def generate_line_code(path, line_new, line_old)
+      "#{Digest::SHA1.hexdigest(path)}_#{line_old}_#{line_new}"
+    end
+
+    def html_escape str
+      replacements = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;', "'" => '&#39;' }
+        str.gsub(/[&"'><]/, replacements)
+    end
+  end
+end

lib/tasks/migrate/migrate_inline_notes.rake

 desc "GITLAB | Migrate inline notes"
 task migrate_inline_notes: :environment do
-  Note.where(noteable_type: 'MergeRequest').find_each(batch_size: 100) do |note|
+  Note.where('line_code IS NOT NULL').find_each(batch_size: 100) do |note|
     begin
       note.set_diff
       if note.save