Rename MergeRequest methods that return commits or shas to be more clear and consistent

11 files changed, 122 insertions, 89 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index dd86b940a08..a03eb8513b6 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -77,12 +77,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diffs
     apply_diff_view_cookie!
 
-    @commit = @merge_request.last_commit
-    @base_commit = @merge_request.diff_base_commit
-
-    # MRs created before 8.4 don't have a diff_base_commit,
-    # but we need it for the "View file @ ..." link by deleted files
-    @base_commit ||= @merge_request.first_commit.parent || @merge_request.first_commit
+    @commit = @merge_request.diff_head_commit
+    @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
 
     @comments_target = {
       noteable_type: 'MergeRequest',
@@ -134,7 +130,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @target_project = merge_request.target_project
     @source_project = merge_request.source_project
     @commits = @merge_request.compare_commits.reverse
-    @commit = @merge_request.last_commit
+    @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit
     @diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
     @diff_notes_disabled = true
@@ -212,7 +208,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return
     end
 
-    if params[:sha] != @merge_request.source_sha
+    if params[:sha] != @merge_request.diff_head_sha
       @status = :sha_mismatch
       return
     end
@@ -274,16 +270,16 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       status ||= "preparing"
     else
       ci_service = @merge_request.source_project.ci_service
-      status = ci_service.commit_status(merge_request.last_commit.sha, merge_request.source_branch) if ci_service
+      status = ci_service.commit_status(merge_request.diff_head_sha, merge_request.source_branch) if ci_service
 
       if ci_service.respond_to?(:commit_coverage)
-        coverage = ci_service.commit_coverage(merge_request.last_commit.sha, merge_request.source_branch)
+        coverage = ci_service.commit_coverage(merge_request.diff_head_sha, merge_request.source_branch)
       end
     end
 
     response = {
       title: merge_request.title,
-      sha: merge_request.last_commit_short_sha,
+      sha: merge_request.diff_head_commit.short_id,
       status: status,
       coverage: coverage
     }
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 1dd07a2a220..c7dedfe9254 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -27,7 +27,7 @@ module MergeRequestsHelper
   end
 
   def ci_build_details_path(merge_request)
-    build_url = merge_request.source_project.ci_service.build_page(merge_request.last_commit.sha, merge_request.source_branch)
+    build_url = merge_request.source_project.ci_service.build_page(merge_request.diff_head_sha, merge_request.source_branch)
     return nil unless build_url
 
     parsed_url = URI.parse(build_url)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4f7e1d2f302..cc85421a815 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -16,7 +16,7 @@ class MergeRequest < ActiveRecord::Base
 
   serialize :merge_params, Hash
 
-  after_create :create_merge_request_diff, unless: :importing
+  after_create :create_merge_request_diff, unless: :importing?
   after_update :update_merge_request_diff
 
   delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil
@@ -29,10 +29,6 @@ class MergeRequest < ActiveRecord::Base
   # when creating new merge request
   attr_accessor :can_be_created, :compare_commits, :compare
 
-  # Temporary fields to store target_sha, and base_sha to
-  # compare when importing pull requests from GitHub
-  attr_accessor :base_target_sha, :head_source_sha
-
   state_machine :state, initial: :opened do
     event :close do
       transition [:reopened, :opened] => :closed
@@ -169,28 +165,88 @@ class MergeRequest < ActiveRecord::Base
     reference
   end
 
-  def last_commit
-    merge_request_diff ? merge_request_diff.last_commit : compare_commits.last
-  end
-
   def first_commit
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
 
+  def last_commit
+    merge_request_diff ? merge_request_diff.last_commit : compare_commits.last
+  end
+
   def diff_size
     merge_request_diff.size
   end
 
   def diff_base_commit
-    if merge_request_diff
+    if persisted?
       merge_request_diff.base_commit
-    elsif source_sha
-      self.target_project.merge_base_commit(self.source_sha, self.target_branch)
+    elsif diff_start_commit && diff_head_commit
+      self.target_project.merge_base_commit(diff_start_sha, diff_head_sha)
+    end
+  end
+
+  # MRs created before 8.4 don't store a MergeRequestDiff#base_commit_sha,
+  # but we need to get a commit for the "View file @ ..." link by deleted files,
+  # so we find the likely one if we can't get the actual one.
+  # This will not be the actual base commit if the target branch was merged into
+  # the source branch after the merge request was created, but it is good enough
+  # for the specific purpose of linking to a commit.
+  # It is not good enough for use in Gitlab::Git::DiffRefs, which need the
+  # true base commit.
+  def likely_diff_base_commit
+    first_commit.parent || first_commit
+  end
+
+  def diff_start_commit
+    if persisted?
+      merge_request_diff.start_commit
+    else
+      target_branch_head
     end
   end
 
-  def last_commit_short_sha
-    last_commit.short_id
+  def diff_head_commit
+    if persisted?
+      merge_request_diff.head_commit
+    else
+      source_branch_head
+    end
+  end
+
+  def diff_start_sha
+    diff_start_commit.try(:sha)
+  end
+
+  def diff_base_sha
+    diff_base_commit.try(:sha)
+  end
+
+  def diff_head_sha
+    diff_head_commit.try(:sha)
+  end
+
+  # When importing a pull request from GitHub, the old and new branches may no
+  # longer actually exist by those names, but we need to recreate the merge
+  # request diff with the right source and target shas.
+  # We use these attributes to force these to the intended values.
+  attr_writer :target_branch_sha, :source_branch_sha
+
+  def source_branch_head
+    source_branch_ref = @source_branch_sha || source_branch
+    source_project.repository.commit(source_branch) if source_branch_ref
+  end
+
+  def target_branch_head
+    target_branch_ref = @target_branch_sha || target_branch
+    target_project.repository.commit(target_branch) if target_branch_ref
+  end
+
+  def target_branch_sha
+    target_branch_head.try(:sha)
+  end
+
+  def source_branch_sha
+    source_branch_head.try(:sha)
   end
 
   def validate_branches
@@ -241,7 +297,7 @@ class MergeRequest < ActiveRecord::Base
     return unless unchecked?
 
     can_be_merged =
-      !broken? && project.repository.can_be_merged?(source_sha, target_branch)
+      !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch)
 
     if can_be_merged
       mark_as_mergeable
