From b54f2393c3e952f8ff9f297b3c848c3c1cede1c9 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Wed, 3 Sep 2014 11:06:08 +0200 Subject: [PATCH 01/23] Parallel diff in two columns. --- .../projects/commits/_parallel_view.html.haml | 50 ++++++------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index 80f5be98f2f..3bbe8b4baaf 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -1,38 +1,18 @@ / Side-by-side diff view -- old_lines, new_lines = parallel_diff_lines(project, @commit, diff, file) -- num_lines = old_lines.length - %div.text-file %table - - num_lines.times do |index| - - new_line = new_lines[index] - - old_line = old_lines[index] - %tr.line_holder.parallel - -# For old line - - if old_line.type == :file_created - %td.old_line= old_line.num - %td.line_content.parallel= "File was created" - - elsif old_line.type == :deleted - %td.old_line.old= old_line.num - %td.line_content{class: "parallel noteable_line old #{old_line.code}", "line_code" => old_line.code}= old_line.content - - else old_line.type == :no_change - %td.old_line= old_line.num - %td.line_content.parallel= old_line.content - - -# For new line - - if new_line.type == :file_deleted - %td.new_line= new_line.num - %td.line_content.parallel= "File was deleted" - - elsif new_line.type == :added - %td.new_line.new= new_line.num - %td.line_content{class: "parallel noteable_line new #{new_line.code}", "line_code" => new_line.code}= new_line.content - - else new_line.type == :no_change - %td.new_line= new_line.num - %td.line_content.parallel= new_line.content - - - if @reply_allowed - - comments1 = @line_notes.select { |n| n.line_code == old_line.code }.sort_by(&:created_at) - - comments2 = @line_notes.select { |n| n.line_code == new_line.code }.sort_by(&:created_at) - - unless comments1.empty? and comments2.empty? - = render "projects/notes/diff_notes_with_reply_parallel", notes1: comments1, notes2: comments2 - + - each_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, e| + %tr.line_holder.parallel{ id: line_code, class: "#{type}" } + - if type != 'match' + %td.old_line + = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code + - if type == 'old' + %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= line + - else + %td.line_content.parallel= line + %td.new_line{data: {linenumber: line_new}} + = link_to raw(type == "old" ? " " : line_new) , "##{line_code}", id: line_code + - if type == 'new' + %td.line_content.parallel{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= line + - else + %td.line_content.parallel= line From 1067b00724c045b4fa46a9f8ff5acd09d65553e0 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Thu, 4 Sep 2014 10:46:35 +0200 Subject: [PATCH 02/23] Duplicate the behaviour and refactor for use with parallel diff. --- app/helpers/commits_helper.rb | 7 +++ .../projects/commits/_parallel_view.html.haml | 13 +++--- lib/gitlab/diff_parser.rb | 45 +++++++++++++++++++ 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index f61aa259154..2c1df6beeab 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -23,6 +23,13 @@ module CommitsHelper end end + def side_diff_line(diff, index) + Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) + .each_for_parallel do |full_line, type, line_code, line_new, line_old, next_line| + yield(full_line, type, line_code, line_new, line_old, next_line) + end + end + def each_diff_line_near(diff, index, expected_line_code) max_number_of_lines = 16 diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index 3bbe8b4baaf..e566b1dfca4 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -1,18 +1,19 @@ / Side-by-side diff view %div.text-file %table - - each_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, e| + - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, next_line| + - next if type == 'new' %tr.line_holder.parallel{ id: line_code, class: "#{type}" } - if type != 'match' %td.old_line - = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code + = link_to raw(line_old), "##{line_code}", id: line_code - if type == 'old' - %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= line + %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= raw line - else %td.line_content.parallel= line %td.new_line{data: {linenumber: line_new}} - = link_to raw(type == "old" ? " " : line_new) , "##{line_code}", id: line_code - - if type == 'new' - %td.line_content.parallel{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= line + = link_to raw(line_new) , "##{line_code}", id: line_code + - if type == 'old' + %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw next_line - else %td.line_content.parallel= line diff --git a/lib/gitlab/diff_parser.rb b/lib/gitlab/diff_parser.rb index b244295027e..baec2e63baa 100644 --- a/lib/gitlab/diff_parser.rb +++ b/lib/gitlab/diff_parser.rb @@ -50,6 +50,51 @@ module Gitlab end end + def each_for_parallel + line_old = 1 + line_new = 1 + type = nil + + lines_arr = ::Gitlab::InlineDiff.processing lines + + lines_arr.each_cons(2) do |line, next_line| + raw_line = line.dup + + next if filename?(line) + + full_line = html_escape(line.gsub(/\n/, '')) + full_line = ::Gitlab::InlineDiff.replace_markers full_line + + next_line = html_escape(next_line.gsub(/\n/, '')) + next_line = ::Gitlab::InlineDiff.replace_markers next_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, line_new, line_old) + 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, next_line) + end + + + if line[0] == "+" + line_new += 1 + elsif line[0] == "-" + line_old += 1 + else + line_new += 1 + line_old += 1 + end + end + end + def empty? @lines.empty? end From f827482c12b3aeec2ed5f60afbf7c676e27435e3 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Thu, 4 Sep 2014 11:25:14 +0200 Subject: [PATCH 03/23] Remove duplication, expand for next_line. --- app/helpers/commits_helper.rb | 4 +- .../projects/commits/_parallel_view.html.haml | 7 ++- .../diffs/_match_line_parallel.html.haml | 4 ++ lib/gitlab/diff_parser.rb | 43 +------------------ 4 files changed, 12 insertions(+), 46 deletions(-) create mode 100644 app/views/projects/commits/diffs/_match_line_parallel.html.haml diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 2c1df6beeab..fe6c303ecf2 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -25,8 +25,8 @@ module CommitsHelper def side_diff_line(diff, index) Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) - .each_for_parallel do |full_line, type, line_code, line_new, line_old, next_line| - yield(full_line, type, line_code, line_new, line_old, next_line) + .each do |full_line, type, line_code, line_new, line_old, raw_line, next_line| + yield(full_line, type, line_code, line_new, line_old, raw_line, next_line) end end diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index e566b1dfca4..92425f36577 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -1,10 +1,13 @@ / Side-by-side diff view %div.text-file %table - - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, next_line| + - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, next_line| - next if type == 'new' %tr.line_holder.parallel{ id: line_code, class: "#{type}" } - - if type != 'match' + - if type == "match" + = render "projects/commits/diffs/match_line_parallel", {line: line, + line_old: line_old, line_new: line_new, bottom: false} + - else %td.old_line = link_to raw(line_old), "##{line_code}", id: line_code - if type == 'old' diff --git a/app/views/projects/commits/diffs/_match_line_parallel.html.haml b/app/views/projects/commits/diffs/_match_line_parallel.html.haml new file mode 100644 index 00000000000..815df16aa4a --- /dev/null +++ b/app/views/projects/commits/diffs/_match_line_parallel.html.haml @@ -0,0 +1,4 @@ +%td.old_line + %td.line_content.parallel.matched= line +%td.new_line + %td.line_content.parallel.matched= line diff --git a/lib/gitlab/diff_parser.rb b/lib/gitlab/diff_parser.rb index baec2e63baa..f226692a63c 100644 --- a/lib/gitlab/diff_parser.rb +++ b/lib/gitlab/diff_parser.rb @@ -14,47 +14,6 @@ module Gitlab line_new = 1 type = nil - lines_arr = ::Gitlab::InlineDiff.processing lines - lines_arr.each do |line| - raw_line = line.dup - - next if filename?(line) - - 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, line_new, line_old) - 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 - - def each_for_parallel - line_old = 1 - line_new = 1 - type = nil - lines_arr = ::Gitlab::InlineDiff.processing lines lines_arr.each_cons(2) do |line, next_line| @@ -80,7 +39,7 @@ module Gitlab 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, next_line) + yield(full_line, type, line_code, line_new, line_old, raw_line, next_line) end From 357bf00921be191fcd401cdf26d9798f2b57a127 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Thu, 4 Sep 2014 12:05:58 +0200 Subject: [PATCH 04/23] File mode changed note. --- app/views/projects/commits/_parallel_view.html.haml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index 92425f36577..d7792e488a2 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -13,10 +13,14 @@ - if type == 'old' %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= raw line - else - %td.line_content.parallel= line + %td.line_content.parallel= raw line %td.new_line{data: {linenumber: line_new}} = link_to raw(line_new) , "##{line_code}", id: line_code - if type == 'old' %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw next_line - else - %td.line_content.parallel= line + %td.line_content.parallel= raw line + +- if diff.diff.blank? && diff_file_mode_changed?(diff) + .file-mode-changed + File mode changed From 75b04a17cf27cd7f7a252a697f347c08d58e2fd4 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Thu, 4 Sep 2014 13:27:57 +0200 Subject: [PATCH 05/23] Take added or removed file into account. --- .../projects/commits/_parallel_view.html.haml | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index d7792e488a2..f23051f2511 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -2,24 +2,29 @@ %div.text-file %table - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, next_line| - - next if type == 'new' %tr.line_holder.parallel{ id: line_code, class: "#{type}" } - if type == "match" = render "projects/commits/diffs/match_line_parallel", {line: line, line_old: line_old, line_new: line_new, bottom: false} - else %td.old_line - = link_to raw(line_old), "##{line_code}", id: line_code - - if type == 'old' - %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= raw line + - if diff.new_file + %td.line_content.parallel= " " - else - %td.line_content.parallel= raw line + = link_to raw(line_old), "##{line_code}", id: line_code + - if type == 'old' + %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= raw line + - else + %td.line_content.parallel= raw line %td.new_line{data: {linenumber: line_new}} - = link_to raw(line_new) , "##{line_code}", id: line_code - - if type == 'old' - %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw next_line + - if diff.deleted_file + %td.line_content.parallel= " " - else - %td.line_content.parallel= raw line + = link_to raw(line_new) , "##{line_code}", id: line_code + - if type == 'old' + %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw next_line + - else + %td.line_content.parallel= raw line - if diff.diff.blank? && diff_file_mode_changed?(diff) .file-mode-changed From dc7554d020f7e278f30c8d4c4113a19f7c3cd82f Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Thu, 4 Sep 2014 13:49:57 +0200 Subject: [PATCH 06/23] Coloring. --- .../projects/commits/_parallel_view.html.haml | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index f23051f2511..97e1c884b7b 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -2,29 +2,32 @@ %div.text-file %table - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, next_line| - %tr.line_holder.parallel{ id: line_code, class: "#{type}" } + %tr.line_holder.parallel{ id: line_code } - if type == "match" = render "projects/commits/diffs/match_line_parallel", {line: line, line_old: line_old, line_new: line_new, bottom: false} - else - %td.old_line - - if diff.new_file - %td.line_content.parallel= " " - - else + - if diff.new_file + %td.old_line{ class: "old" } + %td.line_content.parallel= " " + - else + - next if type == 'new' + %td.old_line{ class: "#{type}" } = link_to raw(line_old), "##{line_code}", id: line_code - if type == 'old' %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= raw line - else %td.line_content.parallel= raw line - %td.new_line{data: {linenumber: line_new}} - - if diff.deleted_file - %td.line_content.parallel= " " - - else + - if diff.deleted_file + %td.new_line{class: "new", data: {linenumber: line_new}} + %td.line_content.parallel= " " + - else + %td.new_line{class: "#{type}", data: {linenumber: line_new}} = link_to raw(line_new) , "##{line_code}", id: line_code - if type == 'old' %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw next_line - else - %td.line_content.parallel= raw line + %td.line_content.parallel{class: "#{type}"}= raw line - if diff.diff.blank? && diff_file_mode_changed?(diff) .file-mode-changed From 721b75733c49117100a5caf04bf6040fe6004dca Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Thu, 4 Sep 2014 14:13:24 +0200 Subject: [PATCH 07/23] Take the next type into consideration --- app/helpers/commits_helper.rb | 4 ++-- app/views/projects/commits/_parallel_view.html.haml | 5 +++-- lib/gitlab/diff_parser.rb | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index fe6c303ecf2..b3249e520a4 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -25,8 +25,8 @@ module CommitsHelper def side_diff_line(diff, index) Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) - .each do |full_line, type, line_code, line_new, line_old, raw_line, next_line| - yield(full_line, type, line_code, line_new, line_old, raw_line, next_line) + .each do |full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line| + yield(full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line) end end diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index 97e1c884b7b..7debc44e132 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -1,7 +1,7 @@ / Side-by-side diff view %div.text-file %table - - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, next_line| + - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, next_type, next_line| %tr.line_holder.parallel{ id: line_code } - if type == "match" = render "projects/commits/diffs/match_line_parallel", {line: line, @@ -25,7 +25,8 @@ %td.new_line{class: "#{type}", data: {linenumber: line_new}} = link_to raw(line_new) , "##{line_code}", id: line_code - if type == 'old' - %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw next_line + - content = next_type == 'new' ? next_line : " " + %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw content - else %td.line_content.parallel{class: "#{type}"}= raw line diff --git a/lib/gitlab/diff_parser.rb b/lib/gitlab/diff_parser.rb index f226692a63c..3f402c4c238 100644 --- a/lib/gitlab/diff_parser.rb +++ b/lib/gitlab/diff_parser.rb @@ -38,8 +38,9 @@ module Gitlab next else type = identification_type(line) + next_type = identification_type(next_line) line_code = generate_line_code(new_path, line_new, line_old) - yield(full_line, type, line_code, line_new, line_old, raw_line, next_line) + yield(full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line) end From 7d45624daff1ed4a3896c6270a0911a73f6f815f Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Fri, 5 Sep 2014 09:46:58 +0200 Subject: [PATCH 08/23] Don't show the line numbers if the lines were removed. --- app/views/projects/commits/_parallel_view.html.haml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index 7debc44e132..1d3aa1bf25a 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -23,11 +23,15 @@ %td.line_content.parallel= " " - else %td.new_line{class: "#{type}", data: {linenumber: line_new}} - = link_to raw(line_new) , "##{line_code}", id: line_code - if type == 'old' - - content = next_type == 'new' ? next_line : " " + - if next_type == 'new' + - content = next_line + = link_to raw(line_new) , "##{line_code}", id: line_code + - else + - content = " " %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw content - else + = link_to raw(line_new) , "##{line_code}", id: line_code %td.line_content.parallel{class: "#{type}"}= raw line - if diff.diff.blank? && diff_file_mode_changed?(diff) From 13cfa49a3dc90e87a5f6de8cd5c64d0cd0f4202d Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Fri, 5 Sep 2014 10:55:17 +0200 Subject: [PATCH 09/23] Color the lines correctly. --- .../projects/commits/_parallel_view.html.haml | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index 1d3aa1bf25a..a940330aec6 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -8,8 +8,8 @@ line_old: line_old, line_new: line_new, bottom: false} - else - if diff.new_file - %td.old_line{ class: "old" } - %td.line_content.parallel= " " + %td.old_line + %td.line_content.parallel= raw " " - else - next if type == 'new' %td.old_line{ class: "#{type}" } @@ -19,18 +19,20 @@ - else %td.line_content.parallel= raw line - if diff.deleted_file - %td.new_line{class: "new", data: {linenumber: line_new}} - %td.line_content.parallel= " " + %td.new_line{ data: {linenumber: line_new}} + %td.line_content.parallel= raw " " - else - %td.new_line{class: "#{type}", data: {linenumber: line_new}} - - if type == 'old' + - if type == 'old' + %td.new_line{class: "#{next_type == 'new' ? 'new' : nil}", data: {linenumber: line_new}} - if next_type == 'new' - content = next_line = link_to raw(line_new) , "##{line_code}", id: line_code + %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw content - else - content = " " - %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw content - - else + %td.line_content.parallel{class: "noteable_line #{line_code}", "line_code" => line_code}= raw content + - else + %td.new_line{class: "#{type}", data: {linenumber: line_new}} = link_to raw(line_new) , "##{line_code}", id: line_code %td.line_content.parallel{class: "#{type}"}= raw line From 23706716fcf57da7ba572a8720c753c75f554b05 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Sat, 6 Sep 2014 20:11:28 +0200 Subject: [PATCH 10/23] Now refactor all to work properly. --- app/helpers/commits_helper.rb | 120 +++++++----------- .../projects/commits/_parallel_view.html.haml | 60 ++++----- 2 files changed, 69 insertions(+), 111 deletions(-) diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index b3249e520a4..7d5b9c3238f 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -23,13 +23,55 @@ module CommitsHelper end end - def side_diff_line(diff, index) + def parallel_diff_line(diff, index) Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) .each do |full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line| yield(full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line) end end + def parallel_diff(diff, 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] + # + parallel_diff_line(diff, index) do |full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line| + 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 each_diff_line_near(diff, index, expected_line_code) max_number_of_lines = 16 @@ -112,82 +154,6 @@ module CommitsHelper branches.sort.map { |branch| link_to(branch, project_tree_path(project, branch)) }.join(", ").html_safe end - def parallel_diff_lines(project, commit, diff, file) - old_file = project.repository.blob_at(commit.parent_id, diff.old_path) if commit.parent_id - deleted_lines = {} - added_lines = {} - each_diff_line(diff, 0) do |line, type, line_code, line_new, line_old| - if type == "old" - deleted_lines[line_old] = { line_code: line_code, type: type, line: line } - elsif type == "new" - added_lines[line_new] = { line_code: line_code, type: type, line: line } - end - end - max_length = old_file ? [old_file.loc, file.loc].max : file.loc - - offset1 = 0 - offset2 = 0 - old_lines = [] - new_lines = [] - - max_length.times do |line_index| - line_index1 = line_index - offset1 - line_index2 = line_index - offset2 - deleted_line = deleted_lines[line_index1 + 1] - added_line = added_lines[line_index2 + 1] - old_line = old_file.lines[line_index1] if old_file - new_line = file.lines[line_index2] - - if deleted_line && added_line - elsif deleted_line - new_line = nil - offset2 += 1 - elsif added_line - old_line = nil - offset1 += 1 - end - - old_lines[line_index] = DiffLine.new - new_lines[line_index] = DiffLine.new - - # old - if line_index == 0 && diff.new_file - old_lines[line_index].type = :file_created - old_lines[line_index].content = 'File was created' - elsif deleted_line - old_lines[line_index].type = :deleted - old_lines[line_index].content = old_line - old_lines[line_index].num = line_index1 + 1 - old_lines[line_index].code = deleted_line[:line_code] - elsif old_line - old_lines[line_index].type = :no_change - old_lines[line_index].content = old_line - old_lines[line_index].num = line_index1 + 1 - else - old_lines[line_index].type = :added - end - - # new - if line_index == 0 && diff.deleted_file - new_lines[line_index].type = :file_deleted - new_lines[line_index].content = "File was deleted" - elsif added_line - new_lines[line_index].type = :added - new_lines[line_index].num = line_index2 + 1 - new_lines[line_index].content = new_line - new_lines[line_index].code = added_line[:line_code] - elsif new_line - new_lines[line_index].type = :no_change - new_lines[line_index].num = line_index2 + 1 - new_lines[line_index].content = new_line - else - new_lines[line_index].type = :deleted - end - end - - return old_lines, new_lines - end - def link_to_browse_code(project, commit) if current_controller?(:projects, :commits) if @repo.blob_at(commit.id, @path) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index a940330aec6..f455bec1d8e 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -1,40 +1,32 @@ / Side-by-side diff view %div.text-file %table - - side_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line, next_type, next_line| - %tr.line_holder.parallel{ id: line_code } - - if type == "match" - = render "projects/commits/diffs/match_line_parallel", {line: line, - line_old: line_old, line_new: line_new, bottom: false} - - else - - if diff.new_file - %td.old_line - %td.line_content.parallel= raw " " - - else - - next if type == 'new' - %td.old_line{ class: "#{type}" } - = link_to raw(line_old), "##{line_code}", id: line_code - - if type == 'old' - %td.line_content{class: "parallel noteable_line old #{line_code}", "line_code" => line_code}= raw line - - else - %td.line_content.parallel= raw line - - if diff.deleted_file - %td.new_line{ data: {linenumber: line_new}} - %td.line_content.parallel= raw " " - - else - - if type == 'old' - %td.new_line{class: "#{next_type == 'new' ? 'new' : nil}", data: {linenumber: line_new}} - - if next_type == 'new' - - content = next_line - = link_to raw(line_new) , "##{line_code}", id: line_code - %td.line_content.parallel{class: "noteable_line new #{line_code}", "line_code" => line_code}= raw content - - else - - content = " " - %td.line_content.parallel{class: "noteable_line #{line_code}", "line_code" => line_code}= raw content - - else - %td.new_line{class: "#{type}", data: {linenumber: line_new}} - = link_to raw(line_new) , "##{line_code}", id: line_code - %td.line_content.parallel{class: "#{type}"}= raw line + - parallel_diff(diff, index).each do |line| + - type_left = line[0] + - line_number_left = line[1] + - line_content_left = line[2] + - type_right = line[3] + - line_number_right = line[4] + - line_content_right = line[5] + + %tr.line_holder.parallel + - if type_left == 'match' + = render "projects/commits/diffs/match_line_parallel", {line: line_content_left, + line_old: line_number_left, line_new: line_number_right, bottom: false} + - elsif type_left == 'old' + %td.old_line{ class: "old" } + = link_to raw(line_number_left) + %td.line_content{class: "parallel noteable_line old"}= raw line_content_left + %td.new_line{class: "#{type_right == 'new' ? 'new' : nil}", data: {linenumber: line_number_right}} + = link_to raw(line_number_right) + %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil}"}= raw line_content_right + - elsif type_left.nil? + %td.old_line + = link_to raw(line_number_left) + %td.line_content{class: "parallel noteable_line"}= raw line_content_left + %td.new_line{class: "#{type_right == 'new' ? 'new' : nil}", data: {linenumber: line_number_right}} + = link_to raw(line_number_right) + %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil}"}= raw line_content_right - if diff.diff.blank? && diff_file_mode_changed?(diff) .file-mode-changed From 205358b1ae71611a04e03789b3bac74bd1052da5 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Mon, 8 Sep 2014 09:04:56 +0200 Subject: [PATCH 11/23] Cleanup --- .../projects/commits/_parallel_view.html.haml | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/commits/_parallel_view.html.haml index f455bec1d8e..dec417bb21f 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/commits/_parallel_view.html.haml @@ -11,20 +11,13 @@ %tr.line_holder.parallel - if type_left == 'match' - = render "projects/commits/diffs/match_line_parallel", {line: line_content_left, - line_old: line_number_left, line_new: line_number_right, bottom: false} - - elsif type_left == 'old' - %td.old_line{ class: "old" } + = render "projects/commits/diffs/match_line_parallel", { line: line_content_left, + line_old: line_number_left, line_new: line_number_right } + - elsif type_left == 'old' || type_left.nil? + %td.old_line{class: "#{type_left}"} = link_to raw(line_number_left) - %td.line_content{class: "parallel noteable_line old"}= raw line_content_left - %td.new_line{class: "#{type_right == 'new' ? 'new' : nil}", data: {linenumber: line_number_right}} - = link_to raw(line_number_right) - %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil}"}= raw line_content_right - - elsif type_left.nil? - %td.old_line - = link_to raw(line_number_left) - %td.line_content{class: "parallel noteable_line"}= raw line_content_left - %td.new_line{class: "#{type_right == 'new' ? 'new' : nil}", data: {linenumber: line_number_right}} + %td.line_content{class: "parallel noteable_line #{type_left}" }= raw line_content_left + %td.new_line{ class: "#{type_right == 'new' ? 'new' : nil}", data: { linenumber: line_number_right }} = link_to raw(line_number_right) %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil}"}= raw line_content_right From 4ef809c77d7f4155709a6d3f0188332c206ba0e0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 16:25:50 +0300 Subject: [PATCH 12/23] Gitlab::Diff classes added Signed-off-by: Dmitriy Zaporozhets --- lib/gitlab/diff/file.rb | 42 +++++++++++++++++++ lib/gitlab/diff/line.rb | 12 ++++++ lib/gitlab/diff/parser.rb | 86 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+) create mode 100644 lib/gitlab/diff/file.rb create mode 100644 lib/gitlab/diff/line.rb create mode 100644 lib/gitlab/diff/parser.rb diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb new file mode 100644 index 00000000000..adc78616f64 --- /dev/null +++ b/lib/gitlab/diff/file.rb @@ -0,0 +1,42 @@ +module Gitlab + module Diff + class File + attr_reader :diff, :blob + + delegate :new_file, :deleted_file, :renamed_file, + :old_path, :new_path, to: :diff, prefix: false + + def initialize(project, commit, 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? + end + + def mode_changed? + diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode + end + + def parser + Gitlab::Diff::Parser.new + end + + def next_line(index) + diff_lines[index + 1] + end + + def prev_line(index) + if index > 0 + diff_lines[index - 1] + end + end + end + end +end diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb new file mode 100644 index 00000000000..e8b9c980a1a --- /dev/null +++ b/lib/gitlab/diff/line.rb @@ -0,0 +1,12 @@ +module Gitlab + module Diff + class Line + attr_reader :type, :text, :index, :code, :old_pos, :new_pos + + def initialize(text, type, index, old_pos, new_pos, code = nil) + @text, @type, @index, @code = text, type, index, code + @old_pos, @new_pos = old_pos, new_pos + end + end + end +end diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb new file mode 100644 index 00000000000..0fd11c69a59 --- /dev/null +++ b/lib/gitlab/diff/parser.rb @@ -0,0 +1,86 @@ +module Gitlab + module Diff + class Parser + include Enumerable + + def parse(lines, old_path, new_path) + @lines = lines, + lines_obj = [] + line_obj_index = 0 + 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 filename?(line) + + 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 + lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) + line_obj_index += 1 + 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) + line_obj_index += 1 + end + + + if line[0] == "+" + line_new += 1 + elsif line[0] == "-" + line_old += 1 + else + line_new += 1 + line_old += 1 + end + end + + lines_obj + end + + def empty? + @lines.empty? + end + + private + + def filename?(line) + line.start_with?('--- /dev/null', '+++ /dev/null', '--- a', '+++ b', + '--- /tmp/diffy', '+++ /tmp/diffy') + end + + 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 = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } + str.gsub(/[&"'><]/, replacements) + end + end + end +end From e0eb48031dc3ed8079c637fa3b82556747f9f8e0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 16:26:49 +0300 Subject: [PATCH 13/23] Refactor diff views Signed-off-by: Dmitriy Zaporozhets --- .../projects/commits/_diff_file.html.haml | 48 ------------------- app/views/projects/commits/_diffs.html.haml | 4 +- app/views/projects/diffs/_diff_file.html.haml | 47 ++++++++++++++++++ .../{commits => diffs}/_diff_stats.html.haml | 0 .../_diff_warning.html.haml | 0 app/views/projects/diffs/_diffs.html.haml | 26 ++++++++++ .../{commits => diffs}/_image.html.haml | 0 .../{commits => }/diffs/_match_line.html.haml | 0 .../diffs/_match_line_parallel.html.haml | 0 .../_parallel_view.html.haml | 6 +-- .../{commits => diffs}/_text_file.html.haml | 25 +++++----- 11 files changed, 92 insertions(+), 64 deletions(-) delete mode 100644 app/views/projects/commits/_diff_file.html.haml create mode 100644 app/views/projects/diffs/_diff_file.html.haml rename app/views/projects/{commits => diffs}/_diff_stats.html.haml (100%) rename app/views/projects/{commits => diffs}/_diff_warning.html.haml (100%) create mode 100644 app/views/projects/diffs/_diffs.html.haml rename app/views/projects/{commits => diffs}/_image.html.haml (100%) rename app/views/projects/{commits => }/diffs/_match_line.html.haml (100%) rename app/views/projects/{commits => }/diffs/_match_line_parallel.html.haml (100%) rename app/views/projects/{commits => diffs}/_parallel_view.html.haml (82%) rename app/views/projects/{commits => diffs}/_text_file.html.haml (53%) diff --git a/app/views/projects/commits/_diff_file.html.haml b/app/views/projects/commits/_diff_file.html.haml deleted file mode 100644 index 31208a227ce..00000000000 --- a/app/views/projects/commits/_diff_file.html.haml +++ /dev/null @@ -1,48 +0,0 @@ -- file = project.repository.blob_for_diff(@commit, diff) -- return unless file -- blob_diff_path = diff_project_blob_path(project, - tree_join(@commit.id, diff.new_path)) -.diff-file{id: "diff-#{i}", data: {blob_diff_path: blob_diff_path }} - .diff-header{id: "file-path-#{hexdigest(diff.new_path || diff.old_path)}"} - - if diff.deleted_file - %span= diff.old_path - - .diff-btn-group - - if @commit.parent_ids.present? - = view_file_btn(@commit.parent_id, diff, project) - - else - %span= diff.new_path - - if diff_file_mode_changed?(diff) - %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}" - - .diff-btn-group - %label - = check_box_tag nil, 1, false, class: "js-toggle-diff-line-wrap" - Wrap text -   - = link_to "#", class: "js-toggle-diff-comments btn btn-small" do - %i.icon-chevron-down - Diff comments -   - - - if @merge_request && @merge_request.source_project - = link_to project_edit_tree_path(@merge_request.source_project, tree_join(@merge_request.source_branch, diff.new_path), from_merge_request_id: @merge_request.id), { class: 'btn btn-small' } do - Edit -   - - = view_file_btn(@commit.id, diff, project) - - .diff-content - -# Skipp all non non-supported blobs - - return unless file.respond_to?('text?') - - if file.text? - - if params[:view] == 'parallel' - = render "projects/commits/parallel_view", diff: diff, project: project, file: file, index: i - - else - = render "projects/commits/text_file", diff: diff, index: i - - elsif file.image? - - old_file = project.repository.prev_blob_for_diff(@commit, diff) - = render "projects/commits/image", diff: diff, old_file: old_file, file: file, index: i - - else - .nothing-here-block No preview for this file type - diff --git a/app/views/projects/commits/_diffs.html.haml b/app/views/projects/commits/_diffs.html.haml index 17efa8debe1..056524fc136 100644 --- a/app/views/projects/commits/_diffs.html.haml +++ b/app/views/projects/commits/_diffs.html.haml @@ -12,11 +12,11 @@ = link_to "Inline Diff", url_for(params_copy), {id: "commit-diff-viewtype"} - if show_diff_size_warninig?(diffs) - = render 'projects/commits/diff_warning', diffs: diffs + = render 'projects/diffs/diff_warning', diffs: diffs .files - safe_diff_files(diffs).each_with_index do |diff, i| - = render 'projects/commits/diff_file', diff: diff, i: i, project: project + = render 'projects/diffs/diff_file', diff: diff_file, i: i, project: project - if @diff_timeout .alert.alert-danger diff --git a/app/views/projects/diffs/_diff_file.html.haml b/app/views/projects/diffs/_diff_file.html.haml new file mode 100644 index 00000000000..c79f9dc0149 --- /dev/null +++ b/app/views/projects/diffs/_diff_file.html.haml @@ -0,0 +1,47 @@ +- 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)) +.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 + %span= diff_file.old_path + + .diff-btn-group + - if @commit.parent_ids.present? + = view_file_btn(@commit.parent_id, diff_file, project) + - else + %span= diff_file.new_path + - if diff_file.mode_changed? + %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}" + + .diff-btn-group + %label + = check_box_tag nil, 1, false, class: "js-toggle-diff-line-wrap" + Wrap text +   + = link_to "#", class: "js-toggle-diff-comments btn btn-small" do + %i.icon-chevron-down + Diff comments +   + + - if @merge_request && @merge_request.source_project + = link_to project_edit_tree_path(@merge_request.source_project, tree_join(@merge_request.source_branch, diff_file.new_path), from_merge_request_id: @merge_request.id), { class: 'btn btn-small' } do + Edit +   + + = view_file_btn(@commit.id, diff_file, project) + + .diff-content + -# Skipp all non non-supported blobs + - return unless blob.respond_to?('text?') + - if blob.text? + - if params[: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.image? + - old_file = project.repository.prev_blob_for_diff(@commit, diff_file) + = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, blob: blob, index: i + - else + .nothing-here-block No preview for this file type + diff --git a/app/views/projects/commits/_diff_stats.html.haml b/app/views/projects/diffs/_diff_stats.html.haml similarity index 100% rename from app/views/projects/commits/_diff_stats.html.haml rename to app/views/projects/diffs/_diff_stats.html.haml diff --git a/app/views/projects/commits/_diff_warning.html.haml b/app/views/projects/diffs/_diff_warning.html.haml similarity index 100% rename from app/views/projects/commits/_diff_warning.html.haml rename to app/views/projects/diffs/_diff_warning.html.haml diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml new file mode 100644 index 00000000000..80a6d8a5691 --- /dev/null +++ b/app/views/projects/diffs/_diffs.html.haml @@ -0,0 +1,26 @@ +.row + .col-md-8 + = render 'projects/diffs/diff_stats', diffs: diffs + .col-md-4 + %ul.nav.nav-tabs + %li.pull-right{class: params[:view] == 'parallel' ? 'active' : ''} + - params_copy = params.dup + - params_copy[:view] = 'parallel' + = link_to "Side-by-side Diff", url_for(params_copy), {id: "commit-diff-viewtype"} + %li.pull-right{class: params[:view] != 'parallel' ? 'active' : ''} + - 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 + +.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 + +- if @diff_timeout + .alert.alert-danger + %h4 + Failed to collect changes + %p + Maybe diff is really big and operation failed with timeout. Try to get diff localy diff --git a/app/views/projects/commits/_image.html.haml b/app/views/projects/diffs/_image.html.haml similarity index 100% rename from app/views/projects/commits/_image.html.haml rename to app/views/projects/diffs/_image.html.haml diff --git a/app/views/projects/commits/diffs/_match_line.html.haml b/app/views/projects/diffs/_match_line.html.haml similarity index 100% rename from app/views/projects/commits/diffs/_match_line.html.haml rename to app/views/projects/diffs/_match_line.html.haml diff --git a/app/views/projects/commits/diffs/_match_line_parallel.html.haml b/app/views/projects/diffs/_match_line_parallel.html.haml similarity index 100% rename from app/views/projects/commits/diffs/_match_line_parallel.html.haml rename to app/views/projects/diffs/_match_line_parallel.html.haml diff --git a/app/views/projects/commits/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml similarity index 82% rename from app/views/projects/commits/_parallel_view.html.haml rename to app/views/projects/diffs/_parallel_view.html.haml index dec417bb21f..e7c0a5a8e58 100644 --- a/app/views/projects/commits/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,7 +1,7 @@ / Side-by-side diff view %div.text-file %table - - parallel_diff(diff, index).each do |line| + - parallel_diff(diff_file, index).each do |line| - type_left = line[0] - line_number_left = line[1] - line_content_left = line[2] @@ -11,7 +11,7 @@ %tr.line_holder.parallel - if type_left == 'match' - = render "projects/commits/diffs/match_line_parallel", { line: line_content_left, + = render "projects/diffs/match_line_parallel", { line: line_content_left, line_old: line_number_left, line_new: line_number_right } - elsif type_left == 'old' || type_left.nil? %td.old_line{class: "#{type_left}"} @@ -21,6 +21,6 @@ = link_to raw(line_number_right) %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil}"}= raw line_content_right -- if diff.diff.blank? && diff_file_mode_changed?(diff) +- if diff_file.diff.diff.blank? && diff_file.mode_changed? .file-mode-changed File mode changed diff --git a/app/views/projects/commits/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml similarity index 53% rename from app/views/projects/commits/_text_file.html.haml rename to app/views/projects/diffs/_text_file.html.haml index 756481c1b21..43be43cc6e9 100644 --- a/app/views/projects/commits/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -1,33 +1,36 @@ -- too_big = diff.diff.lines.count > Commit::DIFF_SAFE_LINES +- too_big = diff_file.diff_lines.count > Commit::DIFF_SAFE_LINES - if too_big %a.supp_diff_link Changes suppressed. Click to show %table.text-file{class: "#{'hide' if too_big}"} - last_line = 0 - - each_diff_line(diff, index) do |line, type, line_code, line_new, line_old, raw_line| - - last_line = line_new + - diff_file.diff_lines.each_with_index do |line, index| + - type = line.type + - last_line = line.new_pos + - line_code = line.code + - line_old = line.old_pos %tr.line_holder{ id: line_code, class: "#{type}" } - if type == "match" - = render "projects/commits/diffs/match_line", {line: line, - line_old: line_old, line_new: line_new, bottom: false} + = render "projects/diffs/match_line", {line: line.text, + line_old: line_old, line_new: line.new_pos, bottom: false} - else %td.old_line = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code - if @comments_allowed = link_to_new_diff_note(line_code) - %td.new_line{data: {linenumber: line_new}} - = 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) + %td.new_line{data: {linenumber: line.new_pos}} + = link_to raw(type == "old" ? " " : line.new_pos) , "##{line_code}", id: line_code + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line.text) - 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, line: line + = render "projects/notes/diff_notes_with_reply", notes: comments, line: line.text - if last_line > 0 - = render "projects/commits/diffs/match_line", {line: "", + = render "projects/diffs/match_line", {line: "", line_old: last_line, line_new: last_line, bottom: true} -- if diff.diff.blank? && diff_file_mode_changed?(diff) +- if diff_file.diff.blank? && diff_file_mode_changed?(diff) .file-mode-changed File mode changed From 531f16beb0a860a94f732f9e697a447513abe363 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 16:27:12 +0300 Subject: [PATCH 14/23] Use new diff parsing logic Signed-off-by: Dmitriy Zaporozhets --- .../projects/edit_tree_controller.rb | 2 +- app/helpers/commits_helper.rb | 61 ++++--------- app/helpers/diff_helper.rb | 8 +- app/models/note.rb | 39 ++++++-- app/views/projects/commit/show.html.haml | 2 +- .../projects/edit_tree/preview.html.haml | 16 ++-- .../merge_requests/show/_diffs.html.haml | 2 +- .../notes/discussions/_diff.html.haml | 16 ++-- lib/gitlab/diff_parser.rb | 88 ------------------- 9 files changed, 72 insertions(+), 162 deletions(-) delete mode 100644 lib/gitlab/diff_parser.rb diff --git a/app/controllers/projects/edit_tree_controller.rb b/app/controllers/projects/edit_tree_controller.rb index ca83b21f429..baae12d92df 100644 --- a/app/controllers/projects/edit_tree_controller.rb +++ b/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 = Gitlab::DiffParser.new(diffy.diff.scan(/.*\n/)) + @diff_lines = Gitlab::Diff::Parser.new.parse(diffy.diff.scan(/.*\n/), @path, @path) render layout: false end diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 7d5b9c3238f..49226a37d08 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -16,21 +16,7 @@ module CommitsHelper commit_person_link(commit, options.merge(source: :committer)) end - def each_diff_line(diff, index) - Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) - .each do |full_line, type, line_code, line_new, line_old| - yield(full_line, type, line_code, line_new, line_old) - end - end - - def parallel_diff_line(diff, index) - Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) - .each do |full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line| - yield(full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line) - end - end - - def parallel_diff(diff, index) + def parallel_diff(diff_file, index) lines = [] skip_next = false @@ -38,7 +24,21 @@ module CommitsHelper # # [left_type, left_line_number, left_line_content, right_line_type, right_line_number, right_line_content] # - parallel_diff_line(diff, index) do |full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line| + 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 @@ -72,31 +72,6 @@ module CommitsHelper lines end - def each_diff_line_near(diff, index, expected_line_code) - max_number_of_lines = 16 - - prev_match_line = nil - prev_lines = [] - - each_diff_line(diff, index) do |full_line, type, line_code, line_new, line_old| - line = [full_line, type, line_code, line_new, line_old] - if line_code != expected_line_code - if type == "match" - prev_lines.clear - prev_match_line = line - else - prev_lines.push(line) - prev_lines.shift if prev_lines.length >= max_number_of_lines - end - else - yield(prev_match_line) if !prev_match_line.nil? - prev_lines.each { |ln| yield(ln) } - yield(line) - break - end - end - end - def image_diff_class(diff) if diff.deleted_file "deleted" @@ -202,10 +177,6 @@ module CommitsHelper end end - def diff_file_mode_changed?(diff) - diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode - end - def unfold_bottom_class(bottom) (bottom) ? 'js-unfold-bottom' : '' end diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index ee4d4fbdff5..7feb07eeb3b 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,14 +1,16 @@ module DiffHelper - def safe_diff_files(diffs) + def safe_diff_files(project, 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) end end - def show_diff_size_warninig?(diffs) - safe_diff_files(diffs).size < diffs.size + def show_diff_size_warninig?(project, diffs) + safe_diff_files(project, diffs).size < diffs.size end def diff_hard_limit_enabled? diff --git a/app/models/note.rb b/app/models/note.rb index 7cbab1130ea..77e3a528f96 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -209,9 +209,10 @@ class Note < ActiveRecord::Base noteable.diffs.each do |mr_diff| next unless mr_diff.new_path == self.diff.new_path - Gitlab::DiffParser.new(mr_diff.diff.lines.to_a, mr_diff.new_path). - each do |full_line, type, line_code, line_new, line_old| - if full_line == diff_line + lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a, mr_diff.old_path, mr_diff.new_path) + + lines.each do |line| + if line.text == diff_line return true end end @@ -244,15 +245,39 @@ class Note < ActiveRecord::Base return @diff_line if @diff_line if diff - Gitlab::DiffParser.new(diff.diff.lines.to_a, diff.new_path) - .each do |full_line, type, line_code, line_new, line_old| - @diff_line = full_line if line_code == self.line_code - end + diff_lines.each do |line| + @diff_line = line.text if line.code == self.line_code + end end @diff_line end + def truncated_diff_lines + max_number_of_lines = 16 + prev_match_line = nil + prev_lines = [] + + diff_lines.each do |line| + if line.code != self.line_code + if line.type == "match" + prev_lines.clear + prev_match_line = line + else + prev_lines.push(line) + prev_lines.shift if prev_lines.length >= max_number_of_lines + end + else + prev_lines << line + return prev_lines + end + end + end + + def diff_lines + @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.lines.to_a, diff.old_path, diff.new_path) + end + def discussion_id @discussion_id ||= Note.build_discussion_id(noteable_type, noteable_id || commit_id, line_code) end diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 0a15aef6cb7..fc721067ed4 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -1,3 +1,3 @@ = render "commit_box" -= render "projects/commits/diffs", diffs: @diffs, project: @project += render "projects/diffs/diffs", diffs: @diffs, project: @project = render "projects/notes/notes_with_form" diff --git a/app/views/projects/edit_tree/preview.html.haml b/app/views/projects/edit_tree/preview.html.haml index 87ce5dc31d3..f3fd94b0a3f 100644 --- a/app/views/projects/edit_tree/preview.html.haml +++ b/app/views/projects/edit_tree/preview.html.haml @@ -9,18 +9,18 @@ = raw render_markup(@blob.name, @content) - else .file-content.code - - unless @diff.empty? + - unless @diff_lines.empty? %table.text-file - - @diff.each do |line, type, line_code, line_new, line_old, raw_line| - %tr.line_holder{ id: line_code, class: "#{type}" } - - if type == "match" + - @diff_lines.each do |line| + %tr.line_holder{ id: line.code, class: "#{line.type}" } + - if line.type == "match" %td.old_line= "..." %td.new_line= "..." - %td.line_content.matched= line + %td.line_content.matched= line.text - 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) + = 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) - else .nothing-here-block No changes. diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index eb63b68106e..d361c5f579a 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,5 +1,5 @@ - if @merge_request_diff.collected? - = render "projects/commits/diffs", diffs: @merge_request.diffs, project: @merge_request.source_project + = render "projects/diffs/diffs", diffs: @merge_request.diffs, project: @merge_request.source_project - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} - else diff --git a/app/views/projects/notes/discussions/_diff.html.haml b/app/views/projects/notes/discussions/_diff.html.haml index 26c5494f466..228af785f73 100644 --- a/app/views/projects/notes/discussions/_diff.html.haml +++ b/app/views/projects/notes/discussions/_diff.html.haml @@ -11,16 +11,16 @@ %br/ .diff-content %table - - each_diff_line_near(diff, note.diff_file_index, note.line_code) do |line, type, line_code, line_new, line_old| - %tr.line_holder{ id: line_code } - - if type == "match" + - note.truncated_diff_lines.each do |line| + %tr.line_holder{ id: line.code } + - if line.type == "match" %td.old_line= "..." %td.new_line= "..." - %td.line_content.matched= line + %td.line_content.matched= line.text - else - %td.old_line= raw(type == "new" ? " " : line_old) - %td.new_line= raw(type == "old" ? " " : line_new) - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw "#{line}  " + %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}  " - - if line_code == note.line_code + - if line.code == note.line_code = render "projects/notes/diff_notes_with_reply", notes: discussion_notes diff --git a/lib/gitlab/diff_parser.rb b/lib/gitlab/diff_parser.rb deleted file mode 100644 index 3f402c4c238..00000000000 --- a/lib/gitlab/diff_parser.rb +++ /dev/null @@ -1,88 +0,0 @@ -module Gitlab - class DiffParser - include Enumerable - - attr_reader :lines, :new_path - - def initialize(lines, new_path = '') - @lines = lines - @new_path = new_path - end - - def each - line_old = 1 - line_new = 1 - type = nil - - lines_arr = ::Gitlab::InlineDiff.processing lines - - lines_arr.each_cons(2) do |line, next_line| - raw_line = line.dup - - next if filename?(line) - - full_line = html_escape(line.gsub(/\n/, '')) - full_line = ::Gitlab::InlineDiff.replace_markers full_line - - next_line = html_escape(next_line.gsub(/\n/, '')) - next_line = ::Gitlab::InlineDiff.replace_markers next_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, line_new, line_old) - next - else - type = identification_type(line) - next_type = identification_type(next_line) - line_code = generate_line_code(new_path, line_new, line_old) - yield(full_line, type, line_code, line_new, line_old, raw_line, next_type, next_line) - end - - - if line[0] == "+" - line_new += 1 - elsif line[0] == "-" - line_old += 1 - else - line_new += 1 - line_old += 1 - end - end - end - - def empty? - @lines.empty? - end - - private - - def filename?(line) - line.start_with?('--- /dev/null', '+++ /dev/null', '--- a', '+++ b', - '--- /tmp/diffy', '+++ /tmp/diffy') - end - - 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 = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } - str.gsub(/[&"'><]/, replacements) - end - end -end From c741fcab9d8f1a28b2b95e3cd2096adc5178eba6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 20:41:55 +0300 Subject: [PATCH 15/23] Fix all broken stuff after diff refactoring Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/commits/_diffs.html.haml | 26 ------------------- app/views/projects/compare/show.html.haml | 2 +- app/views/projects/diffs/_diff_file.html.haml | 2 +- .../projects/diffs/_diff_warning.html.haml | 2 +- app/views/projects/diffs/_image.html.haml | 1 + .../merge_requests/_new_submit.html.haml | 2 +- 6 files changed, 5 insertions(+), 30 deletions(-) delete mode 100644 app/views/projects/commits/_diffs.html.haml diff --git a/app/views/projects/commits/_diffs.html.haml b/app/views/projects/commits/_diffs.html.haml deleted file mode 100644 index 056524fc136..00000000000 --- a/app/views/projects/commits/_diffs.html.haml +++ /dev/null @@ -1,26 +0,0 @@ -.row - .col-md-8 - = render 'projects/commits/diff_stats', diffs: diffs - .col-md-4 - %ul.nav.nav-tabs - %li.pull-right{class: params[:view] == 'parallel' ? 'active' : ''} - - params_copy = params.dup - - params_copy[:view] = 'parallel' - = link_to "Side-by-side Diff", url_for(params_copy), {id: "commit-diff-viewtype"} - %li.pull-right{class: params[:view] != 'parallel' ? 'active' : ''} - - params_copy[:view] = 'inline' - = link_to "Inline Diff", url_for(params_copy), {id: "commit-diff-viewtype"} - -- if show_diff_size_warninig?(diffs) - = render 'projects/diffs/diff_warning', diffs: diffs - -.files - - safe_diff_files(diffs).each_with_index do |diff, i| - = render 'projects/diffs/diff_file', diff: diff_file, i: i, project: project - -- if @diff_timeout - .alert.alert-danger - %h4 - Failed to collect changes - %p - Maybe diff is really big and operation failed with timeout. Try to get diff localy diff --git a/app/views/projects/compare/show.html.haml b/app/views/projects/compare/show.html.haml index aa79d08509b..45269e662cd 100644 --- a/app/views/projects/compare/show.html.haml +++ b/app/views/projects/compare/show.html.haml @@ -18,7 +18,7 @@ - else %ul.well-list= render Commit.decorate(@commits), project: @project - = render "projects/commits/diffs", diffs: @diffs, project: @project + = render "projects/diffs/diffs", diffs: @diffs, project: @project - else .light-well diff --git a/app/views/projects/diffs/_diff_file.html.haml b/app/views/projects/diffs/_diff_file.html.haml index c79f9dc0149..aa68becc4dd 100644 --- a/app/views/projects/diffs/_diff_file.html.haml +++ b/app/views/projects/diffs/_diff_file.html.haml @@ -41,7 +41,7 @@ = render "projects/diffs/text_file", diff_file: diff_file, index: i - elsif blob.image? - old_file = project.repository.prev_blob_for_diff(@commit, diff_file) - = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, blob: blob, index: i + = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i - else .nothing-here-block No preview for this file type diff --git a/app/views/projects/diffs/_diff_warning.html.haml b/app/views/projects/diffs/_diff_warning.html.haml index 05d516efa11..ee85956d876 100644 --- a/app/views/projects/diffs/_diff_warning.html.haml +++ b/app/views/projects/diffs/_diff_warning.html.haml @@ -14,6 +14,6 @@ = link_to "Email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "btn btn-warning btn-small" %p To preserve performance only - %strong #{safe_diff_files(diffs).size} of #{diffs.size} + %strong #{safe_diff_files(@project, diffs).size} of #{diffs.size} files displayed. diff --git a/app/views/projects/diffs/_image.html.haml b/app/views/projects/diffs/_image.html.haml index 6d9ef5964d9..900646dd0a4 100644 --- a/app/views/projects/diffs/_image.html.haml +++ b/app/views/projects/diffs/_image.html.haml @@ -1,3 +1,4 @@ +- diff = diff_file.diff - if diff.renamed_file || diff.new_file || diff.deleted_file .image %span.wrap diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index dc3f9d592f6..e013fd6d1ce 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -75,7 +75,7 @@ %h4 Changes - if @diffs.present? - = render "projects/commits/diffs", diffs: @diffs, project: @project + = render "projects/diffs/diffs", diffs: @diffs, project: @project - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE .bs-callout.bs-callout-danger %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. From bde3f25d262b13d0139276786fe9d9cba29269b8 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 20:42:12 +0300 Subject: [PATCH 16/23] Specs for diff parser! Yay! Signed-off-by: Dmitriy Zaporozhets --- lib/gitlab/diff/file.rb | 2 +- spec/lib/gitlab/diff/file_spec.rb | 25 ++++++++ spec/lib/gitlab/diff/parser_spec.rb | 95 +++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 spec/lib/gitlab/diff/file_spec.rb create mode 100644 spec/lib/gitlab/diff/parser_spec.rb diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index adc78616f64..62c0d38884a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -21,7 +21,7 @@ module Gitlab end def mode_changed? - diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode + !!(diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode) end def parser diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb new file mode 100644 index 00000000000..074c1255930 --- /dev/null +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -0,0 +1,25 @@ +require 'spec_helper' + +describe Gitlab::Diff::File do + include RepoHelpers + + 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) } + + describe :diff_lines do + let(:diff_lines) { diff_file.diff_lines } + + it { diff_lines.size.should == 30 } + 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 +end diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb new file mode 100644 index 00000000000..9ec906e4f9a --- /dev/null +++ b/spec/lib/gitlab/diff/parser_spec.rb @@ -0,0 +1,95 @@ +require 'spec_helper' + +describe Gitlab::Diff::Parser do + include RepoHelpers + + let(:project) { create(:project) } + let(:commit) { project.repository.commit(sample_commit.id) } + let(:diff) { commit.diffs.first } + let(:parser) { Gitlab::Diff::Parser.new } + + describe :parse do + let(:diff) do + < path } +- options = { chdir: path } ++ ++ vars = { ++ "PWD" => path ++ } ++ ++ options = { ++ chdir: path ++ } + + unless File.directory?(path) + FileUtils.mkdir_p(path) +@@ -19,6 +25,7 @@ module Popen + + @cmd_output = "" + @cmd_status = 0 ++ + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + @cmd_output << stdout.read + @cmd_output << stderr.read +eos + end + + let(:path) { 'files/ruby/popen.rb' } + + before do + @lines = parser.parse(diff.lines, path, path) + end + + it { @lines.size.should == 30 } + + describe 'lines' do + describe 'first line' do + let(:line) { @lines.first } + + it { line.type.should == 'match' } + it { line.old_pos.should == 6 } + it { line.new_pos.should == 6 } + it { line.text.should == '@@ -6,12 +6,18 @@ module Popen' } + end + + describe 'removal line' do + let(:line) { @lines[10] } + + it { line.type.should == 'old' } + it { line.old_pos.should == 14 } + it { line.new_pos.should == 13 } + it { line.text.should == '- options = { chdir: path }' } + end + + describe 'addition line' do + let(:line) { @lines[16] } + + it { line.type.should == 'new' } + it { line.old_pos.should == 15 } + it { line.new_pos.should == 18 } + it { line.text.should == '+ options = {' } + end + + describe 'unchanged line' do + let(:line) { @lines.last } + + it { line.type.should == nil } + it { line.old_pos.should == 24 } + it { line.new_pos.should == 31 } + it { line.text.should == ' @cmd_output << stderr.read' } + end + end + end +end From 218219abbdfdc3bc0bc1a9c95cfc0e0ddb5861dd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 21:54:52 +0300 Subject: [PATCH 17/23] Refactoring inside refactoring. We need to go deeper Signed-off-by: Dmitriy Zaporozhets --- .../projects/edit_tree_controller.rb | 2 +- app/helpers/commits_helper.rb | 56 --------------- app/helpers/diff_helper.rb | 69 +++++++++++++++++-- app/models/note.rb | 22 ++++-- app/views/projects/diffs/_diffs.html.haml | 10 +-- .../{_diff_file.html.haml => _file.html.haml} | 6 +- ..._diff_stats.html.haml => _stats.html.haml} | 0 app/views/projects/diffs/_text_file.html.haml | 2 +- ...f_warning.html.haml => _warning.html.haml} | 0 app/views/projects/edit_tree/_diff.html.haml | 13 ---- .../projects/edit_tree/preview.html.haml | 7 +- .../projects/notes/_diff_note_link.html.haml | 10 --- .../notes/discussions/_diff.html.haml | 7 +- lib/gitlab/diff/file.rb | 23 ++++--- lib/gitlab/diff/line.rb | 6 +- lib/gitlab/diff/line_code.rb | 9 +++ lib/gitlab/diff/parser.rb | 9 +-- spec/lib/gitlab/diff/file_spec.rb | 6 +- spec/lib/gitlab/diff/parser_spec.rb | 4 +- 19 files changed, 131 insertions(+), 130 deletions(-) rename app/views/projects/diffs/{_diff_file.html.haml => _file.html.haml} (93%) rename app/views/projects/diffs/{_diff_stats.html.haml => _stats.html.haml} (100%) rename app/views/projects/diffs/{_diff_warning.html.haml => _warning.html.haml} (100%) delete mode 100644 app/views/projects/edit_tree/_diff.html.haml delete mode 100644 app/views/projects/notes/_diff_note_link.html.haml create mode 100644 lib/gitlab/diff/line_code.rb diff --git a/app/controllers/projects/edit_tree_controller.rb b/app/controllers/projects/edit_tree_controller.rb index baae12d92df..72a41f771c0 100644 --- a/app/controllers/projects/edit_tree_controller.rb +++ b/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 diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 49226a37d08..90829963e84 100644 --- a/app/helpers/commits_helper.rb +++ b/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" diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 7feb07eeb3b..c2a19e4ac10 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,16 +1,16 @@ 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(" ") # 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 + end diff --git a/app/models/note.rb b/app/models/note.rb index 77e3a528f96..fa5fdea4eb0 100644 --- a/app/models/note.rb +++ b/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 diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 80a6d8a5691..c4eb7815866 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,6 +1,6 @@ .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 diff --git a/app/views/projects/diffs/_diff_file.html.haml b/app/views/projects/diffs/_file.html.haml similarity index 93% rename from app/views/projects/diffs/_diff_file.html.haml rename to app/views/projects/diffs/_file.html.haml index aa68becc4dd..be0301e75f8 100644 --- a/app/views/projects/diffs/_diff_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -1,6 +1,6 @@ -- 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 diff --git a/app/views/projects/diffs/_diff_stats.html.haml b/app/views/projects/diffs/_stats.html.haml similarity index 100% rename from app/views/projects/diffs/_diff_stats.html.haml rename to app/views/projects/diffs/_stats.html.haml diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 43be43cc6e9..81f726c8e4f 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/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" diff --git a/app/views/projects/diffs/_diff_warning.html.haml b/app/views/projects/diffs/_warning.html.haml similarity index 100% rename from app/views/projects/diffs/_diff_warning.html.haml rename to app/views/projects/diffs/_warning.html.haml diff --git a/app/views/projects/edit_tree/_diff.html.haml b/app/views/projects/edit_tree/_diff.html.haml deleted file mode 100644 index cf044feb9a4..00000000000 --- a/app/views/projects/edit_tree/_diff.html.haml +++ /dev/null @@ -1,13 +0,0 @@ -%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) - diff --git a/app/views/projects/edit_tree/preview.html.haml b/app/views/projects/edit_tree/preview.html.haml index f3fd94b0a3f..e7c3460ad78 100644 --- a/app/views/projects/edit_tree/preview.html.haml +++ b/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. diff --git a/app/views/projects/notes/_diff_note_link.html.haml b/app/views/projects/notes/_diff_note_link.html.haml deleted file mode 100644 index 377c926a204..00000000000 --- a/app/views/projects/notes/_diff_note_link.html.haml +++ /dev/null @@ -1,10 +0,0 @@ -- 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" diff --git a/app/views/projects/notes/discussions/_diff.html.haml b/app/views/projects/notes/discussions/_diff.html.haml index 228af785f73..da71220af17 100644 --- a/app/views/projects/notes/discussions/_diff.html.haml +++ b/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 diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 62c0d38884a..19a1198c68c 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -1,23 +1,18 @@ 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 diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index e8b9c980a1a..8ac1b15e88a 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -1,10 +1,10 @@ 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 diff --git a/lib/gitlab/diff/line_code.rb b/lib/gitlab/diff/line_code.rb new file mode 100644 index 00000000000..f3578ab3d35 --- /dev/null +++ b/lib/gitlab/diff/line_code.rb @@ -0,0 +1,9 @@ +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 diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 0fd11c69a59..9d6309954a4 100644 --- a/lib/gitlab/diff/parser.rb +++ b/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 = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } str.gsub(/[&"'><]/, replacements) diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 074c1255930..cf0b5c282c1 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index 9ec906e4f9a..35b78260acd 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/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 } From 02e87d9223b8398f6d2f06b2ba57313c2ad384be Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 22:12:54 +0300 Subject: [PATCH 18/23] optimize show_diff_size_warning? Signed-off-by: Dmitriy Zaporozhets --- app/helpers/diff_helper.rb | 14 +++++++++----- app/views/projects/diffs/_diffs.html.haml | 1 + app/views/projects/diffs/_warning.html.haml | 2 +- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index c2a19e4ac10..6307790d4e7 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,16 +1,20 @@ module DiffHelper - def safe_diff_files(diffs) + def allowed_diff_size if diff_hard_limit_enabled? - diffs.first(Commit::DIFF_HARD_LIMIT_FILES) + Commit::DIFF_HARD_LIMIT_FILES else - diffs.first(Commit::DIFF_SAFE_FILES) - end.map do |diff| + Commit::DIFF_SAFE_FILES + end + end + + def safe_diff_files(diffs) + diffs.first(allowed_diff_size).map do |diff| Gitlab::Diff::File.new(diff) end end def show_diff_size_warninig?(diffs) - safe_diff_files(diffs).size < diffs.size + diffs.size > allowed_diff_size end def diff_hard_limit_enabled? diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index c4eb7815866..49b3dc6941a 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -11,6 +11,7 @@ - params_copy[:view] = 'inline' = link_to "Inline Diff", url_for(params_copy), {id: "commit-diff-viewtype"} + - if show_diff_size_warninig?(diffs) = render 'projects/diffs/warning', diffs: diffs diff --git a/app/views/projects/diffs/_warning.html.haml b/app/views/projects/diffs/_warning.html.haml index ee85956d876..86ed6bbeaa2 100644 --- a/app/views/projects/diffs/_warning.html.haml +++ b/app/views/projects/diffs/_warning.html.haml @@ -14,6 +14,6 @@ = link_to "Email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "btn btn-warning btn-small" %p To preserve performance only - %strong #{safe_diff_files(@project, diffs).size} of #{diffs.size} + %strong #{allowed_diff_size} of #{diffs.size} files displayed. From 93dc885530aa80b84a2e39841f82a51a6e62f4a5 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 22:25:20 +0300 Subject: [PATCH 19/23] Fix usage of diff file mode change Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/diffs/_text_file.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 81f726c8e4f..b1c987563f1 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -31,6 +31,6 @@ = render "projects/diffs/match_line", {line: "", line_old: last_line, line_new: last_line, bottom: true} -- if diff_file.diff.blank? && diff_file_mode_changed?(diff) +- if diff_file.diff.blank? && diff_file.mode_changed? .file-mode-changed File mode changed From 9c3935c5b0b6feddf91fd2170b50e5910b2cc932 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Tue, 9 Sep 2014 11:10:40 +0200 Subject: [PATCH 20/23] Use the new path to the partial, move the diff related methods to the new helper. --- app/helpers/commits_helper.rb | 12 ------------ app/helpers/diff_helper.rb | 11 +++++++++++ app/views/projects/blob/diff.html.haml | 4 ++-- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 90829963e84..cab2984a4c4 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -31,14 +31,6 @@ module CommitsHelper escape_javascript(render "projects/commits/#{template}", commit: commit, project: project) unless commit.nil? end - def diff_line_content(line) - if line.blank? - "  " - else - line - end - end - # Breadcrumb links for a Project and, if applicable, a tree path def commits_breadcrumbs return unless @project && @ref @@ -121,10 +113,6 @@ module CommitsHelper end end - def unfold_bottom_class(bottom) - (bottom) ? 'js-unfold-bottom' : '' - end - def view_file_btn(commit_sha, diff, project) link_to project_blob_path(project, tree_join(commit_sha, diff.new_path)), class: 'btn btn-small view-file js-view-file' do diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 6307790d4e7..0f4d6cf4032 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -86,4 +86,15 @@ module DiffHelper lines end + def unfold_bottom_class(bottom) + (bottom) ? 'js-unfold-bottom' : '' + end + + def diff_line_content(line) + if line.blank? + "  " + else + line + end + end end diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index cfb91d6568a..5c79d0ef11f 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/app/views/projects/blob/diff.html.haml @@ -1,7 +1,7 @@ - if @lines.present? - if @form.unfold? && @form.since != 1 && !@form.bottom? %tr.line_holder{ id: @form.since } - = render "projects/commits/diffs/match_line", {line: @match_line, + = render "projects/diffs/match_line", {line: @match_line, line_old: @form.since, line_new: @form.since, bottom: false} - @lines.each_with_index do |line, index| @@ -15,5 +15,5 @@ - if @form.unfold? && @form.bottom? && @form.to < @blob.loc %tr.line_holder{ id: @form.to } - = render "projects/commits/diffs/match_line", {line: @match_line, + = render "projects/diffs/match_line", {line: @match_line, line_old: @form.to, line_new: @form.to, bottom: true} From ac6a107ac7d7350d1df46d84bf831ab3d8bcc91a Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Tue, 9 Sep 2014 11:36:29 +0200 Subject: [PATCH 21/23] Add line code to parallel diff for linking. --- app/helpers/diff_helper.rb | 8 ++++---- .../projects/diffs/_parallel_view.html.haml | 17 +++++++++-------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 0f4d6cf4032..0b49748fa37 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -36,7 +36,7 @@ module DiffHelper # Building array of lines # - # [left_type, left_line_number, left_line_content, right_line_type, right_line_number, right_line_content] + # [left_type, left_line_number, left_line_content, line_code, right_line_type, right_line_number, right_line_content] # diff_file.diff_lines.each do |line| @@ -53,10 +53,10 @@ module DiffHelper next_line = next_line.text end - line = [type, line_old, full_line, next_type, line_new] + line = [type, line_old, full_line, line_code, 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] + line = [type, line_old, full_line, line_code, type, line_new, full_line] lines.push(line) elsif type == 'old' if next_type == 'new' @@ -78,7 +78,7 @@ module DiffHelper next else # Change is only on the right side, left side has no change - line = [nil, nil, " ", type, line_new, full_line] + line = [nil, nil, " ", line_code, type, line_new, full_line] lines.push(line) end end diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index e7c0a5a8e58..47fe77ccf75 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -5,21 +5,22 @@ - type_left = line[0] - line_number_left = line[1] - line_content_left = line[2] - - type_right = line[3] - - line_number_right = line[4] - - line_content_right = line[5] + - line_code = line[3] + - type_right = line[4] + - line_number_right = line[5] + - line_content_right = line[6] - %tr.line_holder.parallel + %tr.line_holder.parallel{id: line_code} - if type_left == 'match' = render "projects/diffs/match_line_parallel", { line: line_content_left, line_old: line_number_left, line_new: line_number_right } - elsif type_left == 'old' || type_left.nil? %td.old_line{class: "#{type_left}"} - = link_to raw(line_number_left) - %td.line_content{class: "parallel noteable_line #{type_left}" }= raw line_content_left + = link_to raw(line_number_left), "##{line_code}", id: line_code + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code}", "line_code" => line_code }= raw line_content_left %td.new_line{ class: "#{type_right == 'new' ? 'new' : nil}", data: { linenumber: line_number_right }} - = link_to raw(line_number_right) - %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil}"}= raw line_content_right + = link_to raw(line_number_right), "##{line_code}", id: line_code + %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil} #{line_code}", "line_code" => line_code}= raw line_content_right - if diff_file.diff.diff.blank? && diff_file.mode_changed? .file-mode-changed From 2ff83aa389894c1dc306f1bf3d2d4aebe25eff62 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Tue, 9 Sep 2014 11:52:16 +0200 Subject: [PATCH 22/23] Fix typo. --- app/helpers/diff_helper.rb | 2 +- app/views/projects/diffs/_diffs.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 0b49748fa37..afe7447d4e2 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -13,7 +13,7 @@ module DiffHelper end end - def show_diff_size_warninig?(diffs) + def show_diff_size_warning?(diffs) diffs.size > allowed_diff_size end diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 49b3dc6941a..2d7ecdc3809 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -12,7 +12,7 @@ = link_to "Inline Diff", url_for(params_copy), {id: "commit-diff-viewtype"} -- if show_diff_size_warninig?(diffs) +- if show_diff_size_warning?(diffs) = render 'projects/diffs/warning', diffs: diffs .files From 8ebb26fcc1eb25cc5613be6954c5ca43b3125435 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Tue, 9 Sep 2014 13:17:42 +0200 Subject: [PATCH 23/23] Add diff_helper spec. --- spec/helpers/diff_helper_spec.rb | 98 ++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 spec/helpers/diff_helper_spec.rb diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb new file mode 100644 index 00000000000..4ab415b4ef3 --- /dev/null +++ b/spec/helpers/diff_helper_spec.rb @@ -0,0 +1,98 @@ +require 'spec_helper' + +describe DiffHelper do + include RepoHelpers + + let(:project) { create(:project) } + let(:commit) { project.repository.commit(sample_commit.id) } + let(:diff) { commit.diffs.first } + let(:diff_file) { Gitlab::Diff::File.new(diff) } + + describe 'diff_hard_limit_enabled?' do + it 'should return true if param is provided' do + controller.stub(:params).and_return { { :force_show_diff => true } } + diff_hard_limit_enabled?.should be_true + end + + it 'should return false if param is not provided' do + diff_hard_limit_enabled?.should be_false + end + end + + describe 'allowed_diff_size' do + it 'should return hard limit for a diff if force diff is true' do + controller.stub(:params).and_return { { :force_show_diff => true } } + allowed_diff_size.should eq(1000) + end + + it 'should return safe limit for a diff if force diff is false' do + allowed_diff_size.should eq(100) + end + end + + describe 'parallel_diff' do + it 'should return an array of arrays containing the parsed diff' do + parallel_diff(diff_file, 0).should match_array(parallel_diff_result_array) + end + end + + describe 'generate_line_code' do + it 'should generate correct line code' do + generate_line_code(diff_file.file_path, diff_file.diff_lines.first).should == '2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6' + end + end + + describe 'unfold_bottom_class' do + it 'should return empty string when bottom line shouldnt be unfolded' do + unfold_bottom_class(false).should == '' + end + + it 'should return js class when bottom lines should be unfolded' do + unfold_bottom_class(true).should == 'js-unfold-bottom' + end + end + + describe 'diff_line_content' do + + it 'should return non breaking space when line is empty' do + diff_line_content(nil).should eq("  ") + end + + it 'should return the line itself' do + diff_line_content(diff_file.diff_lines.first.text).should eq("@@ -6,12 +6,18 @@ module Popen") + diff_line_content(diff_file.diff_lines.first.type).should eq("match") + diff_line_content(diff_file.diff_lines.first.new_pos).should eq(6) + end + end + + def parallel_diff_result_array + [ + ["match", 6, "@@ -6,12 +6,18 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", "match", 6, "@@ -6,12 +6,18 @@ module Popen"], + [nil, 6, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", nil, 6, " "], + [nil, 7, " def popen(cmd, path=nil)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7", nil, 7, " def popen(cmd, path=nil)"], + [nil, 8, " unless cmd.is_a?(Array)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8", nil, 8, " unless cmd.is_a?(Array)"], + ["old", 9, "- raise "System commands must be given as an array of strings"", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9", "new", 9, "+ raise RuntimeError, "System commands must be given as an array of strings""], + [nil, 10, " end", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10", nil, 10, " end"], [nil, 11, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11", nil, 11, " "], + [nil, 12, " path ||= Dir.pwd", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12", nil, 12, " path ||= Dir.pwd"], + ["old", 13, "- vars = { "PWD" => path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13", "old", nil, " "], + ["old", 14, "- options = { chdir: path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13", "new", 13, "+"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14", "new", 14, "+ vars = {"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15", "new", 15, "+ "PWD" => path"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16", "new", 16, "+ }"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17", "new", 17, "+"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18", "new", 18, "+ options = {"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19", "new", 19, "+ chdir: path"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20", "new", 20, "+ }"], + [nil, 15, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21", nil, 21, " "], + [nil, 16, " unless File.directory?(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22", nil, 22, " unless File.directory?(path)"], + [nil, 17, " FileUtils.mkdir_p(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23", nil, 23, " FileUtils.mkdir_p(path)"], + ["match", 19, "@@ -19,6 +25,7 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", "match", 25, "@@ -19,6 +25,7 @@ module Popen"], + [nil, 19, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", nil, 25, " "], [nil, 20, " @cmd_output = """, "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26", nil, 26, " @cmd_output = """], + [nil, 21, " @cmd_status = 0", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27", nil, 27, " @cmd_status = 0"], + [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28", "new", 28, "+"], + [nil, 22, " Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29", nil, 29, " Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|"], + [nil, 23, " @cmd_output << stdout.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30", nil, 30, " @cmd_output << stdout.read"], + [nil, 24, " @cmd_output << stderr.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31", nil, 31, " @cmd_output << stderr.read"] + ] + end +end