Merge branch 'merge-request-diffs-table' into 'master'

Add table for files in merge request diffs

See merge request !12047

app/models/merge_request_diff.rb

@@ -10,6 +10,7 @@ class MergeRequestDiff < ActiveRecord::Base
   VALID_CLASSES = [Hash, Rugged::Patch, Rugged::Diff::Delta].freeze
 
   belongs_to :merge_request
+  has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) }
 
   serialize :st_commits # rubocop:disable Cop/ActiverecordSerialize
   serialize :st_diffs # rubocop:disable Cop/ActiverecordSerialize
@@ -91,7 +92,7 @@ class MergeRequestDiff < ActiveRecord::Base
           head_commit_sha).diffs(options)
     else
       @raw_diffs ||= {}
-      @raw_diffs[options] ||= load_diffs(st_diffs, options)
+      @raw_diffs[options] ||= load_diffs(options)
     end
   end
 
@@ -253,24 +254,44 @@ class MergeRequestDiff < ActiveRecord::Base
     update_columns_serialized(new_attributes)
   end
 
-  def dump_diffs(diffs)
-    if diffs.respond_to?(:map)
-      diffs.map(&:to_hash)
+  def create_merge_request_diff_files(diffs)
+    rows = diffs.map.with_index do |diff, index|
+      diff.to_hash.merge(
+        merge_request_diff_id: self.id,
+        relative_order: index
+      )
     end
+
+    Gitlab::Database.bulk_insert('merge_request_diff_files', rows)
   end
 
-  def load_diffs(raw, options)
-    if valid_raw_diff?(raw)
-      if paths = options[:paths]
-        raw = raw.select do |diff|
-          paths.include?(diff[:old_path]) || paths.include?(diff[:new_path])
-        end
-      end
+  def load_diffs(options)
+    return Gitlab::Git::DiffCollection.new([]) unless diffs_from_database
 
-      Gitlab::Git::DiffCollection.new(raw, options)
-    else
-      Gitlab::Git::DiffCollection.new([])
+    raw = diffs_from_database
+
+    if paths = options[:paths]
+      raw = raw.select do |diff|
+        paths.include?(diff[:old_path]) || paths.include?(diff[:new_path])
+      end
     end
+
+    Gitlab::Git::DiffCollection.new(raw, options)
+  end
+
+  def diffs_from_database
+    return @diffs_from_database if defined?(@diffs_from_database)
+
+    @diffs_from_database =
+      if st_diffs.present?
+        if valid_raw_diff?(st_diffs)
+          st_diffs
+        end
+      elsif merge_request_diff_files.present?
+        merge_request_diff_files
+          .as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS)
+          .map(&:with_indifferent_access)
+      end
   end
 
   # Load diffs between branches related to current merge request diff from repo
@@ -285,11 +306,10 @@ class MergeRequestDiff < ActiveRecord::Base
       new_attributes[:real_size] = diff_collection.real_size
 
       if diff_collection.any?
-        new_diffs = dump_diffs(diff_collection)
         new_attributes[:state] = :collected
-      end
 
-      new_attributes[:st_diffs] = new_diffs || []
+        create_merge_request_diff_files(diff_collection)
+      end
 
       # Set our state to 'overflow' to make the #empty? and #collected?
       # methods (generated by StateMachine) return false.

app/models/merge_request_diff_file.rb

+class MergeRequestDiffFile < ActiveRecord::Base
+  include Gitlab::EncodingHelper
+
+  belongs_to :merge_request_diff
+
+  def utf8_diff
+    return '' if diff.blank?
+
+    encode_utf8(diff) if diff.respond_to?(:encoding)
+  end
+end

db/migrate/20170608171156_create_merge_request_diff_files.rb

