From aff5c9f3e5ecdd9eee2b2b60ab6367da878582fc Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 16 Jun 2017 15:00:58 +0100
Subject: [PATCH] Add table for merge request commits

This is an ID-less table with just three columns: an association to the merge
request diff the commit belongs to, the relative order of the commit within the
merge request diff, and the commit SHA itself.

Previously we stored much more information about the commits, so that we could
display them even when they were deleted from the repo. Since 8.0, we ensure
that those commits are kept around for as long as the target repo itself is, so
we don't need to duplicate that data in the database.
---
 .../concerns/issuable_collections.rb          |  2 +-
 app/models/ci/build.rb                        |  2 +-
 app/models/commit.rb                          |  2 +-
 app/models/merge_request.rb                   | 10 +-
 app/models/merge_request_diff.rb              | 99 +++++++------------
 app/models/merge_request_diff_commit.rb       | 38 +++++++
 .../merge_requests/refresh_service.rb         |  6 +-
 ...33147_create_merge_request_diff_commits.rb | 20 ++++
 db/schema.rb                                  | 16 +++
 lib/gitlab/cycle_analytics/metrics_tables.rb  |  4 +
 .../cycle_analytics/plan_event_fetcher.rb     | 43 +++++---
 lib/gitlab/import_export/import_export.yml    |  1 +
 spec/lib/gitlab/import_export/all_models.yml  |  3 +
 spec/lib/gitlab/import_export/project.json    | 44 ++++-----
 .../project_tree_restorer_spec.rb             |  5 +
 .../import_export/project_tree_saver_spec.rb  |  4 +
 .../import_export/safe_model_attributes.yml   | 11 +++
 spec/models/ci/build_spec.rb                  |  2 +-
 spec/models/merge_request_diff_commit_spec.rb | 15 +++
 spec/models/merge_request_diff_spec.rb        |  6 +-
 spec/models/merge_request_spec.rb             | 18 ++--
 21 files changed, 225 insertions(+), 126 deletions(-)
 create mode 100644 app/models/merge_request_diff_commit.rb
 create mode 100644 db/migrate/20170616133147_create_merge_request_diff_commits.rb
 create mode 100644 spec/models/merge_request_diff_commit_spec.rb

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 650ec1e326a..693e2f6365c 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -47,7 +47,7 @@ module IssuableCollections
   end
 
   def merge_requests_collection
-    merge_requests_finder.execute.preload(:source_project, :target_project, :author, :assignee, :labels, :milestone, :merge_request_diff, :head_pipeline, target_project: :namespace)
+    merge_requests_finder.execute.preload(:source_project, :target_project, :author, :assignee, :labels, :milestone, :head_pipeline, target_project: :namespace, merge_request_diff: :merge_request_diff_commits)
   end
 
   def issues_finder
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 48586ba8910..ba2ecbf82a2 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -218,7 +218,7 @@ module Ci
             .reorder(iid: :desc)
 
           merge_requests.find do |merge_request|
-            merge_request.commits_sha.include?(pipeline.sha)
+            merge_request.commit_shas.include?(pipeline.sha)
           end
         end
     end
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 20206d57c4c..c7f62617c4c 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -138,7 +138,7 @@ class Commit
 
     safe_message.split("\n", 2)[1].try(:chomp)
   end
-  
+
   def description?
     description.present?
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2752181ed79..2fc6191e785 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base
   after_create :ensure_merge_request_diff, unless: :importing?
   after_update :reload_diff_if_branch_changed
 
-  delegate :commits, :real_size, :commits_sha, :commits_count,
+  delegate :commits, :real_size, :commit_shas, :commits_count,
     to: :merge_request_diff, prefix: nil
 
   # When this attribute is true some MR validation is ignored
@@ -518,7 +518,7 @@ class MergeRequest < ActiveRecord::Base
   def related_notes
     # Fetch comments only from last 100 commits
     commits_for_notes_limit = 100
