Refactoring inside refactoring. We need to go deeper

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/controllers/projects/edit_tree_controller.rb

@@ -31,7 +31,7 @@ class Projects::EditTreeController < Projects::BaseTreeController
 
     diffy = Diffy::Diff.new(@blob.data, @content, diff: '-U 3',
                             include_diff_info: true)
-    @diff_lines = Gitlab::Diff::Parser.new.parse(diffy.diff.scan(/.*\n/), @path, @path)
+    @diff_lines = Gitlab::Diff::Parser.new.parse(diffy.diff.scan(/.*\n/))
 
     render layout: false
   end

app/helpers/commits_helper.rb

@@ -16,62 +16,6 @@ module CommitsHelper
     commit_person_link(commit, options.merge(source: :committer))
   end
 
-  def parallel_diff(diff_file, index)
-    lines = []
-    skip_next = false
-
-    # Building array of lines
-    #
-    # [left_type, left_line_number, left_line_content, right_line_type, right_line_number, right_line_content]
-    #
-    diff_file.diff_lines.each do |line|
-
-      full_line = line.text
-      type = line.type
-      line_code = line.code
-      line_new = line.new_pos
-      line_old = line.old_pos
-
-      next_line = diff_file.next_line(line.index)
-
-      if next_line
-        next_type = next_line.type
-        next_line = next_line.text
-      end
-
-      line = [type, line_old, full_line, next_type, line_new]
-      if type == 'match' || type.nil?
-        # line in the right panel is the same as in the left one
-        line = [type, line_old, full_line, type, line_new, full_line]
-        lines.push(line)
-      elsif type == 'old'
-        if next_type == 'new'
-          # Left side has text removed, right side has text added
-          line.push(next_line)
-          lines.push(line)
-          skip_next = true
-        elsif next_type == 'old' || next_type.nil?
-          # Left side has text removed, right side doesn't have any change
-          line.pop # remove the newline
-          line.push(nil) # no line number on the right panel
-          line.push(" ") # empty line on the right panel
-          lines.push(line)
-        end
-      elsif type == 'new'
-        if skip_next
-          # Change has been already included in previous line so no need to do it again
-          skip_next = false
-          next
-        else
-          # Change is only on the right side, left side has no change
-          line = [nil, nil, " ", type, line_new, full_line]
-          lines.push(line)
-        end
-      end
-    end
-    lines
-  end
-
   def image_diff_class(diff)
     if diff.deleted_file
       "deleted"

app/helpers/diff_helper.rb

 module DiffHelper
-  def safe_diff_files(project, diffs)
+  def safe_diff_files(diffs)
     if diff_hard_limit_enabled?
       diffs.first(Commit::DIFF_HARD_LIMIT_FILES)
     else
       diffs.first(Commit::DIFF_SAFE_FILES)
     end.map do |diff|
-      Gitlab::Diff::File.new(project, @commit, diff)
+      Gitlab::Diff::File.new(diff)
     end
   end
 
-  def show_diff_size_warninig?(project, diffs)
-    safe_diff_files(project, diffs).size < diffs.size
+  def show_diff_size_warninig?(diffs)
+    safe_diff_files(diffs).size < diffs.size
   end
 
   def diff_hard_limit_enabled?
@@ -21,4 +21,65 @@ module DiffHelper
       false
     end
   end
+
+  def generate_line_code(file_path, line)
+    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
+  end
+
+  def parallel_diff(diff_file, index)
+    lines = []
+    skip_next = false
+
+    # Building array of lines
+    #
+    # [left_type, left_line_number, left_line_content, right_line_type, right_line_number, right_line_content]
+    #
+    diff_file.diff_lines.each do |line|
+
+      full_line = line.text
+      type = line.type
+      line_code = generate_line_code(diff_file.file_path, line)
+      line_new = line.new_pos
+      line_old = line.old_pos
+
+      next_line = diff_file.next_line(line.index)
+
+      if next_line
+        next_type = next_line.type
+        next_line = next_line.text
+      end
+
+      line = [type, line_old, full_line, next_type, line_new]
+      if type == 'match' || type.nil?
+        # line in the right panel is the same as in the left one
+        line = [type, line_old, full_line, type, line_new, full_line]
+        lines.push(line)
+      elsif type == 'old'
+        if next_type == 'new'
+          # Left side has text removed, right side has text added
+          line.push(next_line)
+          lines.push(line)
+          skip_next = true
+        elsif next_type == 'old' || next_type.nil?
+          # Left side has text removed, right side doesn't have any change
+          line.pop # remove the newline
+          line.push(nil) # no line number on the right panel
+          line.push("&nbsp;") # empty line on the right panel
+          lines.push(line)
+        end
+      elsif type == 'new'
+        if skip_next
+          # Change has been already included in previous line so no need to do it again
+          skip_next = false
+          next
+        else
+          # Change is only on the right side, left side has no change
+          line = [nil, nil, "&nbsp;", type, line_new, full_line]
+          lines.push(line)
+        end
+      end
+    end
+    lines
+  end
+
 end

app/models/note.rb

@@ -209,7 +209,7 @@ class Note < ActiveRecord::Base
     noteable.diffs.each do |mr_diff|
       next unless mr_diff.new_path == self.diff.new_path
 
-      lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a, mr_diff.old_path, mr_diff.new_path)
+      lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a)
 
       lines.each do |line|
         if line.text == diff_line
@@ -233,6 +233,14 @@ class Note < ActiveRecord::Base
     diff.new_path if diff
   end
 
+  def file_path
+    if diff.new_path.present?
+      diff.new_path
+    elsif diff.old_path.present?
+      diff.old_path
+    end
+  end
+
   def diff_old_line
     line_code.split('_')[1].to_i
   end
@@ -241,12 +249,18 @@ class Note < ActiveRecord::Base
     line_code.split('_')[2].to_i
   end
 
+  def generate_line_code(line)
+    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
+  end
+
   def diff_line
     return @diff_line if @diff_line
 
     if diff
       diff_lines.each do |line|
-        @diff_line = line.text if line.code == self.line_code
+        if generate_line_code(line) == self.line_code
+          @diff_line = line.text
+        end
       end
     end
 
@@ -259,7 +273,7 @@ class Note < ActiveRecord::Base
     prev_lines = []
 
     diff_lines.each do |line|
-      if line.code != self.line_code
+      if generate_line_code(line) != self.line_code
         if line.type == "match"
           prev_lines.clear
           prev_match_line = line
@@ -275,7 +289,7 @@ class Note < ActiveRecord::Base
   end
 
   def diff_lines
-    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.lines.to_a, diff.old_path, diff.new_path)
+    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.lines.to_a)
   end
 
   def discussion_id

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

 .row
   .col-md-8
-    = render 'projects/diffs/diff_stats', diffs: diffs
+    = render 'projects/diffs/stats', diffs: diffs
   .col-md-4
     %ul.nav.nav-tabs
       %li.pull-right{class: params[:view] == 'parallel' ? 'active' : ''}
@@ -11,12 +11,12 @@
         - params_copy[:view] = 'inline'
         = link_to "Inline Diff", url_for(params_copy), {id: "commit-diff-viewtype"}
 
-- if show_diff_size_warninig?(project, diffs)
-  = render 'projects/diffs/diff_warning', diffs: diffs
+- if show_diff_size_warninig?(diffs)
+  = render 'projects/diffs/warning', diffs: diffs
 
 .files
-  - safe_diff_files(project, diffs).each_with_index do |diff_file, i|
-    = render 'projects/diffs/diff_file', diff_file: diff_file, i: i, project: project
+  - safe_diff_files(diffs).each_with_index do |diff_file, index|
+    = render 'projects/diffs/file', diff_file: diff_file, i: index, project: project
 
 - if @diff_timeout
   .alert.alert-danger

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

