Add table for files in merge request diffs

This adds an ID-less table containing one row per file, per merge request
diff. It has a column for each attribute on Gitlab::Git::Diff that is serialised
currently, with the advantage that we can easily query the attributes of this
new table.

It does not migrate existing data, so we have fallback code when the legacy
st_diffs column is present instead. For a merge request diff to be valid, it
should have at most one of:

* Rows in this new table, with the correct merge_request_diff_id.
* A non-NULL st_diffs column.

It may have neither, if the diff is empty.

24 files changed, 271 insertions, 52 deletions

diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 99dd2130188..f1ee4d3f7a9 100644
--- a/app/models/merge_request_diff.rb
+++ b/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.
diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb
new file mode 100644
index 00000000000..598ebd4d829
--- /dev/null
+++ b/app/models/merge_request_diff_file.rb
@@ -0,0 +1,11 @@
+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
diff --git a/db/migrate/20170608171156_create_merge_request_diff_files.rb b/db/migrate/20170608171156_create_merge_request_diff_files.rb
new file mode 100644
index 00000000000..bf0c0d29adc
--- /dev/null
+++ b/db/migrate/20170608171156_create_merge_request_diff_files.rb
@@ -0,0 +1,22 @@
+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
diff --git a/db/migrate/20170614115405_merge_request_diff_file_limits_to_mysql.rb b/db/migrate/20170614115405_merge_request_diff_file_limits_to_mysql.rb
new file mode 100644
index 00000000000..4c1cf08aa06
--- /dev/null
+++ b/db/migrate/20170614115405_merge_request_diff_file_limits_to_mysql.rb
@@ -0,0 +1 @@
+require_relative 'merge_request_diff_file_limits_to_mysql'
diff --git a/db/migrate/merge_request_diff_file_limits_to_mysql.rb b/db/migrate/merge_request_diff_file_limits_to_mysql.rb
new file mode 100644
index 00000000000..3958380e4b9
--- /dev/null
+++ b/db/migrate/merge_request_diff_file_limits_to_mysql.rb
@@ -0,0 +1,12 @@
+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
diff --git a/db/schema.rb b/db/schema.rb
index 803e36fba5a..f42827991aa 100644
--- a/db/schema.rb
+++ b/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
diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md
index 58d2fd76c61..35960ade3d4 100644
--- a/doc/user/project/settings/import_export.md
+++ b/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)
diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb
index d0bd1299671..0d5a7cf0694 100644
--- a/lib/gitlab/database.rb
+++ b/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)
diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb
index 4b689f0e94f..f825568f194 100644
--- a/lib/gitlab/git/diff.rb
+++ b/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
diff --git a/lib/gitlab/import_export.rb b/lib/gitlab/import_export.rb
index 27d5a9198b6..3470a09eaf0 100644
--- a/lib/gitlab/import_export.rb
+++ b/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:)
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index ff2b1d08c3c..72183e8aad4 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/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:
diff --git a/lib/gitlab/import_export/json_hash_builder.rb b/lib/gitlab/import_export/json_hash_builder.rb
index 48c09dafcb6..b48f63bcd7e 100644
--- a/lib/gitlab/import_export/json_hash_builder.rb
+++ b/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
diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb
index 695852526cb..20580459046 100644
--- a/lib/gitlab/import_export/relation_factory.rb
+++ b/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.
diff --git a/lib/tasks/migrate/add_limits_mysql.rake b/lib/tasks/migrate/add_limits_mysql.rake
index 761f275d42a..151f42a2222 100644
--- a/lib/tasks/migrate/add_limits_mysql.rake
+++ b/lib/tasks/migrate/add_limits_mysql.rake
@@ -1,9 +1,11 @@
 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
diff --git a/spec/features/projects/import_export/test_project_export.tar.gz b/spec/features/projects/import_export/test_project_export.tar.gz
index 4efd5a26a82..e03e7b88174 100644
--- a/spec/features/projects/import_export/test_project_export.tar.gz
+++ b/spec/features/projects/import_export/test_project_export.tar.gz
diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb
index 26e5d73d333..428b6edb7d6 100644
--- a/spec/lib/gitlab/database_spec.rb
+++ b/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)
diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb
index da213f617cc..78d741f0110 100644
--- a/spec/lib/gitlab/git/diff_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 412eb33b35b..a5f09f1856e 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/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
diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json
index e3599d6fe59..98c117b4cd8 100644
--- a/spec/lib/gitlab/import_export/project.json
+++ b/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",
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index 14338515892..c11b15a811b 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb
index 5aeb29b7fec..e52f79513f1 100644
--- a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 50ff6ecc1e0..fadd3ad1330 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/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
diff --git a/spec/models/merge_request_diff_file_spec.rb b/spec/models/merge_request_diff_file_spec.rb
new file mode 100644
index 00000000000..7276f5b5061
--- /dev/null
+++ b/spec/models/merge_request_diff_file_spec.rb
@@ -0,0 +1,11 @@
+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
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 25f7062860b..4ad4abaa572 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/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