Merge branch 'use-st-commits-where-possible' into 'master'

Replace references to MergeRequestDiff#commits with st_commits when we care only about the number of commits

See merge request !7668

app/controllers/projects/merge_requests_controller.rb

@@ -492,7 +492,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def validates_merge_request
     # Show git not found page
     # if there is no saved commits between source & target branch
-    if @merge_request.commits.blank?
+    if @merge_request.has_no_commits?
       # and if target branch doesn't exist
       return invalid_mr unless @merge_request.target_branch_exists?
     end
@@ -500,7 +500,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def define_show_vars
     @noteable = @merge_request
-    @commits_count = @merge_request.commits.count
+    @commits_count = @merge_request.commits_count
 
     if @merge_request.locked_long_ago?
       @merge_request.unlock_mr

app/models/ci/build.rb

@@ -203,7 +203,7 @@ module Ci
                                    .reorder(iid: :asc)
 
       merge_requests.find do |merge_request|
-        merge_request.commits.any? { |ci| ci.id == pipeline.sha }
+        merge_request.commits_sha.include?(pipeline.sha)
       end
     end
 

app/models/merge_request.rb

@@ -22,7 +22,8 @@ class MergeRequest < ActiveRecord::Base
   after_create :ensure_merge_request_diff, unless: :importing?
   after_update :reload_diff_if_branch_changed
 
-  delegate :commits, :real_size, to: :merge_request_diff, prefix: nil
+  delegate :commits, :real_size, :commits_sha, :commits_count,
+    to: :merge_request_diff, prefix: nil
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
@@ -662,7 +663,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def broken?
-    self.commits.blank? || branch_missing? || cannot_be_merged?
+    has_no_commits? || branch_missing? || cannot_be_merged?
   end
 
   def can_be_merged_by?(user)
@@ -770,10 +771,6 @@ class MergeRequest < ActiveRecord::Base
     diverged_commits_count > 0
   end
 
-  def commits_sha
-    commits.map(&:sha)
-  end
-
   def head_pipeline
     return unless diff_head_sha && source_project
 
@@ -871,4 +868,12 @@ class MergeRequest < ActiveRecord::Base
       @conflicts_can_be_resolved_in_ui = false
     end
   end
+
+  def has_commits?
+    commits_count > 0
+  end
+
+  def has_no_commits?
+    !has_commits?
+  end
 end

app/models/merge_request_diff.rb

@@ -127,12 +127,8 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def commits_sha
-    if @commits
-      commits.map(&:sha)
-    else
     st_commits.map { |commit| commit[:id] }
   end
-  end
 
   def diff_refs
     return unless start_commit_sha || base_commit_sha
@@ -176,6 +172,10 @@ class MergeRequestDiff < ActiveRecord::Base
     CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight)
   end
 
+  def commits_count
+    st_commits.count
+  end
+
   private
 
   # Old GitLab implementations may have generated diffs as ["--broken-diff"].

app/services/merge_requests/refresh_service.rb

@@ -63,7 +63,7 @@ module MergeRequests
         if merge_request.source_branch == @branch_name || force_push?
           merge_request.reload_diff
         else
-          mr_commit_ids = merge_request.commits.map(&:id)
+          mr_commit_ids = merge_request.commits_sha
           push_commit_ids = @commits.map(&:id)
           matches = mr_commit_ids & push_commit_ids
           merge_request.reload_diff if matches.any?
@@ -123,7 +123,7 @@ module MergeRequests
       return unless @commits.present?
 
       merge_requests_for_source_branch.each do |merge_request|
-        mr_commit_ids = Set.new(merge_request.commits.map(&:id))
+        mr_commit_ids = Set.new(merge_request.commits_sha)
 
         new_commits, existing_commits = @commits.partition do |commit|
           mr_commit_ids.include?(commit.id)

app/views/projects/merge_requests/show/_versions.html.haml

@@ -27,7 +27,7 @@
                         version #{version_index(merge_request_diff)}
                     .monospace #{short_sha(merge_request_diff.head_commit_sha)}
                     %small
