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

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
     }

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)

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
-    Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_sha, target_sha).size
+    return 0 unless source_branch_sha && target_branch_sha
+    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

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 base
-    self.target_sha || self.target_branch
+  def branch_base_commit
+    return unless self.source_branch_sha && self.target_branch_sha
+    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

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)

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)

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

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

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}…
+      Checking CI status for #{@merge_request.diff_head_commit.short_id}…
     .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.

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

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

features/steps/project/merge_requests.rb

@@ -519,7 +519,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   step '"Bug NS-05" has CI status' do
     project = merge_request.source_project
     project.enable_ci
-    pipeline = create :ci_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch
+    pipeline = create :ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch
     create :ci_build, pipeline: pipeline
   end

lib/api/merge_requests.rb

@@ -233,8 +233,8 @@ module API
           render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?
-          if params[:sha] && merge_request.source_sha != params[:sha]
-            render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409)
+          if params[:sha] && merge_request.diff_head_sha != params[:sha]
+            render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
           end
           merge_params = {

lib/gitlab/github_import/pull_request_formatter.rb

@@ -11,10 +11,10 @@ module Gitlab
           description: description,
           source_project: source_branch_project,
           source_branch: source_branch_name,
-          head_source_sha: source_branch_sha,
+          source_branch_sha: source_branch_sha,
           target_project: target_branch_project,
           target_branch: target_branch_name,
-          base_target_sha: target_branch_sha,
+          target_branch_sha: target_branch_sha,
           state: state,
           milestone: milestone,
           author_id: author_id,

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -212,7 +212,7 @@ describe Projects::MergeRequestsController do
     context 'when the sha parameter matches the source SHA' do
       def merge_with_sha
-        post :merge, base_params.merge(sha: merge_request.source_sha)
+        post :merge, base_params.merge(sha: merge_request.diff_head_sha)
       end
       it 'returns :success' do
@@ -229,11 +229,11 @@ describe Projects::MergeRequestsController do
       context 'when merge_when_build_succeeds is passed' do
         def merge_when_build_succeeds
-          post :merge, base_params.merge(sha: merge_request.source_sha, merge_when_build_succeeds: '1')
+          post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_build_succeeds: '1')
         end
         before do
-          create(:ci_empty_pipeline, project: project, sha: merge_request.source_sha, ref: merge_request.source_branch)
+          create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch)
         end
         it 'returns :merge_when_build_succeeds' do

spec/features/merge_requests/created_from_fork_spec.rb

@@ -30,7 +30,7 @@ feature 'Merge request created from fork' do
     given(:pipeline) do
       create(:ci_pipeline_with_two_job, project: fork_project,
-                                        sha: merge_request.last_commit.id,
+                                        sha: merge_request.diff_head_sha,
                                         ref: merge_request.source_branch)
     end

spec/features/merge_requests/merge_when_build_succeeds_spec.rb

@@ -12,7 +12,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
   end
   context "Active build for Merge Request" do
-    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
     let!(:ci_build) { create(:ci_build, pipeline: pipeline) }
     before do
@@ -47,7 +47,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
                                                   merge_user: user, title: "MepMep", merge_when_build_succeeds: true)
     end
-    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
     let!(:ci_build) { create(:ci_build, pipeline: pipeline) }
     before do

spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb

@@ -19,7 +19,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
   end
   context 'when project has CI enabled' do
-    let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
     context 'when merge requests can only be merged if the build succeeds' do
       before do

spec/lib/gitlab/github_import/pull_request_formatter_spec.rb

@@ -43,10 +43,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: source_sha,
+          source_branch_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: target_sha,
+          target_branch_sha: target_sha,
           state: 'opened',
           milestone: nil,
           author_id: project.creator_id,
@@ -70,10 +70,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: source_sha,
+          source_branch_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: target_sha,
+          target_branch_sha: target_sha,
           state: 'closed',
           milestone: nil,
           author_id: project.creator_id,
@@ -97,10 +97,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: source_sha,
+          source_branch_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: target_sha,
+          target_branch_sha: target_sha,
           state: 'merged',
           milestone: nil,
           author_id: project.creator_id,

spec/lib/gitlab/import_export/project_tree_saver_spec.rb

@@ -125,7 +125,7 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
     ci_pipeline = create(:ci_pipeline,
                        project: project,
-                       sha: merge_request.last_commit.id,
+                       sha: merge_request.diff_head_sha,
                        ref: merge_request.source_branch,
                        statuses: [commit_status])