-    commit_ids = commits.last(commits_for_notes_limit).map(&:id)
+    commit_ids = commit_shas.take(commits_for_notes_limit)
 
     Note.where(
       "(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" +
@@ -841,15 +841,15 @@ class MergeRequest < ActiveRecord::Base
     return Ci::Pipeline.none unless source_project
 
     @all_pipelines ||= source_project.pipelines
-      .where(sha: all_commits_sha, ref: source_branch)
+      .where(sha: all_commit_shas, ref: source_branch)
       .order(id: :desc)
   end
 
   # Note that this could also return SHA from now dangling commits
   #
-  def all_commits_sha
+  def all_commit_shas
     if persisted?
-      merge_request_diffs.flat_map(&:commits_sha).uniq
+      merge_request_diffs.preload(:merge_request_diff_commits).flat_map(&:commit_shas).uniq
     elsif compare_commits
       compare_commits.to_a.reverse.map(&:id)
     else
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 0cbf58a2325..4b141945ab4 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -11,6 +11,7 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
   has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) }
+  has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) }
 
   serialize :st_commits # rubocop:disable Cop/ActiveRecordSerialize
   serialize :st_diffs # rubocop:disable Cop/ActiveRecordSerialize
@@ -47,14 +48,13 @@ class MergeRequestDiff < ActiveRecord::Base
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
-    ensure_commits_sha
+    ensure_commit_shas
     save_commits
-    reload_commits
     save_diffs
     keep_around_commits
   end
 
-  def ensure_commits_sha
+  def ensure_commit_shas
     merge_request.fetch_ref
     self.start_commit_sha ||= merge_request.target_branch_sha
     self.head_commit_sha  ||= merge_request.source_branch_sha
@@ -66,7 +66,7 @@ class MergeRequestDiff < ActiveRecord::Base
   # created before version 8.4 that does not store head_commit_sha in separate db field.
   def head_commit_sha
     if persisted? && super.nil?
-      last_commit.try(:sha)
+      last_commit_sha
     else
       super
     end
@@ -97,16 +97,11 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def commits
-    @commits ||= load_commits(st_commits)
+    @commits ||= load_commits
   end
 
-  def reload_commits
-    @commits = nil
-    commits
-  end
-
-  def last_commit
-    commits.first
+  def last_commit_sha
+    commit_shas.first
   end
 
   def first_commit
@@ -131,8 +126,12 @@ class MergeRequestDiff < ActiveRecord::Base
     project.commit(head_commit_sha)
   end
 
-  def commits_sha
-    st_commits.map { |commit| commit[:id] }
+  def commit_shas
+    if st_commits.present?
+      st_commits.map { |commit| commit[:id] }
+    else
+      merge_request_diff_commits.map(&:sha)
+    end
   end
 
   def diff_refs=(new_diff_refs)
@@ -207,7 +206,11 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def commits_count
-    st_commits.count
+    if st_commits.present?
+      st_commits.size
+    else
+      merge_request_diff_commits.size
+    end
   end
 
   def utf8_st_diffs
@@ -231,29 +234,6 @@ class MergeRequestDiff < ActiveRecord::Base
     raw.any? { |element| VALID_CLASSES.include?(element.class) }
   end
 
-  def dump_commits(commits)
-    commits.map(&:to_hash)
-  end
-
-  def load_commits(array)
-    array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash), merge_request.source_project) }
-  end
-
-  # Load all commits related to current merge request diff from repo
-  # and save it as array of hashes in st_commits db field
-  def save_commits
-    new_attributes = {}
-
-    commits = compare.commits
-
-    if commits.present?
-      commits = Commit.decorate(commits, merge_request.source_project).reverse
-      new_attributes[:st_commits] = dump_commits(commits)
-    end
-
-    update_columns_serialized(new_attributes)
-  end
-
   def create_merge_request_diff_files(diffs)
     rows = diffs.map.with_index do |diff, index|
       diff.to_hash.merge(
@@ -294,12 +274,18 @@ class MergeRequestDiff < ActiveRecord::Base
       end
   end
 
-  # Load diffs between branches related to current merge request diff from repo
-  # and save it as array of hashes in st_diffs db field
+  def load_commits
+    commits = st_commits.presence || merge_request_diff_commits
+
+    commits.map do |commit|
+      Commit.new(Gitlab::Git::Commit.new(commit.to_hash), merge_request.source_project)
+    end
+  end
+
   def save_diffs
     new_attributes = {}
 
-    if commits.size.zero?
+    if compare.commits.size.zero?
       new_attributes[:state] = :empty
     else
       diff_collection = compare.diffs(Commit.max_diff_options)
@@ -319,7 +305,13 @@ class MergeRequestDiff < ActiveRecord::Base
       new_attributes[:state] = :overflow if diff_collection.overflow?
     end
 
-    update_columns_serialized(new_attributes)
+    update(new_attributes)
+  end
+
+  def save_commits
+    MergeRequestDiffCommit.create_bulk(self.id, compare.commits.reverse)
+
+    merge_request_diff_commits.reload
   end
 
   def repository
@@ -332,29 +324,6 @@ class MergeRequestDiff < ActiveRecord::Base
     project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha)
   end
 
