Commit aff5c9f3 authored by Sean McGivern's avatar Sean McGivern

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.
parent 9274c3c1
...@@ -47,7 +47,7 @@ module IssuableCollections ...@@ -47,7 +47,7 @@ module IssuableCollections
end end
def merge_requests_collection 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 end
def issues_finder def issues_finder
......
...@@ -218,7 +218,7 @@ module Ci ...@@ -218,7 +218,7 @@ module Ci
.reorder(iid: :desc) .reorder(iid: :desc)
merge_requests.find do |merge_request| merge_requests.find do |merge_request|
merge_request.commits_sha.include?(pipeline.sha) merge_request.commit_shas.include?(pipeline.sha)
end end
end end
end end
......
...@@ -138,7 +138,7 @@ class Commit ...@@ -138,7 +138,7 @@ class Commit
safe_message.split("\n", 2)[1].try(:chomp) safe_message.split("\n", 2)[1].try(:chomp)
end end
def description? def description?
description.present? description.present?
end end
......
...@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base
after_create :ensure_merge_request_diff, unless: :importing? after_create :ensure_merge_request_diff, unless: :importing?
after_update :reload_diff_if_branch_changed 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 to: :merge_request_diff, prefix: nil
# When this attribute is true some MR validation is ignored # When this attribute is true some MR validation is ignored
...@@ -518,7 +518,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -518,7 +518,7 @@ class MergeRequest < ActiveRecord::Base
def related_notes def related_notes
# Fetch comments only from last 100 commits # Fetch comments only from last 100 commits
commits_for_notes_limit = 100 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( Note.where(
"(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" + "(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" +
...@@ -841,15 +841,15 @@ class MergeRequest < ActiveRecord::Base ...@@ -841,15 +841,15 @@ class MergeRequest < ActiveRecord::Base
return Ci::Pipeline.none unless source_project return Ci::Pipeline.none unless source_project
@all_pipelines ||= source_project.pipelines @all_pipelines ||= source_project.pipelines
.where(sha: all_commits_sha, ref: source_branch) .where(sha: all_commit_shas, ref: source_branch)
.order(id: :desc) .order(id: :desc)
end end
# Note that this could also return SHA from now dangling commits # Note that this could also return SHA from now dangling commits
# #
def all_commits_sha def all_commit_shas
if persisted? 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 elsif compare_commits
compare_commits.to_a.reverse.map(&:id) compare_commits.to_a.reverse.map(&:id)
else else
......
...@@ -11,6 +11,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -11,6 +11,7 @@ class MergeRequestDiff < ActiveRecord::Base
belongs_to :merge_request belongs_to :merge_request
has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) } 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_commits # rubocop:disable Cop/ActiveRecordSerialize
serialize :st_diffs # rubocop:disable Cop/ActiveRecordSerialize serialize :st_diffs # rubocop:disable Cop/ActiveRecordSerialize
...@@ -47,14 +48,13 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -47,14 +48,13 @@ class MergeRequestDiff < ActiveRecord::Base
# Collect information about commits and diff from repository # Collect information about commits and diff from repository
# and save it to the database as serialized data # and save it to the database as serialized data
def save_git_content def save_git_content
ensure_commits_sha ensure_commit_shas
save_commits save_commits
reload_commits
save_diffs save_diffs
keep_around_commits keep_around_commits
end end
def ensure_commits_sha def ensure_commit_shas
merge_request.fetch_ref merge_request.fetch_ref
self.start_commit_sha ||= merge_request.target_branch_sha self.start_commit_sha ||= merge_request.target_branch_sha
self.head_commit_sha ||= merge_request.source_branch_sha self.head_commit_sha ||= merge_request.source_branch_sha
...@@ -66,7 +66,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -66,7 +66,7 @@ class MergeRequestDiff < ActiveRecord::Base
# created before version 8.4 that does not store head_commit_sha in separate db field. # created before version 8.4 that does not store head_commit_sha in separate db field.
def head_commit_sha def head_commit_sha
if persisted? && super.nil? if persisted? && super.nil?
last_commit.try(:sha) last_commit_sha
else else
super super
end end
...@@ -97,16 +97,11 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -97,16 +97,11 @@ class MergeRequestDiff < ActiveRecord::Base
end end
def commits def commits
@commits ||= load_commits(st_commits) @commits ||= load_commits
end end
def reload_commits def last_commit_sha
@commits = nil commit_shas.first
commits
end
def last_commit
commits.first
end end
def first_commit def first_commit
...@@ -131,8 +126,12 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -131,8 +126,12 @@ class MergeRequestDiff < ActiveRecord::Base
project.commit(head_commit_sha) project.commit(head_commit_sha)
end end
def commits_sha def commit_shas
st_commits.map { |commit| commit[:id] } if st_commits.present?
st_commits.map { |commit| commit[:id] }
else
merge_request_diff_commits.map(&:sha)
end
end end
def diff_refs=(new_diff_refs) def diff_refs=(new_diff_refs)
...@@ -207,7 +206,11 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -207,7 +206,11 @@ class MergeRequestDiff < ActiveRecord::Base
end end
def commits_count def commits_count
st_commits.count if st_commits.present?
st_commits.size
else
merge_request_diff_commits.size
end
end end
def utf8_st_diffs def utf8_st_diffs
...@@ -231,29 +234,6 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -231,29 +234,6 @@ class MergeRequestDiff < ActiveRecord::Base
raw.any? { |element| VALID_CLASSES.include?(element.class) } raw.any? { |element| VALID_CLASSES.include?(element.class) }
end 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) def create_merge_request_diff_files(diffs)
rows = diffs.map.with_index do |diff, index| rows = diffs.map.with_index do |diff, index|
diff.to_hash.merge( diff.to_hash.merge(
...@@ -294,12 +274,18 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -294,12 +274,18 @@ class MergeRequestDiff < ActiveRecord::Base
end end
end end
# Load diffs between branches related to current merge request diff from repo def load_commits
# and save it as array of hashes in st_diffs db field 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 def save_diffs
new_attributes = {} new_attributes = {}
if commits.size.zero? if compare.commits.size.zero?
new_attributes[:state] = :empty new_attributes[:state] = :empty
else else
diff_collection = compare.diffs(Commit.max_diff_options) diff_collection = compare.diffs(Commit.max_diff_options)
...@@ -319,7 +305,13 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -319,7 +305,13 @@ class MergeRequestDiff < ActiveRecord::Base
new_attributes[:state] = :overflow if diff_collection.overflow? new_attributes[:state] = :overflow if diff_collection.overflow?
end 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 end
def repository def repository
...@@ -332,29 +324,6 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -332,29 +324,6 @@ class MergeRequestDiff < ActiveRecord::Base
project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha) project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha)
end 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 def keep_around_commits
[repository, merge_request.source_project.repository].each do |repo| [repository, merge_request.source_project.repository].each do |repo|
repo.keep_around(start_commit_sha) repo.keep_around(start_commit_sha)
......
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
...@@ -68,7 +68,7 @@ module MergeRequests ...@@ -68,7 +68,7 @@ module MergeRequests
if merge_request.source_branch == @branch_name || force_push? if merge_request.source_branch == @branch_name || force_push?
merge_request.reload_diff(current_user) merge_request.reload_diff(current_user)
else else
mr_commit_ids = merge_request.commits_sha mr_commit_ids = merge_request.commit_shas
push_commit_ids = @commits.map(&:id) push_commit_ids = @commits.map(&:id)
matches = mr_commit_ids & push_commit_ids matches = mr_commit_ids & push_commit_ids
merge_request.reload_diff(current_user) if matches.any? merge_request.reload_diff(current_user) if matches.any?
...@@ -128,7 +128,7 @@ module MergeRequests ...@@ -128,7 +128,7 @@ module MergeRequests
return unless @commits.present? return unless @commits.present?
merge_requests_for_source_branch.each do |merge_request| 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| new_commits, existing_commits = @commits.partition do |commit|
mr_commit_ids.include?(commit.id) mr_commit_ids.include?(commit.id)
...@@ -144,7 +144,7 @@ module MergeRequests ...@@ -144,7 +144,7 @@ module MergeRequests
return unless @commits.present? return unless @commits.present?
merge_requests_for_source_branch.each do |merge_request| 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| wip_commit = @commits.detect do |commit|
commit.work_in_progress? && commit_shas.include?(commit.sha) commit.work_in_progress? && commit_shas.include?(commit.sha)
......
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
...@@ -693,6 +693,21 @@ ActiveRecord::Schema.define(version: 20170703102400) do ...@@ -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", ["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 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| create_table "merge_request_diff_files", id: false, force: :cascade do |t|
t.integer "merge_request_diff_id", null: false t.integer "merge_request_diff_id", null: false
t.integer "relative_order", null: false t.integer "relative_order", null: false
...@@ -1563,6 +1578,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do ...@@ -1563,6 +1578,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do
add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade 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", "boards", name: "fk_0d3f677137", on_delete: :cascade
add_foreign_key "lists", "labels", name: "fk_7a5553d60f", 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_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_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
......
...@@ -13,6 +13,10 @@ module Gitlab ...@@ -13,6 +13,10 @@ module Gitlab
MergeRequestDiff.arel_table MergeRequestDiff.arel_table
end end
def mr_diff_commits_table
MergeRequestDiffCommit.arel_table
end
def mr_closing_issues_table def mr_closing_issues_table
MergeRequestsClosingIssues.arel_table MergeRequestsClosingIssues.arel_table
end end
......
...@@ -2,40 +2,59 @@ module Gitlab ...@@ -2,40 +2,59 @@ module Gitlab
module CycleAnalytics module CycleAnalytics
class PlanEventFetcher < BaseEventFetcher class PlanEventFetcher < BaseEventFetcher
def initialize(*args) 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]] issue_metrics_table[:first_mentioned_in_commit_at]]
super(*args) super(*args)
end end
def events_query 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 super
end end
private private
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 serialize(event) def serialize(event)
st_commit = first_time_reference_commit(event.delete('commits'), event) commit = first_time_reference_commit(event)
return unless st_commit return unless commit
serialize_commit(event, st_commit, query) serialize_commit(event, commit, query)
end end
def first_time_reference_commit(commits, event) 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? 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'] 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 commit[:committed_date].to_i == DateTime.parse(event['first_mentioned_in_commit_at'].to_s).to_i
end end
end end
def serialize_commit(event, st_commit, query) def serialize_commit(event, commit, query)
commit = Commit.new(Gitlab::Git::Commit.new(st_commit), @project) commit = Commit.new(Gitlab::Git::Commit.new(commit.to_hash), @project)
AnalyticsCommitSerializer.new(project: @project, total_time: event['total_time']).represent(commit) AnalyticsCommitSerializer.new(project: @project, total_time: event['total_time']).represent(commit)
end end
......
...@@ -27,6 +27,7 @@ project_tree: ...@@ -27,6 +27,7 @@ project_tree:
- :author - :author
- :events - :events
- merge_request_diff: - merge_request_diff:
- :merge_request_diff_commits
- :merge_request_diff_files - :merge_request_diff_files
- :events - :events
- :timelogs - :timelogs
......
...@@ -88,7 +88,10 @@ merge_requests: ...@@ -88,7 +88,10 @@ merge_requests:
- head_pipeline - head_pipeline
merge_request_diff: merge_request_diff:
- merge_request - merge_request
- merge_request_diff_commits
- merge_request_diff_files - merge_request_diff_files
merge_request_diff_commits:
- merge_request_diff
merge_request_diff_files: merge_request_diff_files:
- merge_request_diff - merge_request_diff
pipelines: pipelines:
......
...@@ -2741,13 +2741,12 @@ ...@@ -2741,13 +2741,12 @@
"merge_request_diff": { "merge_request_diff": {
"id": 27, "id": 27,
"state": "collected", "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", "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", "authored_date": "2014-08-06T08:35:52.000+02:00",
"author_name": "Dmitriy Zaporozhets", "author_name": "Dmitriy Zaporozhets",
"author_email": "dmitriy.zaporozhets@gmail.com", "author_email": "dmitriy.zaporozhets@gmail.com",
...@@ -2756,11 +2755,10 @@ ...@@ -2756,11 +2755,10 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "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", "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", "authored_date": "2014-02-27T10:01:38.000+01:00",
"author_name": "Dmitriy Zaporozhets", "author_name": "Dmitriy Zaporozhets",
"author_email": "dmitriy.zaporozhets@gmail.com", "author_email": "dmitriy.zaporozhets@gmail.com",
...@@ -2769,11 +2767,10 @@ ...@@ -2769,11 +2767,10 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "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", "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", "authored_date": "2014-02-27T09:57:31.000+01:00",
"author_name": "Dmitriy Zaporozhets", "author_name": "Dmitriy Zaporozhets",
"author_email": "dmitriy.zaporozhets@gmail.com", "author_email": "dmitriy.zaporozhets@gmail.com",
...@@ -2782,11 +2779,10 @@ ...@@ -2782,11 +2779,10 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "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", "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", "authored_date": "2014-02-27T09:54:21.000+01:00",
"author_name": "Dmitriy Zaporozhets", "author_name": "Dmitriy Zaporozhets",
"author_email": "dmitriy.zaporozhets@gmail.com", "author_email": "dmitriy.zaporozhets@gmail.com",
...@@ -2795,11 +2791,10 @@ ...@@ -2795,11 +2791,10 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "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", "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", "authored_date": "2014-02-27T09:49:50.000+01:00",
"author_name": "Dmitriy Zaporozhets", "author_name": "Dmitriy Zaporozhets",
"author_email": "dmitriy.zaporozhets@gmail.com", "author_email": "dmitriy.zaporozhets@gmail.com",
...@@ -2808,11 +2803,10 @@ ...@@ -2808,11 +2803,10 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "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", "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", "authored_date": "2014-02-27T09:48:32.000+01:00",
"author_name": "Dmitriy Zaporozhets", "author_name": "Dmitriy Zaporozhets",
"author_email": "dmitriy.zaporozhets@gmail.com", "author_email": "dmitriy.zaporozhets@gmail.com",
......
...@@ -95,6 +95,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do ...@@ -95,6 +95,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
expect(MergeRequestDiffFile.where.not(diff: nil).count).to eq(9) expect(MergeRequestDiffFile.where.not(diff: nil).count).to eq(9)
end 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 it 'has the correct time for merge request st_commits' do
st_commits = MergeRequestDiff.where.not(st_commits: nil).first.st_commits st_commits = MergeRequestDiff.where.not(st_commits: nil).first.st_commits
......
...@@ -87,6 +87,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do ...@@ -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 expect(saved_project_json['merge_requests'].first['merge_request_diff']['merge_request_diff_files']).not_to be_empty
end 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 it 'has merge requests comments' do
expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty
end end
......
...@@ -172,6 +172,17 @@ MergeRequestDiff: ...@@ -172,6 +172,17 @@ MergeRequestDiff:
- real_size - real_size
- head_commit_sha - head_commit_sha
- start_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: MergeRequestDiffFile:
- merge_request_diff_id - merge_request_diff_id
- relative_order - relative_order
......
...@@ -863,7 +863,7 @@ describe Ci::Build, :models do ...@@ -863,7 +863,7 @@ describe Ci::Build, :models do
pipeline2 = create(:ci_pipeline, project: project) pipeline2 = create(:ci_pipeline, project: project)
@build2 = create(:ci_build, pipeline: pipeline2) @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]) .and_return([pipeline.sha, pipeline2.sha])
allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request]) allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request])
end end
......
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
...@@ -98,7 +98,7 @@ describe MergeRequestDiff, models: true do ...@@ -98,7 +98,7 @@ describe MergeRequestDiff, models: true do
end end
it 'saves empty state' do 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([]) .and_return([])
mr_diff = create(:merge_request).merge_request_diff mr_diff = create(:merge_request).merge_request_diff
...@@ -107,14 +107,14 @@ describe MergeRequestDiff, models: true do ...@@ -107,14 +107,14 @@ describe MergeRequestDiff, models: true do
end end
end end
describe '#commits_sha' do describe '#commit_shas' do
it 'returns all commits SHA using serialized commits' do it 'returns all commits SHA using serialized commits' do
subject.st_commits = [ subject.st_commits = [
{ id: 'sha1' }, { id: 'sha1' },
{ id: 'sha2' } { id: 'sha2' }
] ]
expect(subject.commits_sha).to eq(%w(sha1 sha2)) expect(subject.commit_shas).to eq(%w(sha1 sha2))
end end
end end
......
...@@ -720,14 +720,14 @@ describe MergeRequest, models: true do ...@@ -720,14 +720,14 @@ describe MergeRequest, models: true do
subject { create :merge_request, :simple } subject { create :merge_request, :simple }
end end
describe '#commits_sha' do describe '#commit_shas' do
before do before do
allow(subject.merge_request_diff).to receive(:commits_sha) allow(subject.merge_request_diff).to receive(:commit_shas)
.and_return(['sha1']) .and_return(['sha1'])
end end
it 'delegates to merge request diff' do it 'delegates to merge request diff' do
expect(subject.commits_sha).to eq ['sha1'] expect(subject.commit_shas).to eq ['sha1']
end end
end end
...@@ -752,7 +752,7 @@ describe MergeRequest, models: true do ...@@ -752,7 +752,7 @@ describe MergeRequest, models: true do
describe '#all_pipelines' do describe '#all_pipelines' do
shared_examples 'returning pipelines with proper ordering' do shared_examples 'returning pipelines with proper ordering' do
let!(:all_pipelines) do let!(:all_pipelines) do
subject.all_commits_sha.map do |sha| subject.all_commit_shas.map do |sha|
create(:ci_empty_pipeline, create(:ci_empty_pipeline,
project: subject.source_project, project: subject.source_project,
sha: sha, sha: sha,
...@@ -794,16 +794,16 @@ describe MergeRequest, models: true do ...@@ -794,16 +794,16 @@ describe MergeRequest, models: true do
end end
end end
describe '#all_commits_sha' do describe '#all_commit_shas' do
context 'when merge request is persisted' 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 subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq
end end
shared_examples 'returning all SHA' do shared_examples 'returning all SHA' do
it 'returns all SHA from all merge_request_diffs' do it 'returns all SHA from all merge_request_diffs' do
expect(subject.merge_request_diffs.size).to eq(2) 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
end end
...@@ -834,7 +834,7 @@ describe MergeRequest, models: true do ...@@ -834,7 +834,7 @@ describe MergeRequest, models: true do
end end
it 'returns commits from compare commits temporary data' do 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
end end
...@@ -842,7 +842,7 @@ describe MergeRequest, models: true do ...@@ -842,7 +842,7 @@ describe MergeRequest, models: true do
subject { build(:merge_request) } subject { build(:merge_request) }
it 'returns array with diff head sha element only' do 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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment