Commit 1efa9604 authored by Sean McGivern's avatar Sean McGivern

Merge branch '58805-allow-incomplete-commit-data-to-be-fetched-from-collection' into 'master'

Enrich commits with full data in CommitCollection

Closes #58805

See merge request gitlab-org/gitlab-ce!26144
parents d0ea73e5 5d4e02f1
......@@ -28,10 +28,43 @@ class CommitCollection
def without_merge_commits
strong_memoize(:without_merge_commits) do
commits.reject(&:merge_commit?)
# `#enrich!` the collection to ensure all commits contain
# the necessary parent data
enrich!.commits.reject(&:merge_commit?)
end
end
def unenriched
commits.reject(&:gitaly_commit?)
end
def fully_enriched?
unenriched.empty?
end
# Batch load any commits that are not backed by full gitaly data, and
# replace them in the collection.
def enrich!
# A project is needed in order to fetch data from gitaly. Projects
# can be absent from commits in certain rare situations (like when
# viewing a MR of a deleted fork). In these cases, assume that the
# enriched data is not needed.
return self if project.blank? || fully_enriched?
# Batch load full Commits from the repository
# and map to a Hash of id => Commit
replacements = Hash[unenriched.map do |c|
[c.id, Commit.lazy(project, c.id)]
end.compact]
# Replace the commits, keeping the same order
@commits = @commits.map do |c|
replacements.fetch(c.id, c)
end
self
end
# Sets the pipeline status for every commit.
#
# Setting this status ahead of time removes the need for running a query for
......
......@@ -318,6 +318,10 @@ module Gitlab
parent_ids.size > 1
end
def gitaly_commit?
raw_commit.is_a?(Gitaly::GitCommit)
end
def tree_entry(path)
return unless path.present?
......@@ -340,7 +344,7 @@ module Gitlab
end
def to_gitaly_commit
return raw_commit if raw_commit.is_a?(Gitaly::GitCommit)
return raw_commit if gitaly_commit?
message_split = raw_commit.message.split("\n", 2)
Gitaly::GitCommit.new(
......
......@@ -537,6 +537,18 @@ describe Gitlab::Git::Commit, :seed_helper do
end
end
describe '#gitaly_commit?' do
context 'when the commit data comes from gitaly' do
it { expect(commit.gitaly_commit?).to eq(true) }
end
context 'when the commit data comes from a Hash' do
let(:commit) { described_class.new(repository, sample_commit_hash) }
it { expect(commit.gitaly_commit?).to eq(false) }
end
end
describe '#has_zero_stats?' do
it { expect(commit.has_zero_stats?).to eq(false) }
end
......
......@@ -37,12 +37,92 @@ describe CommitCollection do
describe '#without_merge_commits' do
it 'returns all commits except merge commits' do
merge_commit = project.commit("60ecb67744cb56576c30214ff52294f8ce2def98")
expect(merge_commit).to receive(:merge_commit?).and_return(true)
collection = described_class.new(project, [
build(:commit),
build(:commit, :merge_commit)
commit,
merge_commit
])
expect(collection.without_merge_commits.size).to eq(1)
expect(collection.without_merge_commits).to contain_exactly(commit)
end
end
describe 'enrichment methods' do
let(:gitaly_commit) { commit }
let(:hash_commit) { Commit.from_hash(gitaly_commit.to_hash, project) }
describe '#unenriched' do
it 'returns all commits that are not backed by gitaly data' do
collection = described_class.new(project, [gitaly_commit, hash_commit])
expect(collection.unenriched).to contain_exactly(hash_commit)
end
end
describe '#fully_enriched?' do
it 'returns true when all commits are backed by gitaly data' do
collection = described_class.new(project, [gitaly_commit, gitaly_commit])
expect(collection.fully_enriched?).to eq(true)
end
it 'returns false when any commits are not backed by gitaly data' do
collection = described_class.new(project, [gitaly_commit, hash_commit])
expect(collection.fully_enriched?).to eq(false)
end
it 'returns true when the collection is empty' do
collection = described_class.new(project, [])
expect(collection.fully_enriched?).to eq(true)
end
end
describe '#enrich!' do
it 'replaces commits in the collection with those backed by gitaly data' do
collection = described_class.new(project, [hash_commit])
collection.enrich!
new_commit = collection.commits.first
expect(new_commit.id).to eq(hash_commit.id)
expect(hash_commit.gitaly_commit?).to eq(false)
expect(new_commit.gitaly_commit?).to eq(true)
end
it 'maintains the original order of the commits' do
gitaly_commits = [gitaly_commit] * 3
hash_commits = [hash_commit] * 3
# Interleave the gitaly and hash commits together
original_commits = gitaly_commits.zip(hash_commits).flatten
collection = described_class.new(project, original_commits)
collection.enrich!
original_commits.each_with_index do |original_commit, i|
new_commit = collection.commits[i]
expect(original_commit.id).to eq(new_commit.id)
end
end
it 'fetches data if there are unenriched commits' do
collection = described_class.new(project, [hash_commit])
expect(Commit).to receive(:lazy).exactly(:once)
collection.enrich!
end
it 'does not fetch data if all commits are enriched' do
collection = described_class.new(project, [gitaly_commit])
expect(Commit).not_to receive(:lazy)
collection.enrich!
end
end
end
......
......@@ -84,32 +84,27 @@ describe MergeRequest do
describe '#default_squash_commit_message' do
let(:project) { subject.project }
def commit_collection(commit_hashes)
raw_commits = commit_hashes.map { |raw| Commit.from_hash(raw, project) }
CommitCollection.new(project, raw_commits)
end
let(:is_multiline) { -> (c) { c.description.present? } }
let(:multiline_commits) { subject.commits.select(&is_multiline) }
let(:singleline_commits) { subject.commits.reject(&is_multiline) }
it 'returns the oldest multiline commit message' do
commits = commit_collection([
{ message: 'Singleline', parent_ids: [] },
{ message: "Second multiline\nCommit message", parent_ids: [] },
{ message: "First multiline\nCommit message", parent_ids: [] }
])
expect(subject).to receive(:commits).and_return(commits)
expect(subject.default_squash_commit_message).to eq("First multiline\nCommit message")
expect(subject.default_squash_commit_message).to eq(multiline_commits.last.message)
end
it 'returns the merge request title if there are no multiline commits' do
commits = commit_collection([
{ message: 'Singleline', parent_ids: [] }
])
expect(subject).to receive(:commits).and_return(
CommitCollection.new(project, singleline_commits)
)
expect(subject.default_squash_commit_message).to eq(subject.title)
end
expect(subject).to receive(:commits).and_return(commits)
it 'does not return commit messages from multiline merge commits' do
collection = CommitCollection.new(project, multiline_commits).enrich!
expect(collection.commits).to all( receive(:merge_commit?).and_return(true) )
expect(subject).to receive(:commits).and_return(collection)
expect(subject.default_squash_commit_message).to eq(subject.title)
end
end
......@@ -1045,7 +1040,7 @@ describe MergeRequest do
describe '#commit_authors' do
it 'returns all the authors of every commit in the merge request' do
users = subject.commits.map(&:author_email).uniq.map do |email|
users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email|
create(:user, email: email)
end
......@@ -1059,7 +1054,7 @@ describe MergeRequest do
describe '#authors' do
it 'returns a list with all the commit authors in the merge request and author' do
users = subject.commits.map(&:author_email).uniq.map do |email|
users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email|
create(:user, email: email)
end
......
......@@ -279,13 +279,18 @@ describe MergeRequestWidgetEntity do
end
describe 'commits_without_merge_commits' do
it 'should not include merge commits' do
# Mock all but the first 5 commits to be merge commits
resource.commits.each_with_index do |commit, i|
expect(commit).to receive(:merge_commit?).at_least(:once).and_return(i > 4)
def find_matching_commit(short_id)
resource.commits.find { |c| c.short_id == short_id }
end
expect(subject[:commits_without_merge_commits].size).to eq(5)
it 'should not include merge commits' do
commits_in_widget = subject[:commits_without_merge_commits]
expect(commits_in_widget.length).to be < resource.commits.length
expect(commits_in_widget.length).to eq(resource.commits.without_merge_commits.length)
commits_in_widget.each do |c|
expect(find_matching_commit(c[:short_id]).merge_commit?).to eq(false)
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