Commit fd79df64 authored by Phil Hughes's avatar Phil Hughes

Merge branch 'blackst0ne-squash-and-merge-in-gitlab-core-ce' into 'master'

Resolve "Squash and merge in GitLab Core (CE)"

Closes #34591

See merge request gitlab-org/gitlab-ce!18956
parents e869387c 4cff66a6
/*
The squash-before-merge button is EE only, but it's located right in the middle
of the readyToMerge state component template.
If we didn't declare this component in CE, we'd need to maintain a separate copy
of the readyToMergeState template in EE, which is pretty big and likely to change.
Instead, in CE, we declare the component, but it's hidden and is configured to do nothing.
In EE, the configuration extends this object to add a functioning squash-before-merge
button.
*/
<script> <script>
export default {}; import Icon from '~/vue_shared/components/icon.vue';
import eventHub from '~/vue_merge_request_widget/event_hub';
import tooltip from '~/vue_shared/directives/tooltip';
export default {
components: {
Icon,
},
directives: {
tooltip,
},
props: {
mr: {
type: Object,
required: true,
},
isMergeButtonDisabled: {
type: Boolean,
required: true,
},
},
data() {
return {
squashBeforeMerge: this.mr.squash,
};
},
methods: {
updateSquashModel() {
eventHub.$emit('MRWidgetUpdateSquash', this.squashBeforeMerge);
},
},
};
</script> </script>
<template>
<div class="accept-control inline">
<label class="merge-param-checkbox">
<input
type="checkbox"
name="squash"
class="qa-squash-checkbox"
:disabled="isMergeButtonDisabled"
v-model="squashBeforeMerge"
@change="updateSquashModel"
/>
{{ __('Squash commits') }}
</label>
<a
:href="mr.squashBeforeMergeHelpPath"
data-title="About this feature"
data-placement="bottom"
target="_blank"
rel="noopener noreferrer nofollow"
data-container="body"
v-tooltip
>
<icon
name="question-o"
/>
</a>
</div>
</template>
...@@ -6,11 +6,13 @@ import MergeRequest from '../../../merge_request'; ...@@ -6,11 +6,13 @@ import MergeRequest from '../../../merge_request';
import Flash from '../../../flash'; import Flash from '../../../flash';
import statusIcon from '../mr_widget_status_icon.vue'; import statusIcon from '../mr_widget_status_icon.vue';
import eventHub from '../../event_hub'; import eventHub from '../../event_hub';
import SquashBeforeMerge from './mr_widget_squash_before_merge.vue';
export default { export default {
name: 'ReadyToMerge', name: 'ReadyToMerge',
components: { components: {
statusIcon, statusIcon,
'squash-before-merge': SquashBeforeMerge,
}, },
props: { props: {
mr: { type: Object, required: true }, mr: { type: Object, required: true },
...@@ -101,6 +103,12 @@ export default { ...@@ -101,6 +103,12 @@ export default {
return enableSquashBeforeMerge && commitsCount > 1; return enableSquashBeforeMerge && commitsCount > 1;
}, },
}, },
created() {
eventHub.$on('MRWidgetUpdateSquash', this.handleUpdateSquash);
},
beforeDestroy() {
eventHub.$off('MRWidgetUpdateSquash', this.handleUpdateSquash);
},
methods: { methods: {
shouldShowMergeControls() { shouldShowMergeControls() {
return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText; return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText;
...@@ -128,13 +136,9 @@ export default { ...@@ -128,13 +136,9 @@ export default {
commit_message: this.commitMessage, commit_message: this.commitMessage,
merge_when_pipeline_succeeds: this.setToMergeWhenPipelineSucceeds, merge_when_pipeline_succeeds: this.setToMergeWhenPipelineSucceeds,
should_remove_source_branch: this.removeSourceBranch === true, should_remove_source_branch: this.removeSourceBranch === true,
squash: this.mr.squash,
}; };
// Only truthy in EE extension of this component
if (this.setAdditionalParams) {
this.setAdditionalParams(options);
}
this.isMakingRequest = true; this.isMakingRequest = true;
this.service.merge(options) this.service.merge(options)
.then(res => res.data) .then(res => res.data)
...@@ -154,6 +158,9 @@ export default { ...@@ -154,6 +158,9 @@ export default {
new Flash('Something went wrong. Please try again.'); // eslint-disable-line new Flash('Something went wrong. Please try again.'); // eslint-disable-line
}); });
}, },
handleUpdateSquash(val) {
this.mr.squash = val;
},
initiateMergePolling() { initiateMergePolling() {
simplePoll((continuePolling, stopPolling) => { simplePoll((continuePolling, stopPolling) => {
this.handleMergePolling(continuePolling, stopPolling); this.handleMergePolling(continuePolling, stopPolling);
......
...@@ -15,6 +15,11 @@ export default class MergeRequestStore { ...@@ -15,6 +15,11 @@ export default class MergeRequestStore {
const currentUser = data.current_user; const currentUser = data.current_user;
const pipelineStatus = data.pipeline ? data.pipeline.details.status : null; const pipelineStatus = data.pipeline ? data.pipeline.details.status : null;
this.squash = data.squash;
this.squashBeforeMergeHelpPath = this.squashBeforeMergeHelpPath ||
data.squash_before_merge_help_path;
this.enableSquashBeforeMerge = this.enableSquashBeforeMerge || true;
this.title = data.title; this.title = data.title;
this.targetBranch = data.target_branch; this.targetBranch = data.target_branch;
this.sourceBranch = data.source_branch; this.sourceBranch = data.source_branch;
......
...@@ -24,6 +24,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont ...@@ -24,6 +24,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
:source_branch, :source_branch,
:source_project_id, :source_project_id,
:state_event, :state_event,
:squash,
:target_branch, :target_branch,
:target_project_id, :target_project_id,
:task_num, :task_num,
......
...@@ -253,7 +253,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -253,7 +253,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end end
def merge_params_attributes def merge_params_attributes
[:should_remove_source_branch, :commit_message] [:should_remove_source_branch, :commit_message, :squash]
end end
def merge_when_pipeline_succeeds_active? def merge_when_pipeline_succeeds_active?
...@@ -282,7 +282,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -282,7 +282,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
return :sha_mismatch if params[:sha] != @merge_request.diff_head_sha return :sha_mismatch if params[:sha] != @merge_request.diff_head_sha
@merge_request.update(merge_error: nil) @merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false))
if params[:merge_when_pipeline_succeeds].present? if params[:merge_when_pipeline_succeeds].present?
return :failed unless @merge_request.actual_head_pipeline return :failed unless @merge_request.actual_head_pipeline
......
...@@ -97,8 +97,9 @@ module MergeRequestsHelper ...@@ -97,8 +97,9 @@ module MergeRequestsHelper
{ {
merge_when_pipeline_succeeds: true, merge_when_pipeline_succeeds: true,
should_remove_source_branch: true, should_remove_source_branch: true,
sha: merge_request.diff_head_sha sha: merge_request.diff_head_sha,
}.merge(merge_params_ee(merge_request)) squash: merge_request.squash
}
end end
def tab_link_for(merge_request, tab, options = {}, &block) def tab_link_for(merge_request, tab, options = {}, &block)
...@@ -149,8 +150,4 @@ module MergeRequestsHelper ...@@ -149,8 +150,4 @@ module MergeRequestsHelper
current_user.fork_of(project) current_user.fork_of(project)
end end
end end
def merge_params_ee(merge_request)
{}
end
end end
...@@ -1140,4 +1140,11 @@ class MergeRequest < ActiveRecord::Base ...@@ -1140,4 +1140,11 @@ class MergeRequest < ActiveRecord::Base
maintainer_push_possible? && maintainer_push_possible? &&
Ability.allowed?(user, :push_code, source_project) Ability.allowed?(user, :push_code, source_project)
end end
def squash_in_progress?
# The source project can be deleted
return false unless source_project
source_project.repository.squash_in_progress?(id)
end
end end
...@@ -957,6 +957,14 @@ class Repository ...@@ -957,6 +957,14 @@ class Repository
remote_branch: merge_request.target_branch) remote_branch: merge_request.target_branch)
end end
def squash(user, merge_request)
raw.squash(user, merge_request.id, branch: merge_request.target_branch,
start_sha: merge_request.diff_start_sha,
end_sha: merge_request.diff_head_sha,
author: merge_request.author,
message: merge_request.title)
end
private private
# TODO Generice finder, later split this on finders by Ref or Oid # TODO Generice finder, later split this on finders by Ref or Oid
......
...@@ -10,6 +10,7 @@ class MergeRequestWidgetEntity < IssuableEntity ...@@ -10,6 +10,7 @@ class MergeRequestWidgetEntity < IssuableEntity
expose :merge_when_pipeline_succeeds expose :merge_when_pipeline_succeeds
expose :source_branch expose :source_branch
expose :source_project_id expose :source_project_id
expose :squash
expose :target_branch expose :target_branch
expose :target_project_id expose :target_project_id
expose :allow_maintainer_to_push expose :allow_maintainer_to_push
......
...@@ -34,6 +34,19 @@ module MergeRequests ...@@ -34,6 +34,19 @@ module MergeRequests
handle_merge_error(log_message: e.message, save_message_on_model: true) handle_merge_error(log_message: e.message, save_message_on_model: true)
end end
def source
return merge_request.diff_head_sha unless merge_request.squash
squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute(merge_request)
case squash_result[:status]
when :success
squash_result[:squash_sha]
when :error
raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
end
end
private private
def error_check! def error_check!
...@@ -116,9 +129,5 @@ module MergeRequests ...@@ -116,9 +129,5 @@ module MergeRequests
def merge_request_info def merge_request_info
merge_request.to_reference(full: true) merge_request.to_reference(full: true)
end end
def source
@source ||= @merge_request.diff_head_sha
end
end end
end end
module MergeRequests
class SquashService < MergeRequests::WorkingCopyBaseService
def execute(merge_request)
@merge_request = merge_request
@repository = target_project.repository
squash || error('Failed to squash. Should be done manually.')
end
def squash
if merge_request.commits_count < 2
return success(squash_sha: merge_request.diff_head_sha)
end
if merge_request.squash_in_progress?
return error('Squash task canceled: another squash is already in progress.')
end
squash_sha = repository.squash(current_user, merge_request)
success(squash_sha: squash_sha)
rescue => e
log_error("Failed to squash merge request #{merge_request.to_reference(full: true)}:")
log_error(e.message)
false
end
end
end
...@@ -20,6 +20,8 @@ ...@@ -20,6 +20,8 @@
window.gl = window.gl || {}; window.gl = window.gl || {};
window.gl.mrWidgetData = #{serialize_issuable(@merge_request, serializer: 'widget')} window.gl.mrWidgetData = #{serialize_issuable(@merge_request, serializer: 'widget')}
window.gl.mrWidgetData.squash_before_merge_help_path = '#{help_page_path("user/project/merge_requests/squash_and_merge")}';
#js-vue-mr-widget.mr-widget #js-vue-mr-widget.mr-widget
.content-block.content-block-small.emoji-list-container.js-noteable-awards .content-block.content-block-small.emoji-list-container.js-noteable-awards
......
...@@ -15,3 +15,12 @@ ...@@ -15,3 +15,12 @@
= hidden_field_tag 'merge_request[force_remove_source_branch]', '0', id: nil = hidden_field_tag 'merge_request[force_remove_source_branch]', '0', id: nil
= check_box_tag 'merge_request[force_remove_source_branch]', '1', issuable.force_remove_source_branch? = check_box_tag 'merge_request[force_remove_source_branch]', '1', issuable.force_remove_source_branch?
Remove source branch when merge request is accepted. Remove source branch when merge request is accepted.
.form-group
.col-sm-10.col-sm-offset-2
.checkbox
= label_tag 'merge_request[squash]' do
= hidden_field_tag 'merge_request[squash]', '0', id: nil
= check_box_tag 'merge_request[squash]', '1', issuable.squash
Squash commits when merge request is accepted.
= link_to 'About this feature', help_page_path('user/project/merge_requests/squash_and_merge')
---
title: Add `Squash and merge` to GitLab Core (CE)
merge_request: 18956
author: "@blackst0ne"
type: added
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddSquashToMergeRequests < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
def up
unless column_exists?(:merge_requests, :squash)
add_column_with_default :merge_requests, :squash, :boolean, default: false, allow_null: false
end
end
def down
remove_column :merge_requests, :squash if column_exists?(:merge_requests, :squash)
end
end
...@@ -1217,6 +1217,7 @@ ActiveRecord::Schema.define(version: 20180529093006) do ...@@ -1217,6 +1217,7 @@ ActiveRecord::Schema.define(version: 20180529093006) do
t.integer "latest_merge_request_diff_id" t.integer "latest_merge_request_diff_id"
t.string "rebase_commit_sha" t.string "rebase_commit_sha"
t.boolean "allow_maintainer_to_push" t.boolean "allow_maintainer_to_push"
t.boolean "squash", default: false, null: false
end end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
......
...@@ -107,6 +107,7 @@ Parameters: ...@@ -107,6 +107,7 @@ Parameters:
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"time_stats": { "time_stats": {
"time_estimate": 0, "time_estimate": 0,
...@@ -226,6 +227,7 @@ Parameters: ...@@ -226,6 +227,7 @@ Parameters:
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"discussion_locked": false, "discussion_locked": false,
"time_stats": { "time_stats": {
...@@ -305,6 +307,7 @@ Parameters: ...@@ -305,6 +307,7 @@ Parameters:
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"discussion_locked": false, "discussion_locked": false,
"time_stats": { "time_stats": {
...@@ -541,7 +544,8 @@ POST /projects/:id/merge_requests ...@@ -541,7 +544,8 @@ POST /projects/:id/merge_requests
| `labels` | string | no | Labels for MR as a comma-separated list | | `labels` | string | no | Labels for MR as a comma-separated list |
| `milestone_id` | integer | no | The global ID of a milestone | | `milestone_id` | integer | no | The global ID of a milestone |
| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | | `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging |
| `allow_maintainer_to_push` | boolean | no | Whether or not a maintainer of the target project can push to the source branch | | `allow_maintainer_to_push` | boolean | no | Whether or not a maintainer of the target project can push to the source branch |
| `squash` | boolean | no | Squash commits into a single commit when merging |
```json ```json
{ {
...@@ -595,6 +599,7 @@ POST /projects/:id/merge_requests ...@@ -595,6 +599,7 @@ POST /projects/:id/merge_requests
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"discussion_locked": false, "discussion_locked": false,
"allow_maintainer_to_push": false, "allow_maintainer_to_push": false,
...@@ -627,6 +632,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid ...@@ -627,6 +632,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid
| `description` | string | no | Description of MR | | `description` | string | no | Description of MR |
| `state_event` | string | no | New state (close/reopen) | | `state_event` | string | no | New state (close/reopen) |
| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | | `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging |
| `squash` | boolean | no | Squash commits into a single commit when merging |
| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. | | `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. |
| `allow_maintainer_to_push` | boolean | no | Whether or not a maintainer of the target project can push to the source branch | | `allow_maintainer_to_push` | boolean | no | Whether or not a maintainer of the target project can push to the source branch |
...@@ -683,6 +689,7 @@ Must include at least one non-required attribute from above. ...@@ -683,6 +689,7 @@ Must include at least one non-required attribute from above.
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"discussion_locked": false, "discussion_locked": false,
"allow_maintainer_to_push": false, "allow_maintainer_to_push": false,
...@@ -790,6 +797,7 @@ Parameters: ...@@ -790,6 +797,7 @@ Parameters:
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"discussion_locked": false, "discussion_locked": false,
"time_stats": { "time_stats": {
...@@ -868,6 +876,7 @@ Parameters: ...@@ -868,6 +876,7 @@ Parameters:
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1", "web_url": "http://example.com/example/example/merge_requests/1",
"discussion_locked": false, "discussion_locked": false,
"time_stats": { "time_stats": {
...@@ -1200,6 +1209,7 @@ Example response: ...@@ -1200,6 +1209,7 @@ Example response:
"changes_count": "1", "changes_count": "1",
"should_remove_source_branch": true, "should_remove_source_branch": true,
"force_remove_source_branch": false, "force_remove_source_branch": false,
"squash": false,
"web_url": "http://example.com/example/example/merge_requests/1" "web_url": "http://example.com/example/example/merge_requests/1"
}, },
"target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/merge_requests/7", "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/merge_requests/7",
......
...@@ -29,12 +29,12 @@ With GitLab merge requests, you can: ...@@ -29,12 +29,12 @@ With GitLab merge requests, you can:
- 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 - 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
- [Create new merge requests by email](#create-new-merge-requests-by-email) - [Create new merge requests by email](#create-new-merge-requests-by-email)
- Allow maintainers of the target project to push directly to the fork by [allowing edits from maintainers](maintainer_access.md) - Allow maintainers of the target project to push directly to the fork by [allowing edits from maintainers](maintainer_access.md)
- [Squash and merge](squash_and_merge.md) for a cleaner commit history
With **[GitLab Enterprise Edition][ee]**, you can also: 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) **[PREMIUM]** - 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) **[PREMIUM]**
- Request [approvals](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html) from your managers **[STARTER]** - Request [approvals](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html) from your managers **[STARTER]**
- [Squash and merge](https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html) for a cleaner commit history **[STARTER]**
- Analyze the impact of your changes with [Code Quality reports](https://docs.gitlab.com/ee/user/project/merge_requests/code_quality_diff.html) **[STARTER]** - Analyze the impact of your changes with [Code Quality reports](https://docs.gitlab.com/ee/user/project/merge_requests/code_quality_diff.html) **[STARTER]**
## Use cases ## Use cases
...@@ -57,7 +57,7 @@ B. Consider you're a web developer writing a webpage for your company's: ...@@ -57,7 +57,7 @@ B. Consider you're a web developer writing a webpage for your company's:
1. Your changes are previewed with [Review Apps](../../../ci/review_apps/index.md) 1. Your changes are previewed with [Review Apps](../../../ci/review_apps/index.md)
1. You request your web designers for their implementation 1. You request your web designers for their implementation
1. You request the [approval](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html) from your manager **[STARTER]** 1. You request the [approval](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html) from your manager **[STARTER]**
1. Once approved, your merge request is [squashed and merged](https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html), and [deployed to staging with GitLab Pages](https://about.gitlab.com/2016/08/26/ci-deployment-and-environments/) (Squash and Merge is available in GitLab Starter) 1. Once approved, your merge request is [squashed and merged](squash_and_merge.md), and [deployed to staging with GitLab Pages](https://about.gitlab.com/2016/08/26/ci-deployment-and-environments/)
1. Your production team [cherry picks](#cherry-pick-changes) the merge commit into production 1. Your production team [cherry picks](#cherry-pick-changes) the merge commit into production
## Merge requests per project ## Merge requests per project
......
# Squash and merge
> [Introduced][ee-1024] in [GitLab Starter][ee] 8.17, and in [GitLab CE][ce] [11.0][ce-18956].
Combine all commits of your merge request into one and retain a clean history.
## Overview
Squashing lets you tidy up the commit history of a branch when accepting a merge
request. It applies all of the changes in the merge request as a single commit,
and then merges that commit using the merge method set for the project.
In other words, squashing a merge request turns a long list of commits:
![List of commits from a merge request][mr-commits]
Into a single commit on merge:
![A squashed commit followed by a merge commit][squashed-commit]
The squashed commit's commit message is the merge request title. And note that
the squashed commit is still followed by a merge commit, as the merge
method for this example repository uses a merge commit. Squashing also works
with the fast-forward merge strategy, see
[squashing and fast-forward merge](#squash-and-fast-forward-merge) for more
details.
## Use cases
When working on a feature branch, you sometimes want to commit your current
progress, but don't really care about the commit messages. Those 'work in
progress commits' don't necessarily contain important information and as such
you'd rather not include them in your target branch.
With squash and merge, when the merge request is ready to be merged,
all you have to do is enable squashing before you press merge to join
the commits include in the merge request into a single commit.
This way, the history of your base branch remains clean with
meaningful commit messages and is simpler to [revert] if necessary.
## Enabling squash for a merge request
Anyone who can create or edit a merge request can choose for it to be squashed
on the merge request form:
![Squash commits checkbox on edit form][squash-edit-form]
---
This can then be overridden at the time of accepting the merge request:
![Squash commits checkbox on accept merge request form][squash-mr-widget]
## Commit metadata for squashed commits
The squashed commit has the following metadata:
* Message: the title of the merge request.
* Author: the author of the merge request.
* Committer: the user who initiated the squash.
## Squash and fast-forward merge
When a project has the [fast-forward merge setting enabled][ff-merge], the merge
request must be able to be fast-forwarded without squashing in order to squash
it. This is because squashing is only available when accepting a merge request,
so a merge request may need to be rebased before squashing, even though
squashing can itself be considered equivalent to rebasing.
[ee-1024]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1024
[ce-18956]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18956
[mr-commits]: img/squash_mr_commits.png
[squashed-commit]: img/squash_squashed_commit.png
[squash-edit-form]: img/squash_edit_form.png
[squash-mr-widget]: img/squash_mr_widget.png
[ff-merge]: fast_forward_merge.md#enabling-fast-forward-merges
[ce]: https://about.gitlab.com/products/
[ee]: https://about.gitlab.com/products/
[revert]: revert_changes.md
...@@ -568,6 +568,8 @@ module API ...@@ -568,6 +568,8 @@ module API
expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |merge_request| expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |merge_request|
merge_request merge_request
end end
expose :squash
end end
class MergeRequest < MergeRequestBasic class MergeRequest < MergeRequestBasic
......
...@@ -10,12 +10,6 @@ module API ...@@ -10,12 +10,6 @@ module API
helpers do helpers do
params :optional_params_ee do params :optional_params_ee do
end end
params :merge_params_ee do
end
def update_merge_request_ee(merge_request)
end
end end
def self.update_params_at_least_one_of def self.update_params_at_least_one_of
...@@ -29,6 +23,7 @@ module API ...@@ -29,6 +23,7 @@ module API
target_branch target_branch
title title
discussion_locked discussion_locked
squash
] ]
end end
...@@ -146,6 +141,7 @@ module API ...@@ -146,6 +141,7 @@ module API
optional :labels, type: String, desc: 'Comma-separated list of label names' optional :labels, type: String, desc: 'Comma-separated list of label names'
optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging' optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging'
optional :allow_maintainer_to_push, type: Boolean, desc: 'Whether a maintainer of the target project can push to the source project' optional :allow_maintainer_to_push, type: Boolean, desc: 'Whether a maintainer of the target project can push to the source project'
optional :squash, type: Grape::API::Boolean, desc: 'When true, the commits will be squashed into a single commit on merge'
use :optional_params_ee use :optional_params_ee
end end
...@@ -308,8 +304,7 @@ module API ...@@ -308,8 +304,7 @@ module API
optional :merge_when_pipeline_succeeds, type: Boolean, optional :merge_when_pipeline_succeeds, type: Boolean,
desc: 'When true, this merge request will be merged when the pipeline succeeds' desc: 'When true, this merge request will be merged when the pipeline succeeds'
optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch' optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
optional :squash, type: Grape::API::Boolean, desc: 'When true, the commits will be squashed into a single commit on merge'
use :merge_params_ee
end end
put ':id/merge_requests/:merge_request_iid/merge' do put ':id/merge_requests/:merge_request_iid/merge' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42317') Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42317')
...@@ -327,7 +322,7 @@ module API ...@@ -327,7 +322,7 @@ module API
check_sha_param!(params, merge_request) check_sha_param!(params, merge_request)
update_merge_request_ee(merge_request) merge_request.update(squash: params[:squash]) if params[:squash]
merge_params = { merge_params = {
commit_message: params[:merge_commit_message], commit_message: params[:merge_commit_message],
......
...@@ -134,6 +134,8 @@ module API ...@@ -134,6 +134,8 @@ module API
expose :should_remove_source_branch?, as: :should_remove_source_branch expose :should_remove_source_branch?, as: :should_remove_source_branch
expose :force_remove_source_branch?, as: :force_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch
expose :squash
expose :web_url do |merge_request, options| expose :web_url do |merge_request, options|
Gitlab::UrlBuilder.build(merge_request) Gitlab::UrlBuilder.build(merge_request)
end end
......
...@@ -44,6 +44,7 @@ module API ...@@ -44,6 +44,7 @@ module API
optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request' optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request'
optional :labels, type: String, desc: 'Comma-separated list of label names' optional :labels, type: String, desc: 'Comma-separated list of label names'
optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging' optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging'
optional :squash, type: Boolean, desc: 'Squash commits when merging'
end end
end end
...@@ -166,7 +167,7 @@ module API ...@@ -166,7 +167,7 @@ module API
use :optional_params use :optional_params
at_least_one_of :title, :target_branch, :description, :assignee_id, at_least_one_of :title, :target_branch, :description, :assignee_id,
:milestone_id, :labels, :state_event, :milestone_id, :labels, :state_event,
:remove_source_branch :remove_source_branch, :squash
end end
put path do put path do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42127') Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42127')
...@@ -195,6 +196,7 @@ module API ...@@ -195,6 +196,7 @@ module API
optional :merge_when_build_succeeds, type: Boolean, optional :merge_when_build_succeeds, type: Boolean,
desc: 'When true, this merge request will be merged when the build succeeds' desc: 'When true, this merge request will be merged when the build succeeds'
optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch' optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
optional :squash, type: Boolean, desc: 'When true, the commits will be squashed into a single commit on merge'
end end
put "#{path}/merge" do put "#{path}/merge" do
merge_request = find_project_merge_request(params[:merge_request_id]) merge_request = find_project_merge_request(params[:merge_request_id])
...@@ -211,6 +213,8 @@ module API ...@@ -211,6 +213,8 @@ module API
render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409) render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
end end
merge_request.update(squash: params[:squash]) if params[:squash]
merge_params = { merge_params = {
commit_message: params[:merge_commit_message], commit_message: params[:merge_commit_message],
should_remove_source_branch: params[:should_remove_source_branch] should_remove_source_branch: params[:should_remove_source_branch]
......
...@@ -16,6 +16,10 @@ module QA ...@@ -16,6 +16,10 @@ module QA
element :no_fast_forward_message, 'Fast-forward merge is not possible' element :no_fast_forward_message, 'Fast-forward merge is not possible'
end end
view 'app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_squash_before_merge.vue' do
element :squash_checkbox
end
def rebase! def rebase!
click_element :mr_rebase_button click_element :mr_rebase_button
...@@ -41,6 +45,14 @@ module QA ...@@ -41,6 +45,14 @@ module QA
has_text?('The changes were merged into') has_text?('The changes were merged into')
end end
end end
def mark_to_squash
wait(reload: true) do
has_css?(element_selector_css(:squash_checkbox))
end
click_element :squash_checkbox
end
end end
end end
end end
......
module QA
feature 'merge request squash commits', :core do
scenario 'when squash commits is marked before merge' do
Runtime::Browser.visit(:gitlab, Page::Main::Login)
Page::Main::Login.act { sign_in_using_credentials }
project = Factory::Resource::Project.fabricate! do |project|
project.name = "squash-before-merge"
end
merge_request = Factory::Resource::MergeRequest.fabricate! do |merge_request|
merge_request.project = project
merge_request.title = 'Squashing commits'
end
Factory::Repository::Push.fabricate! do |push|
push.project = project
push.commit_message = 'to be squashed'
push.branch_name = merge_request.source_branch
push.new_branch = false
push.file_name = 'other.txt'
push.file_content = "Test with unicode characters ❤✓€❄"
end
merge_request.visit!
Page::MergeRequest::Show.perform do |merge_request_page|
merge_request_page.mark_to_squash
merge_request_page.merge!
merge_request.project.visit!
Git::Repository.perform do |repository|
repository.uri = Page::Project::Show.act do
choose_repository_clone_http
repository_location.uri
end
repository.use_default_credentials
repository.act { clone }
expect(repository.commits.size).to eq 3
end
end
end
end
end
...@@ -275,6 +275,7 @@ describe Projects::MergeRequestsController do ...@@ -275,6 +275,7 @@ describe Projects::MergeRequestsController do
namespace_id: project.namespace, namespace_id: project.namespace,
project_id: project, project_id: project,
id: merge_request.iid, id: merge_request.iid,
squash: false,
format: 'json' format: 'json'
} }
end end
...@@ -315,8 +316,8 @@ describe Projects::MergeRequestsController do ...@@ -315,8 +316,8 @@ describe Projects::MergeRequestsController do
end end
context 'when the sha parameter matches the source SHA' do context 'when the sha parameter matches the source SHA' do
def merge_with_sha def merge_with_sha(params = {})
post :merge, base_params.merge(sha: merge_request.diff_head_sha) post :merge, base_params.merge(sha: merge_request.diff_head_sha).merge(params)
end end
it 'returns :success' do it 'returns :success' do
...@@ -331,6 +332,24 @@ describe Projects::MergeRequestsController do ...@@ -331,6 +332,24 @@ describe Projects::MergeRequestsController do
merge_with_sha merge_with_sha
end end
context 'when squash is passed as 1' do
it 'updates the squash attribute on the MR to true' do
merge_request.update(squash: false)
merge_with_sha(squash: '1')
expect(merge_request.reload.squash).to be_truthy
end
end
context 'when squash is passed as 0' do
it 'updates the squash attribute on the MR to false' do
merge_request.update(squash: true)
merge_with_sha(squash: '0')
expect(merge_request.reload.squash).to be_falsey
end
end
context 'when the pipeline succeeds is passed' do context 'when the pipeline succeeds is passed' do
let!(:head_pipeline) do let!(:head_pipeline) do
create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request) create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
......
require 'spec_helper'
feature 'User squashes a merge request', :js do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:source_branch) { 'csv' }
let!(:original_head) { project.repository.commit('master') }
shared_examples 'squash' do
it 'squashes the commits into a single commit, and adds a merge commit' do
expect(page).to have_content('Merged')
latest_master_commits = project.repository.commits_between(original_head.sha, 'master').map(&:raw)
squash_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/),
message: "Csv\n",
author_name: user.name,
committer_name: user.name)
merge_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/),
message: a_string_starting_with("Merge branch 'csv' into 'master'"),
author_name: user.name,
committer_name: user.name)
expect(project.repository).not_to be_merged_to_root_ref(source_branch)
expect(latest_master_commits).to match([squash_commit, merge_commit])
end
end
shared_examples 'no squash' do
it 'accepts the merge request without squashing' do
expect(page).to have_content('Merged')
expect(project.repository).to be_merged_to_root_ref(source_branch)
end
end
def accept_mr
expect(page).to have_button('Merge')
uncheck 'Remove source branch'
click_on 'Merge'
end
before do
# Prevent source branch from being removed so we can use be_merged_to_root_ref
# method to check if squash was performed or not
allow_any_instance_of(MergeRequest).to receive(:force_remove_source_branch?).and_return(false)
project.add_master(user)
sign_in user
end
context 'when the MR has only one commit' do
before do
merge_request = create(:merge_request, source_project: project, target_project: project, source_branch: 'master', target_branch: 'branch-merged')
visit project_merge_request_path(project, merge_request)
end
it 'does not show the squash checkbox' do
expect(page).not_to have_field('squash')
end
end
context 'when squash is enabled on merge request creation' do
before do
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: source_branch })
check 'merge_request[squash]'
click_on 'Submit merge request'
wait_for_requests
end
it 'shows the squash checkbox as checked' do
expect(page).to have_checked_field('squash')
end
context 'when accepting with squash checked' do
before do
accept_mr
end
include_examples 'squash'
end
context 'when accepting and unchecking squash' do
before do
uncheck 'squash'
accept_mr
end
include_examples 'no squash'
end
end
context 'when squash is not enabled on merge request creation' do
before do
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: source_branch })
click_on 'Submit merge request'
wait_for_requests
end
it 'shows the squash checkbox as unchecked' do
expect(page).to have_unchecked_field('squash')
end
context 'when accepting and checking squash' do
before do
check 'squash'
accept_mr
end
include_examples 'squash'
end
context 'when accepting with squash unchecked' do
before do
accept_mr
end
include_examples 'no squash'
end
end
end
...@@ -112,7 +112,8 @@ ...@@ -112,7 +112,8 @@
"rebase_commit_sha": { "type": ["string", "null"] }, "rebase_commit_sha": { "type": ["string", "null"] },
"rebase_in_progress": { "type": "boolean" }, "rebase_in_progress": { "type": "boolean" },
"can_push_to_source_branch": { "type": "boolean" }, "can_push_to_source_branch": { "type": "boolean" },
"rebase_path": { "type": ["string", "null"] } "rebase_path": { "type": ["string", "null"] },
"squash": { "type": "boolean" }
}, },
"additionalProperties": false "additionalProperties": false
} }
...@@ -73,7 +73,8 @@ ...@@ -73,7 +73,8 @@
"should_remove_source_branch": { "type": ["boolean", "null"] }, "should_remove_source_branch": { "type": ["boolean", "null"] },
"force_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] },
"web_url": { "type": "uri" }, "web_url": { "type": "uri" },
"subscribed": { "type": ["boolean"] } "subscribed": { "type": ["boolean"] },
"squash": { "type": "boolean" }
}, },
"required": [ "required": [
"id", "iid", "project_id", "title", "description", "id", "iid", "project_id", "title", "description",
...@@ -83,7 +84,7 @@ ...@@ -83,7 +84,7 @@
"labels", "work_in_progress", "milestone", "merge_when_build_succeeds", "labels", "work_in_progress", "milestone", "merge_when_build_succeeds",
"merge_status", "sha", "merge_commit_sha", "user_notes_count", "merge_status", "sha", "merge_commit_sha", "user_notes_count",
"should_remove_source_branch", "force_remove_source_branch", "should_remove_source_branch", "force_remove_source_branch",
"web_url", "subscribed" "web_url", "subscribed", "squash"
], ],
"additionalProperties": false "additionalProperties": false
} }
......
...@@ -75,6 +75,7 @@ ...@@ -75,6 +75,7 @@
"force_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] },
"discussion_locked": { "type": ["boolean", "null"] }, "discussion_locked": { "type": ["boolean", "null"] },
"web_url": { "type": "uri" }, "web_url": { "type": "uri" },
"squash": { "type": "boolean" },
"time_stats": { "time_stats": {
"time_estimate": { "type": "integer" }, "time_estimate": { "type": "integer" },
"total_time_spent": { "type": "integer" }, "total_time_spent": { "type": "integer" },
...@@ -91,7 +92,7 @@ ...@@ -91,7 +92,7 @@
"labels", "work_in_progress", "milestone", "merge_when_pipeline_succeeds", "labels", "work_in_progress", "milestone", "merge_when_pipeline_succeeds",
"merge_status", "sha", "merge_commit_sha", "user_notes_count", "merge_status", "sha", "merge_commit_sha", "user_notes_count",
"should_remove_source_branch", "force_remove_source_branch", "should_remove_source_branch", "force_remove_source_branch",
"web_url" "web_url", "squash"
], ],
"additionalProperties": false "additionalProperties": false
} }
......
...@@ -165,6 +165,7 @@ MergeRequest: ...@@ -165,6 +165,7 @@ MergeRequest:
- approvals_before_merge - approvals_before_merge
- rebase_commit_sha - rebase_commit_sha
- time_estimate - time_estimate
- squash
- last_edited_at - last_edited_at
- last_edited_by_id - last_edited_by_id
- head_pipeline_id - head_pipeline_id
......
...@@ -14,6 +14,65 @@ describe MergeRequest do ...@@ -14,6 +14,65 @@ describe MergeRequest do
it { is_expected.to have_many(:merge_request_diffs) } it { is_expected.to have_many(:merge_request_diffs) }
end end
describe '#squash_in_progress?' do
shared_examples 'checking whether a squash is in progress' do
let(:repo_path) { subject.source_project.repository.path }
let(:squash_path) { File.join(repo_path, "gitlab-worktree", "squash-#{subject.id}") }
before do
system(*%W(#{Gitlab.config.git.bin_path} -C #{repo_path} worktree add --detach #{squash_path} master))
end
it 'returns true when there is a current squash directory' do
expect(subject.squash_in_progress?).to be_truthy
end
it 'returns false when there is no squash directory' do
FileUtils.rm_rf(squash_path)
expect(subject.squash_in_progress?).to be_falsey
end
it 'returns false when the squash directory has expired' do
time = 20.minutes.ago.to_time
File.utime(time, time, squash_path)
expect(subject.squash_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)
expect(subject.squash_in_progress?).to be_falsey
end
end
context 'when Gitaly squash_in_progress is enabled' do
it_behaves_like 'checking whether a squash is in progress'
end
context 'when Gitaly squash_in_progress is disabled', :disable_gitaly do
it_behaves_like 'checking whether a squash is in progress'
end
end
describe '#squash?' do
let(:merge_request) { build(:merge_request, squash: squash) }
subject { merge_request.squash? }
context 'disabled in database' do
let(:squash) { false }
it { is_expected.to be_falsy }
end
context 'enabled in database' do
let(:squash) { true }
it { is_expected.to be_truthy }
end
end
describe 'modules' do describe 'modules' do
subject { described_class } subject { described_class }
......
...@@ -263,6 +263,7 @@ describe API::MergeRequests do ...@@ -263,6 +263,7 @@ describe API::MergeRequests do
expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha) expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha)
expect(json_response.first['merge_commit_sha']).not_to be_nil expect(json_response.first['merge_commit_sha']).not_to be_nil
expect(json_response.first['merge_commit_sha']).to eq(merge_request_merged.merge_commit_sha) expect(json_response.first['merge_commit_sha']).to eq(merge_request_merged.merge_commit_sha)
expect(json_response.first['squash']).to eq(merge_request_merged.squash)
end end
it "returns an array of all merge_requests using simple mode" do it "returns an array of all merge_requests using simple mode" do
...@@ -671,12 +672,14 @@ describe API::MergeRequests do ...@@ -671,12 +672,14 @@ describe API::MergeRequests do
target_branch: 'master', target_branch: 'master',
author: user, author: user,
labels: 'label, label2', labels: 'label, label2',
milestone_id: milestone.id milestone_id: milestone.id,
squash: true
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
expect(json_response['title']).to eq('Test merge_request') expect(json_response['title']).to eq('Test merge_request')
expect(json_response['labels']).to eq(%w(label label2)) expect(json_response['labels']).to eq(%w(label label2))
expect(json_response['milestone']['id']).to eq(milestone.id) expect(json_response['milestone']['id']).to eq(milestone.id)
expect(json_response['squash']).to be_truthy
expect(json_response['force_remove_source_branch']).to be_falsy expect(json_response['force_remove_source_branch']).to be_falsy
end end
...@@ -965,6 +968,14 @@ describe API::MergeRequests do ...@@ -965,6 +968,14 @@ describe API::MergeRequests do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
end end
it "updates the MR's squash attribute" do
expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), squash: true
end.to change { merge_request.reload.squash }
expect(response).to have_gitlab_http_status(200)
end
it "enables merge when pipeline succeeds if the pipeline is active" do it "enables merge when pipeline succeeds if the pipeline is active" do
allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline) allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline)
allow(pipeline).to receive(:active?).and_return(true) allow(pipeline).to receive(:active?).and_return(true)
...@@ -1029,6 +1040,13 @@ describe API::MergeRequests do ...@@ -1029,6 +1040,13 @@ describe API::MergeRequests do
expect(json_response['milestone']['id']).to eq(milestone.id) expect(json_response['milestone']['id']).to eq(milestone.id)
end end
it "updates squash and returns merge_request" do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), squash: true
expect(response).to have_gitlab_http_status(200)
expect(json_response['squash']).to be_truthy
end
it "returns merge_request with renamed target_branch" do it "returns merge_request with renamed target_branch" do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), target_branch: "wiki" put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), target_branch: "wiki"
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
......
...@@ -40,6 +40,7 @@ describe API::MergeRequests do ...@@ -40,6 +40,7 @@ describe API::MergeRequests do
expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha) expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha)
expect(json_response.first['merge_commit_sha']).not_to be_nil expect(json_response.first['merge_commit_sha']).not_to be_nil
expect(json_response.first['merge_commit_sha']).to eq(merge_request_merged.merge_commit_sha) expect(json_response.first['merge_commit_sha']).to eq(merge_request_merged.merge_commit_sha)
expect(json_response.first['squash']).to eq(merge_request_merged.squash)
end end
it "returns an array of all merge_requests" do it "returns an array of all merge_requests" do
...@@ -241,13 +242,15 @@ describe API::MergeRequests do ...@@ -241,13 +242,15 @@ describe API::MergeRequests do
author: user, author: user,
labels: 'label, label2', labels: 'label, label2',
milestone_id: milestone.id, milestone_id: milestone.id,
remove_source_branch: true remove_source_branch: true,
squash: true
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
expect(json_response['title']).to eq('Test merge_request') expect(json_response['title']).to eq('Test merge_request')
expect(json_response['labels']).to eq(%w(label label2)) expect(json_response['labels']).to eq(%w(label label2))
expect(json_response['milestone']['id']).to eq(milestone.id) expect(json_response['milestone']['id']).to eq(milestone.id)
expect(json_response['force_remove_source_branch']).to be_truthy expect(json_response['force_remove_source_branch']).to be_truthy
expect(json_response['squash']).to be_truthy
end end
it "returns 422 when source_branch equals target_branch" do it "returns 422 when source_branch equals target_branch" do
...@@ -489,6 +492,14 @@ describe API::MergeRequests do ...@@ -489,6 +492,14 @@ describe API::MergeRequests do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
end end
it "updates the MR's squash attribute" do
expect do
put v3_api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), squash: true
end.to change { merge_request.reload.squash }
expect(response).to have_gitlab_http_status(200)
end
it "enables merge when pipeline succeeds if the pipeline is active" do it "enables merge when pipeline succeeds if the pipeline is active" do
allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline) allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline)
allow(pipeline).to receive(:active?).and_return(true) allow(pipeline).to receive(:active?).and_return(true)
...@@ -529,6 +540,13 @@ describe API::MergeRequests do ...@@ -529,6 +540,13 @@ describe API::MergeRequests do
expect(json_response['milestone']['id']).to eq(milestone.id) expect(json_response['milestone']['id']).to eq(milestone.id)
end end
it "updates squash and returns merge_request" do
put v3_api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), squash: true
expect(response).to have_gitlab_http_status(200)
expect(json_response['squash']).to be_truthy
end
it "returns merge_request with renamed target_branch" do it "returns merge_request with renamed target_branch" do
put v3_api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), target_branch: "wiki" put v3_api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), target_branch: "wiki"
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
......
...@@ -249,24 +249,58 @@ describe MergeRequests::MergeService do ...@@ -249,24 +249,58 @@ describe MergeRequests::MergeService do
expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
end end
context "when fast-forward merge is not allowed" do context 'when squashing' do
before do before do
allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) merge_request.update!(source_branch: 'master', target_branch: 'feature')
end end
%w(semi-linear ff).each do |merge_method| it 'logs and saves error if there is an error when squashing' do
it "logs and saves error if merge is #{merge_method} only" do error_message = 'Failed to squash. Should be done manually'
merge_method = 'rebase_merge' if merge_method == 'semi-linear'
merge_request.project.update(merge_method: merge_method)
error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch'
allow(service).to receive(:execute_hooks)
service.execute(merge_request) allow_any_instance_of(MergeRequests::SquashService).to receive(:squash).and_return(nil)
merge_request.update(squash: true)
service.execute(merge_request)
expect(merge_request).to be_open
expect(merge_request.merge_commit_sha).to be_nil
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 a squash in progress' do
error_message = 'another squash is already in progress'
allow_any_instance_of(MergeRequest).to receive(:squash_in_progress?).and_return(true)
merge_request.update(squash: true)
service.execute(merge_request)
expect(merge_request).to be_open expect(merge_request).to be_open
expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_commit_sha).to be_nil
expect(merge_request.merge_error).to include(error_message) expect(merge_request.merge_error).to include(error_message)
expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
end
context "when fast-forward merge is not allowed" do
before do
allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil)
end
%w(semi-linear ff).each do |merge_method|
it "logs and saves error if merge is #{merge_method} only" do
merge_method = 'rebase_merge' if merge_method == 'semi-linear'
merge_request.project.update(merge_method: merge_method)
error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch'
allow(service).to receive(:execute_hooks)
service.execute(merge_request)
expect(merge_request).to be_open
expect(merge_request.merge_commit_sha).to be_nil
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 end
end end
......
require 'spec_helper'
describe MergeRequests::SquashService do
let(:service) { described_class.new(project, user, {}) }
let(:user) { project.owner }
let(:project) { create(:project, :repository) }
let(:repository) { project.repository.raw }
let(:log_error) { "Failed to squash merge request #{merge_request.to_reference(full: true)}:" }
let(:squash_dir_path) do
File.join(Gitlab.config.shared.path, 'tmp/squash', repository.gl_repository, merge_request.id.to_s)
end
let(:merge_request_with_one_commit) do
create(:merge_request,
source_branch: 'feature', source_project: project,
target_branch: 'master', target_project: project)
end
let(:merge_request_with_only_new_files) do
create(:merge_request,
source_branch: 'video', source_project: project,
target_branch: 'master', target_project: project)
end
let(:merge_request_with_large_files) do
create(:merge_request,
source_branch: 'squash-large-files', source_project: project,
target_branch: 'master', target_project: project)
end
shared_examples 'the squash succeeds' do
it 'returns the squashed commit SHA' do
result = service.execute(merge_request)
expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/))
expect(result[:squash_sha]).not_to eq(merge_request.diff_head_sha)
end
it 'cleans up the temporary directory' do
service.execute(merge_request)
expect(File.exist?(squash_dir_path)).to be(false)
end
it 'does not keep the branch push event' do
expect { service.execute(merge_request) }.not_to change { Event.count }
end
context 'the squashed commit' do
let(:squash_sha) { service.execute(merge_request)[:squash_sha] }
let(:squash_commit) { project.repository.commit(squash_sha) }
it 'copies the author info and message from the merge request' do
expect(squash_commit.author_name).to eq(merge_request.author.name)
expect(squash_commit.author_email).to eq(merge_request.author.email)
# Commit messages have a trailing newline, but titles don't.
expect(squash_commit.message.chomp).to eq(merge_request.title)
end
it 'sets the current user as the committer' do
expect(squash_commit.committer_name).to eq(user.name.chomp('.'))
expect(squash_commit.committer_email).to eq(user.email)
end
it 'has the same diff as the merge request, but a different SHA' do
rugged = project.repository.rugged
mr_diff = rugged.diff(merge_request.diff_base_sha, merge_request.diff_head_sha)
squash_diff = rugged.diff(merge_request.diff_start_sha, squash_sha)
expect(squash_diff.patch.length).to eq(mr_diff.patch.length)
expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha)
end
end
end
describe '#execute' do
context 'when there is only one commit in the merge request' do
it 'returns that commit SHA' do
result = service.execute(merge_request_with_one_commit)
expect(result).to match(status: :success, squash_sha: merge_request_with_one_commit.diff_head_sha)
end
it 'does not perform any git actions' do
expect(repository).not_to receive(:popen)
service.execute(merge_request_with_one_commit)
end
end
context 'when squashing only new files' do
let(:merge_request) { merge_request_with_only_new_files }
include_examples 'the squash succeeds'
end
context 'when squashing with files too large to display' do
let(:merge_request) { merge_request_with_large_files }
include_examples 'the squash succeeds'
end
context 'git errors' do
let(:merge_request) { merge_request_with_only_new_files }
let(:error) { 'A test error' }
context 'with gitaly enabled' do
before do
allow(repository.gitaly_operation_client).to receive(:user_squash)
.and_raise(Gitlab::Git::Repository::GitError, error)
end
it 'logs the stage and output' do
expect(service).to receive(:log_error).with(log_error)
expect(service).to receive(:log_error).with(error)
service.execute(merge_request)
end
it 'returns an error' do
expect(service.execute(merge_request)).to match(status: :error,
message: a_string_including('squash'))
end
end
context 'with Gitaly disabled', :skip_gitaly_mock do
stages = {
'add worktree for squash' => 'worktree',
'configure sparse checkout' => 'config',
'get files in diff' => 'diff --name-only',
'check out target branch' => 'checkout',
'apply patch' => 'diff --binary',
'commit squashed changes' => 'commit',
'get SHA of squashed commit' => 'rev-parse'
}
stages.each do |stage, command|
context "when the #{stage} stage fails" do
before do
git_command = a_collection_containing_exactly(
a_string_starting_with("#{Gitlab.config.git.bin_path} #{command}")
).or(
a_collection_starting_with([Gitlab.config.git.bin_path] + command.split)
)
allow(repository).to receive(:popen).and_return(['', 0])
allow(repository).to receive(:popen).with(git_command, anything, anything, anything).and_return([error, 1])
end
it 'logs the stage and output' do
expect(service).to receive(:log_error).with(log_error)
expect(service).to receive(:log_error).with(error)
service.execute(merge_request)
end
it 'returns an error' do
expect(service.execute(merge_request)).to match(status: :error,
message: a_string_including('squash'))
end
it 'cleans up the temporary directory' do
expect(File.exist?(squash_dir_path)).to be(false)
service.execute(merge_request)
end
end
end
end
end
context 'when any other exception is thrown' do
let(:merge_request) { merge_request_with_only_new_files }
let(:error) { 'A test error' }
before do
allow(merge_request).to receive(:commits_count).and_raise(error)
end
it 'logs the MR reference and exception' do
expect(service).to receive(:log_error).with(a_string_including("#{project.full_path}#{merge_request.to_reference}"))
expect(service).to receive(:log_error).with(error)
service.execute(merge_request)
end
it 'returns an error' do
expect(service.execute(merge_request)).to match(status: :error,
message: a_string_including('squash'))
end
it 'cleans up the temporary directory' do
service.execute(merge_request)
expect(File.exist?(squash_dir_path)).to be(false)
end
end
end
end
...@@ -47,6 +47,7 @@ module TestEnv ...@@ -47,6 +47,7 @@ module TestEnv
'v1.1.0' => 'b83d6e3', 'v1.1.0' => 'b83d6e3',
'add-ipython-files' => '93ee732', 'add-ipython-files' => '93ee732',
'add-pdf-file' => 'e774ebd', 'add-pdf-file' => 'e774ebd',
'squash-large-files' => '54cec52',
'add-pdf-text-binary' => '79faa7b', 'add-pdf-text-binary' => '79faa7b',
'add_images_and_changes' => '010d106' 'add_images_and_changes' => '010d106'
}.freeze }.freeze
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment