diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index a1af125547c7d989f51d4bc45468479f1f79aa14..54e7d81de6a03db185749fbbc347cc3dae43cf38 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -187,7 +187,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo begin @merge_request.environments_for(current_user).map do |environment| project = environment.project - deployment = environment.first_deployment_for(@merge_request.diff_head_commit) + deployment = environment.first_deployment_for(@merge_request.diff_head_sha) stop_url = if environment.stop_action? && can?(current_user, :create_deployment, environment) diff --git a/app/models/environment.rb b/app/models/environment.rb index 582a7818502bcfb50f6edf4a4b29c8382fc15206..2b0a88ac5b40c3b44a417480bfc7233e4d566f0a 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -99,8 +99,8 @@ class Environment < ActiveRecord::Base folder_name == "production" end - def first_deployment_for(commit) - ref = project.repository.ref_name_for_sha(ref_path, commit.sha) + def first_deployment_for(commit_sha) + ref = project.repository.ref_name_for_sha(ref_path, commit_sha) return nil unless ref diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5bec68ce4f619eb706994f2778702e461e552607..9a7e66a9cbb64c66b9dcf7a822886ba0e3ee5be5 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -375,15 +375,27 @@ class MergeRequest < ActiveRecord::Base end def diff_start_sha - diff_start_commit.try(:sha) + if persisted? + merge_request_diff.start_commit_sha + else + target_branch_head.try(:sha) + end end def diff_base_sha - diff_base_commit.try(:sha) + if persisted? + merge_request_diff.base_commit_sha + else + branch_merge_base_commit.try(:sha) + end end def diff_head_sha - diff_head_commit.try(:sha) + if persisted? + merge_request_diff.head_commit_sha + else + source_branch_head.try(:sha) + end end # When importing a pull request from GitHub, the old and new branches may no @@ -646,7 +658,7 @@ class MergeRequest < ActiveRecord::Base !ProtectedBranch.protected?(source_project, source_branch) && !source_project.root_ref?(source_branch) && Ability.allowed?(current_user, :push_code, source_project) && - diff_head_commit == source_branch_head + diff_head_sha == source_branch_head.try(:sha) end def should_remove_source_branch? diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index 4e8ef320af2e0463c92a50af4aa5fe8bfb34340e..a3ebec0efc6375a22e6b7c22b9043a5d8a3e7ed0 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -38,7 +38,7 @@ class MergeRequestWidgetEntity < IssuableEntity # Diff sha's expose :diff_head_sha do |merge_request| - merge_request.diff_head_sha if merge_request.diff_head_commit + merge_request.diff_head_sha.presence end expose :merge_commit_message diff --git a/changelogs/unreleased/mr-commit-optimization.yml b/changelogs/unreleased/mr-commit-optimization.yml new file mode 100644 index 0000000000000000000000000000000000000000..522d8951b18138ac7561d32ff214672204f798e4 --- /dev/null +++ b/changelogs/unreleased/mr-commit-optimization.yml @@ -0,0 +1,5 @@ +--- +title: Use persisted/memoized value for MRs shas instead of doing git lookups +merge_request: 17555 +author: +type: performance diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index ceb570ac777232b9e4ec4d1c7b3222a4028f5c08..412eca4a56bfad034f80b328c48a9c91196427c5 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -142,15 +142,15 @@ describe Environment do let(:commit) { project.commit.parent } it 'returns deployment id for the environment' do - expect(environment.first_deployment_for(commit)).to eq deployment1 + expect(environment.first_deployment_for(commit.id)).to eq deployment1 end it 'return nil when no deployment is found' do - expect(environment.first_deployment_for(head_commit)).to eq nil + expect(environment.first_deployment_for(head_commit.id)).to eq nil end it 'returns a UTF-8 ref' do - expect(environment.first_deployment_for(commit).ref).to be_utf8 + expect(environment.first_deployment_for(commit.id).ref).to be_utf8 end end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index 80a271ba7fbc317b7e92cd51e79053a529f2cd02..d2072198d83d833db8de1b3630f48dc2a0bca66c 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -147,9 +147,9 @@ describe MergeRequestWidgetEntity do allow(resource).to receive(:diff_head_sha) { 'sha' } end - context 'when no diff head commit' do + context 'when diff head commit is empty' do it 'returns nil' do - allow(resource).to receive(:diff_head_commit) { nil } + allow(resource).to receive(:diff_head_sha) { '' } expect(subject[:diff_head_sha]).to be_nil end @@ -157,8 +157,6 @@ describe MergeRequestWidgetEntity do context 'when diff head commit present' do it 'returns diff head commit short id' do - allow(resource).to receive(:diff_head_commit) { double } - expect(subject[:diff_head_sha]).to eq('sha') end end