-  #
-  # #save or #update_attributes providing changes on serialized attributes do a lot of
-  # serialization and deserialization calls resulting in bad performance.
-  # Using #update_columns solves the problem with just one YAML.dump per serialized attribute that we provide.
-  # As a tradeoff we need to reload the current instance to properly manage time objects on those serialized
-  # attributes. So to keep the same behaviour as the attribute assignment we reload the instance.
-  # The difference is in the usage of
-  # #write_attribute= (#update_attributes) and #raw_write_attribute= (#update_columns)
-  #
-  # Ex:
-  #
-  #   new_attributes[:st_commits].first.slice(:committed_date)
-  #   => {:committed_date=>2014-02-27 11:01:38 +0200}
-  #   YAML.load(YAML.dump(new_attributes[:st_commits].first.slice(:committed_date)))
-  #   => {:committed_date=>2014-02-27 10:01:38 +0100}
-  #
-  def update_columns_serialized(new_attributes)
-    return unless new_attributes.any?
-
-    update_columns(new_attributes.merge(updated_at: current_time_from_proper_timezone))
-    reload
-  end
-
   def keep_around_commits
     [repository, merge_request.source_project.repository].each do |repo|
       repo.keep_around(start_commit_sha)
diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb
new file mode 100644
index 00000000000..cafdbe11849
--- /dev/null
+++ b/app/models/merge_request_diff_commit.rb
@@ -0,0 +1,38 @@
+class MergeRequestDiffCommit < ActiveRecord::Base
+  include ShaAttribute
+
+  belongs_to :merge_request_diff
+
+  sha_attribute :sha
+  alias_attribute :id, :sha
+
+  def self.create_bulk(merge_request_diff_id, commits)
+    sha_attribute = Gitlab::Database::ShaAttribute.new
+
+    rows = commits.map.with_index do |commit, index|
+      # See #parent_ids.
+      commit_hash = commit.to_hash.except(:parent_ids)
+      sha = commit_hash.delete(:id)
+
+      commit_hash.merge(
+        merge_request_diff_id: merge_request_diff_id,
+        relative_order: index,
+        sha: sha_attribute.type_cast_for_database(sha)
+      )
+    end
+
+    Gitlab::Database.bulk_insert(self.table_name, rows)
+  end
+
+  def to_hash
+    Gitlab::Git::Commit::SERIALIZE_KEYS.each_with_object({}) do |key, hash|
+      hash[key] = public_send(key)
+    end
+  end
+
+  # We don't save these, because they would need a table or a serialised
+  # field. They aren't used anywhere, so just pretend the commit has no parents.
+  def parent_ids
+    []
+  end
+end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index e0e7c43f802..c5f959a1874 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -68,7 +68,7 @@ module MergeRequests
         if merge_request.source_branch == @branch_name || force_push?
           merge_request.reload_diff(current_user)
         else
-          mr_commit_ids = merge_request.commits_sha
+          mr_commit_ids = merge_request.commit_shas
           push_commit_ids = @commits.map(&:id)
           matches = mr_commit_ids & push_commit_ids
           merge_request.reload_diff(current_user) if matches.any?
@@ -128,7 +128,7 @@ module MergeRequests
       return unless @commits.present?
 
       merge_requests_for_source_branch.each do |merge_request|