+class CreateMergeRequestDiffFiles < ActiveRecord::Migration
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    create_table :merge_request_diff_files, id: false do |t|
+      t.belongs_to :merge_request_diff, null: false, foreign_key: { on_delete: :cascade }
+      t.integer :relative_order, null: false
+      t.boolean :new_file, null: false
+      t.boolean :renamed_file, null: false
+      t.boolean :deleted_file, null: false
+      t.boolean :too_large, null: false
+      t.string :a_mode, null: false
+      t.string :b_mode, null: false
+      t.text :new_path, null: false
+      t.text :old_path, null: false
+      t.text :diff, null: false
+      t.index [:merge_request_diff_id, :relative_order], name: 'index_merge_request_diff_files_on_mr_diff_id_and_order', unique: true
+    end
+  end
+end

db/migrate/20170614115405_merge_request_diff_file_limits_to_mysql.rb

+require_relative 'merge_request_diff_file_limits_to_mysql'

db/migrate/merge_request_diff_file_limits_to_mysql.rb

+class MergeRequestDiffFileLimitsToMysql < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    return unless Gitlab::Database.mysql?
+
+    change_column :merge_request_diff_files, :diff, :text, limit: 2147483647
+  end
+
+  def down
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170609183112) do
+ActiveRecord::Schema.define(version: 20170614115405) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -692,6 +692,22 @@ ActiveRecord::Schema.define(version: 20170609183112) do
   add_index "members", ["source_id", "source_type"], name: "index_members_on_source_id_and_source_type", using: :btree
   add_index "members", ["user_id"], name: "index_members_on_user_id", using: :btree
 
+  create_table "merge_request_diff_files", id: false, force: :cascade do |t|
+    t.integer "merge_request_diff_id", null: false
+    t.integer "relative_order", null: false
+    t.boolean "new_file", null: false
+    t.boolean "renamed_file", null: false
+    t.boolean "deleted_file", null: false
+    t.boolean "too_large", null: false
+    t.string "a_mode", null: false
+    t.string "b_mode", null: false
+    t.text "new_path", null: false
+    t.text "old_path", null: false
+    t.text "diff", null: false
+  end
+
+  add_index "merge_request_diff_files", ["merge_request_diff_id", "relative_order"], name: "index_merge_request_diff_files_on_mr_diff_id_and_order", unique: true, using: :btree
+
   create_table "merge_request_diffs", force: :cascade do |t|
     t.string "state"
     t.text "st_commits"
@@ -1530,6 +1546,7 @@ ActiveRecord::Schema.define(version: 20170609183112) do
   add_foreign_key "labels", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "lists", "boards"
   add_foreign_key "lists", "labels"
+  add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade

doc/user/project/settings/import_export.md

@@ -27,14 +27,15 @@ with all their related data and be moved into a new GitLab instance.
 
 | GitLab version | Import/Export version |
 | -------- | -------- |
-| 9.2.0 to current | 0.1.7 |
-| 8.17.0   | 0.1.6    |
-| 8.13.0   | 0.1.5    |
-| 8.12.0   | 0.1.4    |
-| 8.10.3   | 0.1.3    |
-| 8.10.0   | 0.1.2    |
-| 8.9.5    | 0.1.1    |
-| 8.9.0    | 0.1.0    |
+| 9.4.0 to current | 0.1.8 |
+| 9.2.0    | 0.1.7 |
+| 8.17.0   | 0.1.6 |
+| 8.13.0   | 0.1.5 |
+| 8.12.0   | 0.1.4 |
+| 8.10.3   | 0.1.3 |
+| 8.10.0   | 0.1.2 |
+| 8.9.5    | 0.1.1 |
+| 8.9.0    | 0.1.0 |
 
  > The table reflects what GitLab version we updated the Import/Export version at.
  > For instance, 8.10.3 and 8.11 will have the same Import/Export version (0.1.3)

lib/gitlab/database.rb

@@ -83,6 +83,22 @@ module Gitlab
       end
     end
 
