From f46739191a71d881501bb3fc4740b953824a9fb3 Mon Sep 17 00:00:00 2001
From: Oswaldo Ferreira <oswaldo@gitlab.com>
Date: Mon, 4 Jun 2018 19:20:58 -0300
Subject: 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.
---
 ...gnore-diff-header-when-persisting-diff-hunk.yml |   5 +
 lib/gitlab/diff/file.rb                            |   9 +-
 lib/gitlab/diff/line.rb                            |   4 +
 spec/lib/gitlab/diff/file_spec.rb                  | 113 ++++++++++++---------
 4 files changed, 78 insertions(+), 53 deletions(-)
 create mode 100644 changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml

diff --git a/changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml b/changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml
new file mode 100644
index 00000000000..ef66deaa0ef
--- /dev/null
+++ b/changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml
@@ -0,0 +1,5 @@
+---
+title: Adjust insufficient diff hunks being persisted on NoteDiffFile
+merge_request:
+author:
+type: fixed
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 765fb0289a8..2820293ad5c 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -78,9 +78,12 @@ module Gitlab
 
       # Returns the raw diff content up to the given line index
       def diff_hunk(diff_line)
-        # Adding 2 because of the @@ diff header and Enum#take should consider
-        # an extra line, because we're passing an index.
-        raw_diff.each_line.take(diff_line.index + 2).join
+        diff_line_index = diff_line.index
+        # @@ (match) header is not kept if it's found in the top of the file,
+        # 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
 
       def old_sha
diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb
index 0603141e441..a1e904cfef4 100644
--- a/lib/gitlab/diff/line.rb
+++ b/lib/gitlab/diff/line.rb
@@ -53,6 +53,10 @@ module Gitlab
         %w[match new-nonewline old-nonewline].include?(type)
       end
 
+      def match?
+        type == :match
+      end
+
       def discussable?
         !meta?
       end
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index 0588fe935c3..f0e83ccfc7a 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -470,56 +470,69 @@ describe Gitlab::Diff::File do
   end
 
   describe '#diff_hunk' do
-    let(:raw_diff) do
-      <<EOS
-@@ -6,12 +6,18 @@ module Popen
-
-   def popen(cmd, path=nil)
-     unless cmd.is_a?(Array)
--      raise "System commands must be given as an array of strings"
-+      raise RuntimeError, "System commands must be given as an array of strings"
-     end
-
-     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
-
-    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
-@@ -6,12 +6,18 @@ module Popen
-
-   def popen(cmd, path=nil)
-     unless cmd.is_a?(Array)
--      raise "System commands must be given as an array of strings"
-+      raise RuntimeError, "System commands must be given as an array of strings"
-     end
-EOS
-
-      expect(diff_file.diff_hunk(diff_line)).to eq(diff_hunk)
+    context 'when first line is a match' do
+      let(:raw_diff) do
+        <<~EOS
+          --- a/files/ruby/popen.rb
+          +++ b/files/ruby/popen.rb
+          @@ -6,12 +6,18 @@ module Popen
+
+             def popen(cmd, path=nil)
+               unless cmd.is_a?(Array)
+          -      raise "System commands must be given as an array of strings"
+          +      raise RuntimeError, "System commands must be given as an array of strings"
+               end
+        EOS
+      end
+
+      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: 4)
+
+        diff_hunk = <<~EOS
+          @@ -6,12 +6,18 @@ module Popen
+
+             def popen(cmd, path=nil)
+               unless cmd.is_a?(Array)
+          -      raise "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
+
+      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
-- 
cgit v1.2.1