-        mr_commit_ids = Set.new(merge_request.commits_sha)
+        mr_commit_ids = Set.new(merge_request.commit_shas)
 
         new_commits, existing_commits = @commits.partition do |commit|
           mr_commit_ids.include?(commit.id)
@@ -144,7 +144,7 @@ module MergeRequests
       return unless @commits.present?
 
       merge_requests_for_source_branch.each do |merge_request|
-        commit_shas = merge_request.commits_sha
+        commit_shas = merge_request.commit_shas
 
         wip_commit = @commits.detect do |commit|
           commit.work_in_progress? && commit_shas.include?(commit.sha)
diff --git a/db/migrate/20170616133147_create_merge_request_diff_commits.rb b/db/migrate/20170616133147_create_merge_request_diff_commits.rb
new file mode 100644
index 00000000000..616464cb470
--- /dev/null
+++ b/db/migrate/20170616133147_create_merge_request_diff_commits.rb
@@ -0,0 +1,20 @@
+class CreateMergeRequestDiffCommits < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    create_table :merge_request_diff_commits, id: false do |t|
+      t.datetime_with_timezone :authored_date
+      t.datetime_with_timezone :committed_date
+      t.belongs_to :merge_request_diff, null: false, foreign_key: { on_delete: :cascade }
+      t.integer :relative_order, null: false
+      t.binary :sha, null: false, limit: 20
+      t.text :author_name
+      t.text :author_email
+      t.text :committer_name
+      t.text :committer_email
+      t.text :message
+
+      t.index [:merge_request_diff_id, :relative_order], name: 'index_merge_request_diff_commits_on_mr_diff_id_and_order', unique: true
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f12fdf903c5..a47a6ae9a98 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -693,6 +693,21 @@ ActiveRecord::Schema.define(version: 20170703102400) 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_commits", id: false, force: :cascade do |t|
+    t.datetime "authored_date"
+    t.datetime "committed_date"
+    t.integer "merge_request_diff_id", null: false
+    t.integer "relative_order", null: false
+    t.binary "sha", null: false
+    t.text "author_name"
+    t.text "author_email"
+    t.text "committer_name"
+    t.text "committer_email"
+    t.text "message"
+  end
+
+  add_index "merge_request_diff_commits", ["merge_request_diff_id", "relative_order"], name: "index_merge_request_diff_commits_on_mr_diff_id_and_order", unique: true, 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
@@ -1563,6 +1578,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do
   add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade
   add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade
   add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade
+  add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
diff --git a/lib/gitlab/cycle_analytics/metrics_tables.rb b/lib/gitlab/cycle_analytics/metrics_tables.rb
index 9d25ef078e8..f5d08c0b658 100644
--- a/lib/gitlab/cycle_analytics/metrics_tables.rb
+++ b/lib/gitlab/cycle_analytics/metrics_tables.rb
@@ -13,6 +13,10 @@ module Gitlab
         MergeRequestDiff.arel_table
       end
 
+      def mr_diff_commits_table
+        MergeRequestDiffCommit.arel_table
+      end
+
       def mr_closing_issues_table
         MergeRequestsClosingIssues.arel_table
       end
diff --git a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb
index 7d342a2d2cb..b260822788d 100644
--- a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb
+++ b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb
@@ -2,40 +2,59 @@ module Gitlab
   module CycleAnalytics
     class PlanEventFetcher < BaseEventFetcher
       def initialize(*args)
