Commit 2b7dd017 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Nick Thomas

Allow custom squash commit messages

parent 5bfa8e2f
...@@ -32,10 +32,10 @@ export default class MergeRequestStore { ...@@ -32,10 +32,10 @@ export default class MergeRequestStore {
this.sourceBranchProtected = data.source_branch_protected; this.sourceBranchProtected = data.source_branch_protected;
this.conflictsDocsPath = data.conflicts_docs_path; this.conflictsDocsPath = data.conflicts_docs_path;
this.mergeStatus = data.merge_status; this.mergeStatus = data.merge_status;
this.commitMessage = data.merge_commit_message; this.commitMessage = data.default_merge_commit_message;
this.shortMergeCommitSha = data.short_merge_commit_sha; this.shortMergeCommitSha = data.short_merge_commit_sha;
this.mergeCommitSha = data.merge_commit_sha; this.mergeCommitSha = data.merge_commit_sha;
this.commitMessageWithDescription = data.merge_commit_message_with_description; this.commitMessageWithDescription = data.default_merge_commit_message_with_description;
this.commitsCount = data.commits_count; this.commitsCount = data.commits_count;
this.divergedCommitsCount = data.diverged_commits_count; this.divergedCommitsCount = data.diverged_commits_count;
this.pipeline = data.pipeline || {}; this.pipeline = data.pipeline || {};
......
...@@ -240,7 +240,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -240,7 +240,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end end
def merge_params_attributes def merge_params_attributes
[:should_remove_source_branch, :commit_message, :squash] [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash]
end end
def merge_when_pipeline_succeeds_active? def merge_when_pipeline_succeeds_active?
......
...@@ -39,7 +39,8 @@ module Types ...@@ -39,7 +39,8 @@ module Types
field :rebase_commit_sha, GraphQL::STRING_TYPE, null: true field :rebase_commit_sha, GraphQL::STRING_TYPE, null: true
field :rebase_in_progress, GraphQL::BOOLEAN_TYPE, method: :rebase_in_progress?, null: false field :rebase_in_progress, GraphQL::BOOLEAN_TYPE, method: :rebase_in_progress?, null: false
field :diff_head_sha, GraphQL::STRING_TYPE, null: true field :diff_head_sha, GraphQL::STRING_TYPE, null: true
field :merge_commit_message, GraphQL::STRING_TYPE, null: true field :merge_commit_message, GraphQL::STRING_TYPE, method: :default_merge_commit_message, null: true, deprecation_reason: "Renamed to defaultMergeCommitMessage"
field :default_merge_commit_message, GraphQL::STRING_TYPE, null: true
field :merge_ongoing, GraphQL::BOOLEAN_TYPE, method: :merge_ongoing?, null: false field :merge_ongoing, GraphQL::BOOLEAN_TYPE, method: :merge_ongoing?, null: false
field :source_branch_exists, GraphQL::BOOLEAN_TYPE, method: :source_branch_exists?, null: false field :source_branch_exists, GraphQL::BOOLEAN_TYPE, method: :source_branch_exists?, null: false
field :mergeable_discussions_state, GraphQL::BOOLEAN_TYPE, null: true field :mergeable_discussions_state, GraphQL::BOOLEAN_TYPE, null: true
......
...@@ -379,7 +379,7 @@ class Commit ...@@ -379,7 +379,7 @@ class Commit
end end
def merge_commit? def merge_commit?
parents.size > 1 parent_ids.size > 1
end end
def merged_merge_request(current_user) def merged_merge_request(current_user)
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
# A collection of Commit instances for a specific project and Git reference. # A collection of Commit instances for a specific project and Git reference.
class CommitCollection class CommitCollection
include Enumerable include Enumerable
include Gitlab::Utils::StrongMemoize
attr_reader :project, :ref, :commits attr_reader :project, :ref, :commits
...@@ -20,11 +21,17 @@ class CommitCollection ...@@ -20,11 +21,17 @@ class CommitCollection
end end
def committers def committers
emails = commits.reject(&:merge_commit?).map(&:committer_email).uniq emails = without_merge_commits.map(&:committer_email).uniq
User.by_any_email(emails) User.by_any_email(emails)
end end
def without_merge_commits
strong_memoize(:without_merge_commits) do
commits.reject(&:merge_commit?)
end
end
# Sets the pipeline status for every commit. # Sets the pipeline status for every commit.
# #
# Setting this status ahead of time removes the need for running a query for # Setting this status ahead of time removes the need for running a query for
......
...@@ -939,7 +939,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -939,7 +939,7 @@ class MergeRequest < ActiveRecord::Base
self.target_project.repository.branch_exists?(self.target_branch) self.target_project.repository.branch_exists?(self.target_branch)
end end
def merge_commit_message(include_description: false) def default_merge_commit_message(include_description: false)
closes_issues_references = visible_closing_issues_for.map do |issue| closes_issues_references = visible_closing_issues_for.map do |issue|
issue.to_reference(target_project) issue.to_reference(target_project)
end end
...@@ -959,6 +959,13 @@ class MergeRequest < ActiveRecord::Base ...@@ -959,6 +959,13 @@ class MergeRequest < ActiveRecord::Base
message.join("\n\n") message.join("\n\n")
end end
# Returns the oldest multi-line commit message, or the MR title if none found
def default_squash_commit_message
strong_memoize(:default_squash_commit_message) do
commits.without_merge_commits.reverse.find(&:description?)&.safe_message || title
end
end
def reset_merge_when_pipeline_succeeds def reset_merge_when_pipeline_succeeds
return unless merge_when_pipeline_succeeds? return unless merge_when_pipeline_succeeds?
...@@ -967,6 +974,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -967,6 +974,7 @@ class MergeRequest < ActiveRecord::Base
if merge_params if merge_params
merge_params.delete('should_remove_source_branch') merge_params.delete('should_remove_source_branch')
merge_params.delete('commit_message') merge_params.delete('commit_message')
merge_params.delete('squash_commit_message')
end end
self.save self.save
......
...@@ -1030,12 +1030,12 @@ class Repository ...@@ -1030,12 +1030,12 @@ class Repository
remote_branch: merge_request.target_branch) remote_branch: merge_request.target_branch)
end end
def squash(user, merge_request) def squash(user, merge_request, message)
raw.squash(user, merge_request.id, branch: merge_request.target_branch, raw.squash(user, merge_request.id, branch: merge_request.target_branch,
start_sha: merge_request.diff_start_sha, start_sha: merge_request.diff_start_sha,
end_sha: merge_request.diff_head_sha, end_sha: merge_request.diff_head_sha,
author: merge_request.author, author: merge_request.author,
message: merge_request.title) message: message)
end end
def update_submodule(user, submodule, commit_sha, message:, branch:) def update_submodule(user, submodule, commit_sha, message:, branch:)
......
# frozen_string_literal: true
class MergeRequestWidgetCommitEntity < Grape::Entity
expose :safe_message, as: :message
expose :short_id
expose :title
end
...@@ -56,10 +56,23 @@ class MergeRequestWidgetEntity < IssuableEntity ...@@ -56,10 +56,23 @@ class MergeRequestWidgetEntity < IssuableEntity
merge_request.diff_head_sha.presence merge_request.diff_head_sha.presence
end end
expose :merge_commit_message
expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline, if: -> (mr, _) { presenter(mr).can_read_pipeline? } expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline, if: -> (mr, _) { presenter(mr).can_read_pipeline? }
expose :merge_pipeline, with: PipelineDetailsEntity, if: ->(mr, _) { mr.merged? && can?(request.current_user, :read_pipeline, mr.target_project)} expose :merge_pipeline, with: PipelineDetailsEntity, if: ->(mr, _) { mr.merged? && can?(request.current_user, :read_pipeline, mr.target_project)}
expose :default_squash_commit_message
expose :default_merge_commit_message
expose :default_merge_commit_message_with_description do |merge_request|
merge_request.default_merge_commit_message(include_description: true)
end
expose :commits_without_merge_commits, using: MergeRequestWidgetCommitEntity do |merge_request|
merge_request.commits.without_merge_commits
end
expose :commits_count
# Booleans # Booleans
expose :merge_ongoing?, as: :merge_ongoing expose :merge_ongoing?, as: :merge_ongoing
expose :work_in_progress?, as: :work_in_progress expose :work_in_progress?, as: :work_in_progress
...@@ -77,7 +90,6 @@ class MergeRequestWidgetEntity < IssuableEntity ...@@ -77,7 +90,6 @@ class MergeRequestWidgetEntity < IssuableEntity
end end
expose :branch_missing?, as: :branch_missing expose :branch_missing?, as: :branch_missing
expose :commits_count
expose :cannot_be_merged?, as: :has_conflicts expose :cannot_be_merged?, as: :has_conflicts
expose :can_be_merged?, as: :can_be_merged expose :can_be_merged?, as: :can_be_merged
expose :mergeable?, as: :mergeable expose :mergeable?, as: :mergeable
...@@ -205,10 +217,6 @@ class MergeRequestWidgetEntity < IssuableEntity ...@@ -205,10 +217,6 @@ class MergeRequestWidgetEntity < IssuableEntity
ci_environments_status_project_merge_request_path(merge_request.project, merge_request) ci_environments_status_project_merge_request_path(merge_request.project, merge_request)
end end
expose :merge_commit_message_with_description do |merge_request|
merge_request.merge_commit_message(include_description: true)
end
expose :diverged_commits_count do |merge_request| expose :diverged_commits_count do |merge_request|
if merge_request.open? && merge_request.diverged_from_target_branch? if merge_request.open? && merge_request.diverged_from_target_branch?
merge_request.diverged_commits_count merge_request.diverged_commits_count
......
...@@ -8,6 +8,8 @@ module MergeRequests ...@@ -8,6 +8,8 @@ module MergeRequests
# Executed when you do merge via GitLab UI # Executed when you do merge via GitLab UI
# #
class MergeService < MergeRequests::BaseService class MergeService < MergeRequests::BaseService
include Gitlab::Utils::StrongMemoize
MergeError = Class.new(StandardError) MergeError = Class.new(StandardError)
attr_reader :merge_request, :source attr_reader :merge_request, :source
...@@ -37,15 +39,10 @@ module MergeRequests ...@@ -37,15 +39,10 @@ module MergeRequests
end end
def source def source
return merge_request.diff_head_sha unless merge_request.squash if merge_request.squash
squash_sha!
squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute(merge_request) else
merge_request.diff_head_sha
case squash_result[:status]
when :success
squash_result[:squash_sha]
when :error
raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
end end
end end
...@@ -82,8 +79,22 @@ module MergeRequests ...@@ -82,8 +79,22 @@ module MergeRequests
merge_request.update!(merge_commit_sha: commit_id) merge_request.update!(merge_commit_sha: commit_id)
end end
def squash_sha!
strong_memoize(:squash_sha) do
params[:merge_request] = merge_request
squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
case squash_result[:status]
when :success
squash_result[:squash_sha]
when :error
raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
end
end
end
def try_merge def try_merge
message = params[:commit_message] || merge_request.merge_commit_message message = params[:commit_message] || merge_request.default_merge_commit_message
repository.merge(current_user, source, merge_request, message) repository.merge(current_user, source, merge_request, message)
rescue Gitlab::Git::PreReceiveError => e rescue Gitlab::Git::PreReceiveError => e
......
...@@ -2,15 +2,10 @@ ...@@ -2,15 +2,10 @@
module MergeRequests module MergeRequests
class SquashService < MergeRequests::WorkingCopyBaseService class SquashService < MergeRequests::WorkingCopyBaseService
def execute(merge_request) def execute
@merge_request = merge_request # If performing a squash would result in no change, then
@repository = target_project.repository # immediately return a success message without performing a squash
if merge_request.commits_count < 2 && message.nil?
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) return success(squash_sha: merge_request.diff_head_sha)
end end
...@@ -18,7 +13,13 @@ module MergeRequests ...@@ -18,7 +13,13 @@ module MergeRequests
return error('Squash task canceled: another squash is already in progress.') return error('Squash task canceled: another squash is already in progress.')
end end
squash_sha = repository.squash(current_user, merge_request) squash! || error('Failed to squash. Should be done manually.')
end
private
def squash!
squash_sha = repository.squash(current_user, merge_request, message || merge_request.default_squash_commit_message)
success(squash_sha: squash_sha) success(squash_sha: squash_sha)
rescue => e rescue => e
...@@ -26,5 +27,17 @@ module MergeRequests ...@@ -26,5 +27,17 @@ module MergeRequests
log_error(e.message) log_error(e.message)
false false
end end
def repository
target_project.repository
end
def merge_request
params[:merge_request]
end
def message
params[:squash_commit_message].presence
end
end end
end end
---
title: Default squash commit message is now selected from the longest commit when
squashing merge requests
merge_request: 24518
author:
type: changed
...@@ -18,10 +18,14 @@ Into a single commit on merge: ...@@ -18,10 +18,14 @@ Into a single commit on merge:
![A squashed commit followed by a merge commit][squashed-commit] ![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's commit message will be either:
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 - Taken from the first multi-line commit message in the merge.
with the fast-forward merge strategy, see - The merge request's title if no multi-line commit message is found.
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 [squashing and fast-forward merge](#squash-and-fast-forward-merge) for more
details. details.
...@@ -34,7 +38,7 @@ you'd rather not include them in your target branch. ...@@ -34,7 +38,7 @@ you'd rather not include them in your target branch.
With squash and merge, when the merge request is ready to be merged, 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 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. the commits in the merge request into a single commit.
This way, the history of your base branch remains clean with This way, the history of your base branch remains clean with
meaningful commit messages and is simpler to [revert] if necessary. meaningful commit messages and is simpler to [revert] if necessary.
...@@ -56,7 +60,7 @@ This can then be overridden at the time of accepting the merge request: ...@@ -56,7 +60,7 @@ This can then be overridden at the time of accepting the merge request:
The squashed commit has the following metadata: The squashed commit has the following metadata:
- Message: the title of the merge request. - Message: the message of the squash commit.
- Author: the author of the merge request. - Author: the author of the merge request.
- Committer: the user who initiated the squash. - Committer: the user who initiated the squash.
......
...@@ -387,6 +387,23 @@ describe Projects::MergeRequestsController do ...@@ -387,6 +387,23 @@ describe Projects::MergeRequestsController do
end end
end end
context 'when a squash commit message is passed' do
let(:message) { 'My custom squash commit message' }
it 'passes the same message to SquashService' do
params = { squash: '1', squash_commit_message: message }
expect_next_instance_of(MergeRequests::SquashService, project, user, params.merge(merge_request: merge_request)) do |squash_service|
expect(squash_service).to receive(:execute).and_return({
status: :success,
squash_sha: SecureRandom.hex(20)
})
end
merge_with_sha(params)
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)
......
...@@ -16,14 +16,24 @@ FactoryBot.define do ...@@ -16,14 +16,24 @@ FactoryBot.define do
commit commit
end end
project project
skip_create # Commits cannot be persisted
initialize_with do initialize_with do
new(git_commit, project) new(git_commit, project)
end end
after(:build) do |commit, evaluator| after(:build) do |commit, evaluator|
allow(commit).to receive(:author).and_return(evaluator.author || build_stubbed(:author)) allow(commit).to receive(:author).and_return(evaluator.author || build_stubbed(:author))
allow(commit).to receive(:parent_ids).and_return([])
end
trait :merge_commit do
after(:build) do |commit|
allow(commit).to receive(:parent_ids).and_return(Array.new(2) { SecureRandom.hex(20) })
end
end end
trait :without_author do trait :without_author do
......
...@@ -14,7 +14,7 @@ describe 'User squashes a merge request', :js do ...@@ -14,7 +14,7 @@ describe 'User squashes a merge request', :js do
latest_master_commits = project.repository.commits_between(original_head.sha, 'master').map(&:raw) 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}/), squash_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/),
message: "Csv\n", message: a_string_starting_with(project.merge_requests.first.default_squash_commit_message),
author_name: user.name, author_name: user.name,
committer_name: user.name) committer_name: user.name)
......
...@@ -44,7 +44,7 @@ ...@@ -44,7 +44,7 @@
"merge_user": { "type": ["object", "null"] }, "merge_user": { "type": ["object", "null"] },
"diff_head_sha": { "type": ["string", "null"] }, "diff_head_sha": { "type": ["string", "null"] },
"diff_head_commit_short_id": { "type": ["string", "null"] }, "diff_head_commit_short_id": { "type": ["string", "null"] },
"merge_commit_message": { "type": ["string", "null"] }, "default_merge_commit_message": { "type": ["string", "null"] },
"pipeline": { "type": ["object", "null"] }, "pipeline": { "type": ["object", "null"] },
"merge_pipeline": { "type": ["object", "null"] }, "merge_pipeline": { "type": ["object", "null"] },
"work_in_progress": { "type": "boolean" }, "work_in_progress": { "type": "boolean" },
...@@ -102,7 +102,9 @@ ...@@ -102,7 +102,9 @@
"new_blob_path": { "type": ["string", "null"] }, "new_blob_path": { "type": ["string", "null"] },
"merge_check_path": { "type": "string" }, "merge_check_path": { "type": "string" },
"ci_environments_status_path": { "type": "string" }, "ci_environments_status_path": { "type": "string" },
"merge_commit_message_with_description": { "type": "string" }, "default_merge_commit_message_with_description": { "type": "string" },
"default_squash_commit_message": { "type": "string" },
"commits_without_merge_commits": { "type": "array" },
"diverged_commits_count": { "type": "integer" }, "diverged_commits_count": { "type": "integer" },
"commit_change_content_path": { "type": "string" }, "commit_change_content_path": { "type": "string" },
"merge_commit_path": { "type": ["string", "null"] }, "merge_commit_path": { "type": ["string", "null"] },
......
...@@ -58,7 +58,7 @@ export default { ...@@ -58,7 +58,7 @@ export default {
merge_user: null, merge_user: null,
diff_head_sha: '104096c51715e12e7ae41f9333e9fa35b73f385d', diff_head_sha: '104096c51715e12e7ae41f9333e9fa35b73f385d',
diff_head_commit_short_id: '104096c5', diff_head_commit_short_id: '104096c5',
merge_commit_message: default_merge_commit_message:
"Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22", "Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22",
pipeline: { pipeline: {
id: 172, id: 172,
...@@ -213,7 +213,7 @@ export default { ...@@ -213,7 +213,7 @@ export default {
merge_check_path: '/root/acets-app/merge_requests/22/merge_check', merge_check_path: '/root/acets-app/merge_requests/22/merge_check',
ci_environments_status_url: '/root/acets-app/merge_requests/22/ci_environments_status', ci_environments_status_url: '/root/acets-app/merge_requests/22/ci_environments_status',
project_archived: false, project_archived: false,
merge_commit_message_with_description: default_merge_commit_message_with_description:
"Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22", "Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22",
diverged_commits_count: 0, diverged_commits_count: 0,
only_allow_merge_if_pipeline_succeeds: false, only_allow_merge_if_pipeline_succeeds: false,
......
...@@ -35,6 +35,17 @@ describe CommitCollection do ...@@ -35,6 +35,17 @@ describe CommitCollection do
end end
end end
describe '#without_merge_commits' do
it 'returns all commits except merge commits' do
collection = described_class.new(project, [
build(:commit),
build(:commit, :merge_commit)
])
expect(collection.without_merge_commits.size).to eq(1)
end
end
describe '#with_pipeline_status' do describe '#with_pipeline_status' do
it 'sets the pipeline status for every commit so no additional queries are necessary' do it 'sets the pipeline status for every commit so no additional queries are necessary' do
create( create(
......
...@@ -82,6 +82,38 @@ describe MergeRequest do ...@@ -82,6 +82,38 @@ describe MergeRequest do
end end
end end
describe '#default_squash_commit_message' do
let(:project) { subject.project }
def commit_collection(commit_hashes)
raw_commits = commit_hashes.map { |raw| Commit.from_hash(raw, project) }
CommitCollection.new(project, raw_commits)
end
it 'returns the oldest multiline commit message' do
commits = commit_collection([
{ message: 'Singleline', parent_ids: [] },
{ message: "Second multiline\nCommit message", parent_ids: [] },
{ message: "First multiline\nCommit message", parent_ids: [] }
])
expect(subject).to receive(:commits).and_return(commits)
expect(subject.default_squash_commit_message).to eq("First multiline\nCommit message")
end
it 'returns the merge request title if there are no multiline commits' do
commits = commit_collection([
{ message: 'Singleline', parent_ids: [] }
])
expect(subject).to receive(:commits).and_return(commits)
expect(subject.default_squash_commit_message).to eq(subject.title)
end
end
describe 'modules' do describe 'modules' do
subject { described_class } subject { described_class }
...@@ -920,18 +952,18 @@ describe MergeRequest do ...@@ -920,18 +952,18 @@ describe MergeRequest do
end end
end end
describe '#merge_commit_message' do describe '#default_merge_commit_message' do
it 'includes merge information as the title' do it 'includes merge information as the title' do
request = build(:merge_request, source_branch: 'source', target_branch: 'target') request = build(:merge_request, source_branch: 'source', target_branch: 'target')
expect(request.merge_commit_message) expect(request.default_merge_commit_message)
.to match("Merge branch 'source' into 'target'\n\n") .to match("Merge branch 'source' into 'target'\n\n")
end end
it 'includes its title in the body' do it 'includes its title in the body' do
request = build(:merge_request, title: 'Remove all technical debt') request = build(:merge_request, title: 'Remove all technical debt')
expect(request.merge_commit_message) expect(request.default_merge_commit_message)
.to match("Remove all technical debt\n\n") .to match("Remove all technical debt\n\n")
end end
...@@ -943,34 +975,34 @@ describe MergeRequest do ...@@ -943,34 +975,34 @@ describe MergeRequest do
allow(subject.project).to receive(:default_branch).and_return(subject.target_branch) allow(subject.project).to receive(:default_branch).and_return(subject.target_branch)
subject.cache_merge_request_closes_issues! subject.cache_merge_request_closes_issues!
expect(subject.merge_commit_message) expect(subject.default_merge_commit_message)
.to match("Closes #{issue.to_reference}") .to match("Closes #{issue.to_reference}")
end end
it 'includes its reference in the body' do it 'includes its reference in the body' do
request = build_stubbed(:merge_request) request = build_stubbed(:merge_request)
expect(request.merge_commit_message) expect(request.default_merge_commit_message)
.to match("See merge request #{request.to_reference(full: true)}") .to match("See merge request #{request.to_reference(full: true)}")
end end
it 'excludes multiple linebreak runs when description is blank' do it 'excludes multiple linebreak runs when description is blank' do
request = build(:merge_request, title: 'Title', description: nil) request = build(:merge_request, title: 'Title', description: nil)
expect(request.merge_commit_message).not_to match("Title\n\n\n\n") expect(request.default_merge_commit_message).not_to match("Title\n\n\n\n")
end end
it 'includes its description in the body' do it 'includes its description in the body' do
request = build(:merge_request, description: 'By removing all code') request = build(:merge_request, description: 'By removing all code')
expect(request.merge_commit_message(include_description: true)) expect(request.default_merge_commit_message(include_description: true))
.to match("By removing all code\n\n") .to match("By removing all code\n\n")
end end
it 'does not includes its description in the body' do it 'does not includes its description in the body' do
request = build(:merge_request, description: 'By removing all code') request = build(:merge_request, description: 'By removing all code')
expect(request.merge_commit_message) expect(request.default_merge_commit_message)
.not_to match("By removing all code\n\n") .not_to match("By removing all code\n\n")
end end
end end
......
# frozen_string_literal: true
require 'spec_helper'
describe MergeRequestWidgetCommitEntity do
let(:project) { create(:project, :repository) }
let(:commit) { project.commit }
let(:request) { double('request') }
let(:entity) do
described_class.new(commit, request: request)
end
context 'as json' do
subject { entity.as_json }
it { expect(subject[:message]).to eq(commit.safe_message) }
it { expect(subject[:short_id]).to eq(commit.short_id) }
it { expect(subject[:title]).to eq(commit.title) }
end
end
...@@ -188,9 +188,14 @@ describe MergeRequestWidgetEntity do ...@@ -188,9 +188,14 @@ describe MergeRequestWidgetEntity do
.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.diff") .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.diff")
end end
it 'has merge_commit_message_with_description' do it 'has default_merge_commit_message_with_description' do
expect(subject[:merge_commit_message_with_description]) expect(subject[:default_merge_commit_message_with_description])
.to eq(resource.merge_commit_message(include_description: true)) .to eq(resource.default_merge_commit_message(include_description: true))
end
it 'has default_squash_commit_message' do
expect(subject[:default_squash_commit_message])
.to eq(resource.default_squash_commit_message)
end end
describe 'new_blob_path' do describe 'new_blob_path' do
...@@ -272,4 +277,15 @@ describe MergeRequestWidgetEntity do ...@@ -272,4 +277,15 @@ describe MergeRequestWidgetEntity do
expect(entity[:rebase_path]).to be_nil expect(entity[:rebase_path]).to be_nil
end end
end end
describe 'commits_without_merge_commits' do
it 'should not include merge commits' do
# Mock all but the first 5 commits to be merge commits
resource.commits.each_with_index do |commit, i|
expect(commit).to receive(:merge_commit?).at_least(:once).and_return(i > 4)
end
expect(subject[:commits_without_merge_commits].size).to eq(5)
end
end
end end
...@@ -258,7 +258,7 @@ describe MergeRequests::MergeService do ...@@ -258,7 +258,7 @@ describe MergeRequests::MergeService do
it 'logs and saves error if there is an error when squashing' do it 'logs and saves error if there is an error when squashing' do
error_message = 'Failed to squash. Should be done manually' error_message = 'Failed to squash. Should be done manually'
allow_any_instance_of(MergeRequests::SquashService).to receive(:squash).and_return(nil) allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil)
merge_request.update(squash: true) merge_request.update(squash: true)
service.execute(merge_request) service.execute(merge_request)
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe MergeRequests::SquashService do describe MergeRequests::SquashService do
include GitHelpers include GitHelpers
let(:service) { described_class.new(project, user, {}) } let(:service) { described_class.new(project, user, { merge_request: merge_request }) }
let(:user) { project.owner } let(:user) { project.owner }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:repository) { project.repository.raw } let(:repository) { project.repository.raw }
...@@ -31,32 +31,49 @@ describe MergeRequests::SquashService do ...@@ -31,32 +31,49 @@ describe MergeRequests::SquashService do
shared_examples 'the squash succeeds' do shared_examples 'the squash succeeds' do
it 'returns the squashed commit SHA' do it 'returns the squashed commit SHA' do
result = service.execute(merge_request) result = service.execute
expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/)) 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) expect(result[:squash_sha]).not_to eq(merge_request.diff_head_sha)
end end
it 'cleans up the temporary directory' do it 'cleans up the temporary directory' do
service.execute(merge_request) service.execute
expect(File.exist?(squash_dir_path)).to be(false) expect(File.exist?(squash_dir_path)).to be(false)
end end
it 'does not keep the branch push event' do it 'does not keep the branch push event' do
expect { service.execute(merge_request) }.not_to change { Event.count } expect { service.execute }.not_to change { Event.count }
end
context 'when there is a single commit in the merge request' do
before do
expect(merge_request).to receive(:commits_count).at_least(:once).and_return(1)
end
it 'will skip performing the squash, as the outcome would be the same' do
expect(merge_request.target_project.repository).not_to receive(:squash)
service.execute
end
it 'will still perform the squash when a custom squash commit message has been provided' do
service = described_class.new(project, user, { merge_request: merge_request, squash_commit_message: 'A custom commit message' })
expect(merge_request.target_project.repository).to receive(:squash).and_return('sha')
service.execute
end
end end
context 'the squashed commit' do context 'the squashed commit' do
let(:squash_sha) { service.execute(merge_request)[:squash_sha] } let(:squash_sha) { service.execute[:squash_sha] }
let(:squash_commit) { project.repository.commit(squash_sha) } let(:squash_commit) { project.repository.commit(squash_sha) }
it 'copies the author info and message from the merge request' do it 'copies the author info from the merge request' do
expect(squash_commit.author_name).to eq(merge_request.author.name) expect(squash_commit.author_name).to eq(merge_request.author.name)
expect(squash_commit.author_email).to eq(merge_request.author.email) 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 end
it 'sets the current user as the committer' do it 'sets the current user as the committer' do
...@@ -72,21 +89,37 @@ describe MergeRequests::SquashService do ...@@ -72,21 +89,37 @@ describe MergeRequests::SquashService do
expect(squash_diff.patch.length).to eq(mr_diff.patch.length) expect(squash_diff.patch.length).to eq(mr_diff.patch.length)
expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha) expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha)
end end
it 'has a default squash commit message if no message was provided' do
expect(squash_commit.message.chomp).to eq(merge_request.default_squash_commit_message.chomp)
end
context 'if a message was provided' do
let(:service) { described_class.new(project, user, { merge_request: merge_request, squash_commit_message: message }) }
let(:message) { 'My custom message' }
let(:squash_sha) { service.execute[:squash_sha] }
it 'has the same message as the message provided' do
expect(squash_commit.message.chomp).to eq(message)
end
end
end end
end end
describe '#execute' do describe '#execute' do
context 'when there is only one commit in the merge request' do context 'when there is only one commit in the merge request' do
let(:merge_request) { merge_request_with_one_commit }
it 'returns that commit SHA' do it 'returns that commit SHA' do
result = service.execute(merge_request_with_one_commit) result = service.execute
expect(result).to match(status: :success, squash_sha: merge_request_with_one_commit.diff_head_sha) expect(result).to match(status: :success, squash_sha: merge_request.diff_head_sha)
end end
it 'does not perform any git actions' do it 'does not perform any git actions' do
expect(repository).not_to receive(:popen) expect(repository).not_to receive(:popen)
service.execute(merge_request_with_one_commit) service.execute
end end
end end
...@@ -116,12 +149,11 @@ describe MergeRequests::SquashService do ...@@ -116,12 +149,11 @@ describe MergeRequests::SquashService do
expect(service).to receive(:log_error).with(log_error) expect(service).to receive(:log_error).with(log_error)
expect(service).to receive(:log_error).with(error) expect(service).to receive(:log_error).with(error)
service.execute(merge_request) service.execute
end end
it 'returns an error' do it 'returns an error' do
expect(service.execute(merge_request)).to match(status: :error, expect(service.execute).to match(status: :error, message: a_string_including('squash'))
message: a_string_including('squash'))
end end
end end
end end
...@@ -131,23 +163,22 @@ describe MergeRequests::SquashService do ...@@ -131,23 +163,22 @@ describe MergeRequests::SquashService do
let(:error) { 'A test error' } let(:error) { 'A test error' }
before do before do
allow(merge_request).to receive(:commits_count).and_raise(error) allow(merge_request.target_project.repository).to receive(:squash).and_raise(error)
end end
it 'logs the MR reference and exception' do 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(a_string_including("#{project.full_path}#{merge_request.to_reference}"))
expect(service).to receive(:log_error).with(error) expect(service).to receive(:log_error).with(error)
service.execute(merge_request) service.execute
end end
it 'returns an error' do it 'returns an error' do
expect(service.execute(merge_request)).to match(status: :error, expect(service.execute).to match(status: :error, message: a_string_including('squash'))
message: a_string_including('squash'))
end end
it 'cleans up the temporary directory' do it 'cleans up the temporary directory' do
service.execute(merge_request) service.execute
expect(File.exist?(squash_dir_path)).to be(false) expect(File.exist?(squash_dir_path)).to be(false)
end end
......
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