+    def self.bulk_insert(table, rows)
+      return if rows.empty?
+
+      keys = rows.first.keys
+      columns = keys.map { |key| connection.quote_column_name(key) }
+
+      tuples = rows.map do |row|
+        row.values_at(*keys).map { |value| connection.quote(value) }
+      end
+
+      connection.execute <<-EOF.strip_heredoc
+        INSERT INTO #{table} (#{columns.join(', ')})
+        VALUES #{tuples.map { |tuple| "(#{tuple.join(', ')})" }.join(', ')}
+      EOF
+    end
+
     # pool_size - The size of the DB pool.
     # host - An optional host name to use instead of the default one.
     def self.create_connection_pool(pool_size, host = nil)

lib/gitlab/git/diff.rb

@@ -16,11 +16,11 @@ module Gitlab
       alias_method :renamed_file?, :renamed_file
 
       attr_accessor :expanded
+      attr_writer :too_large
 
       alias_method :expanded?, :expanded
 
-      # We need this accessor because of `to_hash` and `init_from_hash`
-      attr_accessor :too_large
+      SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze
 
       class << self
         # The maximum size of a diff to display.
@@ -231,16 +231,10 @@ module Gitlab
         end
       end
 
-      def serialize_keys
-        @serialize_keys ||= %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large)
-      end
-
       def to_hash
         hash = {}
 
-        keys = serialize_keys
-
-        keys.each do |key|
+        SERIALIZE_KEYS.each do |key|
           hash[key] = send(key)
         end
 
@@ -267,6 +261,9 @@ module Gitlab
         end
       end
 
+      # This is used by `to_hash` and `init_from_hash`.
+      alias_method :too_large, :too_large?
+
       def too_large!
         @diff = ''
         @line_count = 0
@@ -315,7 +312,7 @@ module Gitlab
       def init_from_hash(hash)
         raw_diff = hash.symbolize_keys
 
-        serialize_keys.each do |key|
+        SERIALIZE_KEYS.each do |key|
           send(:"#{key}=", raw_diff[key.to_sym])
         end
       end

lib/gitlab/import_export.rb

@@ -3,7 +3,7 @@ module Gitlab
     extend self
 
     # For every version update, the version history in import_export.md has to be kept up to date.
-    VERSION = '0.1.7'.freeze
+    VERSION = '0.1.8'.freeze
     FILENAME_LIMIT = 50
 
     def export_path(relative_path:)

lib/gitlab/import_export/import_export.yml

@@ -26,7 +26,8 @@ project_tree:
     - notes:
       - :author
       - :events
-    - :merge_request_diff
+    - merge_request_diff:
+      - :merge_request_diff_files
     - :events
     - :timelogs
     - label_links:
@@ -92,6 +93,8 @@ excluded_attributes:
     - :expired_at
   merge_request_diff:
     - :st_diffs
+  merge_request_diff_files:
+    - :diff
   issues:
     - :milestone_id
   merge_requests:
@@ -113,6 +116,8 @@ methods:
     - :type
   merge_request_diff:
     - :utf8_st_diffs
+  merge_request_diff_files:
+    - :utf8_diff
   merge_requests:
     - :diff_head_sha
   project:

lib/gitlab/import_export/json_hash_builder.rb

@@ -83,7 +83,9 @@ module Gitlab
       # +value+ existing model to be included in the hash
       # +json_config_hash+ the original hash containing the root model
       def add_model_value(current_key, value, json_config_hash)
-        @attributes_finder.parse(value) { |hash| value = { value => hash } }
+        @attributes_finder.parse(value) do |hash|
+          value = { value => hash } unless value.is_a?(Hash)
+        end
 
         add_to_array(current_key, json_config_hash, value)
       end

lib/gitlab/import_export/relation_factory.rb

@@ -71,6 +71,7 @@ module Gitlab
 
         @relation_hash['data'].deep_symbolize_keys! if @relation_name == :events && @relation_hash['data']
         set_st_diff_commits if @relation_name == :merge_request_diff
