Merge branch 'mr-commit-optimization' into 'master'

Use persisted/memoized value for MRs sha's instead of doing git lookups

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

7 files changed, 30 insertions, 15 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index a1af125547c..54e7d81de6a 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 582a7818502..2b0a88ac5b4 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 5bec68ce4f6..9a7e66a9cbb 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 4e8ef320af2..a3ebec0efc6 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 00000000000..522d8951b18
--- /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 ceb570ac777..412eca4a56b 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 80a271ba7fb..d2072198d83 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