-                      #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)},
+                      #{number_with_delimiter(merge_request_diff.commits_count)} #{'commit'.pluralize(merge_request_diff.commits_count)},
                       = time_ago_with_tooltip(merge_request_diff.created_at)
 
       - if @merge_request_diff.base_commit_sha

app/views/projects/merge_requests/widget/_open.html.haml

@@ -11,7 +11,7 @@
       = render 'projects/merge_requests/widget/open/archived'
     - elsif @merge_request.branch_missing?
       = render 'projects/merge_requests/widget/open/missing_branch'
-    - elsif @merge_request.commits.blank?
+    - elsif @merge_request.has_no_commits?
       = render 'projects/merge_requests/widget/open/nothing'
     - elsif @merge_request.unchecked?
       = render 'projects/merge_requests/widget/open/check'

changelogs/unreleased/use-st-commits-where-possible.yml

+---
+title: Replace references to MergeRequestDiff#commits with st_commits when we care
+  only about the number of commits
+merge_request: 7668
+author:

spec/models/build_spec.rb

@@ -730,8 +730,8 @@ describe Ci::Build, models: true do
         pipeline2 = create(:ci_pipeline, project: project)
         @build2 = create(:ci_build, pipeline: pipeline2)
 
-        commits = [double(id: pipeline.sha), double(id: pipeline2.sha)]
-        allow(@merge_request).to receive(:commits).and_return(commits)
+        allow(@merge_request).to receive(:commits_sha).
+          and_return([pipeline.sha, pipeline2.sha])
         allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request])
       end
 

spec/models/merge_request_diff_spec.rb

@@ -77,24 +77,13 @@ describe MergeRequestDiff, models: true do
   end
 
   describe '#commits_sha' do
-    shared_examples 'returning all commits SHA' do
-      it 'returns all commits SHA' do
-        commits_sha = subject.commits_sha
+    it 'returns all commits SHA using serialized commits' do
+      subject.st_commits = [
+        { id: 'sha1' },
+        { id: 'sha2' }
+      ]
 
-        expect(commits_sha).to eq(subject.commits.map(&:sha))
-      end
-    end
-
-    context 'when commits were loaded' do
-      before do
-        subject.commits
-      end
-
-      it_behaves_like 'returning all commits SHA'
-    end
-
-    context 'when commits were not loaded' do
-      it_behaves_like 'returning all commits SHA'
+      expect(subject.commits_sha).to eq(['sha1', 'sha2'])
     end
   end
 
@@ -113,4 +102,15 @@ describe MergeRequestDiff, models: true do
       expect(diffs.size).to eq(3)
     end
   end
+
+  describe '#commits_count' do
+    it 'returns number of commits using serialized commits' do
+      subject.st_commits = [
+        { id: 'sha1' },
+        { id: 'sha2' }
+      ]
+
+      expect(subject.commits_count).to eq 2
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -557,16 +557,13 @@ describe MergeRequest, models: true do
   end
 
   describe '#commits_sha' do
-    let(:commit0) { double('commit0', sha: 'sha1') }
-    let(:commit1) { double('commit1', sha: 'sha2') }
-    let(:commit2) { double('commit2', sha: 'sha3') }
-
     before do
-      allow(subject.merge_request_diff).to receive(:commits).and_return([commit0, commit1, commit2])
+      allow(subject.merge_request_diff).to receive(:commits_sha).
+        and_return(['sha1'])
     end
 
-    it 'returns sha of commits' do
-      expect(subject.commits_sha).to contain_exactly('sha1', 'sha2', 'sha3')
+    it 'delegates to merge request diff' do
+      expect(subject.commits_sha).to eq ['sha1']
     end
   end
 
@@ -1440,4 +1437,26 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe '#has_commits?' do
+    before do
+      allow(subject.merge_request_diff).to receive(:commits_count).
+        and_return(2)
+    end
+
+    it 'returns true when merge request diff has commits' do
+      expect(subject.has_commits?).to be_truthy
+    end
+  end
+
+  describe '#has_no_commits?' do
+    before do
+      allow(subject.merge_request_diff).to receive(:commits_count).
+        and_return(0)
+    end
+
+    it 'returns true when merge request diff has 0 commits' do
+      expect(subject.has_no_commits?).to be_truthy
+    end
+  end
 end