+        set_diff if @relation_name == :merge_request_diff_files
       end
 
       def update_user_references
@@ -202,6 +203,10 @@ module Gitlab
         HashUtil.deep_symbolize_array_with_date!(@relation_hash['st_commits'])
       end
 
+      def set_diff
+        @relation_hash['diff'] = @relation_hash.delete('utf8_diff')
+      end
+
       def existing_or_new_object
         # Only find existing records to avoid mapping tables such as milestones
         # Otherwise always create the record, skipping the extra SELECT clause.

lib/tasks/migrate/add_limits_mysql.rake

 require Rails.root.join('db/migrate/limits_to_mysql')
 require Rails.root.join('db/migrate/markdown_cache_limits_to_mysql')
+require Rails.root.join('db/migrate/merge_request_diff_file_limits_to_mysql')
 
 desc "GitLab | Add limits to strings in mysql database"
 task add_limits_mysql: :environment do
   puts "Adding limits to schema.rb for mysql"
   LimitsToMysql.new.up
   MarkdownCacheLimitsToMysql.new.up
+  MergeRequestDiffFileLimitsToMysql.new.up
 end

spec/lib/gitlab/database_spec.rb

@@ -129,6 +129,55 @@ describe Gitlab::Database, lib: true do
     end
   end
 
+  describe '.bulk_insert' do
+    before do
+      allow(described_class).to receive(:connection).and_return(connection)
+      allow(connection).to receive(:quote_column_name, &:itself)
+      allow(connection).to receive(:quote, &:itself)
+      allow(connection).to receive(:execute)
+    end
+
+    let(:connection) { double(:connection) }
+
+    let(:rows) do
+      [
+        { a: 1, b: 2, c: 3 },
+        { c: 6, a: 4, b: 5 }
+      ]
+    end
+
+    it 'does nothing with empty rows' do
+      expect(connection).not_to receive(:execute)
+
+      described_class.bulk_insert('test', [])
+    end
+
+    it 'uses the ordering from the first row' do
+      expect(connection).to receive(:execute) do |sql|
+        expect(sql).to include('(1, 2, 3)')
+        expect(sql).to include('(4, 5, 6)')
+      end
+
+      described_class.bulk_insert('test', rows)
+    end
+
+    it 'quotes column names' do
+      expect(connection).to receive(:quote_column_name).with(:a)
+      expect(connection).to receive(:quote_column_name).with(:b)
+      expect(connection).to receive(:quote_column_name).with(:c)
+
+      described_class.bulk_insert('test', rows)
+    end
+
+    it 'quotes values' do
+      1.upto(6) do |i|
+        expect(connection).to receive(:quote).with(i)
+      end
+
+      described_class.bulk_insert('test', rows)
+    end
+  end
+
   describe '.create_connection_pool' do
     it 'creates a new connection pool with specific pool size' do
       pool = described_class.create_connection_pool(5)

spec/lib/gitlab/git/diff_spec.rb

@@ -90,7 +90,7 @@ EOT
         let(:diff) { described_class.new(@rugged_diff) }
 
         it 'initializes the diff' do
-          expect(diff.to_hash).to eq(@raw_diff_hash.merge(too_large: nil))
+          expect(diff.to_hash).to eq(@raw_diff_hash)
         end
 
         it 'does not prune the diff' do

spec/lib/gitlab/import_export/all_models.yml

@@ -88,6 +88,9 @@ merge_requests:
 - head_pipeline
 merge_request_diff:
 - merge_request
+- merge_request_diff_files
+merge_request_diff_files:
+- merge_request_diff
 pipelines:
 - project
 - user

spec/lib/gitlab/import_export/project.json

@@ -2821,9 +2821,11 @@
             "committer_email": "dmitriy.zaporozhets@gmail.com"
           }
         ],
