Merge branch 'revert-1efa9604' into 'master'

Revert "Enrich commits with full data in CommitCollection"

See merge request gitlab-org/gitlab-ce!26412

app/models/commit_collection.rb

@@ -28,43 +28,10 @@ class CommitCollection
 
   def without_merge_commits
     strong_memoize(:without_merge_commits) do
-      # `#enrich!` the collection to ensure all commits contain
-      # the necessary parent data
-      enrich!.commits.reject(&:merge_commit?)
+      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

lib/gitlab/git/commit.rb

@@ -318,10 +318,6 @@ 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?
 
@@ -344,7 +340,7 @@ module Gitlab
       end
 
       def to_gitaly_commit
-        return raw_commit if gitaly_commit?
+        return raw_commit if raw_commit.is_a?(Gitaly::GitCommit)
 
         message_split = raw_commit.message.split("\n", 2)
         Gitaly::GitCommit.new(

spec/lib/gitlab/git/commit_spec.rb

@@ -537,18 +537,6 @@ 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

spec/models/commit_collection_spec.rb

@@ -37,92 +37,12 @@ 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, [
-        commit,
-        merge_commit
+        build(:commit),
+        build(:commit, :merge_commit)
       ])
 
-      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
+      expect(collection.without_merge_commits.size).to eq(1)
     end
   end
 

spec/models/merge_request_spec.rb

@@ -84,27 +84,32 @@ describe MergeRequest do
 
   describe '#default_squash_commit_message' do
     let(:project) { subject.project }
-    let(:is_multiline) { -> (c) { c.description.present? } }
-    let(:multiline_commits) { subject.commits.select(&is_multiline) }
-    let(:singleline_commits) { subject.commits.reject(&is_multiline) }
+
+    def commit_collection(commit_hashes)
+      raw_commits = commit_hashes.map { |raw| Commit.from_hash(raw, project) }
+
+      CommitCollection.new(project, raw_commits)
+    end
 
     it 'returns the oldest multiline commit message' do
-      expect(subject.default_squash_commit_message).to eq(multiline_commits.last.message)
+      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")
     end
 
     it 'returns the merge request title if there are no multiline commits' do
-      expect(subject).to receive(:commits).and_return(
-        CommitCollection.new(project, singleline_commits)
-      )
+      commits = commit_collection([
+        { message: 'Singleline', parent_ids: [] }
+      ])
 
-      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
@@ -1040,7 +1045,7 @@ describe MergeRequest do
 
   describe '#commit_authors' do
     it 'returns all the authors of every commit in the merge request' do
-      users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email|
+      users = subject.commits.map(&:author_email).uniq.map do |email|
         create(:user, email: email)
       end
 
@@ -1054,7 +1059,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.without_merge_commits.map(&:author_email).uniq.map do |email|
+      users = subject.commits.map(&:author_email).uniq.map do |email|
         create(:user, email: email)
       end
 

spec/serializers/merge_request_widget_entity_spec.rb

@@ -279,18 +279,13 @@ 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
-      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)
+      # 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
+
+      expect(subject[:commits_without_merge_commits].size).to eq(5)
     end
   end
 end