-        @projections = [mr_diff_table[:st_commits].as('commits'),
+        @projections = [mr_diff_table[:id],
+                        mr_diff_table[:st_commits],
                         issue_metrics_table[:first_mentioned_in_commit_at]]
 
         super(*args)
       end
 
       def events_query
-        base_query.join(mr_diff_table).on(mr_diff_table[:merge_request_id].eq(mr_table[:id]))
+        base_query
+          .join(mr_diff_table)
+          .on(mr_diff_table[:merge_request_id].eq(mr_table[:id]))
 
         super
       end
 
       private
 
-      def serialize(event)
-        st_commit = first_time_reference_commit(event.delete('commits'), event)
-
-        return unless st_commit
-
-        serialize_commit(event, st_commit, query)
+      def merge_request_diff_commits
+        @merge_request_diff_commits ||=
+          MergeRequestDiffCommit
+            .where(merge_request_diff_id: event_result.map { |event| event['id'] })
+            .group_by(&:merge_request_diff_id)
       end
 
-      def first_time_reference_commit(commits, event)
+      def serialize(event)
+        commit = first_time_reference_commit(event)
+
+        return unless commit
+
+        serialize_commit(event, commit, query)
+      end
+
+      def first_time_reference_commit(event)
+        return nil unless event && merge_request_diff_commits
+
+        commits =
+          if event['st_commits'].present?
+            YAML.load(event['st_commits'])
+          else
+            merge_request_diff_commits[event['id'].to_i]
+          end
+
         return nil if commits.blank?
 
-        YAML.load(commits).find do |commit|
+        commits.find do |commit|
           next unless commit[:committed_date] && event['first_mentioned_in_commit_at']
 
           commit[:committed_date].to_i == DateTime.parse(event['first_mentioned_in_commit_at'].to_s).to_i
         end
       end
 
-      def serialize_commit(event, st_commit, query)
-        commit = Commit.new(Gitlab::Git::Commit.new(st_commit), @project)
+      def serialize_commit(event, commit, query)
+        commit = Commit.new(Gitlab::Git::Commit.new(commit.to_hash), @project)
 
         AnalyticsCommitSerializer.new(project: @project, total_time: event['total_time']).represent(commit)
       end
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index 1860352c96d..c8ad3a7a5e0 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -27,6 +27,7 @@ project_tree:
       - :author
       - :events
     - merge_request_diff:
+      - :merge_request_diff_commits
       - :merge_request_diff_files
     - :events
     - :timelogs
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index a5f09f1856e..562f0c2991c 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -88,7 +88,10 @@ merge_requests:
 - head_pipeline
 merge_request_diff:
 - merge_request
+- merge_request_diff_commits
 - merge_request_diff_files
+merge_request_diff_commits:
+- merge_request_diff
 merge_request_diff_files:
 - merge_request_diff
 pipelines:
diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json
index 98c117b4cd8..469a014e4d2 100644
--- a/spec/lib/gitlab/import_export/project.json
+++ b/spec/lib/gitlab/import_export/project.json
@@ -2741,13 +2741,12 @@
       "merge_request_diff": {
         "id": 27,
         "state": "collected",
-        "st_commits": [
+        "merge_request_diff_commits": [
           {
-            "id": "bb5206fee213d983da88c47f9cf4cc6caf9c66dc",
+            "merge_request_diff_id": 27,
+            "relative_order": 0,
+            "sha": "bb5206fee213d983da88c47f9cf4cc6caf9c66dc",
             "message": "Feature conflcit added\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
-            "parent_ids": [
-              "5937ac0a7beb003549fc5fd26fc247adbce4a52e"
-            ],
             "authored_date": "2014-08-06T08:35:52.000+02:00",
             "author_name": "Dmitriy Zaporozhets",
             "author_email": "dmitriy.zaporozhets@gmail.com",
@@ -2756,11 +2755,10 @@
             "committer_email": "dmitriy.zaporozhets@gmail.com"
           },
           {
-            "id": "5937ac0a7beb003549fc5fd26fc247adbce4a52e",
+            "merge_request_diff_id": 27,
+            "relative_order": 1,
+            "sha": "5937ac0a7beb003549fc5fd26fc247adbce4a52e",
             "message": "Add submodule from gitlab.com\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
-            "parent_ids": [
-              "570e7b2abdd848b95f2f578043fc23bd6f6fd24d"
-            ],
             "authored_date": "2014-02-27T10:01:38.000+01:00",
             "author_name": "Dmitriy Zaporozhets",
             "author_email": "dmitriy.zaporozhets@gmail.com",
@@ -2769,11 +2767,10 @@
             "committer_email": "dmitriy.zaporozhets@gmail.com"
           },
           {
-            "id": "570e7b2abdd848b95f2f578043fc23bd6f6fd24d",
+            "merge_request_diff_id": 27,
+            "relative_order": 2,
+            "sha": "570e7b2abdd848b95f2f578043fc23bd6f6fd24d",
             "message": "Change some files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
-            "parent_ids": [
-              "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9"
-            ],
             "authored_date": "2014-02-27T09:57:31.000+01:00",
             "author_name": "Dmitriy Zaporozhets",
             "author_email": "dmitriy.zaporozhets@gmail.com",
@@ -2782,11 +2779,10 @@
             "committer_email": "dmitriy.zaporozhets@gmail.com"
           },
           {
-            "id": "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9",
+            "merge_request_diff_id": 27,
+            "relative_order": 3,
+            "sha": "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9",
             "message": "More submodules\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
-            "parent_ids": [
-              "d14d6c0abdd253381df51a723d58691b2ee1ab08"
-            ],
             "authored_date": "2014-02-27T09:54:21.000+01:00",
             "author_name": "Dmitriy Zaporozhets",
             "author_email": "dmitriy.zaporozhets@gmail.com",
@@ -2795,11 +2791,10 @@
             "committer_email": "dmitriy.zaporozhets@gmail.com"
           },
           {
-            "id": "d14d6c0abdd253381df51a723d58691b2ee1ab08",
+            "merge_request_diff_id": 27,
+            "relative_order": 4,
+            "sha": "d14d6c0abdd253381df51a723d58691b2ee1ab08",
             "message": "Remove ds_store files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
-            "parent_ids": [
-              "c1acaa58bbcbc3eafe538cb8274ba387047b69f8"
-            ],
             "authored_date": "2014-02-27T09:49:50.000+01:00",
             "author_name": "Dmitriy Zaporozhets",
             "author_email": "dmitriy.zaporozhets@gmail.com",
@@ -2808,11 +2803,10 @@
             "committer_email": "dmitriy.zaporozhets@gmail.com"
           },
           {
-            "id": "c1acaa58bbcbc3eafe538cb8274ba387047b69f8",
+            "merge_request_diff_id": 27,
+            "relative_order": 5,
+            "sha": "c1acaa58bbcbc3eafe538cb8274ba387047b69f8",
             "message": "Ignore DS files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
-            "parent_ids": [
-              "ae73cb07c9eeaf35924a10f713b364d32b2dd34f"
-            ],
             "authored_date": "2014-02-27T09:48:32.000+01:00",
             "author_name": "Dmitriy Zaporozhets",
             "author_email": "dmitriy.zaporozhets@gmail.com",
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 c11b15a811b..d50d238ddcd 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -95,6 +95,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
         expect(MergeRequestDiffFile.where.not(diff: nil).count).to eq(9)
       end
 
+      it 'has the correct data for merge request diff commits in serialised and table formats' do
+        expect(MergeRequestDiff.where.not(st_commits: nil).count).to eq(7)
+        expect(MergeRequestDiffCommit.count).to eq(6)
+      end
+
       it 'has the correct time for merge request st_commits' do
         st_commits = MergeRequestDiff.where.not(st_commits: nil).first.st_commits
 
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 e52f79513f1..22a65e24f26 100644
--- a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb
@@ -87,6 +87,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
         expect(saved_project_json['merge_requests'].first['merge_request_diff']['merge_request_diff_files']).not_to be_empty
       end
 
+      it 'has merge request diff commits' do
+        expect(saved_project_json['merge_requests'].first['merge_request_diff']['merge_request_diff_commits']).not_to be_empty
+      end
+
       it 'has merge requests comments' do
         expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty
       end
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 697ddf52af9..b4a7e956686 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
+MergeRequestDiffCommit:
+- merge_request_diff_id
+- relative_order
+- sha
+- authored_date
+- committed_date
+- author_name
+- author_email
+- committer_name
+- committer_email
+- message
 MergeRequestDiffFile:
 - merge_request_diff_id
 - relative_order
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 2b10791ad6d..431fcda165d 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -863,7 +863,7 @@ describe Ci::Build, :models do
         pipeline2 = create(:ci_pipeline, project: project)
         @build2 = create(:ci_build, pipeline: pipeline2)
 
-        allow(@merge_request).to receive(:commits_sha)
+        allow(@merge_request).to receive(:commit_shas)
           .and_return([pipeline.sha, pipeline2.sha])
         allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request])
       end
diff --git a/spec/models/merge_request_diff_commit_spec.rb b/spec/models/merge_request_diff_commit_spec.rb
new file mode 100644
index 00000000000..dbfd1526518
--- /dev/null
+++ b/spec/models/merge_request_diff_commit_spec.rb
@@ -0,0 +1,15 @@
+require 'rails_helper'
+
+describe MergeRequestDiffCommit, type: :model do
+  let(:merge_request) { create(:merge_request) }
+  subject { merge_request.commits.first }
+
+  describe '#to_hash' do
+    it 'returns the same results as Commit#to_hash, except for parent_ids' do
+      commit_from_repo = merge_request.project.repository.commit(subject.sha)
+      commit_from_repo_hash = commit_from_repo.to_hash.merge(parent_ids: [])
+
+      expect(subject.to_hash).to eq(commit_from_repo_hash)
+    end
+  end
+end
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 4ad4abaa572..edc2f4bb9f0 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -98,7 +98,7 @@ describe MergeRequestDiff, models: true do
     end
 
     it 'saves empty state' do
-      allow_any_instance_of(MergeRequestDiff).to receive(:commits)
+      allow_any_instance_of(MergeRequestDiff).to receive_message_chain(:compare, :commits)
         .and_return([])
 
       mr_diff = create(:merge_request).merge_request_diff
@@ -107,14 +107,14 @@ describe MergeRequestDiff, models: true do
     end
   end
 
-  describe '#commits_sha' do
+  describe '#commit_shas' do
     it 'returns all commits SHA using serialized commits' do
       subject.st_commits = [
         { id: 'sha1' },
         { id: 'sha2' }
       ]
 
-      expect(subject.commits_sha).to eq(%w(sha1 sha2))
+      expect(subject.commit_shas).to eq(%w(sha1 sha2))
     end
   end
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index d91f1f1a11c..ea405fabff0 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -720,14 +720,14 @@ describe MergeRequest, models: true do
     subject { create :merge_request, :simple }
   end
 
-  describe '#commits_sha' do
+  describe '#commit_shas' do
     before do
-      allow(subject.merge_request_diff).to receive(:commits_sha)
+      allow(subject.merge_request_diff).to receive(:commit_shas)
         .and_return(['sha1'])
     end
 
     it 'delegates to merge request diff' do
-      expect(subject.commits_sha).to eq ['sha1']
+      expect(subject.commit_shas).to eq ['sha1']
     end
   end
 
@@ -752,7 +752,7 @@ describe MergeRequest, models: true do
   describe '#all_pipelines' do
     shared_examples 'returning pipelines with proper ordering' do
       let!(:all_pipelines) do
-        subject.all_commits_sha.map do |sha|
+        subject.all_commit_shas.map do |sha|
           create(:ci_empty_pipeline,
                  project: subject.source_project,
                  sha: sha,
@@ -794,16 +794,16 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#all_commits_sha' do
+  describe '#all_commit_shas' do
     context 'when merge request is persisted' do
-      let(:all_commits_sha) do
+      let(:all_commit_shas) do
         subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq
       end
 
       shared_examples 'returning all SHA' do
         it 'returns all SHA from all merge_request_diffs' do
           expect(subject.merge_request_diffs.size).to eq(2)
-          expect(subject.all_commits_sha).to eq(all_commits_sha)
+          expect(subject.all_commit_shas).to eq(all_commit_shas)
         end
       end
 
@@ -834,7 +834,7 @@ describe MergeRequest, models: true do
         end
 
         it 'returns commits from compare commits temporary data' do
-          expect(subject.all_commits_sha).to eq [commit, commit]
+          expect(subject.all_commit_shas).to eq [commit, commit]
         end
       end
 
@@ -842,7 +842,7 @@ describe MergeRequest, models: true do
         subject { build(:merge_request) }
 
         it 'returns array with diff head sha element only' do
-          expect(subject.all_commits_sha).to eq [subject.diff_head_sha]
+          expect(subject.all_commit_shas).to eq [subject.diff_head_sha]
         end
       end
     end