Commit f4673919 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Adjust insufficient diff hunks being persisted on NoteDiffFile

This currently causes 500's errors when loading the MR page
(Discussion) in a few scenarios.

We were not considering detailed diff headers such as
"--- a/doc/update/mysql_to_postgresql.md\n+++ b/doc/update/mysql_to_postgresql.md"
to crop the diff. In order to address it, we're now using
Gitlab::Diff::Parser, clean the diffs and builds Gitlab::Diff::Line objects
we can iterate and filter on.
parent 9c296194
---
title: Adjust insufficient diff hunks being persisted on NoteDiffFile
merge_request:
author:
type: fixed
...@@ -78,9 +78,12 @@ module Gitlab ...@@ -78,9 +78,12 @@ module Gitlab
# Returns the raw diff content up to the given line index # Returns the raw diff content up to the given line index
def diff_hunk(diff_line) def diff_hunk(diff_line)
# Adding 2 because of the @@ diff header and Enum#take should consider diff_line_index = diff_line.index
# an extra line, because we're passing an index. # @@ (match) header is not kept if it's found in the top of the file,
raw_diff.each_line.take(diff_line.index + 2).join # therefore we should keep an extra line on this scenario.
diff_line_index += 1 unless diff_lines.first.match?
diff_lines.select { |line| line.index <= diff_line_index }.map(&:text).join("\n")
end end
def old_sha def old_sha
......
...@@ -53,6 +53,10 @@ module Gitlab ...@@ -53,6 +53,10 @@ module Gitlab
%w[match new-nonewline old-nonewline].include?(type) %w[match new-nonewline old-nonewline].include?(type)
end end
def match?
type == :match
end
def discussable? def discussable?
!meta? !meta?
end end
......
...@@ -470,56 +470,69 @@ describe Gitlab::Diff::File do ...@@ -470,56 +470,69 @@ describe Gitlab::Diff::File do
end end
describe '#diff_hunk' do describe '#diff_hunk' do
context 'when first line is a match' do
let(:raw_diff) do let(:raw_diff) do
<<EOS <<~EOS
@@ -6,12 +6,18 @@ module Popen --- a/files/ruby/popen.rb
+++ b/files/ruby/popen.rb
@@ -6,12 +6,18 @@ module Popen
def popen(cmd, path=nil) def popen(cmd, path=nil)
unless cmd.is_a?(Array) unless cmd.is_a?(Array)
- raise "System commands must be given as an array of strings" - raise "System commands must be given as an array of strings"
+ raise RuntimeError, "System commands must be given as an array of strings" + raise RuntimeError, "System commands must be given as an array of strings"
end end
EOS
path ||= Dir.pwd
- vars = { "PWD" => 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 end
it 'returns raw diff up to given line index' do it 'returns raw diff up to given line index' do
allow(diff_file).to receive(:raw_diff) { raw_diff } allow(diff_file).to receive(:raw_diff) { raw_diff }
diff_line = instance_double(Gitlab::Diff::Line, index: 5) diff_line = instance_double(Gitlab::Diff::Line, index: 4)
diff_hunk = <<EOS diff_hunk = <<~EOS
@@ -6,12 +6,18 @@ module Popen @@ -6,12 +6,18 @@ module Popen
def popen(cmd, path=nil) def popen(cmd, path=nil)
unless cmd.is_a?(Array) unless cmd.is_a?(Array)
- raise "System commands must be given as an array of strings" - raise "System commands must be given as an array of strings"
+ raise RuntimeError, "System commands must be given as an array of strings" + raise RuntimeError, "System commands must be given as an array of strings"
EOS
expect(diff_file.diff_hunk(diff_line)).to eq(diff_hunk.strip)
end
end
context 'when first line is not a match' do
let(:raw_diff) do
<<~EOS
@@ -1,4 +1,4 @@
-Copyright (c) 2011-2017 GitLab B.V.
+Copyright (c) 2011-2019 GitLab B.V.
With regard to the GitLab Software:
@@ -9,17 +9,21 @@ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:
EOS
end end
EOS
expect(diff_file.diff_hunk(diff_line)).to eq(diff_hunk) it 'returns raw diff up to given line index' do
allow(diff_file).to receive(:raw_diff) { raw_diff }
diff_line = instance_double(Gitlab::Diff::Line, index: 5)
diff_hunk = <<~EOS
-Copyright (c) 2011-2017 GitLab B.V.
+Copyright (c) 2011-2019 GitLab B.V.
With regard to the GitLab Software:
@@ -9,17 +9,21 @@ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
EOS
expect(diff_file.diff_hunk(diff_line)).to eq(diff_hunk.strip)
end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment