From cb6f8cdfc23a18daf33288c95ae8badec63f53ad Mon Sep 17 00:00:00 2001
From: Adam Niedzielski <adamsunday@gmail.com>
Date: Thu, 1 Dec 2016 12:17:30 +0100
Subject: [PATCH] Replace references to MergeRequestDiff#commits with
st_commits when we care only about the number of commits
We do not have to instantiate all objects in this case.
---
.../projects/merge_requests_controller.rb | 4 +--
app/models/ci/build.rb | 2 +-
app/models/merge_request.rb | 17 ++++++----
app/models/merge_request_diff.rb | 10 +++---
.../merge_requests/refresh_service.rb | 4 +--
.../merge_requests/show/_versions.html.haml | 2 +-
.../merge_requests/widget/_open.html.haml | 2 +-
.../use-st-commits-where-possible.yml | 5 +++
spec/models/build_spec.rb | 4 +--
spec/models/merge_request_diff_spec.rb | 34 +++++++++----------
spec/models/merge_request_spec.rb | 33 ++++++++++++++----
11 files changed, 73 insertions(+), 44 deletions(-)
create mode 100644 changelogs/unreleased/use-st-commits-where-possible.yml
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index f47df8b623b..d2cef52842c 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index e7d33bd26db..88c46076df6 100644
--- a/app/models/ci/build.rb
+++ b/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
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 64990f8134e..bfb016df46d 100644
--- a/app/models/merge_request.rb
+++ b/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
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 58a24eb84cb..b8f36a2c958 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -127,11 +127,7 @@ class MergeRequestDiff < ActiveRecord::Base
end
def commits_sha
- if @commits
- commits.map(&:sha)
- else
- st_commits.map { |commit| commit[:id] }
- end
+ st_commits.map { |commit| commit[:id] }
end
def diff_refs
@@ -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"].
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 22596b4014a..e4056306bc4 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/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)
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index eab48b78cb3..5cc92595fe0 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/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
diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml
index 20c93930abc..eee711dc5af 100644
--- a/app/views/projects/merge_requests/widget/_open.html.haml
+++ b/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'
diff --git a/changelogs/unreleased/use-st-commits-where-possible.yml b/changelogs/unreleased/use-st-commits-where-possible.yml
new file mode 100644
index 00000000000..e4395461560
--- /dev/null
+++ b/changelogs/unreleased/use-st-commits-where-possible.yml
@@ -0,0 +1,5 @@
+---
+title: Replace references to MergeRequestDiff#commits with st_commits when we care
+ only about the number of commits
+merge_request: 7668
+author:
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index ef07f2275b1..d4970e38f7c 100644
--- a/spec/models/build_spec.rb
+++ b/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
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index e5007424041..eb876d105da 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 26034cb1c7b..ec22ef93465 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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
--
GitLab