@@ -293,7 +349,7 @@ class MergeRequest < ActiveRecord::Base
     !source_project.protected_branch?(source_branch) &&
       !source_project.root_ref?(source_branch) &&
       Ability.abilities.allowed?(current_user, :push_code, source_project) &&
-      last_commit == source_project.commit(source_branch)
+      diff_head_commit == source_branch_head
   end
 
   def should_remove_source_branch?
@@ -331,8 +387,8 @@ class MergeRequest < ActiveRecord::Base
       work_in_progress: work_in_progress?
     }
 
-    if last_commit
-      attrs.merge!(last_commit: last_commit.hook_attrs)
+    if diff_head_commit
+      attrs.merge!(last_commit: diff_head_commit.hook_attrs)
     end
 
     attributes.merge!(attrs)
@@ -510,22 +566,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def target_sha
-    return @base_target_sha if defined?(@base_target_sha)
-
-    target_project.repository.commit(target_branch).try(:sha)
-  end
-
-  def source_sha
-    return @head_source_sha if defined?(@head_source_sha)
-
-    last_commit.try(:sha) || source_tip.try(:sha)
-  end
-
-  def source_tip
-    source_branch && source_project.repository.commit(source_branch)
-  end
-
   def fetch_ref
     target_project.repository.fetch_ref(
       source_project.repository.path_to_repo,
@@ -558,10 +598,10 @@ class MergeRequest < ActiveRecord::Base
   def diverged_commits_count
     cache = Rails.cache.read(:"merge_request_#{id}_diverged_commits")
 
-    if cache.blank? || cache[:source_sha] != source_sha || cache[:target_sha] != target_sha
+    if cache.blank? || cache[:source_sha] != source_branch_sha || cache[:target_sha] != target_branch_sha
       cache = {
-        source_sha: source_sha,
-        target_sha: target_sha,
+        source_sha: source_branch_sha,
+        target_sha: target_branch_sha,
         diverged_commits_count: compute_diverged_commits_count
       }
       Rails.cache.write(:"merge_request_#{id}_diverged_commits", cache)
@@ -571,9 +611,9 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def compute_diverged_commits_count
-    return 0 unless source_sha && target_sha
+    return 0 unless source_branch_sha && target_branch_sha
 
-    Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_sha, target_sha).size
+    Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_branch_sha, target_branch_sha).size
   end
   private :compute_diverged_commits_count
 
