Enrich commits with full data in CommitCollection

Allow incomplete commit records to load their full data from gitaly.

Commits can be based on a Hash of data retrieved from PostgreSQL, and
this data can be intentionally incomplete in order to save space.

A new method #gitaly? has been added to Gitlab::Git::Commit, which
returns true if the underlying data source of the Commit is a
Gitaly::GitCommit.

CommitCollection now has a method #enrich which replaces non-gitaly
commits in place with commits from gitaly.

CommitCollection#without_merge_commits has been updated to call this
method, as in order to determine a merge commit we need to have parent
data.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/58805

4 files changed, 119 insertions, 27 deletions

diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index 3fb41a626b2..4a4ac833e39 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -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
diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb
index 0f5d03ff458..30c504ebea8 100644
--- a/spec/models/commit_collection_spec.rb
+++ b/spec/models/commit_collection_spec.rb
@@ -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
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 42c49e330cc..c487ae45ceb 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -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
diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index 4dbd79f2fc0..727fd8951f2 100644
--- a/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -279,13 +279,18 @@ describe MergeRequestWidgetEntity do
   end
 
   describe 'commits_without_merge_commits' do
+    def find_matching_commit(short_id)
+      resource.commits.find { |c| c.short_id == short_id }
+    end
+
     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)
-      end
+      commits_in_widget = subject[:commits_without_merge_commits]
 
-      expect(subject[:commits_without_merge_commits].size).to eq(5)
+      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