-- return unless diff_file.blob_exists?
-- blob = diff_file.blob
-- blob_diff_path = diff_project_blob_path(project, tree_join(@commit.id, diff_file.new_path))
+- blob = project.repository.blob_for_diff(@commit, diff_file.diff)
+- return unless blob
+- blob_diff_path = diff_project_blob_path(project, tree_join(@commit.id, diff_file.file_path))
 .diff-file{id: "diff-#{i}", data: {blob_diff_path: blob_diff_path }}
   .diff-header{id: "file-path-#{hexdigest(diff_file.new_path || diff_file.old_path)}"}
     - if diff_file.deleted_file

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

@@ -7,7 +7,7 @@
   - diff_file.diff_lines.each_with_index do |line, index|
     - type = line.type
     - last_line = line.new_pos
-    - line_code = line.code
+    - line_code = generate_line_code(diff_file.file_path, line)
     - line_old = line.old_pos
     %tr.line_holder{ id: line_code, class: "#{type}" }
       - if type == "match"

app/views/projects/edit_tree/_diff.html.haml

-%table.text-file
-  - each_diff_line(diff, 1) do |line, type, line_code, line_new, line_old, raw_line|
-    %tr.line_holder{ id: line_code, class: "#{type}" }
-      - if type == "match"
-        %td.old_line= "..."
-        %td.new_line= "..."
-        %td.line_content.matched= line
-      - else
-        %td.old_line
-          = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code
-        %td.new_line= link_to raw(type == "old" ? " " : line_new) , "##{line_code}", id: line_code
-        %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line)
-

app/views/projects/edit_tree/preview.html.haml

@@ -12,15 +12,14 @@
         - unless @diff_lines.empty?
           %table.text-file
             - @diff_lines.each do |line|
-              %tr.line_holder{ id: line.code, class: "#{line.type}" }
+              %tr.line_holder{ class: "#{line.type}" }
                 - if line.type == "match"
                   %td.old_line= "..."
                   %td.new_line= "..."
                   %td.line_content.matched= line.text
                 - else
                   %td.old_line
-                    = link_to raw(line.type == "new" ? " " : line.old_pos), "##{line.code}", id: line.code
-                  %td.new_line= link_to raw(line.type == "old" ? " " : line.new_pos) , "##{line.code}", id: line.code
-                  %td.line_content{class: "noteable_line #{line.type} #{line.code}", "line.code" => line.code}= raw diff_line_content(line.text)
+                  %td.new_line
+                  %td.line_content{class: "#{line.type}"}= raw diff_line_content(line.text)
         - else
           .nothing-here-block No changes.

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

-- note = @project.notes.new(@comments_target.merge({ line_code: line_code }))
-= link_to "",
-          "javascript:;",
-          class: "add-diff-note js-add-diff-note-button",
-          data: { noteable_type: note.noteable_type,
-                  noteable_id:   note.noteable_id,
-                  commit_id:     note.commit_id,
-                  line_code:     note.line_code,
-                  discussion_id: note.discussion_id },
-          title: "Add a comment to this line"

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

@@ -12,7 +12,8 @@
     .diff-content
       %table
         - note.truncated_diff_lines.each do |line|
-          %tr.line_holder{ id: line.code }
+          - line_code = generate_line_code(note.file_path, line)
+          %tr.line_holder{ id: line_code }
             - if line.type == "match"
               %td.old_line= "..."
               %td.new_line= "..."
@@ -20,7 +21,7 @@
             - else
               %td.old_line= raw(line.type == "new" ? " " : line.old_pos)
               %td.new_line= raw(line.type == "old" ? " " : line.new_pos)
-              %td.line_content{class: "noteable_line #{line.type} #{line.code}", "line_code" => line.code}= raw "#{line.text}  "
+              %td.line_content{class: "noteable_line #{line.type} #{line_code}", "line_code" => line_code}= raw "#{line.text}  "
 
-              - if line.code == note.line_code
+              - if line_code == note.line_code
                 = render "projects/notes/diff_notes_with_reply", notes: discussion_notes

lib/gitlab/diff/file.rb

 module Gitlab
   module Diff
     class File
-      attr_reader :diff, :blob
+      attr_reader :diff
 
       delegate :new_file, :deleted_file, :renamed_file,
         :old_path, :new_path, to: :diff, prefix: false
 
-      def initialize(project, commit, diff)
+      def initialize(diff)
         @diff = diff
-        @blob = project.repository.blob_for_diff(commit, diff)
       end
 
       # Array of Gitlab::DIff::Line objects
       def diff_lines
-        @lines ||= parser.parse(diff.diff.lines, old_path, new_path)
-      end
-
-      def blob_exists?
-        !@blob.nil?
+        @lines ||= parser.parse(raw_diff.lines)
       end
 
       def mode_changed?
@@ -28,6 +23,10 @@ module Gitlab
         Gitlab::Diff::Parser.new
       end
 
+      def raw_diff
+        diff.diff
+      end
+
       def next_line(index)
         diff_lines[index + 1]
       end
@@ -37,6 +36,14 @@ module Gitlab
           diff_lines[index - 1]
         end
       end
+
+      def file_path
+        if diff.new_path.present?
+          diff.new_path
+        elsif diff.old_path.present?
+          diff.old_path
+        end
+      end
     end
   end
 end

lib/gitlab/diff/line.rb

 module Gitlab
   module Diff
     class Line
-      attr_reader :type, :text, :index, :code, :old_pos, :new_pos
+      attr_reader :type, :text, :index, :old_pos, :new_pos
 
-      def initialize(text, type, index, old_pos, new_pos, code = nil)
-        @text, @type, @index, @code = text, type, index, code
+      def initialize(text, type, index, old_pos, new_pos)
+        @text, @type, @index = text, type, index
         @old_pos, @new_pos = old_pos, new_pos
       end
     end

lib/gitlab/diff/line_code.rb

+module Gitlab
+  module Diff
+    class LineCode
+      def self.generate(file_path, new_line_position, old_line_position)
+        "#{Digest::SHA1.hexdigest(file_path)}_#{old_line_position}_#{new_line_position}"
+      end
+    end
+  end
+end

lib/gitlab/diff/parser.rb

@@ -3,7 +3,7 @@ module Gitlab
     class Parser
       include Enumerable
 
-      def parse(lines, old_path, new_path)
+      def parse(lines)
         @lines = lines,
         lines_obj = []
         line_obj_index = 0
@@ -33,8 +33,7 @@ module Gitlab
             next
           else
             type = identification_type(line)
-            line_code = generate_line_code(new_path, line_new, line_old)
-            lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, line_code)
+            lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
             line_obj_index += 1
           end
 
@@ -73,10 +72,6 @@ module Gitlab
         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)

spec/lib/gitlab/diff/file_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Diff::File do
   let(:project) { create(:project) }
   let(:commit) { project.repository.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
-  let(:diff_file) { Gitlab::Diff::File.new(project, commit, diff) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff) }
 
   describe :diff_lines do
     let(:diff_lines) { diff_file.diff_lines }
@@ -15,10 +15,6 @@ describe Gitlab::Diff::File do
     it { diff_lines.first.should be_kind_of(Gitlab::Diff::Line) }
   end
 
-  describe :blob_exists? do
-    it { diff_file.blob_exists?.should be_true }
-  end
-
   describe :mode_changed? do
     it { diff_file.mode_changed?.should be_false }
   end

spec/lib/gitlab/diff/parser_spec.rb

@@ -46,10 +46,8 @@ describe Gitlab::Diff::Parser do
 eos
     end
 
-    let(:path) { 'files/ruby/popen.rb' }
-
     before do
-      @lines = parser.parse(diff.lines, path, path)
+      @lines = parser.parse(diff.lines)
     end
 
     it { @lines.size.should == 30 }