From 6ce25e7b4caa9e94de74378729178c7060d640b2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 20 Jun 2016 18:48:04 +0200 Subject: Rename MergeRequest methods that return commits or shas to be more clear and consistent --- .../projects/merge_requests_controller.rb | 18 ++-- app/helpers/merge_requests_helper.rb | 2 +- app/models/merge_request.rb | 120 ++++++++++++++------- app/models/merge_request_diff.rb | 43 ++++---- app/models/project_services/jira_service.rb | 2 +- app/services/merge_requests/merge_service.rb | 2 +- .../merge_when_build_succeeds_service.rb | 2 +- app/services/merge_requests/refresh_service.rb | 8 +- .../merge_requests/widget/_heading.html.haml | 10 +- .../merge_requests/widget/open/_accept.html.haml | 2 +- .../open/_merge_when_build_succeeds.html.haml | 2 +- features/steps/project/merge_requests.rb | 2 +- lib/api/merge_requests.rb | 4 +- lib/gitlab/github_import/pull_request_formatter.rb | 4 +- .../projects/merge_requests_controller_spec.rb | 6 +- .../merge_requests/created_from_fork_spec.rb | 2 +- .../merge_when_build_succeeds_spec.rb | 4 +- .../only_allow_merge_if_build_succeeds.rb | 2 +- .../github_import/pull_request_formatter_spec.rb | 12 +-- .../import_export/project_tree_saver_spec.rb | 2 +- spec/models/merge_request_spec.rb | 29 ++--- spec/models/project_spec.rb | 2 +- spec/models/repository_spec.rb | 4 +- spec/requests/api/merge_requests_spec.rb | 4 +- spec/services/system_note_service_spec.rb | 2 +- 25 files changed, 162 insertions(+), 128 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}… + 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. 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 diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 640f1720a6c..3611c187202 100644 --- a/features/steps/project/merge_requests.rb +++ b/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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 0e94efd4acd..4fcdf8968c9 100644 --- a/lib/api/merge_requests.rb +++ b/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 = { diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index 498b00cb658..a4ea2210abd 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/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, diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index eff74e12869..2d2fb87f14e 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/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 diff --git a/spec/features/merge_requests/created_from_fork_spec.rb b/spec/features/merge_requests/created_from_fork_spec.rb index b4d2201c729..f676200ecf3 100644 --- a/spec/features/merge_requests/created_from_fork_spec.rb +++ b/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 diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index c5e6412d7bf..96f7b8c9932 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/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 diff --git a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb index 65e9185ec24..80e8b8fc642 100644 --- a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb +++ b/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 diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 9587252b990..79931ecd134 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/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, diff --git a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb index a75eaa4d51f..1424de9e60b 100644 --- a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb +++ b/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]) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index ceb4d64698f..bb83676cddf 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -62,7 +62,7 @@ describe MergeRequest, models: true do end end - describe '#target_sha' do + describe '#target_branch_sha' do context 'when the target branch does not exist anymore' do let(:project) { create(:project) } @@ -73,32 +73,32 @@ describe MergeRequest, models: true do end it 'returns nil' do - expect(subject.target_sha).to be_nil + expect(subject.target_branch_sha).to be_nil end end end - describe '#source_sha' do + describe '#source_branch_sha' do let(:last_branch_commit) { subject.source_project.repository.commit(subject.source_branch) } context 'with diffs' do subject { create(:merge_request, :with_diffs) } it 'returns the sha of the source branch last commit' do - expect(subject.source_sha).to eq(last_branch_commit.sha) + expect(subject.source_branch_sha).to eq(last_branch_commit.sha) end end context 'without diffs' do subject { create(:merge_request, :without_diffs) } it 'returns the sha of the source branch last commit' do - expect(subject.source_sha).to eq(last_branch_commit.sha) + expect(subject.source_branch_sha).to eq(last_branch_commit.sha) end end context 'when the merge request is being created' do subject { build(:merge_request, source_branch: nil, compare_commits: []) } it 'returns nil' do - expect(subject.source_sha).to be_nil + expect(subject.source_branch_sha).to be_nil end end end @@ -252,12 +252,14 @@ describe MergeRequest, models: true do end it "can be removed if the last commit is the head of the source branch" do - allow(subject.source_project).to receive(:commit).and_return(subject.last_commit) + allow(subject.source_project).to receive(:commit).and_return(subject.diff_head_commit) expect(subject.can_remove_source_branch?(user)).to be_truthy end it "cannot be removed if the last commit is not also the head of the source branch" do + subject.source_branch = "lfs" + expect(subject.can_remove_source_branch?(user)).to be_falsey end end @@ -363,7 +365,7 @@ describe MergeRequest, models: true do and_return(2) subject.diverged_commits_count - allow(subject).to receive(:source_sha).and_return('123abc') + allow(subject).to receive(:source_branch_sha).and_return('123abc') subject.diverged_commits_count end @@ -373,7 +375,7 @@ describe MergeRequest, models: true do and_return(2) subject.diverged_commits_count - allow(subject).to receive(:target_sha).and_return('123abc') + allow(subject).to receive(:target_branch_sha).and_return('123abc') subject.diverged_commits_count end end @@ -392,11 +394,10 @@ describe MergeRequest, models: true do describe '#pipeline' do describe 'when the source project exists' do - it 'returns the latest commit' do - commit = double(:commit, id: '123abc') + it 'returns the latest pipeline' do pipeline = double(:ci_pipeline, ref: 'master') - allow(subject).to receive(:last_commit).and_return(commit) + allow(subject).to receive(:diff_head_sha).and_return('123abc') expect(subject.source_project).to receive(:pipeline). with('123abc', 'master'). @@ -464,7 +465,7 @@ describe MergeRequest, models: true do context 'when it is not broken and has no conflicts' do it 'is marked as mergeable' do allow(subject).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?) { true } + allow(project.repository).to receive(:can_be_merged?).and_return(true) expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') end @@ -481,7 +482,7 @@ describe MergeRequest, models: true do context 'when it has conflicts' do before do allow(subject).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?) { false } + allow(project.repository).to receive(:can_be_merged?).and_return(false) end it 'becomes unmergeable' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2e89d6de3a2..1b434a726dc 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -312,7 +312,7 @@ describe Project, models: true do it 'should update merge request commits with new one if pushed to source branch' do project.update_merge_requests(prev_commit_id, commit_id, "refs/heads/#{merge_request.source_branch}", key.user) merge_request.reload - expect(merge_request.last_commit.id).to eq(commit_id) + expect(merge_request.diff_head_sha).to eq(commit_id) end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 7975fc64e59..24e49c8def3 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -12,8 +12,8 @@ describe Repository, models: true do end let(:merge_commit) do source_sha = repository.find_branch('feature').target - merge_commit_id = repository.merge(user, source_sha, 'master', commit_options) - repository.commit(merge_commit_id) + merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options) + repository.commit(merge_commit_sha) end describe :branch_names_contains do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 5d81844fb84..4a1b5600bdf 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -439,14 +439,14 @@ describe API::API, api: true do end it "returns 409 if the SHA parameter doesn't match" do - put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha.succ + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.diff_head_sha.reverse expect(response).to have_http_status(409) expect(json_response['message']).to start_with('SHA does not match HEAD of source branch') end it "succeeds if the SHA parameter matches" do - put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.diff_head_sha expect(response).to have_http_status(200) end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 85dd30bf48c..43693441450 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -213,7 +213,7 @@ describe SystemNoteService, services: true do create(:merge_request, source_project: project, target_project: project) end - subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) } + subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.diff_head_commit) } it_behaves_like 'a system note' -- cgit v1.2.1