diff options
40 files changed, 825 insertions, 13 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue new file mode 100644 index 00000000000..09276ba2769 --- /dev/null +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue @@ -0,0 +1,133 @@ +<script> + import simplePoll from '../../../lib/utils/simple_poll'; + import eventHub from '../../event_hub'; + import statusIcon from '../mr_widget_status_icon'; + import loadingIcon from '../../../vue_shared/components/loading_icon.vue'; + import Flash from '../../../flash'; + + export default { + name: 'MRWidgetRebase', + props: { + mr: { + type: Object, + required: true, + }, + service: { + type: Object, + required: true, + }, + }, + components: { + statusIcon, + loadingIcon, + }, + data() { + return { + isMakingRequest: false, + rebasingError: null, + }; + }, + computed: { + status() { + if (this.mr.rebaseInProgress || this.isMakingRequest) { + return 'loading'; + } + if (!this.mr.canPushToSourceBranch && !this.mr.rebaseInProgress) { + return 'warning'; + } + return 'success'; + }, + showDisabledButton() { + return ['failed', 'loading'].includes(this.status); + }, + }, + methods: { + rebase() { + this.isMakingRequest = true; + this.rebasingError = null; + + this.service.rebase() + .then(() => { + simplePoll(this.checkRebaseStatus); + }) + .catch((error) => { + this.rebasingError = error.merge_error; + this.isMakingRequest = false; + Flash('Something went wrong. Please try again.'); + }); + }, + checkRebaseStatus(continuePolling, stopPolling) { + this.service.poll() + .then(res => res.data) + .then((res) => { + if (res.rebase_in_progress) { + continuePolling(); + } else { + this.isMakingRequest = false; + + if (res.merge_error && res.merge_error.length) { + this.rebasingError = res.merge_error; + Flash('Something went wrong. Please try again.'); + } + + eventHub.$emit('MRWidgetUpdateRequested'); + stopPolling(); + } + }) + .catch(() => { + this.isMakingRequest = false; + Flash('Something went wrong. Please try again.'); + stopPolling(); + }); + }, + }, + }; +</script> +<template> + <div class="mr-widget-body media"> + <status-icon + :status="status" + :show-disabled-button="showDisabledButton" + /> + + <div class="rebase-state-find-class-convention media media-body space-children"> + <template v-if="mr.rebaseInProgress || isMakingRequest"> + <span class="bold"> + Rebase in progress + </span> + </template> + <template v-if="!mr.rebaseInProgress && !mr.canPushToSourceBranch"> + <span class="bold"> + Fast-forward merge is not possible. + Rebase the source branch onto + <span class="label-branch">{{mr.targetBranch}}</span> + to allow this merge request to be merged. + </span> + </template> + <template v-if="!mr.rebaseInProgress && mr.canPushToSourceBranch && !isMakingRequest"> + <div class="accept-merge-holder clearfix js-toggle-container accept-action media space-children"> + <button + type="button" + class="btn btn-sm btn-reopen btn-success" + :disabled="isMakingRequest" + @click="rebase"> + <loading-icon v-if="isMakingRequest" /> + Rebase + </button> + <span + v-if="!rebasingError" + class="bold"> + Fast-forward merge is not possible. + Rebase the source branch onto the target branch or merge target + branch into source branch to allow this merge request to be merged. + </span> + <span + v-else + class="bold danger"> + {{rebasingError}} + </span> + </div> + </template> + </div> + </div> +</template> diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index 5bd8b99420a..940f3d9b2d0 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -32,6 +32,7 @@ export { default as UnresolvedDiscussionsState } from './components/states/mr_wi export { default as PipelineBlockedState } from './components/states/mr_widget_pipeline_blocked'; export { default as PipelineFailedState } from './components/states/mr_widget_pipeline_failed'; export { default as MergeWhenPipelineSucceedsState } from './components/states/mr_widget_merge_when_pipeline_succeeds'; +export { default as RebaseState } from './components/states/mr_widget_rebase.vue'; export { default as AutoMergeFailed } from './components/states/mr_widget_auto_merge_failed'; export { default as CheckingState } from './components/states/mr_widget_checking'; export { default as MRWidgetStore } from './stores/mr_widget_store'; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index fdae06200de..2075f8e4fec 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -10,6 +10,7 @@ import { MergedState, ClosedState, MergingState, + RebaseState, WipState, ArchivedState, ConflictsState, @@ -79,6 +80,7 @@ export default { ciEnvironmentsStatusPath: store.ciEnvironmentsStatusPath, statusPath: store.statusPath, mergeActionsContentPath: store.mergeActionsContentPath, + rebasePath: store.rebasePath, }; return new MRWidgetService(endpoints); }, @@ -232,6 +234,7 @@ export default { 'mr-widget-pipeline-failed': PipelineFailedState, 'mr-widget-merge-when-pipeline-succeeds': MergeWhenPipelineSucceedsState, 'mr-widget-auto-merge-failed': AutoMergeFailed, + 'mr-widget-rebase': RebaseState, }, template: ` <div class="mr-state-widget prepend-top-default"> diff --git a/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js b/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js index 7c0bbdd403f..fecbfec2214 100644 --- a/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js +++ b/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js @@ -37,6 +37,10 @@ export default class MRWidgetService { return axios.get(this.endpoints.mergeActionsContentPath); } + rebase() { + return axios.post(this.endpoints.rebasePath); + } + static stopEnvironment(url) { return axios.post(url); } diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 2bace3311c8..f7f0c1b6cb7 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -25,6 +25,8 @@ export default function deviseState(data) { return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds; } else if (!this.canMerge) { return stateKey.notAllowedToMerge; + } else if (this.shouldBeRebased) { + return stateKey.rebase; } else if (this.canBeMerged) { return stateKey.readyToMerge; } diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 474c17ec133..ed004b3bb08 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -26,6 +26,7 @@ export default class MergeRequestStore { this.divergedCommitsCount = data.diverged_commits_count; this.pipeline = data.pipeline || {}; this.deployments = this.deployments || data.deployments || []; + this.initRebase(data); if (data.issues_links) { const links = data.issues_links; @@ -124,6 +125,13 @@ export default class MergeRequestStore { return this.state === stateKey.nothingToMerge; } + initRebase(data) { + this.canPushToSourceBranch = data.can_push_to_source_branch; + this.rebaseInProgress = data.rebase_in_progress; + this.approvalsLeft = !data.approved; + this.rebasePath = data.rebase_path; + } + static buildMetrics(metrics) { if (!metrics) { return {}; diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index de980c175fb..29d5bd4a1da 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -17,6 +17,7 @@ const stateToComponentMap = { failedToMerge: 'mr-widget-failed-to-merge', autoMergeFailed: 'mr-widget-auto-merge-failed', shaMismatch: 'mr-widget-sha-mismatch', + rebase: 'mr-widget-rebase', }; const statesToShowHelpWidget = [ @@ -29,6 +30,7 @@ const statesToShowHelpWidget = [ 'pipelineFailed', 'pipelineBlocked', 'autoMergeFailed', + 'rebase', ]; export const stateKey = { @@ -46,6 +48,7 @@ export const stateKey = { mergeWhenPipelineSucceeds: 'mergeWhenPipelineSucceeds', notAllowedToMerge: 'notAllowedToMerge', readyToMerge: 'readyToMerge', + rebase: 'rebase', }; export default { diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 6b59c8461a3..2e8a738b6d9 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -10,6 +10,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :set_issuables_index, only: [:index] before_action :authenticate_user!, only: [:assign_related_issues] + before_action :check_user_can_push_to_source_branch!, only: [:rebase] def index @merge_requests = @issuables @@ -223,6 +224,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo render json: environments end + def rebase + RebaseWorker.perform_async(@merge_request.id, current_user.id) + + render nothing: true, status: 200 + end + protected alias_method :subscribable_resource, :merge_request @@ -322,4 +329,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @finder_type = MergeRequestsFinder super end + + def check_user_can_push_to_source_branch! + return access_denied! unless @merge_request.source_branch_exists? + + access_check = ::Gitlab::UserAccess + .new(current_user, project: @merge_request.source_project) + .can_push_to_branch?(@merge_request.source_branch) + + access_denied! unless access_check + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c39789b047d..ef58816937c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -156,6 +156,13 @@ class MergeRequest < ActiveRecord::Base '!' end + def rebase_in_progress? + # The source project can be deleted + return false unless source_project + + source_project.repository.rebase_in_progress?(id) + end + # Use this method whenever you need to make sure the head_pipeline is synced with the # branch head commit, for example checking if a merge request can be merged. # For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004 @@ -607,7 +614,7 @@ class MergeRequest < ActiveRecord::Base check_if_can_be_merged - can_be_merged? + can_be_merged? && !should_be_rebased? end def mergeable_state?(skip_ci_check: false) diff --git a/app/models/repository.rb b/app/models/repository.rb index b1fd981965c..4bedcbfb6a2 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1099,6 +1099,13 @@ class Repository @project.repository_storage_path end + def rebase(user, merge_request) + raw.rebase(user, merge_request.id, branch: merge_request.source_branch, + branch_sha: merge_request.source_branch_sha, + remote_repository: merge_request.target_project.repository.raw, + remote_branch: merge_request.target_branch) + end + private # TODO Generice finder, later split this on finders by Ref or Oid diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index ab4c87c0169..c6806b7cc26 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -76,6 +76,12 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end end + def rebase_path + if !rebase_in_progress? && should_be_rebased? && user_can_push_to_source_branch? + rebase_project_merge_request_path(project, merge_request) + end + end + def target_branch_tree_path if target_branch_exists? project_tree_path(project, target_branch) @@ -152,6 +158,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated user_can_collaborate_with_project? && can_be_cherry_picked? end + def can_push_to_source_branch? + source_branch_exists? && user_can_push_to_source_branch? + end + private def conflicts @@ -174,6 +184,14 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end.sort.to_sentence end + def user_can_push_to_source_branch? + return false unless source_branch_exists? + + ::Gitlab::UserAccess + .new(current_user, project: source_project) + .can_push_to_branch?(source_branch) + end + def user_can_collaborate_with_project? can?(current_user, :push_code, project) || (current_user && current_user.already_forked?(project)) diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index d54a6516aed..e4aec977f01 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/app/serializers/merge_request_basic_entity.rb @@ -4,4 +4,5 @@ class MergeRequestBasicEntity < IssuableSidebarEntity expose :merge_error expose :state expose :source_branch_exists?, as: :source_branch_exists + expose :rebase_in_progress?, as: :rebase_in_progress end diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index e905e6876c2..48cd2317f46 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -23,6 +23,16 @@ class MergeRequestWidgetEntity < IssuableEntity MergeRequestMetricsEntity.new(metrics).as_json end + expose :rebase_commit_sha + expose :rebase_in_progress?, as: :rebase_in_progress + + expose :can_push_to_source_branch do |merge_request| + presenter(merge_request).can_push_to_source_branch? + end + expose :rebase_path do |merge_request| + presenter(merge_request).rebase_path + end + # User entities expose :merge_user, using: UserEntity diff --git a/app/services/merge_requests/rebase_service.rb b/app/services/merge_requests/rebase_service.rb new file mode 100644 index 00000000000..0d5a25fa28e --- /dev/null +++ b/app/services/merge_requests/rebase_service.rb @@ -0,0 +1,30 @@ +module MergeRequests + class RebaseService < MergeRequests::WorkingCopyBaseService + def execute(merge_request) + @merge_request = merge_request + + if rebase + success + else + error('Failed to rebase. Should be done manually') + end + end + + def rebase + if merge_request.rebase_in_progress? + log_error('Rebase task canceled: Another rebase is already in progress', save_message_on_model: true) + return false + end + + rebase_sha = repository.rebase(current_user, merge_request) + + merge_request.update_attributes(rebase_commit_sha: rebase_sha) + + true + rescue => e + log_error('Failed to rebase branch:') + log_error(e.message, save_message_on_model: true) + false + end + end +end diff --git a/app/services/merge_requests/working_copy_base_service.rb b/app/services/merge_requests/working_copy_base_service.rb new file mode 100644 index 00000000000..186e05bf966 --- /dev/null +++ b/app/services/merge_requests/working_copy_base_service.rb @@ -0,0 +1,24 @@ +module MergeRequests + class WorkingCopyBaseService < MergeRequests::BaseService + attr_reader :merge_request + + def source_project + @source_project ||= merge_request.source_project + end + + def target_project + @target_project ||= merge_request.target_project + end + + def log_error(message, save_message_on_model: false) + Gitlab::GitLogger.error("#{self.class.name} error (#{merge_request.to_reference(full: true)}): #{message}") + + merge_request.update(merge_error: message) if save_message_on_model + end + + # Don't try to print expensive instance variables. + def inspect + "#<#{self.class} #{merge_request.to_reference(full: true)}>" + end + end +end diff --git a/app/views/projects/_merge_request_fast_forward_settings.html.haml b/app/views/projects/_merge_request_fast_forward_settings.html.haml index 9d357293a2f..8129c72feb2 100644 --- a/app/views/projects/_merge_request_fast_forward_settings.html.haml +++ b/app/views/projects/_merge_request_fast_forward_settings.html.haml @@ -10,4 +10,4 @@ No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. %br %span.descr - When fast-forward merge is not possible, the user must first rebase locally. + When fast-forward merge is not possible, the user is given the option to rebase. diff --git a/app/views/projects/_merge_request_rebase_settings.html.haml b/app/views/projects/_merge_request_rebase_settings.html.haml index c52e09573a6..54e0b73d24c 100644 --- a/app/views/projects/_merge_request_rebase_settings.html.haml +++ b/app/views/projects/_merge_request_rebase_settings.html.haml @@ -10,4 +10,4 @@ This way you could make sure that if this merge request would build, after merging to target branch it would also build. %br %span.descr - When fast-forward merge is not possible, the user must first rebase locally. + When fast-forward merge is not possible, the user is given the option to rebase. diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 268b7028fd9..fafd9e5ef00 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -89,6 +89,7 @@ - project_service - propagate_service_template - reactive_caching +- rebase - repository_fork - repository_import - storage_migrator diff --git a/app/workers/rebase_worker.rb b/app/workers/rebase_worker.rb new file mode 100644 index 00000000000..090987778a2 --- /dev/null +++ b/app/workers/rebase_worker.rb @@ -0,0 +1,12 @@ +class RebaseWorker + include ApplicationWorker + + def perform(merge_request_id, current_user_id) + current_user = User.find(current_user_id) + merge_request = MergeRequest.find(merge_request_id) + + MergeRequests::RebaseService + .new(merge_request.source_project, current_user) + .execute(merge_request) + end +end diff --git a/changelogs/unreleased/40301-rebase.yml b/changelogs/unreleased/40301-rebase.yml new file mode 100644 index 00000000000..1c0fc0cd8ae --- /dev/null +++ b/changelogs/unreleased/40301-rebase.yml @@ -0,0 +1,5 @@ +--- +title: Allow user to rebase merge requests. +merge_request: +author: +type: added diff --git a/config/routes/project.rb b/config/routes/project.rb index c3ad53a387f..1354c4c5537 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -96,6 +96,7 @@ constraints(ProjectUrlConstrainer.new) do post :toggle_subscription post :remove_wip post :assign_related_issues + post :rebase scope constraints: { format: nil }, action: :show do get :commits, defaults: { tab: 'commits' } diff --git a/db/migrate/20171230123729_add_rebase_commit_sha_to_merge_requests.rb b/db/migrate/20171230123729_add_rebase_commit_sha_to_merge_requests.rb new file mode 100644 index 00000000000..2ce156fa92e --- /dev/null +++ b/db/migrate/20171230123729_add_rebase_commit_sha_to_merge_requests.rb @@ -0,0 +1,7 @@ +class AddRebaseCommitShaToMergeRequests < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :merge_requests, :rebase_commit_sha, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 778d66f16b0..740e80ccfd4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171229225929) do +ActiveRecord::Schema.define(version: 20171230123729) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1099,6 +1099,7 @@ ActiveRecord::Schema.define(version: 20171229225929) do t.string "merge_jid" t.boolean "discussion_locked" t.integer "latest_merge_request_diff_id" + t.string "rebase_commit_sha" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/user/project/merge_requests/fast_forward_merge.md b/doc/user/project/merge_requests/fast_forward_merge.md index 085170d9f03..3cd91a185e3 100644 --- a/doc/user/project/merge_requests/fast_forward_merge.md +++ b/doc/user/project/merge_requests/fast_forward_merge.md @@ -9,7 +9,7 @@ When the fast-forward merge ([`--ff-only`][ffonly]) setting is enabled, no merge commits will be created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. -When a fast-forward merge is not possible, the user must rebase the branch manually. +When a fast-forward merge is not possible, the user is given the option to rebase. ## Use cases @@ -25,7 +25,7 @@ merge commits. In such cases, the fast-forward merge is the perfect candidate. Now, when you visit the merge request page, you will be able to accept it **only if a fast-forward merge is possible**. - + If the target branch is ahead of the source branch, you need to rebase the source branch locally before you will be able to do a fast-forward merge. diff --git a/doc/user/project/merge_requests/img/ff_merge_mr.png b/doc/user/project/merge_requests/img/ff_merge_mr.png Binary files differdeleted file mode 100644 index 241cc990343..00000000000 --- a/doc/user/project/merge_requests/img/ff_merge_mr.png +++ /dev/null diff --git a/doc/user/project/merge_requests/img/ff_merge_rebase.png b/doc/user/project/merge_requests/img/ff_merge_rebase.png Binary files differnew file mode 100644 index 00000000000..f6139f189ce --- /dev/null +++ b/doc/user/project/merge_requests/img/ff_merge_rebase.png diff --git a/features/project/ff_merge_requests.feature b/features/project/ff_merge_requests.feature index 995e52f9332..39035d551d1 100644 --- a/features/project/ff_merge_requests.feature +++ b/features/project/ff_merge_requests.feature @@ -22,3 +22,20 @@ Feature: Project Ff Merge Requests Then I should see ff-only merge button When I accept this merge request Then I should see merged request + + @javascript + Scenario: I do rebase before ff-only merge + Given ff merge enabled + And rebase before merge enabled + When I visit merge request page "Bug NS-05" + Then I should see rebase button + When I press rebase button + Then I should see rebase in progress message + + @javascript + Scenario: I do rebase before regular merge + Given rebase before merge enabled + When I visit merge request page "Bug NS-05" + Then I should see rebase button + When I press rebase button + Then I should see rebase in progress message diff --git a/features/steps/project/ff_merge_requests.rb b/features/steps/project/ff_merge_requests.rb index d68fe71e16e..27efcfd65b6 100644 --- a/features/steps/project/ff_merge_requests.rb +++ b/features/steps/project/ff_merge_requests.rb @@ -17,6 +17,10 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps author: project.users.first) end + step 'merge request is mergeable' do + expect(page).to have_button 'Merge' + end + step 'I should see ff-only merge button' do expect(page).to have_content "Fast-forward merge without a merge commit" expect(page).to have_button 'Merge' @@ -45,6 +49,10 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps project.save! end + step 'I should see rebase button' do + expect(page).to have_button "Rebase" + end + step 'merge request "Bug NS-05" is rebased' do merge_request.source_branch = 'flatten-dir' merge_request.target_branch = 'improve/awesome' @@ -59,6 +67,20 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps merge_request.save! end + step 'rebase before merge enabled' do + project = merge_request.target_project + project.merge_requests_rebase_enabled = true + project.save! + end + + step 'I press rebase button' do + click_button "Rebase" + end + + step "I should see rebase in progress message" do + expect(page).to have_content("Rebase in progress") + end + def merge_request @merge_request ||= MergeRequest.find_by!(title: "Bug NS-05") end diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb index ef5bdbaf819..3fb0e2eed93 100644 --- a/lib/gitlab/git/operation_service.rb +++ b/lib/gitlab/git/operation_service.rb @@ -97,6 +97,11 @@ module Gitlab end end + def update_branch(branch_name, newrev, oldrev) + ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name + update_ref_in_hooks(ref, newrev, oldrev) + end + private # Returns [newrev, should_run_after_create, should_run_after_create_branch] diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 45c424af8c4..c8cc6b374f6 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -684,4 +684,62 @@ describe Projects::MergeRequestsController do format: :json end end + + describe 'POST #rebase' do + let(:viewer) { user } + + def post_rebase + post :rebase, namespace_id: project.namespace, project_id: project, id: merge_request + end + + def expect_rebase_worker_for(user) + expect(RebaseWorker).to receive(:perform_async).with(merge_request.id, user.id) + end + + context 'successfully' do + it 'enqeues a RebaseWorker' do + expect_rebase_worker_for(viewer) + + post_rebase + + expect(response.status).to eq(200) + end + end + + context 'with a forked project' do + let(:fork_project) { create(:project, :repository, forked_from_project: project) } + let(:fork_owner) { fork_project.owner } + + before do + merge_request.update!(source_project: fork_project) + fork_project.add_reporter(user) + end + + context 'user cannot push to source branch' do + it 'returns 404' do + expect_rebase_worker_for(viewer).never + + post_rebase + + expect(response.status).to eq(404) + end + end + + context 'user can push to source branch' do + before do + project.add_reporter(fork_owner) + + sign_in(fork_owner) + end + + it 'returns 200' do + expect_rebase_worker_for(fork_owner) + + post_rebase + + expect(response.status).to eq(200) + end + end + end + end end diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index 995f13381ad..f1199468d53 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json @@ -9,6 +9,7 @@ "human_time_estimate": { "type": ["string", "null"] }, "human_total_time_spent": { "type": ["string", "null"] }, "merge_error": { "type": ["string", "null"] }, + "rebase_in_progress": { "type": "boolean" }, "assignee_id": { "type": ["integer", "null"] }, "subscribed": { "type": ["boolean", "null"] }, "participants": { "type": "array" } diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json index 9de27bee751..7f662098216 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -103,7 +103,11 @@ "remove_source_branch": { "type": ["boolean", "null"] }, "merge_ongoing": { "type": "boolean" }, "ff_only_enabled": { "type": ["boolean", false] }, - "should_be_rebased": { "type": "boolean" } + "should_be_rebased": { "type": "boolean" }, + "rebase_commit_sha": { "type": ["string", "null"] }, + "rebase_in_progress": { "type": "boolean" }, + "can_push_to_source_branch": { "type": "boolean" }, + "rebase_path": { "type": ["string", "null"] } }, "additionalProperties": false } diff --git a/spec/javascripts/vue_mr_widget/components/mr_widget_rebase_spec.js b/spec/javascripts/vue_mr_widget/components/mr_widget_rebase_spec.js new file mode 100644 index 00000000000..66ecaa316c8 --- /dev/null +++ b/spec/javascripts/vue_mr_widget/components/mr_widget_rebase_spec.js @@ -0,0 +1,115 @@ +import Vue from 'vue'; +import eventHub from '~/vue_merge_request_widget/event_hub'; +import component from '~/vue_merge_request_widget/components/states/mr_widget_rebase.vue'; +import mountComponent from '../../helpers/vue_mount_component_helper'; + +describe('Merge request widget rebase component', () => { + let Component; + let vm; + beforeEach(() => { + Component = Vue.extend(component); + }); + + afterEach(() => { + vm.$destroy(); + }); + + describe('While rebasing', () => { + it('should show progress message', () => { + vm = mountComponent(Component, { + mr: { rebaseInProgress: true }, + service: {}, + }); + + expect( + vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim(), + ).toContain('Rebase in progress'); + }); + }); + + describe('With permissions', () => { + beforeEach(() => { + vm = mountComponent(Component, { + mr: { + rebaseInProgress: false, + canPushToSourceBranch: true, + }, + service: {}, + }); + }); + + it('it should render rebase button and warning message', () => { + const text = vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim(); + expect(text).toContain('Fast-forward merge is not possible.'); + expect(text).toContain('Rebase the source branch onto the target branch or merge target'); + expect(text).toContain('branch into source branch to allow this merge request to be merged.'); + }); + + it('it should render error message when it fails', (done) => { + vm.rebasingError = 'Something went wrong!'; + + Vue.nextTick(() => { + expect( + vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim(), + ).toContain('Something went wrong!'); + done(); + }); + }); + }); + + describe('Without permissions', () => { + it('should render a message explaining user does not have permissions', () => { + vm = mountComponent(Component, { + mr: { + rebaseInProgress: false, + canPushToSourceBranch: false, + targetBranch: 'foo', + }, + service: {}, + }); + + const text = vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim(); + + expect(text).toContain('Fast-forward merge is not possible.'); + expect(text).toContain('Rebase the source branch onto'); + expect(text).toContain('foo'); + expect(text).toContain('to allow this merge request to be merged.'); + }); + }); + + describe('methods', () => { + it('checkRebaseStatus', (done) => { + spyOn(eventHub, '$emit'); + vm = mountComponent(Component, { + mr: {}, + service: { + rebase() { + return Promise.resolve(); + }, + poll() { + return Promise.resolve({ + data: { + rebase_in_progress: false, + merge_error: null, + }, + }); + }, + }, + }); + + vm.rebase(); + + // Wait for the rebase request + vm.$nextTick() + // Wait for the polling request + .then(vm.$nextTick()) + // Wait for the eventHub to be called + .then(vm.$nextTick()) + .then(() => { + expect(eventHub.$emit).toHaveBeenCalledWith('MRWidgetUpdateRequested'); + }) + .then(done) + .catch(done.fail); + }); + }); +}); diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index d8ebd46faab..07b3e1c1758 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1903,4 +1903,50 @@ describe MergeRequest do end end end + + describe '#should_be_rebased?' do + let(:project) { create(:project, :repository) } + + it 'returns false for the same source and target branches' do + merge_request = create(:merge_request, source_project: project, target_project: project) + + expect(merge_request.should_be_rebased?).to be_falsey + end + end + + describe '#rebase_in_progress?' do + # Create merge request and project before we stub file calls + before do + subject + end + + it 'returns true when there is a current rebase directory' do + allow(File).to receive(:exist?).and_return(true) + allow(File).to receive(:mtime).and_return(Time.now) + + expect(subject.rebase_in_progress?).to be_truthy + end + + it 'returns false when there is no rebase directory' do + allow(File).to receive(:exist?).and_return(false) + + expect(subject.rebase_in_progress?).to be_falsey + end + + it 'returns false when the rebase directory has expired' do + allow(File).to receive(:exist?).and_return(true) + allow(File).to receive(:mtime).and_return(20.minutes.ago) + + expect(subject.rebase_in_progress?).to be_falsey + end + + it 'returns false when the source project has been removed' do + allow(subject).to receive(:source_project).and_return(nil) + allow(File).to receive(:exist?).and_return(true) + allow(File).to receive(:mtime).and_return(Time.now) + + expect(File).not_to have_received(:exist?) + expect(subject.rebase_in_progress?).to be_falsey + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3c2ed043b82..13e5345ee4c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -418,14 +418,21 @@ describe Project do end describe '#merge_method' do - it 'returns "ff" merge_method when ff is enabled' do - project = build(:project, merge_requests_ff_only_enabled: true) - expect(project.merge_method).to be :ff + using RSpec::Parameterized::TableSyntax + + where(:ff, :rebase, :method) do + true | true | :ff + true | false | :ff + false | true | :rebase_merge + false | false | :merge end - it 'returns "merge" merge_method when ff is disabled' do - project = build(:project, merge_requests_ff_only_enabled: false) - expect(project.merge_method).to be :merge + with_them do + let(:project) { build(:project, merge_requests_rebase_enabled: rebase, merge_requests_ff_only_enabled: ff) } + + subject { project.merge_method } + + it { is_expected.to eq(method) } end end diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index 969c4753f33..e3b37739e8e 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -404,4 +404,67 @@ describe MergeRequestPresenter do .to eq("<a href=\"/#{resource.source_project.full_path}/tree/#{resource.source_branch}\">#{resource.source_branch}</a>") end end + + describe '#rebase_path' do + before do + allow(resource).to receive(:rebase_in_progress?) { rebase_in_progress } + allow(resource).to receive(:should_be_rebased?) { should_be_rebased } + + allow_any_instance_of(Gitlab::UserAccess::RequestCacheExtension) + .to receive(:can_push_to_branch?) + .with(resource.source_branch) + .and_return(can_push_to_branch) + end + + subject do + described_class.new(resource, current_user: user).rebase_path + end + + context 'when can rebase' do + let(:rebase_in_progress) { false } + let(:can_push_to_branch) { true } + let(:should_be_rebased) { true } + + before do + allow(resource).to receive(:source_branch_exists?) { true } + end + + it 'returns path' do + is_expected + .to eq("/#{project.full_path}/merge_requests/#{resource.iid}/rebase") + end + end + + context 'when cannot rebase' do + context 'when rebase in progress' do + let(:rebase_in_progress) { true } + let(:can_push_to_branch) { true } + let(:should_be_rebased) { true } + + it 'returns nil' do + is_expected.to be_nil + end + end + + context 'when user cannot merge' do + let(:rebase_in_progress) { false } + let(:can_push_to_branch) { false } + let(:should_be_rebased) { true } + + it 'returns nil' do + is_expected.to be_nil + end + end + + context 'should not be rebased' do + let(:rebase_in_progress) { false } + let(:can_push_to_branch) { true } + let(:should_be_rebased) { false } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + end end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index e25552eb0d8..80a271ba7fb 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -190,4 +190,20 @@ describe MergeRequestWidgetEntity do end end end + + describe 'when source project is deleted' do + let(:project) { create(:project, :repository) } + let(:fork_project) { create(:project, :repository, forked_from_project: project) } + let(:merge_request) { create(:merge_request, source_project: fork_project, target_project: project) } + + it 'returns a blank rebase_path' do + allow(merge_request).to receive(:should_be_rebased?).and_return(true) + fork_project.destroy + merge_request.reload + + entity = described_class.new(merge_request, request: request).as_json + + expect(entity[:rebase_path]).to be_nil + end + end end diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb new file mode 100644 index 00000000000..d1b37cdd073 --- /dev/null +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -0,0 +1,134 @@ +require 'spec_helper' + +describe MergeRequests::RebaseService do + include ProjectForksHelper + + let(:user) { create(:user) } + let(:merge_request) do + create(:merge_request, + source_branch: 'feature_conflict', + target_branch: 'master') + end + let(:project) { merge_request.project } + let(:repository) { project.repository.raw } + + subject(:service) { described_class.new(project, user, {}) } + + before do + project.add_master(user) + end + + describe '#execute' do + context 'when another rebase is already in progress' do + before do + allow(merge_request).to receive(:rebase_in_progress?).and_return(true) + end + + it 'saves the error message' do + subject.execute(merge_request) + + expect(merge_request.reload.merge_error).to eq 'Rebase task canceled: Another rebase is already in progress' + end + + it 'returns an error' do + expect(service.execute(merge_request)).to match(status: :error, + message: 'Failed to rebase. Should be done manually') + end + end + + context 'when unexpected error occurs' do + before do + allow(repository).to receive(:run_git!).and_raise('Something went wrong') + end + + it 'saves the error message' do + subject.execute(merge_request) + + expect(merge_request.reload.merge_error).to eq 'Something went wrong' + end + + it 'returns an error' do + expect(service.execute(merge_request)).to match(status: :error, + message: 'Failed to rebase. Should be done manually') + end + end + + context 'with git command failure' do + before do + allow(repository).to receive(:run_git!).and_raise(Gitlab::Git::Repository::GitError, 'Something went wrong') + end + + it 'saves the error message' do + subject.execute(merge_request) + + expect(merge_request.reload.merge_error).to eq 'Something went wrong' + end + + it 'returns an error' do + expect(service.execute(merge_request)).to match(status: :error, + message: 'Failed to rebase. Should be done manually') + end + end + + context 'valid params' do + before do + service.execute(merge_request) + end + + it 'rebases source branch' do + parent_sha = merge_request.source_project.repository.commit(merge_request.source_branch).parents.first.sha + target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha + expect(parent_sha).to eq(target_branch_sha) + end + + it 'records the new SHA on the merge request' do + head_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha + expect(merge_request.reload.rebase_commit_sha).to eq(head_sha) + end + + it 'logs correct author and commiter' do + head_commit = merge_request.source_project.repository.commit(merge_request.source_branch) + + expect(head_commit.author_email).to eq('dmitriy.zaporozhets@gmail.com') + expect(head_commit.author_name).to eq('Dmitriy Zaporozhets') + expect(head_commit.committer_email).to eq(user.email) + expect(head_commit.committer_name).to eq(user.name) + end + + context 'git commands' do + it 'sets GL_REPOSITORY env variable when calling git commands' do + expect(repository).to receive(:popen).exactly(3) + .with(anything, anything, hash_including('GL_REPOSITORY')) + .and_return(['', 0]) + + service.execute(merge_request) + end + end + + context 'fork' do + let(:forked_project) do + fork_project(project, user, repository: true) + end + + let(:merge_request_from_fork) do + forked_project.repository.create_file( + user, + 'new-file-to-target', + '', + message: 'Add new file to target', + branch_name: 'master') + + create(:merge_request, + source_branch: 'master', source_project: forked_project, + target_branch: 'master', target_project: project) + end + + it 'rebases source branch' do + parent_sha = forked_project.repository.commit(merge_request_from_fork.source_branch).parents.first.sha + target_branch_sha = project.repository.commit(merge_request_from_fork.target_branch).sha + expect(parent_sha).to eq(target_branch_sha) + end + end + end + end +end diff --git a/spec/views/projects/merge_requests/show.html.haml_spec.rb b/spec/views/projects/merge_requests/show.html.haml_spec.rb index 28d54c2fb77..264e0ce0b40 100644 --- a/spec/views/projects/merge_requests/show.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/show.html.haml_spec.rb @@ -54,6 +54,8 @@ describe 'projects/merge_requests/show.html.haml' do it 'closes the merge request if the source project does not exist' do closed_merge_request.update_attributes(state: 'open') forked_project.destroy + # Reload merge request so MergeRequest#source_project turns to `nil` + closed_merge_request.reload render diff --git a/spec/workers/rebase_worker_spec.rb b/spec/workers/rebase_worker_spec.rb new file mode 100644 index 00000000000..20aff020dbb --- /dev/null +++ b/spec/workers/rebase_worker_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe RebaseWorker, '#perform' do + context 'when rebasing an MR from a fork where upstream has protected branches' do + let(:upstream_project) { create(:project, :repository) } + let(:fork_project) { create(:project, :repository) } + + let(:merge_request) do + create(:merge_request, + source_project: fork_project, + source_branch: 'feature_conflict', + target_project: upstream_project, + target_branch: 'master') + end + + before do + create(:forked_project_link, forked_to_project: fork_project, forked_from_project: upstream_project) + end + + it 'sets the correct project for running hooks' do + expect(MergeRequests::RebaseService) + .to receive(:new).with(fork_project, merge_request.author).and_call_original + + subject.perform(merge_request, merge_request.author) + end + end +end |