-        "utf8_st_diffs": [
+        "merge_request_diff_files": [
           {
-            "diff": "Binary files a/.DS_Store and /dev/null differ\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 0,
+            "utf8_diff": "Binary files a/.DS_Store and /dev/null differ\n",
             "new_path": ".DS_Store",
             "old_path": ".DS_Store",
             "a_mode": "100644",
@@ -2834,7 +2836,9 @@
             "too_large": false
           },
           {
-            "diff": "--- a/.gitignore\n+++ b/.gitignore\n@@ -17,3 +17,4 @@ rerun.txt\n pickle-email-*.html\n .project\n config/initializers/secret_token.rb\n+.DS_Store\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 1,
+            "utf8_diff": "--- a/.gitignore\n+++ b/.gitignore\n@@ -17,3 +17,4 @@ rerun.txt\n pickle-email-*.html\n .project\n config/initializers/secret_token.rb\n+.DS_Store\n",
             "new_path": ".gitignore",
             "old_path": ".gitignore",
             "a_mode": "100644",
@@ -2845,7 +2849,9 @@
             "too_large": false
           },
           {
-            "diff": "--- a/.gitmodules\n+++ b/.gitmodules\n@@ -1,3 +1,9 @@\n [submodule \"six\"]\n \tpath = six\n \turl = git://github.com/randx/six.git\n+[submodule \"gitlab-shell\"]\n+\tpath = gitlab-shell\n+\turl = https://github.com/gitlabhq/gitlab-shell.git\n+[submodule \"gitlab-grack\"]\n+\tpath = gitlab-grack\n+\turl = https://gitlab.com/gitlab-org/gitlab-grack.git\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 2,
+            "utf8_diff": "--- a/.gitmodules\n+++ b/.gitmodules\n@@ -1,3 +1,9 @@\n [submodule \"six\"]\n \tpath = six\n \turl = git://github.com/randx/six.git\n+[submodule \"gitlab-shell\"]\n+\tpath = gitlab-shell\n+\turl = https://github.com/gitlabhq/gitlab-shell.git\n+[submodule \"gitlab-grack\"]\n+\tpath = gitlab-grack\n+\turl = https://gitlab.com/gitlab-org/gitlab-grack.git\n",
             "new_path": ".gitmodules",
             "old_path": ".gitmodules",
             "a_mode": "100644",
@@ -2856,7 +2862,9 @@
             "too_large": false
           },
           {
-            "diff": "Binary files a/files/.DS_Store and /dev/null differ\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 3,
+            "utf8_diff": "Binary files a/files/.DS_Store and /dev/null differ\n",
             "new_path": "files/.DS_Store",
             "old_path": "files/.DS_Store",
             "a_mode": "100644",
@@ -2867,7 +2875,9 @@
             "too_large": false
           },
           {
-            "diff": "--- /dev/null\n+++ b/files/ruby/feature.rb\n@@ -0,0 +1,4 @@\n+# This file was changed in feature branch\n+# We put different code here to make merge conflict\n+class Conflict\n+end\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 4,
+            "utf8_diff": "--- /dev/null\n+++ b/files/ruby/feature.rb\n@@ -0,0 +1,4 @@\n+# This file was changed in feature branch\n+# We put different code here to make merge conflict\n+class Conflict\n+end\n",
             "new_path": "files/ruby/feature.rb",
             "old_path": "files/ruby/feature.rb",
             "a_mode": "0",
@@ -2878,7 +2888,9 @@
             "too_large": false
           },
           {
-            "diff": "--- a/files/ruby/popen.rb\n+++ b/files/ruby/popen.rb\n@@ -6,12 +6,18 @@ module Popen\n \n   def popen(cmd, path=nil)\n     unless cmd.is_a?(Array)\n-      raise \"System commands must be given as an array of strings\"\n+      raise RuntimeError, \"System commands must be given as an array of strings\"\n     end\n \n     path ||= Dir.pwd\n-    vars = { \"PWD\" =\u003e path }\n-    options = { chdir: path }\n+\n+    vars = {\n+      \"PWD\" =\u003e path\n+    }\n+\n+    options = {\n+      chdir: path\n+    }\n \n     unless File.directory?(path)\n       FileUtils.mkdir_p(path)\n@@ -19,6 +25,7 @@ module Popen\n \n     @cmd_output = \"\"\n     @cmd_status = 0\n+\n     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|\n       @cmd_output \u003c\u003c stdout.read\n       @cmd_output \u003c\u003c stderr.read\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 5,
+            "utf8_diff": "--- a/files/ruby/popen.rb\n+++ b/files/ruby/popen.rb\n@@ -6,12 +6,18 @@ module Popen\n \n   def popen(cmd, path=nil)\n     unless cmd.is_a?(Array)\n-      raise \"System commands must be given as an array of strings\"\n+      raise RuntimeError, \"System commands must be given as an array of strings\"\n     end\n \n     path ||= Dir.pwd\n-    vars = { \"PWD\" =\u003e path }\n-    options = { chdir: path }\n+\n+    vars = {\n+      \"PWD\" =\u003e path\n+    }\n+\n+    options = {\n+      chdir: path\n+    }\n \n     unless File.directory?(path)\n       FileUtils.mkdir_p(path)\n@@ -19,6 +25,7 @@ module Popen\n \n     @cmd_output = \"\"\n     @cmd_status = 0\n+\n     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|\n       @cmd_output \u003c\u003c stdout.read\n       @cmd_output \u003c\u003c stderr.read\n",
             "new_path": "files/ruby/popen.rb",
             "old_path": "files/ruby/popen.rb",
             "a_mode": "100644",
@@ -2889,7 +2901,9 @@
             "too_large": false
           },
           {
-            "diff": "--- a/files/ruby/regex.rb\n+++ b/files/ruby/regex.rb\n@@ -19,14 +19,12 @@ module Gitlab\n     end\n \n     def archive_formats_regex\n-      #|zip|tar|    tar.gz    |         tar.bz2         |\n-      /(zip|tar|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n+      /(zip|tar|7z|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n     end\n \n     def git_reference_regex\n       # Valid git ref regex, see:\n       # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html\n-\n       %r{\n         (?!\n            (?# doesn't begins with)\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 6,
+            "utf8_diff": "--- a/files/ruby/regex.rb\n+++ b/files/ruby/regex.rb\n@@ -19,14 +19,12 @@ module Gitlab\n     end\n \n     def archive_formats_regex\n-      #|zip|tar|    tar.gz    |         tar.bz2         |\n-      /(zip|tar|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n+      /(zip|tar|7z|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n     end\n \n     def git_reference_regex\n       # Valid git ref regex, see:\n       # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html\n-\n       %r{\n         (?!\n            (?# doesn't begins with)\n",
             "new_path": "files/ruby/regex.rb",
             "old_path": "files/ruby/regex.rb",
             "a_mode": "100644",
@@ -2900,7 +2914,9 @@
             "too_large": false
           },
           {
-            "diff": "--- /dev/null\n+++ b/gitlab-grack\n@@ -0,0 +1 @@\n+Subproject commit 645f6c4c82fd3f5e06f67134450a570b795e55a6\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 7,
+            "utf8_diff": "--- /dev/null\n+++ b/gitlab-grack\n@@ -0,0 +1 @@\n+Subproject commit 645f6c4c82fd3f5e06f67134450a570b795e55a6\n",
             "new_path": "gitlab-grack",
             "old_path": "gitlab-grack",
             "a_mode": "0",
@@ -2911,7 +2927,9 @@
             "too_large": false
           },
           {
-            "diff": "--- /dev/null\n+++ b/gitlab-shell\n@@ -0,0 +1 @@\n+Subproject commit 79bceae69cb5750d6567b223597999bfa91cb3b9\n",
+            "merge_request_diff_id": 27,
+            "relative_order": 8,
+            "utf8_diff": "--- /dev/null\n+++ b/gitlab-shell\n@@ -0,0 +1 @@\n+Subproject commit 79bceae69cb5750d6567b223597999bfa91cb3b9\n",
             "new_path": "gitlab-shell",
             "old_path": "gitlab-shell",
             "a_mode": "0",

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -86,8 +86,13 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
 
       it 'has the correct data for merge request st_diffs' do
         # makes sure we are renaming the custom method +utf8_st_diffs+ into +st_diffs+
+        # one MergeRequestDiff uses the new format, where st_diffs is expected to be nil
 
-        expect(MergeRequestDiff.where.not(st_diffs: nil).count).to eq(9)
+        expect(MergeRequestDiff.where.not(st_diffs: nil).count).to eq(8)
+      end
+
+      it 'has the correct data for merge request diff files' do
+        expect(MergeRequestDiffFile.where.not(diff: nil).count).to eq(9)
       end
 
       it 'has the correct time for merge request st_commits' do

spec/lib/gitlab/import_export/project_tree_saver_spec.rb

@@ -83,6 +83,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
         expect(saved_project_json['merge_requests'].first['merge_request_diff']['utf8_st_diffs']).not_to be_nil
       end
 
+      it 'has merge request diff files' do
+        expect(saved_project_json['merge_requests'].first['merge_request_diff']['merge_request_diff_files']).not_to be_empty
+      end
+
       it 'has merge requests comments' do
         expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty
       end
@@ -145,6 +149,12 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
         expect(project_tree_saver.save).to be true
       end
 
+      it 'does not complain about non UTF-8 characters in MR diff files' do
+        ActiveRecord::Base.connection.execute("UPDATE merge_request_diff_files SET diff = '---\n- :diff: !binary |-\n    LS0tIC9kZXYvbnVsbAorKysgYi9pbWFnZXMvbnVjb3IucGRmCkBAIC0wLDAg\n    KzEsMTY3OSBAQAorJVBERi0xLjUNJeLjz9MNCisxIDAgb2JqDTw8L01ldGFk\n    YXR'")
+
+        expect(project_tree_saver.save).to be true
+      end
+
       context 'group members' do
         let(:user2) { create(:user, email: 'group@member.com') }
         let(:member_emails) do

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -172,6 +172,17 @@ MergeRequestDiff:
 - real_size
 - head_commit_sha
 - start_commit_sha
+MergeRequestDiffFile:
+- merge_request_diff_id
+- relative_order
+- new_file
+- renamed_file
+- deleted_file
+- new_path
+- old_path
+- a_mode
+- b_mode
+- too_large
 Ci::Pipeline:
 - id
 - project_id

spec/models/merge_request_diff_file_spec.rb

+require 'rails_helper'
+
+describe MergeRequestDiffFile, type: :model do
+  describe '#utf8_diff' do
+    it 'does not raise error when a hash value is in binary' do
+      subject.diff = "\x05\x00\x68\x65\x6c\x6c\x6f"
+
+      expect { subject.utf8_diff }.not_to raise_error
+    end
+  end
+end

spec/models/merge_request_diff_spec.rb

@@ -37,7 +37,7 @@ describe MergeRequestDiff, models: true do
 
     context 'when the raw diffs are empty' do
       before do
-        mr_diff.update_attributes(st_diffs: '')
+        MergeRequestDiffFile.delete_all(merge_request_diff_id: mr_diff.id)
       end
 
       it 'returns an empty DiffCollection' do
@@ -48,6 +48,7 @@ describe MergeRequestDiff, models: true do
 
     context 'when the raw diffs have invalid content' do
       before do
+        MergeRequestDiffFile.delete_all(merge_request_diff_id: mr_diff.id)
         mr_diff.update_attributes(st_diffs: ["--broken-diff"])
       end