Merge branch 'merge-commit-template-tweak-blank-line-removal' into 'master'

Merge/squash commit templates: avoid breaking Git trailers

See merge request gitlab-org/gitlab!77758

doc/user/project/merge_requests/commit_templates.md

@@ -87,7 +87,8 @@ Commit message templates support these variables:
 | `%{merged_by}`     | User who merged the merge request. | `Alex Garcia <agarcia@example.com>` |
 | `%{co_authored_by}` | Names and emails of commit authors in a `Co-authored-by` Git commit trailer format. Limited to authors of 100 most recent commits in merge request. | `Co-authored-by: Zane Doe <zdoe@example.com>` <br> `Co-authored-by: Blake Smith <bsmith@example.com>` |
 
-Empty variables that are the only word in a line are removed, along with all newline characters preceding it.  
+Any line containing only an empty variable is removed. If the line to be removed is both
+preceded and followed by an empty line, the preceding empty line is also removed. 
 
 ## Related topics
 

lib/gitlab/merge_requests/commit_message_generator.rb

@@ -65,9 +65,13 @@ module Gitlab
         end
         names_of_empty_variables = values.filter_map { |name, value| name if value.blank? }
 
-        # Remove placeholders that correspond to empty values and are the only word in a line
-        # along with all whitespace characters preceding them.
-        message = message.gsub(/[\n\r]+#{Regexp.union(names_of_empty_variables)}$/, '') if names_of_empty_variables.present?
+        # Remove lines that contain empty variable placeholder and nothing else.
+        if names_of_empty_variables.present?
+          # If there is blank line or EOF after it, remove blank line before it as well.
+          message = message.gsub(/\n\n#{Regexp.union(names_of_empty_variables)}(\n\n|\Z)/, '\1')
+          # Otherwise, remove only the line it is in.
+          message = message.gsub(/^#{Regexp.union(names_of_empty_variables)}\n/, '')
+        end
         # Substitute all variables with their values.
         message = message.gsub(Regexp.union(values.keys), values) if values.present?
 

spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb

@@ -292,45 +292,84 @@ RSpec.describe Gitlab::MergeRequests::CommitMessageGenerator do
     context 'when project has merge commit template with approvers' do
       let(:user1) { create(:user) }
       let(:user2) { create(:user) }
-      let(message_template_name) do
-        "Merge Request approved by:\n%{approved_by}"
-      end
+      let(message_template_name) { <<~MSG.rstrip }
+        Merge branch '%{source_branch}' into '%{target_branch}'
 
-      context "and mr has no approval" do
+        %{approved_by}
+      MSG
+
+      context 'and mr has no approval' do
         before do
           merge_request.approved_by_users = []
         end
 
-        it "returns empty string" do
+        it 'removes variable and blank line' do
           expect(result_message).to eq <<~MSG.rstrip
-          Merge Request approved by:
+            Merge branch 'feature' into 'master'
           MSG
         end
+
+        context 'when there is blank line after approved_by' do
+          let(message_template_name) { <<~MSG.rstrip }
+            Merge branch '%{source_branch}' into '%{target_branch}'
+
+            %{approved_by}
+
+            Type: merge
+          MSG
+
+          it 'removes blank line before it' do
+            expect(result_message).to eq <<~MSG.rstrip
+              Merge branch 'feature' into 'master'
+
+              Type: merge
+            MSG
+          end
+        end
+
+        context 'when there is no blank line after approved_by' do
+          let(message_template_name) { <<~MSG.rstrip }
+            Merge branch '%{source_branch}' into '%{target_branch}'
+
+            %{approved_by}
+            Type: merge
+          MSG
+
+          it 'does not remove blank line before it' do
+            expect(result_message).to eq <<~MSG.rstrip
+              Merge branch 'feature' into 'master'
+
+              Type: merge
+            MSG
+          end
+        end
       end
 
-      context "and mr has one approval" do
+      context 'and mr has one approval' do
         before do
           merge_request.approved_by_users = [user1]
         end
 
-        it "returns user name and email" do
+        it 'returns user name and email' do
           expect(result_message).to eq <<~MSG.rstrip
-          Merge Request approved by:
-          Approved-by: #{user1.name} <#{user1.email}>
+            Merge branch 'feature' into 'master'
+
+            Approved-by: #{user1.name} <#{user1.email}>
           MSG
         end
       end
 
-      context "and mr has multiple approvals" do
+      context 'and mr has multiple approvals' do
         before do
           merge_request.approved_by_users = [user1, user2]
         end
 
-        it "returns users names and emails" do
+        it 'returns users names and emails' do
           expect(result_message).to eq <<~MSG.rstrip
-          Merge Request approved by:
-          Approved-by: #{user1.name} <#{user1.email}>
-          Approved-by: #{user2.name} <#{user2.email}>
+            Merge branch 'feature' into 'master'
+
+            Approved-by: #{user1.name} <#{user1.email}>
+            Approved-by: #{user2.name} <#{user2.email}>
           MSG
         end
       end