From c389a705f7b35a9ddcd36057ec6006d3612d6310 Mon Sep 17 00:00:00 2001
From: Max Coplan <mchcopl@yahoo.com>
Date: Sun, 31 Jan 2021 17:00:54 -0500
Subject: [PATCH] Set default MR title/description to first multi-line commit
Closes https://gitlab.com/gitlab-org/gitlab/-/issues/300479
When creating a merge request, the behaviour of the prefilled title and
discription is different than the behaviour of squashed commit messages.
For squashed commit messages, the behaviour is described in the
[docs](https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html#overview)
as
> Taken from the first multi-line commit message in the merge.
However, for merge requests, the current behavior is to use the branch
name. This commit sets the prefilled MR title/description to the first
multi-line commit in the MR, if applicable. Otherwise falling back to
the previous behaviour.
---
app/models/merge_request.rb | 9 +-
app/services/merge_requests/build_service.rb | 16 ++-
...-change-prefilled-mr-title-description.yml | 5 +
.../merge_requests/creating_merge_requests.md | 9 +-
.../merge_requests/build_service_spec.rb | 123 +++++++++++++-----
5 files changed, 123 insertions(+), 39 deletions(-)
create mode 100644 changelogs/unreleased/300479-change-prefilled-mr-title-description.yml
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7b8c4e844d447..07092e5068044 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1282,7 +1282,14 @@ def default_merge_commit_message(include_description: false)
# 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
- recent_commits.without_merge_commits.reverse_each.find(&:description?)&.safe_message || title
+ first_multiline_commit&.safe_message || title
+ end
+ end
+
+ # Returns the oldest multi-line commit
+ def first_multiline_commit
+ strong_memoize(:first_multiline_commit) do
+ recent_commits.without_merge_commits.reverse_each.find(&:description?)
end
end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 12c901aa1a153..e4d3c91d13e1f 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -58,6 +58,7 @@ def execute
:compare_commits,
:wip_title,
:description,
+ :first_multiline_commit,
:errors,
to: :merge_request
@@ -196,7 +197,8 @@ def target_branch_exists?
# interpreted as the user wants to close that issue on this project.
#
# For example:
- # - Issue 112 exists, title: Emoji don't show up in commit title
+ # - Issue 112 exists
+ # - title: Emoji don't show up in commit title
# - Source branch is: 112-fix-mep-mep
#
# Will lead to:
@@ -205,7 +207,7 @@ def target_branch_exists?
# more than one commit in the MR
#
def assign_title_and_description
- assign_title_and_description_from_single_commit
+ assign_title_and_description_from_commits
merge_request.title ||= title_from_issue if target_project.issues_enabled? || target_project.external_issue_tracker
merge_request.title ||= source_branch.titleize.humanize
merge_request.title = wip_title if compare_commits.empty?
@@ -240,12 +242,16 @@ def append_closes_description
end
end
- def assign_title_and_description_from_single_commit
+ def assign_title_and_description_from_commits
commits = compare_commits
- return unless commits&.count == 1
+ if commits&.count == 1
+ commit = commits.first
+ else
+ commit = first_multiline_commit
+ return unless commit
+ end
- commit = commits.first
merge_request.title ||= commit.title
merge_request.description ||= commit.description.try(:strip)
end
diff --git a/changelogs/unreleased/300479-change-prefilled-mr-title-description.yml b/changelogs/unreleased/300479-change-prefilled-mr-title-description.yml
new file mode 100644
index 0000000000000..6b360bde0ae26
--- /dev/null
+++ b/changelogs/unreleased/300479-change-prefilled-mr-title-description.yml
@@ -0,0 +1,5 @@
+---
+title: Prefill first multiline commit message for new MRs
+merge_request: 52984
+author: Max Coplan @vegerot
+type: changed
diff --git a/doc/user/project/merge_requests/creating_merge_requests.md b/doc/user/project/merge_requests/creating_merge_requests.md
index dfd160fc91b8e..9bd520b66a7d6 100644
--- a/doc/user/project/merge_requests/creating_merge_requests.md
+++ b/doc/user/project/merge_requests/creating_merge_requests.md
@@ -32,9 +32,12 @@ button and start a merge request from there.
On the **New Merge Request** page, start by filling in the title
and description for the merge request. If there are already
-commits on the branch, the title is prefilled with the first
-line of the first commit message, and the description is
-prefilled with any additional lines in the commit message.
+commits on the branch, the title is prefilled with either:
+- The first line of the first multi-line commit message
+ and the description is prefilled with any additional lines
+ in that commit message.
+- The branch name, if there are no multi-line commit messages.
+
The title is the only field that is mandatory in all cases.
From there, you can fill it with information (title, description,
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index 22b3456708f88..8adf6d69f7316 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -19,8 +19,21 @@
let(:label_ids) { [] }
let(:merge_request) { service.execute }
let(:compare) { double(:compare, commits: commits) }
- let(:commit_1) { double(:commit_1, sha: 'f00ba7', safe_message: "Initial commit\n\nCreate the app") }
- let(:commit_2) { double(:commit_2, sha: 'f00ba7', safe_message: 'This is a bad commit message!') }
+ let(:commit_1) do
+ double(:commit_1, sha: 'f00ba6', safe_message: 'Initial commit',
+ gitaly_commit?: false, id: 'f00ba6', parent_ids: ['f00ba5'])
+ end
+
+ let(:commit_2) do
+ double(:commit_2, sha: 'f00ba7', safe_message: "Closes #1234 Second commit\n\nCreate the app",
+ gitaly_commit?: false, id: 'f00ba7', parent_ids: ['f00ba6'])
+ end
+
+ let(:commit_3) do
+ double(:commit_3, sha: 'f00ba8', safe_message: 'This is a bad commit message!',
+ gitaly_commit?: false, id: 'f00ba8', parent_ids: ['f00ba7'])
+ end
+
let(:commits) { nil }
let(:params) do
@@ -47,6 +60,7 @@ def stub_compare
allow(CompareService).to receive_message_chain(:new, :execute).and_return(compare)
allow(project).to receive(:commit).and_return(commit_1)
allow(project).to receive(:commit).and_return(commit_2)
+ allow(project).to receive(:commit).and_return(commit_3)
end
shared_examples 'allows the merge request to be created' do
@@ -137,7 +151,7 @@ def stub_compare
context 'when target branch is missing' do
let(:target_branch) { nil }
- let(:commits) { Commit.decorate([commit_1], project) }
+ let(:commits) { Commit.decorate([commit_2], project) }
before do
stub_compare
@@ -199,8 +213,8 @@ def stub_compare
end
context 'one commit in the diff' do
- let(:commits) { Commit.decorate([commit_1], project) }
- let(:commit_description) { commit_1.safe_message.split(/\n+/, 2).last }
+ let(:commits) { Commit.decorate([commit_2], project) }
+ let(:commit_description) { commit_2.safe_message.split(/\n+/, 2).last }
before do
stub_compare
@@ -209,7 +223,7 @@ def stub_compare
it_behaves_like 'allows the merge request to be created'
it 'uses the title of the commit as the title of the merge request' do
- expect(merge_request.title).to eq(commit_1.safe_message.split("\n").first)
+ expect(merge_request.title).to eq(commit_2.safe_message.split("\n").first)
end
it 'uses the description of the commit as the description of the merge request' do
@@ -225,10 +239,10 @@ def stub_compare
end
context 'commit has no description' do
- let(:commits) { Commit.decorate([commit_2], project) }
+ let(:commits) { Commit.decorate([commit_3], project) }
it 'uses the title of the commit as the title of the merge request' do
- expect(merge_request.title).to eq(commit_2.safe_message)
+ expect(merge_request.title).to eq(commit_3.safe_message)
end
it 'sets the description to nil' do
@@ -257,7 +271,7 @@ def stub_compare
end
it 'uses the title of the commit as the title of the merge request' do
- expect(merge_request.title).to eq('Initial commit')
+ expect(merge_request.title).to eq('Closes #1234 Second commit')
end
it 'appends the closing description' do
@@ -310,8 +324,8 @@ def stub_compare
end
end
- context 'more than one commit in the diff' do
- let(:commits) { Commit.decorate([commit_1, commit_2], project) }
+ context 'no multi-line commit messages in the diff' do
+ let(:commits) { Commit.decorate([commit_1, commit_3], project) }
before do
stub_compare
@@ -365,6 +379,55 @@ def stub_compare
end
end
end
+ end
+
+ context 'a multi-line commit message in the diff' do
+ let(:commits) { Commit.decorate([commit_1, commit_2, commit_3], project) }
+
+ before do
+ stub_compare
+ end
+
+ it_behaves_like 'allows the merge request to be created'
+
+ it 'uses the first line of the first multi-line commit message as the title' do
+ expect(merge_request.title).to eq('Closes #1234 Second commit')
+ end
+
+ it 'adds the remaining lines of the first multi-line commit message as the description' do
+ expect(merge_request.description).to eq('Create the app')
+ end
+
+ context 'when the source branch matches an issue' do
+ where(:issue_tracker, :source_branch, :title, :closing_message) do
+ :jira | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123'
+ :jira | 'fix-issue' | 'Fix issue' | nil
+ :custom_issue_tracker | '123-fix-issue' | 'Resolve #123 "Fix issue"' | 'Closes #123'
+ :custom_issue_tracker | 'fix-issue' | 'Fix issue' | nil
+ :internal | '123-fix-issue' | 'Resolve "A bug"' | 'Closes #123'
+ :internal | 'fix-issue' | 'Fix issue' | nil
+ :internal | '124-fix-issue' | '124 fix issue' | nil
+ end
+
+ with_them do
+ before do
+ if issue_tracker == :internal
+ issue.update!(iid: 123)
+ else
+ create(:"#{issue_tracker}_service", project: project)
+ project.reload
+ end
+ end
+
+ it 'sets the correct title' do
+ expect(merge_request.title).to eq('Closes #1234 Second commit')
+ end
+
+ it 'sets the closing description' do
+ expect(merge_request.description).to eq("Create the app#{closing_message ? "\n\n" + closing_message : ''}")
+ end
+ end
+ end
context 'when the issue is not accessible to user' do
let(:source_branch) { "#{issue.iid}-fix-issue" }
@@ -373,12 +436,12 @@ def stub_compare
project.team.truncate
end
- it 'uses branch title as the merge request title' do
- expect(merge_request.title).to eq("#{issue.iid} fix issue")
+ it 'uses the first line of the first multi-line commit message as the title' do
+ expect(merge_request.title).to eq('Closes #1234 Second commit')
end
- it 'does not set a description' do
- expect(merge_request.description).to be_nil
+ it 'adds the remaining lines of the first multi-line commit message as the description' do
+ expect(merge_request.description).to eq('Create the app')
end
end
@@ -386,12 +449,12 @@ def stub_compare
let(:source_branch) { "#{issue.iid}-fix-issue" }
let(:issue_confidential) { true }
- it 'uses the title of the branch as the merge request title' do
- expect(merge_request.title).to eq("#{issue.iid} fix issue")
+ it 'uses the first line of the first multi-line commit message as the title' do
+ expect(merge_request.title).to eq('Closes #1234 Second commit')
end
- it 'does not set a description' do
- expect(merge_request.description).to be_nil
+ it 'adds the remaining lines of the first multi-line commit message as the description' do
+ expect(merge_request.description).to eq('Create the app')
end
end
end
@@ -399,7 +462,7 @@ def stub_compare
context 'source branch does not exist' do
before do
allow(project).to receive(:commit).with(source_branch).and_return(nil)
- allow(project).to receive(:commit).with(target_branch).and_return(commit_1)
+ allow(project).to receive(:commit).with(target_branch).and_return(commit_2)
end
it_behaves_like 'forbids the merge request from being created' do
@@ -409,7 +472,7 @@ def stub_compare
context 'target branch does not exist' do
before do
- allow(project).to receive(:commit).with(source_branch).and_return(commit_1)
+ allow(project).to receive(:commit).with(source_branch).and_return(commit_2)
allow(project).to receive(:commit).with(target_branch).and_return(nil)
end
@@ -433,7 +496,7 @@ def stub_compare
context 'upstream project has disabled merge requests' do
let(:upstream_project) { create(:project, :merge_requests_disabled) }
let(:project) { create(:project, forked_from_project: upstream_project) }
- let(:commits) { Commit.decorate([commit_1], project) }
+ let(:commits) { Commit.decorate([commit_2], project) }
it 'sets target project correctly' do
expect(merge_request.target_project).to eq(project)
@@ -441,8 +504,8 @@ def stub_compare
end
context 'target_project is set and accessible by current_user' do
- let(:target_project) { create(:project, :public, :repository)}
- let(:commits) { Commit.decorate([commit_1], project) }
+ let(:target_project) { create(:project, :public, :repository) }
+ let(:commits) { Commit.decorate([commit_2], project) }
it 'sets target project correctly' do
expect(merge_request.target_project).to eq(target_project)
@@ -450,8 +513,8 @@ def stub_compare
end
context 'target_project is set but not accessible by current_user' do
- let(:target_project) { create(:project, :private, :repository)}
- let(:commits) { Commit.decorate([commit_1], project) }
+ let(:target_project) { create(:project, :private, :repository) }
+ let(:commits) { Commit.decorate([commit_2], project) }
it 'sets target project correctly' do
expect(merge_request.target_project).to eq(project)
@@ -469,8 +532,8 @@ def stub_compare
end
context 'source_project is set and accessible by current_user' do
- let(:source_project) { create(:project, :public, :repository)}
- let(:commits) { Commit.decorate([commit_1], project) }
+ let(:source_project) { create(:project, :public, :repository) }
+ let(:commits) { Commit.decorate([commit_2], project) }
before do
# To create merge requests _from_ a project the user needs at least
@@ -484,8 +547,8 @@ def stub_compare
end
context 'source_project is set but not accessible by current_user' do
- let(:source_project) { create(:project, :private, :repository)}
- let(:commits) { Commit.decorate([commit_1], project) }
+ let(:source_project) { create(:project, :private, :repository) }
+ let(:commits) { Commit.decorate([commit_2], project) }
it 'sets source project correctly' do
expect(merge_request.source_project).to eq(project)
--
GitLab