Merge/squash commit templates: avoid breaking Git trailers

This change tweaks blank line removal regex to avoid removing
blank lines that could break git trailers.
Now we will only remove blank line before an empty variable
when it is also followed by a blank line or EOF.
That way we won't break formatting of the message.

Changelog: fixed

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