Use persisted/memoized value for MRs shas instead of doing git lookups

5 files changed, 27 insertions, 10 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 f78c21aebe5..6f7dabdfefe 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/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 6f24a039998..16fdd154e7f 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