Refactor commit message template evaluation

- Avoid evaluating variables that are not used in template.
- Avoid evaluating variables twice.
- Don't leave variable name in message when value is nil.

lib/gitlab/merge_requests/commit_message_generator.rb

@@ -29,48 +29,41 @@ module Gitlab
         'target_branch' => ->(merge_request, _) { merge_request.target_branch.to_s },
         'title' => ->(merge_request, _) { merge_request.title },
         'issues' => ->(merge_request, _) do
-          return "" if merge_request.visible_closing_issues_for.blank?
+          return if merge_request.visible_closing_issues_for.blank?
 
           closes_issues_references = merge_request.visible_closing_issues_for.map do |issue|
             issue.to_reference(merge_request.target_project)
           end
           "Closes #{closes_issues_references.to_sentence}"
         end,
-        'description' => ->(merge_request, _) { merge_request.description.presence || '' },
+        'description' => ->(merge_request, _) { merge_request.description },
         'reference' => ->(merge_request, _) { merge_request.to_reference(full: true) },
-        'first_commit' => -> (merge_request, _) { merge_request.first_commit&.safe_message&.strip.presence || '' },
+        'first_commit' => -> (merge_request, _) { merge_request.first_commit&.safe_message&.strip },
         'first_multiline_commit' => -> (merge_request, _) { merge_request.first_multiline_commit&.safe_message&.strip.presence || merge_request.title },
         'url' => ->(merge_request, _) { Gitlab::UrlBuilder.build(merge_request) },
         'approved_by' => ->(merge_request, _) { merge_request.approved_by_users.map { |user| "Approved-by: #{user.name} <#{user.commit_email_or_default}>" }.join("\n") },
         'merged_by' => ->(_, user) { "#{user&.name} <#{user&.commit_email_or_default}>" }
       }.freeze
 
-      PLACEHOLDERS_REGEX = Regexp.union(PLACEHOLDERS.keys.map do |key|
-        Regexp.new(Regexp.escape(key))
-      end).freeze
-
-      BLANK_PLACEHOLDERS_REGEXES = (PLACEHOLDERS.map do |key, value|
-        [key, Regexp.new("[\n\r]+%{#{Regexp.escape(key)}}$")]
-      end).to_h.freeze
+      PLACEHOLDERS_COMBINED_REGEX = /%{(#{Regexp.union(PLACEHOLDERS.keys)})}/.freeze
 
       def replace_placeholders(message)
-        # convert CRLF to LF
+        # Convert CRLF to LF.
         message = message.delete("\r")
 
-        # Remove placeholders that correspond to empty values and are the last word in the line
-        # along with all whitespace characters preceding them.
-        # This allows us to recreate previous default merge commit message behaviour - we skipped new line character
-        # before empty description and before closed issues when none were present.
-        PLACEHOLDERS.each do |key, value|
-          unless value.call(merge_request, current_user).present?
-            message = message.gsub(BLANK_PLACEHOLDERS_REGEXES[key], '')
-          end
+        used_variables = message.scan(PLACEHOLDERS_COMBINED_REGEX).map { |value| value[0] }.uniq
+        values = used_variables.to_h do |variable_name|
+          ["%{#{variable_name}}", PLACEHOLDERS[variable_name].call(merge_request, current_user)]
         end
+        names_of_empty_variables = values.filter_map { |name, value| name if value.blank? }
 
-        Gitlab::StringPlaceholderReplacer
-          .replace_string_placeholders(message, PLACEHOLDERS_REGEX) do |key|
-          PLACEHOLDERS[key].call(merge_request, current_user)
-        end
+        # 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?
+        # Substitute all variables with their values.
+        message = message.gsub(Regexp.union(values.keys), values) if values.present?
+
+        message
       end
     end
   end

spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb

@@ -58,6 +58,19 @@ RSpec.describe Gitlab::MergeRequests::CommitMessageGenerator do
       end
     end
 
+    context 'when project has commit template with only the title' do
+      let(:merge_request) do
+        double(:merge_request, title: 'Fixes', target_project: project, to_reference: '!123', metrics: nil, merge_user: nil)
+      end
+
+      let(message_template_name) { '%{title}' }
+
+      it 'evaluates only necessary variables' do
+        expect(result_message).to eq 'Fixes'
+        expect(merge_request).not_to have_received(:to_reference)
+      end
+    end
+
     context 'when project has commit template with closed issues' do
       let(message_template_name) { <<~MSG.rstrip }
         Merge branch '%{source_branch}' into '%{target_branch}'
@@ -381,6 +394,57 @@ RSpec.describe Gitlab::MergeRequests::CommitMessageGenerator do
         end
       end
     end
+
+    context 'when project has commit template with the same variable used twice' do
+      let(message_template_name) { '%{title} %{title}' }
+
+      it 'uses custom template' do
+        expect(result_message).to eq 'Bugfix Bugfix'
+      end
+    end
+
+    context 'when project has commit template without any variable' do
+      let(message_template_name) { 'static text' }
+
+      it 'uses custom template' do
+        expect(result_message).to eq 'static text'
+      end
+    end
+
+    context 'when project has template with all variables' do
+      let(message_template_name) { <<~MSG.rstrip }
+        source_branch:%{source_branch}
+        target_branch:%{target_branch}
+        title:%{title}
+        issues:%{issues}
+        description:%{description}
+        first_commit:%{first_commit}
+        first_multiline_commit:%{first_multiline_commit}
+        url:%{url}
+        approved_by:%{approved_by}
+        merged_by:%{merged_by}
+      MSG
+
+      it 'uses custom template' do
+        expect(result_message).to eq <<~MSG.rstrip
+          source_branch:feature
+          target_branch:master
+          title:Bugfix
+          issues:
+          description:Merge Request Description
+          Next line
+          first_commit:Feature added
+
+          Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
+          first_multiline_commit:Feature added
+
+          Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
+          url:#{Gitlab::UrlBuilder.build(merge_request)}
+          approved_by:
+          merged_by:#{maintainer.name} <#{maintainer.commit_email_or_default}>
+        MSG
+      end
+    end
   end
 
   describe '#merge_message' do

spec/models/merge_request_spec.rb

@@ -1647,13 +1647,10 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
 
     it 'uses template from target project' do
-      subject.title = 'Fix everything'
-      subject.compare_commits = [
-        double(safe_message: 'Commit message', gitaly_commit?: true, merge_commit?: false, description?: false)
-      ]
-      subject.target_project.merge_commit_template = '%{title}'
+      request = build(:merge_request, title: 'Fix everything')
+      request.target_project.merge_commit_template = '%{title}'
 
-      expect(subject.default_merge_commit_message)
+      expect(request.default_merge_commit_message)
         .to eq('Fix everything')
     end