@@ -582,13 +622,13 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def pipeline
-    @pipeline ||= source_project.pipeline(last_commit.id, source_branch) if last_commit && source_project
   end
 
   def diff_refs
     return nil unless diff_base_commit
 
     [diff_base_commit, last_commit]
+    @pipeline ||= source_project.pipeline(diff_head_sha, source_branch) if diff_head_sha && source_project
   end
 
   def merge_commit
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index fd69f915bd2..60f4b44a5d1 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -7,7 +7,7 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
 
-  delegate :head_source_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
+  delegate :source_branch_sha, :target_branch_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
 
   state_machine :state, initial: :empty do
     state :collected
@@ -40,8 +40,8 @@ class MergeRequestDiff < ActiveRecord::Base
       @diffs_no_whitespace ||= begin
         compare = Gitlab::Git::Compare.new(
           self.repository.raw_repository,
-          self.base,
-          self.head,
+          self.target_branch_sha,
+          self.source_branch_sha,
         )
         compare.diffs(options)
       end
@@ -63,13 +63,21 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def base_commit
-    return nil unless self.base_commit_sha
+    return unless self.base_commit_sha
 
     merge_request.target_project.commit(self.base_commit_sha)
   end
 
-  def last_commit_short_sha
-    @last_commit_short_sha ||= last_commit.short_id
+  def start_commit
+    return unless self.start_commit_sha
+
+    merge_request.target_project.commit(self.start_commit_sha)
+  end
+
+  def head_commit
+    return last_commit unless self.head_commit_sha
+
+    merge_request.target_project.commit(self.head_commit_sha)
   end
 
   def dump_commits(commits)
@@ -166,23 +174,14 @@ class MergeRequestDiff < ActiveRecord::Base
     merge_request.target_project.repository
   end
 
-  def source_sha
-    return head_source_sha if head_source_sha.present?
-
-    source_commit = merge_request.source_project.commit(source_branch)
-    source_commit.try(:sha)
-  end
-
-  def target_sha
-    merge_request.target_sha
-  end
+  def branch_base_commit
+    return unless self.source_branch_sha && self.target_branch_sha
 
-  def base
-    self.target_sha || self.target_branch
+    merge_request.target_project.merge_base_commit(self.source_branch_sha, self.target_branch_sha)
   end
 
-  def head
-    self.source_sha
+  def branch_base_sha
+    branch_base_commit.try(:sha)
   end
 
   def compare
@@ -193,8 +192,8 @@ class MergeRequestDiff < ActiveRecord::Base
 
         Gitlab::Git::Compare.new(
           self.repository.raw_repository,
-          self.base,
-          self.head
+          self.target_branch_sha,
+          self.source_branch_sha
         )
       end
   end
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 27bf08bf7d9..97bcbacf2b9 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -144,7 +144,7 @@ class JiraService < IssueTrackerService
     commit_id = if entity.is_a?(Commit)
                   entity.id
                 elsif entity.is_a?(MergeRequest)
-                  entity.last_commit.id
+                  entity.diff_head_sha
                 end
     commit_url = build_entity_url(:commit, commit_id)
 
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 3bec66cea88..f1b1d90c457 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -34,7 +34,7 @@ module MergeRequests
         committer: committer
       }
 
-      commit_id = repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options)
+      commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, options)
       merge_request.update(merge_commit_sha: commit_id)
     rescue GitHooksService::PreReceiveError => e
       merge_request.update(merge_error: e.message)
diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb
index 870f5705184..4ad5fb08311 100644
--- a/app/services/merge_requests/merge_when_build_succeeds_service.rb
+++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb
@@ -12,7 +12,7 @@ module MergeRequests
         merge_request.merge_when_build_succeeds = true
         merge_request.merge_user                = @current_user
 
-        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.last_commit)
+        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
       end
 
       merge_request.save
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index fe0579744b4..de79c024428 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -34,10 +34,10 @@ module MergeRequests
     def close_merge_requests
       commit_ids = @commits.map(&:id)
       merge_requests = @project.merge_requests.opened.where(target_branch: @branch_name).to_a
-      merge_requests = merge_requests.select(&:last_commit)
+      merge_requests = merge_requests.select(&:diff_head_commit)
 
       merge_requests = merge_requests.select do |merge_request|
-        commit_ids.include?(merge_request.last_commit.id)
+        commit_ids.include?(merge_request.diff_head_sha)
       end
 
       merge_requests.uniq.select(&:source_project).each do |merge_request|
@@ -94,12 +94,10 @@ module MergeRequests
         merge_request = merge_requests_for_source_branch.first
         return unless merge_request
 
-        last_commit = merge_request.last_commit
-
         begin
           # Since any number of commits could have been made to the restored branch,
           # find the common root to see what has been added.
-          common_ref = @project.repository.merge_base(last_commit.id, @newrev)
+          common_ref = @project.repository.merge_base(merge_request.diff_head_sha, @newrev)
           # If the a commit no longer exists in this repo, gitlab_git throws
           # a Rugged::OdbError. This is fixed in https://gitlab.com/gitlab-org/gitlab_git/merge_requests/52
           @commits = @project.repository.commits_between(common_ref, @newrev) if common_ref
diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml
index 08a38d283d2..489c632ae22 100644
--- a/app/views/projects/merge_requests/widget/_heading.html.haml
+++ b/app/views/projects/merge_requests/widget/_heading.html.haml
@@ -7,7 +7,7 @@
           CI build
           = ci_label_for_status(status)
         for
-        - commit = @merge_request.last_commit
+        - commit = @merge_request.diff_head_commit
         = succeed "." do
           = link_to @pipeline.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @pipeline.sha), class: "monospace"
         %span.ci-coverage
@@ -24,7 +24,7 @@
           CI build
           = ci_label_for_status(status)
         for
-        - commit = @merge_request.last_commit
+        - commit = @merge_request.diff_head_commit
         = succeed "." do
           = link_to commit.short_id, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, commit), class: "monospace"
         %span.ci-coverage
@@ -33,12 +33,12 @@
 
     .ci_widget
       = icon("spinner spin")
-      Checking CI status for #{@merge_request.last_commit_short_sha}&hellip;
+      Checking CI status for #{@merge_request.diff_head_commit.short_id}&hellip;
 
     .ci_widget.ci-not_found{style: "display:none"}
       = icon("times-circle")
-      Could not find CI status for #{@merge_request.last_commit_short_sha}.
+      Could not find CI status for #{@merge_request.diff_head_commit.short_id}.
 
     .ci_widget.ci-error{style: "display:none"}
       = icon("times-circle")
-      Could not connect to the CI server. Please check your settings and try again.
\ No newline at end of file
+      Could not connect to the CI server. Please check your settings and try again.
diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml
index 941513febbd..bf2e76f0083 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -2,7 +2,7 @@
 
 = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f|
   = hidden_field_tag :authenticity_token, form_authenticity_token
-  = hidden_field_tag :sha, @merge_request.source_sha
+  = hidden_field_tag :sha, @merge_request.diff_head_sha
   .accept-merge-holder.clearfix.js-toggle-container
     .clearfix
       .accept-action
diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
index ad898ff153b..2b6b5e05e86 100644
--- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
@@ -16,7 +16,7 @@
   - if remove_source_branch_button || user_can_cancel_automatic_merge
     .clearfix.prepend-top-10
       - if remove_source_branch_button
-        = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.source_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
+        = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.diff_head_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
           = icon('times')
           Remove Source Branch When Merged