From 4cff66a6c46361e8d775ea3f5a80bf147d4020b3 Mon Sep 17 00:00:00 2001 From: blackst0ne Date: Tue, 29 May 2018 20:51:43 +1100 Subject: Add 'squash and rebase' feature to CE --- .../states/mr_widget_squash_before_merge.vue | 74 ++++++-- .../components/states/ready_to_merge.vue | 17 +- .../stores/mr_widget_store.js | 5 + .../merge_requests/application_controller.rb | 1 + .../projects/merge_requests_controller.rb | 4 +- app/helpers/merge_requests_helper.rb | 9 +- app/models/merge_request.rb | 7 + app/models/repository.rb | 8 + app/serializers/merge_request_widget_entity.rb | 1 + app/services/merge_requests/merge_service.rb | 17 +- app/services/merge_requests/squash_service.rb | 28 +++ app/views/projects/merge_requests/show.html.haml | 2 + .../shared/issuable/form/_merge_params.html.haml | 9 + ...ackst0ne-squash-and-merge-in-gitlab-core-ce.yml | 5 + .../20180515005612_add_squash_to_merge_requests.rb | 19 ++ db/schema.rb | 1 + doc/api/merge_requests.md | 12 +- .../merge_requests/img/squash_edit_form.png | Bin 0 -> 4232 bytes .../merge_requests/img/squash_mr_commits.png | Bin 0 -> 85635 bytes .../merge_requests/img/squash_mr_widget.png | Bin 0 -> 6496 bytes .../merge_requests/img/squash_squashed_commit.png | Bin 0 -> 63371 bytes doc/user/project/merge_requests/index.md | 4 +- .../project/merge_requests/squash_and_merge.md | 80 +++++++++ lib/api/entities.rb | 2 + lib/api/merge_requests.rb | 13 +- lib/api/v3/entities.rb | 2 + lib/api/v3/merge_requests.rb | 6 +- qa/qa/page/merge_request/show.rb | 12 ++ qa/qa/specs/features/merge_request/squash_spec.rb | 48 +++++ .../projects/merge_requests_controller_spec.rb | 23 ++- .../user_squashes_merge_request_spec.rb | 124 +++++++++++++ .../api/schemas/entities/merge_request_widget.json | 3 +- .../api/schemas/public_api/v3/merge_requests.json | 5 +- .../api/schemas/public_api/v4/merge_requests.json | 3 +- .../gitlab/import_export/safe_model_attributes.yml | 1 + spec/models/merge_request_spec.rb | 59 ++++++ spec/requests/api/merge_requests_spec.rb | 20 ++- spec/requests/api/v3/merge_requests_spec.rb | 20 ++- spec/services/merge_requests/merge_service_spec.rb | 60 +++++-- .../services/merge_requests/squash_service_spec.rb | 199 +++++++++++++++++++++ spec/support/helpers/test_env.rb | 1 + 41 files changed, 840 insertions(+), 64 deletions(-) create mode 100644 app/services/merge_requests/squash_service.rb create mode 100644 changelogs/unreleased/blackst0ne-squash-and-merge-in-gitlab-core-ce.yml create mode 100644 db/migrate/20180515005612_add_squash_to_merge_requests.rb create mode 100644 doc/user/project/merge_requests/img/squash_edit_form.png create mode 100644 doc/user/project/merge_requests/img/squash_mr_commits.png create mode 100644 doc/user/project/merge_requests/img/squash_mr_widget.png create mode 100644 doc/user/project/merge_requests/img/squash_squashed_commit.png create mode 100644 doc/user/project/merge_requests/squash_and_merge.md create mode 100644 qa/qa/specs/features/merge_request/squash_spec.rb create mode 100644 spec/features/merge_requests/user_squashes_merge_request_spec.rb create mode 100644 spec/services/merge_requests/squash_service_spec.rb diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_squash_before_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_squash_before_merge.vue index 926a3172412..875c3323dbb 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_squash_before_merge.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_squash_before_merge.vue @@ -1,15 +1,63 @@ -/* -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. -*/ - + + diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue index 1d1c8ebc179..3a194320bd8 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue @@ -6,11 +6,13 @@ import MergeRequest from '../../../merge_request'; import Flash from '../../../flash'; import statusIcon from '../mr_widget_status_icon.vue'; import eventHub from '../../event_hub'; +import SquashBeforeMerge from './mr_widget_squash_before_merge.vue'; export default { name: 'ReadyToMerge', components: { statusIcon, + 'squash-before-merge': SquashBeforeMerge, }, props: { mr: { type: Object, required: true }, @@ -101,6 +103,12 @@ export default { return enableSquashBeforeMerge && commitsCount > 1; }, }, + created() { + eventHub.$on('MRWidgetUpdateSquash', this.handleUpdateSquash); + }, + beforeDestroy() { + eventHub.$off('MRWidgetUpdateSquash', this.handleUpdateSquash); + }, methods: { shouldShowMergeControls() { return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText; @@ -128,13 +136,9 @@ export default { commit_message: this.commitMessage, merge_when_pipeline_succeeds: this.setToMergeWhenPipelineSucceeds, 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.service.merge(options) .then(res => res.data) @@ -154,6 +158,9 @@ export default { new Flash('Something went wrong. Please try again.'); // eslint-disable-line }); }, + handleUpdateSquash(val) { + this.mr.squash = val; + }, initiateMergePolling() { simplePoll((continuePolling, stopPolling) => { this.handleMergePolling(continuePolling, stopPolling); 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 83b7b054e6f..e5b7e1f1c68 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 @@ -15,6 +15,11 @@ export default class MergeRequestStore { const currentUser = data.current_user; 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.targetBranch = data.target_branch; this.sourceBranch = data.source_branch; diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 67d4ea2ca8f..29632bef7e5 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -24,6 +24,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont :source_branch, :source_project_id, :state_event, + :squash, :target_branch, :target_project_id, :task_num, diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 62b739918e6..507a07c6e1b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -253,7 +253,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def merge_params_attributes - [:should_remove_source_branch, :commit_message] + [:should_remove_source_branch, :commit_message, :squash] end def merge_when_pipeline_succeeds_active? @@ -282,7 +282,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo 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? return :failed unless @merge_request.actual_head_pipeline diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index c19c5b9cc82..74251c260f0 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -97,8 +97,9 @@ module MergeRequestsHelper { merge_when_pipeline_succeeds: true, should_remove_source_branch: true, - sha: merge_request.diff_head_sha - }.merge(merge_params_ee(merge_request)) + sha: merge_request.diff_head_sha, + squash: merge_request.squash + } end def tab_link_for(merge_request, tab, options = {}, &block) @@ -149,8 +150,4 @@ module MergeRequestsHelper current_user.fork_of(project) end end - - def merge_params_ee(merge_request) - {} - end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9c4384a6e42..bc97fc3a5d9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1140,4 +1140,11 @@ class MergeRequest < ActiveRecord::Base maintainer_push_possible? && Ability.allowed?(user, :push_code, source_project) 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 diff --git a/app/models/repository.rb b/app/models/repository.rb index 0e1bf11d7c0..6165808cd9a 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -957,6 +957,14 @@ class Repository remote_branch: merge_request.target_branch) 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 # TODO Generice finder, later split this on finders by Ref or Oid diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index d0165c148eb..141070aef45 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -10,6 +10,7 @@ class MergeRequestWidgetEntity < IssuableEntity expose :merge_when_pipeline_succeeds expose :source_branch expose :source_project_id + expose :squash expose :target_branch expose :target_project_id expose :allow_maintainer_to_push diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 2209a60a840..126da891c78 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -34,6 +34,19 @@ module MergeRequests handle_merge_error(log_message: e.message, save_message_on_model: true) 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 def error_check! @@ -116,9 +129,5 @@ module MergeRequests def merge_request_info merge_request.to_reference(full: true) end - - def source - @source ||= @merge_request.diff_head_sha - end end end diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb new file mode 100644 index 00000000000..a40fb2786bd --- /dev/null +++ b/app/services/merge_requests/squash_service.rb @@ -0,0 +1,28 @@ +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 diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index aa3fb623e58..01e38ffee20 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -20,6 +20,8 @@ window.gl = window.gl || {}; 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 .content-block.content-block-small.emoji-list-container.js-noteable-awards diff --git a/app/views/shared/issuable/form/_merge_params.html.haml b/app/views/shared/issuable/form/_merge_params.html.haml index 1df881e4102..90fbf19e843 100644 --- a/app/views/shared/issuable/form/_merge_params.html.haml +++ b/app/views/shared/issuable/form/_merge_params.html.haml @@ -15,3 +15,12 @@ = 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? 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') diff --git a/changelogs/unreleased/blackst0ne-squash-and-merge-in-gitlab-core-ce.yml b/changelogs/unreleased/blackst0ne-squash-and-merge-in-gitlab-core-ce.yml new file mode 100644 index 00000000000..e603c835b5e --- /dev/null +++ b/changelogs/unreleased/blackst0ne-squash-and-merge-in-gitlab-core-ce.yml @@ -0,0 +1,5 @@ +--- +title: Add `Squash and merge` to GitLab Core (CE) +merge_request: 18956 +author: "@blackst0ne" +type: added diff --git a/db/migrate/20180515005612_add_squash_to_merge_requests.rb b/db/migrate/20180515005612_add_squash_to_merge_requests.rb new file mode 100644 index 00000000000..f526b45bd4b --- /dev/null +++ b/db/migrate/20180515005612_add_squash_to_merge_requests.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 884e333874c..ee9c85da7a2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1217,6 +1217,7 @@ ActiveRecord::Schema.define(version: 20180521171529) do t.integer "latest_merge_request_diff_id" t.string "rebase_commit_sha" t.boolean "allow_maintainer_to_push" + t.boolean "squash", default: false, null: false end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 4e34831422a..8849f490c4f 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -107,6 +107,7 @@ Parameters: "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "time_stats": { "time_estimate": 0, @@ -226,6 +227,7 @@ Parameters: "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, "time_stats": { @@ -305,6 +307,7 @@ Parameters: "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, "time_stats": { @@ -541,7 +544,8 @@ POST /projects/:id/merge_requests | `labels` | string | no | Labels for MR as a comma-separated list | | `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 | -| `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 { @@ -595,6 +599,7 @@ POST /projects/:id/merge_requests "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, "allow_maintainer_to_push": false, @@ -627,6 +632,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid | `description` | string | no | Description of MR | | `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 | +| `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. | | `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. "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, "allow_maintainer_to_push": false, @@ -790,6 +797,7 @@ Parameters: "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, "time_stats": { @@ -868,6 +876,7 @@ Parameters: "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, "time_stats": { @@ -1200,6 +1209,7 @@ Example response: "changes_count": "1", "should_remove_source_branch": true, "force_remove_source_branch": false, + "squash": false, "web_url": "http://example.com/example/example/merge_requests/1" }, "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/merge_requests/7", diff --git a/doc/user/project/merge_requests/img/squash_edit_form.png b/doc/user/project/merge_requests/img/squash_edit_form.png new file mode 100644 index 00000000000..496c6f44ea7 Binary files /dev/null and b/doc/user/project/merge_requests/img/squash_edit_form.png differ diff --git a/doc/user/project/merge_requests/img/squash_mr_commits.png b/doc/user/project/merge_requests/img/squash_mr_commits.png new file mode 100644 index 00000000000..5fc6a8c48bb Binary files /dev/null and b/doc/user/project/merge_requests/img/squash_mr_commits.png differ diff --git a/doc/user/project/merge_requests/img/squash_mr_widget.png b/doc/user/project/merge_requests/img/squash_mr_widget.png new file mode 100644 index 00000000000..9cb458b2a35 Binary files /dev/null and b/doc/user/project/merge_requests/img/squash_mr_widget.png differ diff --git a/doc/user/project/merge_requests/img/squash_squashed_commit.png b/doc/user/project/merge_requests/img/squash_squashed_commit.png new file mode 100644 index 00000000000..0cf5875f82c Binary files /dev/null and b/doc/user/project/merge_requests/img/squash_squashed_commit.png differ diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index 5932f5a2bc1..b75bcacc9d7 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -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 - [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) +- [Squash and merge](squash_and_merge.md) for a cleaner commit history 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]** - 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]** ## Use cases @@ -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. 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. 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 ## Merge requests per project diff --git a/doc/user/project/merge_requests/squash_and_merge.md b/doc/user/project/merge_requests/squash_and_merge.md new file mode 100644 index 00000000000..a6efe893853 --- /dev/null +++ b/doc/user/project/merge_requests/squash_and_merge.md @@ -0,0 +1,80 @@ +# 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 diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 174c5af91d5..b630fd54354 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -568,6 +568,8 @@ module API expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |merge_request| merge_request end + + expose :squash end class MergeRequest < MergeRequestBasic diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index bc4df16e3a8..1ba9a09346f 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -10,12 +10,6 @@ module API helpers do params :optional_params_ee do end - - params :merge_params_ee do - end - - def update_merge_request_ee(merge_request) - end end def self.update_params_at_least_one_of @@ -29,6 +23,7 @@ module API target_branch title discussion_locked + squash ] end @@ -146,6 +141,7 @@ module API optional :labels, type: String, desc: 'Comma-separated list of label names' 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 :squash, type: Grape::API::Boolean, desc: 'When true, the commits will be squashed into a single commit on merge' use :optional_params_ee end @@ -308,8 +304,7 @@ module API optional :merge_when_pipeline_succeeds, type: Boolean, 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' - - use :merge_params_ee + optional :squash, type: Grape::API::Boolean, desc: 'When true, the commits will be squashed into a single commit on merge' end put ':id/merge_requests/:merge_request_iid/merge' do Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42317') @@ -327,7 +322,7 @@ module API check_sha_param!(params, merge_request) - update_merge_request_ee(merge_request) + merge_request.update(squash: params[:squash]) if params[:squash] merge_params = { commit_message: params[:merge_commit_message], diff --git a/lib/api/v3/entities.rb b/lib/api/v3/entities.rb index 68b4d7c3982..28fcf6c6e84 100644 --- a/lib/api/v3/entities.rb +++ b/lib/api/v3/entities.rb @@ -134,6 +134,8 @@ module API expose :should_remove_source_branch?, as: :should_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch + expose :squash + expose :web_url do |merge_request, options| Gitlab::UrlBuilder.build(merge_request) end diff --git a/lib/api/v3/merge_requests.rb b/lib/api/v3/merge_requests.rb index 9b0f70e2bfe..af5afd1c334 100644 --- a/lib/api/v3/merge_requests.rb +++ b/lib/api/v3/merge_requests.rb @@ -44,6 +44,7 @@ module API 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 :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging' + optional :squash, type: Boolean, desc: 'Squash commits when merging' end end @@ -166,7 +167,7 @@ module API use :optional_params at_least_one_of :title, :target_branch, :description, :assignee_id, :milestone_id, :labels, :state_event, - :remove_source_branch + :remove_source_branch, :squash end put path do Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42127') @@ -195,6 +196,7 @@ module API optional :merge_when_build_succeeds, type: Boolean, 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 :squash, type: Boolean, desc: 'When true, the commits will be squashed into a single commit on merge' end put "#{path}/merge" do merge_request = find_project_merge_request(params[:merge_request_id]) @@ -211,6 +213,8 @@ module API render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409) end + merge_request.update(squash: params[:squash]) if params[:squash] + merge_params = { commit_message: params[:merge_commit_message], should_remove_source_branch: params[:should_remove_source_branch] diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index 166861e6c4a..9507f92f4b2 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -16,6 +16,10 @@ module QA element :no_fast_forward_message, 'Fast-forward merge is not possible' end + view 'app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_squash_before_merge.vue' do + element :squash_checkbox + end + def rebase! click_element :mr_rebase_button @@ -41,6 +45,14 @@ module QA has_text?('The changes were merged into') 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 diff --git a/qa/qa/specs/features/merge_request/squash_spec.rb b/qa/qa/specs/features/merge_request/squash_spec.rb new file mode 100644 index 00000000000..dbbdf852a38 --- /dev/null +++ b/qa/qa/specs/features/merge_request/squash_spec.rb @@ -0,0 +1,48 @@ +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 diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index d3042be9e8b..5efaaf2bb3a 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -275,6 +275,7 @@ describe Projects::MergeRequestsController do namespace_id: project.namespace, project_id: project, id: merge_request.iid, + squash: false, format: 'json' } end @@ -315,8 +316,8 @@ describe Projects::MergeRequestsController do end context 'when the sha parameter matches the source SHA' do - def merge_with_sha - post :merge, base_params.merge(sha: merge_request.diff_head_sha) + def merge_with_sha(params = {}) + post :merge, base_params.merge(sha: merge_request.diff_head_sha).merge(params) end it 'returns :success' do @@ -331,6 +332,24 @@ describe Projects::MergeRequestsController do merge_with_sha 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 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) diff --git a/spec/features/merge_requests/user_squashes_merge_request_spec.rb b/spec/features/merge_requests/user_squashes_merge_request_spec.rb new file mode 100644 index 00000000000..6c952791591 --- /dev/null +++ b/spec/features/merge_requests/user_squashes_merge_request_spec.rb @@ -0,0 +1,124 @@ +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 diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json index 233102c4314..7be8c9e3e67 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -112,7 +112,8 @@ "rebase_commit_sha": { "type": ["string", "null"] }, "rebase_in_progress": { "type": "boolean" }, "can_push_to_source_branch": { "type": "boolean" }, - "rebase_path": { "type": ["string", "null"] } + "rebase_path": { "type": ["string", "null"] }, + "squash": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/fixtures/api/schemas/public_api/v3/merge_requests.json b/spec/fixtures/api/schemas/public_api/v3/merge_requests.json index b5c74bcc26e..9af7a8c9f4f 100644 --- a/spec/fixtures/api/schemas/public_api/v3/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v3/merge_requests.json @@ -73,7 +73,8 @@ "should_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] }, "web_url": { "type": "uri" }, - "subscribed": { "type": ["boolean"] } + "subscribed": { "type": ["boolean"] }, + "squash": { "type": "boolean" } }, "required": [ "id", "iid", "project_id", "title", "description", @@ -83,7 +84,7 @@ "labels", "work_in_progress", "milestone", "merge_when_build_succeeds", "merge_status", "sha", "merge_commit_sha", "user_notes_count", "should_remove_source_branch", "force_remove_source_branch", - "web_url", "subscribed" + "web_url", "subscribed", "squash" ], "additionalProperties": false } diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index 0dc2eabec5d..f97461ce9cc 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -75,6 +75,7 @@ "force_remove_source_branch": { "type": ["boolean", "null"] }, "discussion_locked": { "type": ["boolean", "null"] }, "web_url": { "type": "uri" }, + "squash": { "type": "boolean" }, "time_stats": { "time_estimate": { "type": "integer" }, "total_time_spent": { "type": "integer" }, @@ -91,7 +92,7 @@ "labels", "work_in_progress", "milestone", "merge_when_pipeline_succeeds", "merge_status", "sha", "merge_commit_sha", "user_notes_count", "should_remove_source_branch", "force_remove_source_branch", - "web_url" + "web_url", "squash" ], "additionalProperties": false } diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 62da967cf96..3d5271cd030 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -165,6 +165,7 @@ MergeRequest: - approvals_before_merge - rebase_commit_sha - time_estimate +- squash - last_edited_at - last_edited_by_id - head_pipeline_id diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 92e33a64d26..9ffa91fc265 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -14,6 +14,65 @@ describe MergeRequest do it { is_expected.to have_many(:merge_request_diffs) } 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 subject { described_class } diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 1eeeb4f1045..6b91e48ae6a 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -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['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['squash']).to eq(merge_request_merged.squash) end it "returns an array of all merge_requests using simple mode" do @@ -671,12 +672,14 @@ describe API::MergeRequests do target_branch: 'master', author: user, labels: 'label, label2', - milestone_id: milestone.id + milestone_id: milestone.id, + squash: true expect(response).to have_gitlab_http_status(201) expect(json_response['title']).to eq('Test merge_request') expect(json_response['labels']).to eq(%w(label label2)) 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 end @@ -965,6 +968,14 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(200) 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 allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline) allow(pipeline).to receive(:active?).and_return(true) @@ -1029,6 +1040,13 @@ describe API::MergeRequests do expect(json_response['milestone']['id']).to eq(milestone.id) 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 put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), target_branch: "wiki" expect(response).to have_gitlab_http_status(200) diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb index be70cb24dce..79a16fbd1b0 100644 --- a/spec/requests/api/v3/merge_requests_spec.rb +++ b/spec/requests/api/v3/merge_requests_spec.rb @@ -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['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['squash']).to eq(merge_request_merged.squash) end it "returns an array of all merge_requests" do @@ -241,13 +242,15 @@ describe API::MergeRequests do author: user, labels: 'label, label2', milestone_id: milestone.id, - remove_source_branch: true + remove_source_branch: true, + squash: true expect(response).to have_gitlab_http_status(201) expect(json_response['title']).to eq('Test merge_request') expect(json_response['labels']).to eq(%w(label label2)) expect(json_response['milestone']['id']).to eq(milestone.id) expect(json_response['force_remove_source_branch']).to be_truthy + expect(json_response['squash']).to be_truthy end it "returns 422 when source_branch equals target_branch" do @@ -489,6 +492,14 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(200) 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 allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline) allow(pipeline).to receive(:active?).and_return(true) @@ -529,6 +540,13 @@ describe API::MergeRequests do expect(json_response['milestone']['id']).to eq(milestone.id) 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 put v3_api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), target_branch: "wiki" expect(response).to have_gitlab_http_status(200) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index e8568bf8bb3..dc30a9bccc1 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -249,24 +249,58 @@ describe MergeRequests::MergeService do expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) end - context "when fast-forward merge is not allowed" do + context 'when squashing' do before do - allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) + merge_request.update!(source_branch: 'master', target_branch: 'feature') 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) + it 'logs and saves error if there is an error when squashing' do + error_message = 'Failed to squash. Should be done manually' - 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.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)) + 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 + + 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 diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb new file mode 100644 index 00000000000..bd884787425 --- /dev/null +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -0,0 +1,199 @@ +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 diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index 57aa07cf4fa..1fef50a52ec 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -47,6 +47,7 @@ module TestEnv 'v1.1.0' => 'b83d6e3', 'add-ipython-files' => '93ee732', 'add-pdf-file' => 'e774ebd', + 'squash-large-files' => '54cec52', 'add-pdf-text-binary' => '79faa7b', 'add_images_and_changes' => '010d106' }.freeze -- cgit v1.2.1