Commit cb6f8cdf authored by Adam Niedzielski's avatar Adam Niedzielski

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.
parent 2adc6568
...@@ -492,7 +492,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -492,7 +492,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def validates_merge_request def validates_merge_request
# Show git not found page # Show git not found page
# if there is no saved commits between source & target branch # 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 # and if target branch doesn't exist
return invalid_mr unless @merge_request.target_branch_exists? return invalid_mr unless @merge_request.target_branch_exists?
end end
...@@ -500,7 +500,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -500,7 +500,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def define_show_vars def define_show_vars
@noteable = @merge_request @noteable = @merge_request
@commits_count = @merge_request.commits.count @commits_count = @merge_request.commits_count
if @merge_request.locked_long_ago? if @merge_request.locked_long_ago?
@merge_request.unlock_mr @merge_request.unlock_mr
......
...@@ -203,7 +203,7 @@ module Ci ...@@ -203,7 +203,7 @@ module Ci
.reorder(iid: :asc) .reorder(iid: :asc)
merge_requests.find do |merge_request| merge_requests.find do |merge_request|
merge_request.commits.any? { |ci| ci.id == pipeline.sha } merge_request.commits_sha.include?(pipeline.sha)
end end
end end
......
...@@ -22,7 +22,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -22,7 +22,8 @@ class MergeRequest < ActiveRecord::Base
after_create :ensure_merge_request_diff, unless: :importing? after_create :ensure_merge_request_diff, unless: :importing?
after_update :reload_diff_if_branch_changed 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 # When this attribute is true some MR validation is ignored
# It allows us to close or modify broken merge requests # It allows us to close or modify broken merge requests
...@@ -662,7 +663,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -662,7 +663,7 @@ class MergeRequest < ActiveRecord::Base
end end
def broken? def broken?
self.commits.blank? || branch_missing? || cannot_be_merged? has_no_commits? || branch_missing? || cannot_be_merged?
end end
def can_be_merged_by?(user) def can_be_merged_by?(user)
...@@ -770,10 +771,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -770,10 +771,6 @@ class MergeRequest < ActiveRecord::Base
diverged_commits_count > 0 diverged_commits_count > 0
end end
def commits_sha
commits.map(&:sha)
end
def head_pipeline def head_pipeline
return unless diff_head_sha && source_project return unless diff_head_sha && source_project
...@@ -871,4 +868,12 @@ class MergeRequest < ActiveRecord::Base ...@@ -871,4 +868,12 @@ class MergeRequest < ActiveRecord::Base
@conflicts_can_be_resolved_in_ui = false @conflicts_can_be_resolved_in_ui = false
end end
end end
def has_commits?
commits_count > 0
end
def has_no_commits?
!has_commits?
end
end end
...@@ -127,12 +127,8 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -127,12 +127,8 @@ class MergeRequestDiff < ActiveRecord::Base
end end
def commits_sha def commits_sha
if @commits
commits.map(&:sha)
else
st_commits.map { |commit| commit[:id] } st_commits.map { |commit| commit[:id] }
end end
end
def diff_refs def diff_refs
return unless start_commit_sha || base_commit_sha return unless start_commit_sha || base_commit_sha
...@@ -176,6 +172,10 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -176,6 +172,10 @@ class MergeRequestDiff < ActiveRecord::Base
CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight) CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight)
end end
def commits_count
st_commits.count
end
private private
# Old GitLab implementations may have generated diffs as ["--broken-diff"]. # Old GitLab implementations may have generated diffs as ["--broken-diff"].
......
...@@ -63,7 +63,7 @@ module MergeRequests ...@@ -63,7 +63,7 @@ module MergeRequests
if merge_request.source_branch == @branch_name || force_push? if merge_request.source_branch == @branch_name || force_push?
merge_request.reload_diff merge_request.reload_diff
else else
mr_commit_ids = merge_request.commits.map(&:id) mr_commit_ids = merge_request.commits_sha
push_commit_ids = @commits.map(&:id) push_commit_ids = @commits.map(&:id)
matches = mr_commit_ids & push_commit_ids matches = mr_commit_ids & push_commit_ids
merge_request.reload_diff if matches.any? merge_request.reload_diff if matches.any?
...@@ -123,7 +123,7 @@ module MergeRequests ...@@ -123,7 +123,7 @@ module MergeRequests
return unless @commits.present? return unless @commits.present?
merge_requests_for_source_branch.each do |merge_request| 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| new_commits, existing_commits = @commits.partition do |commit|
mr_commit_ids.include?(commit.id) mr_commit_ids.include?(commit.id)
......
...@@ -27,7 +27,7 @@ ...@@ -27,7 +27,7 @@
version #{version_index(merge_request_diff)} version #{version_index(merge_request_diff)}
.monospace #{short_sha(merge_request_diff.head_commit_sha)} .monospace #{short_sha(merge_request_diff.head_commit_sha)}
%small %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) = time_ago_with_tooltip(merge_request_diff.created_at)
- if @merge_request_diff.base_commit_sha - if @merge_request_diff.base_commit_sha
......
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
= render 'projects/merge_requests/widget/open/archived' = render 'projects/merge_requests/widget/open/archived'
- elsif @merge_request.branch_missing? - elsif @merge_request.branch_missing?
= render 'projects/merge_requests/widget/open/missing_branch' = 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' = render 'projects/merge_requests/widget/open/nothing'
- elsif @merge_request.unchecked? - elsif @merge_request.unchecked?
= render 'projects/merge_requests/widget/open/check' = render 'projects/merge_requests/widget/open/check'
......
---
title: Replace references to MergeRequestDiff#commits with st_commits when we care
only about the number of commits
merge_request: 7668
author:
...@@ -730,8 +730,8 @@ describe Ci::Build, models: true do ...@@ -730,8 +730,8 @@ describe Ci::Build, models: true do
pipeline2 = create(:ci_pipeline, project: project) pipeline2 = create(:ci_pipeline, project: project)
@build2 = create(:ci_build, pipeline: pipeline2) @build2 = create(:ci_build, pipeline: pipeline2)
commits = [double(id: pipeline.sha), double(id: pipeline2.sha)] allow(@merge_request).to receive(:commits_sha).
allow(@merge_request).to receive(:commits).and_return(commits) and_return([pipeline.sha, pipeline2.sha])
allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request]) allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request])
end end
......
...@@ -77,24 +77,13 @@ describe MergeRequestDiff, models: true do ...@@ -77,24 +77,13 @@ describe MergeRequestDiff, models: true do
end end
describe '#commits_sha' do describe '#commits_sha' do
shared_examples 'returning all commits SHA' do it 'returns all commits SHA using serialized commits' do
it 'returns all commits SHA' do subject.st_commits = [
commits_sha = subject.commits_sha { id: 'sha1' },
{ id: 'sha2' }
]
expect(commits_sha).to eq(subject.commits.map(&:sha)) expect(subject.commits_sha).to eq(['sha1', 'sha2'])
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'
end end
end end
...@@ -113,4 +102,15 @@ describe MergeRequestDiff, models: true do ...@@ -113,4 +102,15 @@ describe MergeRequestDiff, models: true do
expect(diffs.size).to eq(3) expect(diffs.size).to eq(3)
end end
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 end
...@@ -557,16 +557,13 @@ describe MergeRequest, models: true do ...@@ -557,16 +557,13 @@ describe MergeRequest, models: true do
end end
describe '#commits_sha' do describe '#commits_sha' do
let(:commit0) { double('commit0', sha: 'sha1') }
let(:commit1) { double('commit1', sha: 'sha2') }
let(:commit2) { double('commit2', sha: 'sha3') }
before do 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 end
it 'returns sha of commits' do it 'delegates to merge request diff' do
expect(subject.commits_sha).to contain_exactly('sha1', 'sha2', 'sha3') expect(subject.commits_sha).to eq ['sha1']
end end
end end
...@@ -1440,4 +1437,26 @@ describe MergeRequest, models: true do ...@@ -1440,4 +1437,26 @@ describe MergeRequest, models: true do
end end
end 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 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