diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-10-03 08:24:31 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-10-03 08:24:31 +0000 |
commit | 0ac06c89e9cfc0fc8044b645294242d66af36150 (patch) | |
tree | cf72e186622982907975f06fc588c0542a56c35c | |
parent | 75f257ad57d29955fc25c49b39fb1833f7dd3c75 (diff) | |
parent | e559b8511b2817b9c57a5190ff9ed95c739528bf (diff) | |
download | gitlab-ce-0ac06c89e9cfc0fc8044b645294242d66af36150.tar.gz |
Merge branch 'ff_port_from_ee' into 'master'
Move Fast-Forward Merge to CE
See merge request gitlab-org/gitlab-ce!14272
38 files changed, 679 insertions, 95 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_status_icon.js b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_status_icon.js index 703f3a56a34..4998a47b691 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_status_icon.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_status_icon.js @@ -27,7 +27,7 @@ export default { <button v-if="showDisabledButton" type="button" - class="btn btn-success btn-sm" + class="js-disabled-merge-button btn btn-success btn-sm" disabled="true"> Merge </button> diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js index f9cb79a0bc1..dc252f8a9b7 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js @@ -10,27 +10,37 @@ export default { }, template: ` <div class="mr-widget-body media"> - <status-icon status="failed" showDisabledButton /> + <status-icon + status="failed" + showDisabledButton /> <div class="media-body space-children"> - <span class="bold"> - There are merge conflicts<span v-if="!mr.canMerge">.</span> - <span v-if="!mr.canMerge"> - Resolve these conflicts or ask someone with write access to this repository to merge it locally - </span> + <span + v-if="mr.shouldBeRebased" + class="bold"> + Fast-forward merge is not possible. + To merge this request, first rebase locally. </span> - <a - v-if="mr.canMerge && mr.conflictResolutionPath" - :href="mr.conflictResolutionPath" - class="btn btn-default btn-xs js-resolve-conflicts-button"> - Resolve conflicts - </a> - <a - v-if="mr.canMerge" - class="btn btn-default btn-xs js-merge-locally-button" - data-toggle="modal" - href="#modal_merge_info"> - Merge locally - </a> + <template v-else> + <span class="bold"> + There are merge conflicts<span v-if="!mr.canMerge">.</span> + <span v-if="!mr.canMerge"> + Resolve these conflicts or ask someone with write access to this repository to merge it locally + </span> + </span> + <a + v-if="mr.canMerge && mr.conflictResolutionPath" + :href="mr.conflictResolutionPath" + class="js-resolve-conflicts-button btn btn-default btn-xs"> + Resolve conflicts + </a> + <a + v-if="mr.canMerge" + class="js-merge-locally-button btn btn-default btn-xs" + data-toggle="modal" + href="#modal_merge_info"> + Merge locally + </a> + </template> </div> </div> `, diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js index ad709da51ee..f83d3ca00dd 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js @@ -284,10 +284,16 @@ export default { :mr="mr" :is-merge-button-disabled="isMergeButtonDisabled" /> + <span + v-if="mr.ffOnlyEnabled" + class="js-fast-forward-message"> + Fast-forward merge without a merge commit + </span> <button + v-else @click="toggleCommitMessageEditor" :disabled="isMergeButtonDisabled" - class="btn btn-default btn-xs" + class="js-modify-commit-message-button btn btn-default btn-xs" type="button"> Modify commit message </button> 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 29464662578..8255fd3e792 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 @@ -57,6 +57,8 @@ export default class MergeRequestStore { this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false; this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false; this.mergePath = data.merge_path; + this.ffOnlyEnabled = data.ff_only_enabled; + this.shouldBeRebased = !!data.should_be_rebased; this.statusPath = data.status_path; this.emailPatchesPath = data.email_patches_path; this.plainDiffPath = data.plain_diff_path; diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b13034d3333..a738ca9f361 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -344,6 +344,7 @@ class ProjectsController < Projects::ApplicationController :tag_list, :visibility_level, :template_name, + :merge_method, project_feature_attributes: %i[ builds_access_level diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8d9a30397a9..e85b83daf9e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -524,6 +524,14 @@ class MergeRequest < ActiveRecord::Base true end + def ff_merge_possible? + project.repository.ancestor?(target_branch_sha, diff_head_sha) + end + + def should_be_rebased? + project.ff_merge_must_be_possible? && !ff_merge_possible? + end + def can_cancel_merge_when_pipeline_succeeds?(current_user) can_be_merged_by?(current_user) || self.author == current_user end diff --git a/app/models/project.rb b/app/models/project.rb index 44d1190cc5b..952e9e22b28 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1566,6 +1566,34 @@ class Project < ActiveRecord::Base persisted? && path_changed? end + def merge_method + if self.merge_requests_ff_only_enabled + :ff + elsif self.merge_requests_rebase_enabled + :rebase_merge + else + :merge + end + end + + def merge_method=(method) + case method.to_s + when "ff" + self.merge_requests_ff_only_enabled = true + self.merge_requests_rebase_enabled = true + when "rebase_merge" + self.merge_requests_ff_only_enabled = false + self.merge_requests_rebase_enabled = true + when "merge" + self.merge_requests_ff_only_enabled = false + self.merge_requests_rebase_enabled = false + end + end + + def ff_merge_must_be_possible? + self.merge_requests_ff_only_enabled || self.merge_requests_rebase_enabled + end + def migrate_to_hashed_storage! return if hashed_storage? diff --git a/app/models/repository.rb b/app/models/repository.rb index a0f57f1e54d..d47dc9a05cd 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -850,6 +850,25 @@ class Repository end end + def ff_merge(user, source, target_branch, merge_request: nil) + our_commit = rugged.branches[target_branch].target + their_commit = + if source.is_a?(Gitlab::Git::Commit) + source.raw_commit + else + rugged.lookup(source) + end + + raise 'Invalid merge target' if our_commit.nil? + raise 'Invalid merge source' if their_commit.nil? + + with_branch(user, target_branch) do |start_commit| + merge_request&.update(in_progress_merge_commit_sha: their_commit.oid) + + their_commit.oid + end + end + def revert( user, commit, branch_name, message, start_branch_name: nil, start_project: project) diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index 07650ce6f20..7d3c752b8e6 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -13,6 +13,11 @@ class MergeRequestEntity < IssuableEntity expose :target_branch expose :target_project_id + expose :should_be_rebased?, as: :should_be_rebased + expose :ff_only_enabled do |merge_request| + merge_request.project.merge_requests_ff_only_enabled + end + # Events expose :merge_event, using: EventEntity expose :closed_event, using: EventEntity diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb new file mode 100644 index 00000000000..ba6853b835a --- /dev/null +++ b/app/services/merge_requests/ff_merge_service.rb @@ -0,0 +1,24 @@ +module MergeRequests + # MergeService class + # + # Do git fast-forward merge and in case of success + # mark merge request as merged and execute all hooks and notifications + # Executed when you do fast-forward merge via GitLab UI + # + class FfMergeService < MergeRequests::MergeService + private + + def commit + repository.ff_merge(current_user, + source, + merge_request.target_branch, + merge_request: merge_request) + rescue Gitlab::Git::HooksService::PreReceiveError => e + raise MergeError, e.message + rescue StandardError => e + raise MergeError, "Something went wrong during merge: #{e.message}" + ensure + merge_request.update(in_progress_merge_commit_sha: nil) + end + end +end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index bf26859dd6d..a110abf8256 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -11,6 +11,11 @@ module MergeRequests attr_reader :merge_request, :source def execute(merge_request) + if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService) + FfMergeService.new(project, current_user, params).execute(merge_request) + return + end + @merge_request = merge_request unless @merge_request.mergeable? diff --git a/app/views/projects/_merge_request_fast_forward_settings.html.haml b/app/views/projects/_merge_request_fast_forward_settings.html.haml new file mode 100644 index 00000000000..9d357293a2f --- /dev/null +++ b/app/views/projects/_merge_request_fast_forward_settings.html.haml @@ -0,0 +1,13 @@ +- form = local_assigns.fetch(:form) +- project = local_assigns.fetch(:project) + +.radio + = label_tag :project_merge_method_ff do + = form.radio_button :merge_method, :ff, class: "js-merge-method-radio" + %strong Fast-forward merge + %br + %span.descr + 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. diff --git a/app/views/projects/_merge_request_rebase_settings.html.haml b/app/views/projects/_merge_request_rebase_settings.html.haml new file mode 100644 index 00000000000..c52e09573a6 --- /dev/null +++ b/app/views/projects/_merge_request_rebase_settings.html.haml @@ -0,0 +1,13 @@ +- form = local_assigns.fetch(:form) + +.radio + = label_tag :project_merge_method_rebase_merge do + = form.radio_button :merge_method, :rebase_merge, class: "js-merge-method-radio" + %strong Merge commit with semi-linear history + %br + %span.descr + A merge commit is created for every merge, but merging is only allowed if fast-forward merge is possible. + 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. diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml index cc5afa943cf..fd0c419cdac 100644 --- a/app/views/projects/_merge_request_settings.html.haml +++ b/app/views/projects/_merge_request_settings.html.haml @@ -1,3 +1,18 @@ - form = local_assigns.fetch(:form) +.form-group + = label_tag :merge_method_merge, class: 'label-light' do + Merge method + .radio + = label_tag :project_merge_method_merge do + = form.radio_button :merge_method, :merge, class: "js-merge-method-radio" + %strong Merge commit + %br + %span.descr + A merge commit is created for every merge, and merging is allowed as long as there are no conflicts. + + = render 'merge_request_rebase_settings', form: form + + = render 'merge_request_fast_forward_settings', project: @project, form: form + = render 'projects/merge_request_merge_settings', form: form diff --git a/changelogs/unreleased/ff_port_from_ee.yml b/changelogs/unreleased/ff_port_from_ee.yml new file mode 100644 index 00000000000..e1cb7804a47 --- /dev/null +++ b/changelogs/unreleased/ff_port_from_ee.yml @@ -0,0 +1,5 @@ +--- +title: Move Custom merge methods from EE +merge_request: +author: +type: added diff --git a/db/migrate/20141126120926_add_merge_request_rebase_enabled_to_projects.rb b/db/migrate/20141126120926_add_merge_request_rebase_enabled_to_projects.rb new file mode 100644 index 00000000000..3dafdf0fde4 --- /dev/null +++ b/db/migrate/20141126120926_add_merge_request_rebase_enabled_to_projects.rb @@ -0,0 +1,17 @@ +# rubocop:disable all +class AddMergeRequestRebaseEnabledToProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:projects, :merge_requests_rebase_enabled, :boolean, default: false) + end + + def down + remove_column(:projects, :merge_requests_rebase_enabled) + end +end diff --git a/db/migrate/20150827121444_add_fast_forward_option_to_project.rb b/db/migrate/20150827121444_add_fast_forward_option_to_project.rb new file mode 100644 index 00000000000..014f5b2f372 --- /dev/null +++ b/db/migrate/20150827121444_add_fast_forward_option_to_project.rb @@ -0,0 +1,17 @@ +# rubocop:disable all +class AddFastForwardOptionToProject < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def add + add_column_with_default(:projects, :merge_requests_ff_only_enabled, :boolean, default: false) + end + + def down + remove_column(:projects, :merge_requests_ff_only_enabled) + end +end diff --git a/db/schema.rb b/db/schema.rb index e8e64b9d36b..c238efbedbe 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1217,6 +1217,8 @@ ActiveRecord::Schema.define(version: 20170928100231) do t.integer "storage_version", limit: 2 t.boolean "resolve_outdated_diff_discussions" t.boolean "repository_read_only" + t.boolean "merge_requests_ff_only_enabled", default: false + t.boolean "merge_requests_rebase_enabled", default: false, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_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 new file mode 100644 index 00000000000..085170d9f03 --- /dev/null +++ b/doc/user/project/merge_requests/fast_forward_merge.md @@ -0,0 +1,35 @@ +# Fast-forward merge requests + +Retain a linear Git history and a way to accept merge requests without +creating merge commits. + +## Overview + +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. + +## Use cases + +Sometimes, a workflow policy might mandate a clean commit history without +merge commits. In such cases, the fast-forward merge is the perfect candidate. + +## Enabling fast-forward merges + +1. Navigate to your project's **Settings** and search for the 'Merge method' +1. Select the **Fast-forward merge** option +1. Hit **Save changes** for the changes to take effect + +Now, when you visit the merge request page, you will be able to accept it +**only if a fast-forward merge is possible**. + +![Fast forward merge request](img/ff_merge_mr.png) + +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. + +![Fast forward merge rebase locally](img/ff_merge_rebase_locally.png) + +[ffonly]: https://git-scm.com/docs/git-merge#git-merge---ff-only 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 differnew file mode 100644 index 00000000000..241cc990343 --- /dev/null +++ b/doc/user/project/merge_requests/img/ff_merge_mr.png diff --git a/doc/user/project/merge_requests/img/ff_merge_rebase_locally.png b/doc/user/project/merge_requests/img/ff_merge_rebase_locally.png Binary files differnew file mode 100644 index 00000000000..fb412296efc --- /dev/null +++ b/doc/user/project/merge_requests/img/ff_merge_rebase_locally.png diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index 26c6277d33a..8e081b4f0b8 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -23,12 +23,14 @@ With GitLab merge requests, you can: - Organize your issues and merge requests consistently throughout the project with [labels](../../project/labels.md) - Add a time estimation and the time spent with that merge request with [Time Tracking](../../../workflow/time_tracking.html#time-tracking) - [Resolve merge conflicts from the UI](#resolve-conflicts) +- Enable [fast-forward merge requests](#fast-forward-merge-requests) +- Enable [semi-linear history merge requests](#semi-linear-history-merge-requests) as another security layer to guarantee the pipeline is passing in the target branch + With **[GitLab Enterprise Edition][ee]**, you can also: - View the deployment process across projects with [Multi-Project Pipeline Graphs](https://docs.gitlab.com/ee/ci/multi_project_pipeline_graphs.html#multi-project-pipeline-graphs) (available only in GitLab Enterprise Edition Premium) - Request [approvals](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html) from your managers (available in GitLab Enterprise Edition Starter) -- Enable [fast-forward merge requests](https://docs.gitlab.com/ee/user/project/merge_requests/fast_forward_merge.html) (available in GitLab Enterprise Edition Starter) - [Squash and merge](https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html) for a cleaner commit history (available in GitLab Enterprise Edition Starter) - Enable [semi-linear history merge requests](https://docs.gitlab.com/ee/user/project/merge_requests/index.html#semi-linear-history-merge-requests) as another security layer to guarantee the pipeline is passing in the target branch (available in GitLab Enterprise Edition Starter) - Analise the impact of your changes with [Code Quality reports](https://docs.gitlab.com/ee/user/project/merge_requests/code_quality_diff.html) (available in GitLab Enterprise Edition Starter) @@ -89,6 +91,22 @@ in a merged merge requests or a commit. [Learn more about cherry-picking changes.](cherry_pick_changes.md) +## Semi-linear history merge requests + +A merge commit is created for every merge, but the branch is only merged if +a fast-forward merge is possible. This ensures that if the merge request build +succeeded, the target branch build will also succeed after merging. + +Navigate to a project's settings, select the **Merge commit with semi-linear +history** option under **Merge Requests: Merge method** and save your changes. + +## Fast-forward merge requests + +If you prefer a linear Git history and a way to accept merge requests without +creating merge commits, you can configure this on a per-project basis. + +[Read more about fast-forward merge requests.](fast_forward_merge.md) + ## Merge when pipeline succeeds When reviewing a merge request that looks ready to merge but still has one or @@ -254,4 +272,4 @@ git checkout origin/merge-requests/1 ``` [protected branches]: ../protected_branches.md -[ee]: https://about.gitlab.com/gitlab-ee/ "GitLab Enterprise Edition"
\ No newline at end of file +[ee]: https://about.gitlab.com/gitlab-ee/ "GitLab Enterprise Edition" diff --git a/doc/user/project/settings/index.md b/doc/user/project/settings/index.md index 22c343dc027..b7c7a5e5d28 100644 --- a/doc/user/project/settings/index.md +++ b/doc/user/project/settings/index.md @@ -23,7 +23,7 @@ Add an [issue description template](../description_templates.md#description-temp Set up your project's merge request settings: -- Set up the merge request method (merge commit, [fast-forward merge](https://docs.gitlab.com/ee/user/project/merge_requests/fast_forward_merge.html#fast-forward-merge-requests)). _Fast-forward is available in [GitLab Enterprise Edition Starter](https://about.gitlab.com/gitlab-ee/)._ +- Set up the merge request method (merge commit, [fast-forward merge](../merge_requests/fast_forward_merge.html)). - Merge request [description templates](../description_templates.md#description-templates). - Enable [merge request approvals](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html#merge-request-approvals), _available in [GitLab Enterprise Edition Starter](https://about.gitlab.com/gitlab-ee/)_. - Enable [merge only of pipeline succeeds](../merge_requests/merge_when_pipeline_succeeds.md). diff --git a/doc/workflow/README.md b/doc/workflow/README.md index 673e08287a3..6b2aba47f54 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -36,6 +36,7 @@ - [Revert changes in the UI](../user/project/merge_requests/revert_changes.md) - [Merge requests versions](../user/project/merge_requests/versions.md) - ["Work In Progress" merge requests](../user/project/merge_requests/work_in_progress_merge_requests.md) + - [Fast-forward merge requests](../user/project/merge_requests/fast_forward_merge.md) - [Manage large binaries with Git LFS](lfs/manage_large_binaries_with_git_lfs.md) - [Importing from SVN, GitHub, Bitbucket, etc](importing/README.md) - [Todos](todos.md) diff --git a/features/project/ff_merge_requests.feature b/features/project/ff_merge_requests.feature new file mode 100644 index 00000000000..995e52f9332 --- /dev/null +++ b/features/project/ff_merge_requests.feature @@ -0,0 +1,24 @@ +Feature: Project Ff Merge Requests + Background: + Given I sign in as a user + And I own project "Shop" + And project "Shop" have "Bug NS-05" open merge request with diffs inside + And merge request "Bug NS-05" is mergeable + + @javascript + Scenario: I do ff-only merge for rebased branch + Given ff merge enabled + And merge request "Bug NS-05" is rebased + When I visit merge request page "Bug NS-05" + Then I should see ff-only merge button + When I accept this merge request + Then I should see merged request + + @javascript + Scenario: I do ff-only merge for merged branch + Given ff merge enabled + And merge request "Bug NS-05" merged target + When I visit merge request page "Bug NS-05" + Then I should see ff-only merge button + When I accept this merge request + Then I should see merged request diff --git a/features/steps/project/ff_merge_requests.rb b/features/steps/project/ff_merge_requests.rb new file mode 100644 index 00000000000..d68fe71e16e --- /dev/null +++ b/features/steps/project/ff_merge_requests.rb @@ -0,0 +1,65 @@ +class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps + include SharedAuthentication + include SharedIssuable + include SharedProject + include SharedNote + include SharedPaths + include SharedMarkdown + include SharedDiffNote + include SharedUser + include WaitForRequests + + step 'project "Shop" have "Bug NS-05" open merge request with diffs inside' do + create(:merge_request_with_diffs, + title: "Bug NS-05", + source_project: project, + target_project: project, + author: project.users.first) + 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' + end + + step 'merge request "Bug NS-05" is mergeable' do + merge_request.mark_as_mergeable + end + + step 'I accept this merge request' do + page.within '.mr-state-widget' do + click_button "Merge" + end + end + + step 'I should see merged request' do + page.within '.status-box' do + expect(page).to have_content "Merged" + wait_for_requests + end + end + + step 'ff merge enabled' do + project = merge_request.target_project + project.merge_requests_ff_only_enabled = true + project.save! + end + + step 'merge request "Bug NS-05" is rebased' do + merge_request.source_branch = 'flatten-dir' + merge_request.target_branch = 'improve/awesome' + merge_request.reload_diff + merge_request.save! + end + + step 'merge request "Bug NS-05" merged target' do + merge_request.source_branch = 'merged-target' + merge_request.target_branch = 'improve/awesome' + merge_request.reload_diff + merge_request.save! + end + + def merge_request + @merge_request ||= MergeRequest.find_by!(title: "Bug NS-05") + end +end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 4459e227fb3..2a91a6613e6 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -289,6 +289,24 @@ describe ProjectsController do end end + it 'updates Fast Forward Merge attributes' do + controller.instance_variable_set(:@project, project) + + params = { + merge_method: :ff + } + + put :update, + namespace_id: project.namespace, + id: project.id, + project: params + + expect(response).to have_http_status(302) + params.each do |param, value| + expect(project.public_send(param)).to eq(value) + end + end + def update_project(**parameters) put :update, namespace_id: project.namespace.path, diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb index 443b596b3c6..791cfa308c3 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -202,6 +202,28 @@ describe 'Merge request', :js do end end + context 'view merge request where fast-forward merge is not possible' do + before do + project.update(merge_requests_ff_only_enabled: true) + + merge_request.update( + merge_user: merge_request.author, + merge_status: :cannot_be_merged + ) + + visit project_merge_request_path(project, merge_request) + end + + it 'shows information about the merge error' do + # Wait for the `ci_status` and `merge_check` requests + wait_for_requests + + page.within('.mr-widget-body') do + expect(page).to have_content('Fast-forward merge is not possible') + end + end + end + context 'merge error' do before do allow_any_instance_of(Repository).to receive(:merge).and_return(false) diff --git a/spec/features/projects/project_settings_spec.rb b/spec/features/projects/project_settings_spec.rb index 5d77cd1ccd5..06568817757 100644 --- a/spec/features/projects/project_settings_spec.rb +++ b/spec/features/projects/project_settings_spec.rb @@ -32,6 +32,32 @@ describe 'Edit Project Settings' do end end + describe 'Merge request settings section' do + it 'shows "Merge commit" strategy' do + visit edit_project_path(project) + + page.within '.merge-requests-feature' do + expect(page).to have_content 'Merge commit' + end + end + + it 'shows "Merge commit with semi-linear history " strategy' do + visit edit_project_path(project) + + page.within '.merge-requests-feature' do + expect(page).to have_content 'Merge commit with semi-linear history' + end + end + + it 'shows "Fast-forward merge" strategy' do + visit edit_project_path(project) + + page.within '.merge-requests-feature' do + expect(page).to have_content 'Fast-forward merge' + end + end + end + describe 'Rename repository section' do context 'with invalid characters' do it 'shows errors for invalid project path/name' do diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json index 1030f323a1f..0796d9b8af9 100644 --- a/spec/fixtures/api/schemas/entities/merge_request.json +++ b/spec/fixtures/api/schemas/entities/merge_request.json @@ -96,7 +96,9 @@ "remove_wip_path": { "type": "string" }, "commits_count": { "type": "integer" }, "remove_source_branch": { "type": ["boolean", "null"] }, - "merge_ongoing": { "type": "boolean" } + "merge_ongoing": { "type": "boolean" }, + "ff_only_enabled": { "type": ["boolean", false] }, + "should_be_rebased": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js index 3b7b7d93662..5d4c7ec09dc 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js @@ -1,20 +1,9 @@ import Vue from 'vue'; import conflictsComponent from '~/vue_merge_request_widget/components/states/mr_widget_conflicts'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; +const ConflictsComponent = Vue.extend(conflictsComponent); const path = '/conflicts'; -const createComponent = () => { - const Component = Vue.extend(conflictsComponent); - - return new Component({ - el: document.createElement('div'), - propsData: { - mr: { - canMerge: true, - conflictResolutionPath: path, - }, - }, - }); -}; describe('MRWidgetConflicts', () => { describe('props', () => { @@ -27,44 +16,90 @@ describe('MRWidgetConflicts', () => { }); describe('template', () => { - it('should have correct elements', () => { - const el = createComponent().$el; - const resolveButton = el.querySelector('.js-resolve-conflicts-button'); - const mergeButton = el.querySelector('.mr-widget-body .btn'); - const mergeLocallyButton = el.querySelector('.js-merge-locally-button'); - - expect(el.textContent).toContain('There are merge conflicts'); - expect(el.textContent).not.toContain('ask someone with write access'); - expect(el.querySelector('.btn-success').disabled).toBeTruthy(); - expect(resolveButton.textContent).toContain('Resolve conflicts'); - expect(resolveButton.getAttribute('href')).toEqual(path); - expect(mergeButton.textContent).toContain('Merge'); - expect(mergeLocallyButton.textContent).toContain('Merge locally'); + describe('when allowed to merge', () => { + let vm; + + beforeEach(() => { + vm = mountComponent(ConflictsComponent, { + mr: { + canMerge: true, + conflictResolutionPath: path, + }, + }); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should tell you about conflicts without bothering other people', () => { + expect(vm.$el.textContent).toContain('There are merge conflicts'); + expect(vm.$el.textContent).not.toContain('ask someone with write access'); + }); + + it('should allow you to resolve the conflicts', () => { + const resolveButton = vm.$el.querySelector('.js-resolve-conflicts-button'); + + expect(resolveButton.textContent).toContain('Resolve conflicts'); + expect(resolveButton.getAttribute('href')).toEqual(path); + }); + + it('should have merge buttons', () => { + const mergeButton = vm.$el.querySelector('.js-disabled-merge-button'); + const mergeLocallyButton = vm.$el.querySelector('.js-merge-locally-button'); + + expect(mergeButton.textContent).toContain('Merge'); + expect(mergeButton.disabled).toBeTruthy(); + expect(mergeButton.classList.contains('btn-success')).toEqual(true); + expect(mergeLocallyButton.textContent).toContain('Merge locally'); + }); }); describe('when user does not have permission to merge', () => { let vm; beforeEach(() => { - vm = createComponent(); - vm.mr.canMerge = false; + vm = mountComponent(ConflictsComponent, { + mr: { + canMerge: false, + }, + }); }); - it('should show proper message', (done) => { - Vue.nextTick(() => { - expect(vm.$el.textContent).toContain('ask someone with write access'); - done(); - }); + afterEach(() => { + vm.$destroy(); + }); + + it('should show proper message', () => { + expect(vm.$el.textContent).toContain('ask someone with write access'); + }); + + it('should not have action buttons', () => { + expect(vm.$el.querySelector('.js-disabled-merge-button')).toBeDefined(); + expect(vm.$el.querySelector('.js-resolve-conflicts-button')).toBeNull(); + expect(vm.$el.querySelector('.js-merge-locally-button')).toBeNull(); }); + }); - it('should not have action buttons', (done) => { - Vue.nextTick(() => { - expect(vm.$el.querySelectorAll('.btn').length).toBe(1); - expect(vm.$el.querySelector('.js-resolve-conflicts-button')).toEqual(null); - expect(vm.$el.querySelector('.js-merge-locally-button')).toEqual(null); - done(); + describe('when fast-forward or semi-linear merge enabled', () => { + let vm; + + beforeEach(() => { + vm = mountComponent(ConflictsComponent, { + mr: { + shouldBeRebased: true, + }, }); }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should tell you to rebase locally', () => { + expect(vm.$el.textContent).toContain('Fast-forward merge is not possible.'); + expect(vm.$el.textContent).toContain('To merge this request, first rebase locally'); + }); }); }); }); diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js index 03a52f1f91c..2422e844e97 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -182,36 +182,6 @@ describe('MRWidgetReadyToMerge', () => { expect(vm.isMergeButtonDisabled).toBeTruthy(); }); }); - - describe('Remove source branch checkbox', () => { - describe('when user can merge but cannot delete branch', () => { - it('isRemoveSourceBranchButtonDisabled should be true', () => { - expect(vm.isRemoveSourceBranchButtonDisabled).toBe(true); - }); - - it('should be disabled in the rendered output', () => { - const checkboxElement = vm.$el.querySelector('#remove-source-branch-input'); - expect(checkboxElement.getAttribute('disabled')).toBe('disabled'); - }); - }); - - describe('when user can merge and can delete branch', () => { - beforeEach(() => { - this.customVm = createComponent({ - mr: { canRemoveSourceBranch: true }, - }); - }); - - it('isRemoveSourceBranchButtonDisabled should be false', () => { - expect(this.customVm.isRemoveSourceBranchButtonDisabled).toBe(false); - }); - - it('should be enabled in rendered output', () => { - const checkboxElement = this.customVm.$el.querySelector('#remove-source-branch-input'); - expect(checkboxElement.getAttribute('disabled')).toBeNull(); - }); - }); - }); }); describe('methods', () => { @@ -467,4 +437,54 @@ describe('MRWidgetReadyToMerge', () => { }); }); }); + + describe('Remove source branch checkbox', () => { + describe('when user can merge but cannot delete branch', () => { + it('isRemoveSourceBranchButtonDisabled should be true', () => { + expect(vm.isRemoveSourceBranchButtonDisabled).toBe(true); + }); + + it('should be disabled in the rendered output', () => { + const checkboxElement = vm.$el.querySelector('#remove-source-branch-input'); + expect(checkboxElement.getAttribute('disabled')).toBe('disabled'); + }); + }); + + describe('when user can merge and can delete branch', () => { + beforeEach(() => { + this.customVm = createComponent({ + mr: { canRemoveSourceBranch: true }, + }); + }); + + it('isRemoveSourceBranchButtonDisabled should be false', () => { + expect(this.customVm.isRemoveSourceBranchButtonDisabled).toBe(false); + }); + + it('should be enabled in rendered output', () => { + const checkboxElement = this.customVm.$el.querySelector('#remove-source-branch-input'); + expect(checkboxElement.getAttribute('disabled')).toBeNull(); + }); + }); + }); + + describe('Commit message area', () => { + it('when using merge commits, should show "Modify commit message" button', () => { + const customVm = createComponent({ + mr: { ffOnlyEnabled: false }, + }); + + expect(customVm.$el.querySelector('.js-fast-forward-message')).toBeNull(); + expect(customVm.$el.querySelector('.js-modify-commit-message-button')).toBeDefined(); + }); + + it('when fast-forward merge is enabled, only show fast-forward message', () => { + const customVm = createComponent({ + mr: { ffOnlyEnabled: true }, + }); + + expect(customVm.$el.querySelector('.js-fast-forward-message')).toBeDefined(); + expect(customVm.$el.querySelector('.js-modify-commit-message-button')).toBeNull(); + }); + }); }); diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 899d17d97c2..7268226112c 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -412,6 +412,8 @@ Project: - last_repository_updated_at - ci_config_path - delete_error +- merge_requests_ff_only_enabled +- merge_requests_rebase_enabled Author: - name ProjectFeature: diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c94481d53c3..176bb568cbe 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -408,6 +408,18 @@ describe Project do end 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 + 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 + end + end + describe '#repository_storage_path' do let(:project) { create(:project, repository_storage: 'custom') } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 157a10edd73..8a4dcdc311e 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1345,6 +1345,34 @@ describe Repository do end end + describe '#ff_merge' do + before do + repository.add_branch(user, 'ff-target', 'feature~5') + end + + it 'merges the code and return the commit id' do + merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'ff-target', source_project: project) + merge_commit_id = repository.ff_merge(user, + merge_request.diff_head_sha, + merge_request.target_branch, + merge_request: merge_request) + merge_commit = repository.commit(merge_commit_id) + + expect(merge_commit).to be_present + expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present + end + + it 'sets the `in_progress_merge_commit_sha` flag for the given merge request' do + merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'ff-target', source_project: project) + merge_commit_id = repository.ff_merge(user, + merge_request.diff_head_sha, + merge_request.target_branch, + merge_request: merge_request) + + expect(merge_request.in_progress_merge_commit_sha).to eq(merge_commit_id) + end + end + describe '#revert' do let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb index a2fd5b7daae..4288955ddbc 100644 --- a/spec/serializers/merge_request_entity_spec.rb +++ b/spec/serializers/merge_request_entity_spec.rb @@ -47,7 +47,8 @@ describe MergeRequestEntity do :cancel_merge_when_pipeline_succeeds_path, :create_issue_to_resolve_discussions_path, :source_branch_path, :target_branch_commits_path, - :target_branch_tree_path, :commits_count, :merge_ongoing) + :target_branch_tree_path, :commits_count, :merge_ongoing, + :ff_only_enabled) end it 'has email_patches_path' do diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb new file mode 100644 index 00000000000..aaabf3ed2b0 --- /dev/null +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -0,0 +1,84 @@ +require 'spec_helper' + +describe MergeRequests::FfMergeService do + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:merge_request) do + create(:merge_request, + source_branch: 'flatten-dir', + target_branch: 'improve/awesome', + assignee: user2) + end + let(:project) { merge_request.project } + + before do + project.team << [user, :master] + project.team << [user2, :developer] + end + + describe '#execute' do + context 'valid params' do + let(:service) { described_class.new(project, user, {}) } + + before do + allow(service).to receive(:execute_hooks) + + perform_enqueued_jobs do + service.execute(merge_request) + end + end + + it "does not create merge commit" do + source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha + target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha + expect(source_branch_sha).to eq(target_branch_sha) + end + + it { expect(merge_request).to be_valid } + it { expect(merge_request).to be_merged } + + it 'sends email to user2 about merge of new merge_request' do + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end + + it 'creates system note about merge_request merge' do + note = merge_request.notes.last + expect(note.note).to include 'merged' + end + end + + context "error handling" do + let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } + + before do + allow(Rails.logger).to receive(:error) + end + + it 'logs and saves error if there is an exception' do + error_message = 'error message' + + allow(service).to receive(:repository).and_raise("error message") + allow(service).to receive(:execute_hooks) + + service.execute(merge_request) + + expect(merge_request.merge_error).to include(error_message) + expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + end + + it 'logs and saves error if there is an PreReceiveError exception' do + error_message = 'error message' + + allow(service).to receive(:repository).and_raise(Gitlab::Git::HooksService::PreReceiveError, error_message) + allow(service).to receive(:execute_hooks) + + service.execute(merge_request) + + expect(merge_request.merge_error).to include(error_message) + expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + end + end + end +end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index b4e8b5ea67b..79395f4c564 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -17,6 +17,7 @@ module TestEnv 'feature_conflict' => 'bb5206f', 'fix' => '48f0be4', 'improve/awesome' => '5937ac0', + 'merged-target' => '21751bf', 'markdown' => '0ed8c6c', 'lfs' => 'be93687', 'master' => 'b83d6e3', |