Speed up diff comparisons by limiting number of commit messages rendered

When a diff has a significant number of commits, the previous behavior would
attempt to render the Markdown on all the commit messages but only display
1000 of them. To avoid additional work, we only need to render the Markdown
on the set that is displayed.

13 files changed, 112 insertions, 19 deletions

diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb
index fb41dc1e8a8..b1c9b1e532f 100644
--- a/app/controllers/concerns/renders_commits.rb
+++ b/app/controllers/concerns/renders_commits.rb
@@ -1,4 +1,24 @@
 module RendersCommits
+  def limited_commits(commits)
+    if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
+      [
+        commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE),
+        commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE
+      ]
+    else
+      [commits, 0]
+    end
+  end
+
+  # This is used as a helper method in a controller.
+  # rubocop: disable Gitlab/ModuleWithInstanceVariables
+  def set_commits_for_rendering(commits)
+    @total_commit_count = commits.size
+    limited, @hidden_commit_count = limited_commits(commits)
+    prepare_commits_for_rendering(limited)
+  end
+  # rubocop: enable Gitlab/ModuleWithInstanceVariables
+
   def prepare_commits_for_rendering(commits)
     Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb
index 36faea8056e..5546bef850b 100644
--- a/app/controllers/projects/commits_controller.rb
+++ b/app/controllers/projects/commits_controller.rb
@@ -63,7 +63,7 @@ class Projects::CommitsController < Projects::ApplicationController
       end
 
     @commits = @commits.with_pipeline_status
-    @commits = prepare_commits_for_rendering(@commits)
+    @commits = set_commits_for_rendering(@commits)
   end
 
   # Rails 5 sets request.format from the extension.
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index f93e500a07a..a1e12821caf 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -78,7 +78,7 @@ class Projects::CompareController < Projects::ApplicationController
   end
 
   def define_commits
-    @commits = compare.present? ? prepare_commits_for_rendering(compare.commits) : []
+    @commits = compare.present? ? set_commits_for_rendering(@compare.commits) : []
   end
 
   def define_diffs
diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index 81129456ad8..03d0290ac1d 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/app/controllers/projects/merge_requests/creations_controller.rb
@@ -101,7 +101,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
     @target_project = @merge_request.target_project
     @source_project = @merge_request.source_project
-    @commits = prepare_commits_for_rendering(@merge_request.commits)
+    @commits = set_commits_for_rendering(@merge_request.commits)
     @commit = @merge_request.diff_head_commit
 
     @labels = LabelsFinder.new(current_user, project_id: @project.id).execute
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 1b069fe507b..d31b58972ca 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -79,7 +79,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     # Get commits from repository
     # or from cache if already merged
     @commits =
-      prepare_commits_for_rendering(@merge_request.commits.with_pipeline_status)
+      set_commits_for_rendering(@merge_request.commits.with_pipeline_status)
 
     render json: { html: view_to_html_string('projects/merge_requests/_commits') }
   end
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index 89fe90fd801..7a942c44ac4 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -210,17 +210,6 @@ module CommitsHelper
     Sanitize.clean(string, remove_contents: true)
   end
 
-  def limited_commits(commits)
-    if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
-      [
-        commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE),
-        commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE
-      ]
-    else
-      [commits, 0]
-    end
-  end
-
   def commit_path(project, commit, merge_request: nil)
     if merge_request&.persisted?
       diffs_project_merge_request_path(project, merge_request, commit_id: commit.id)
diff --git a/app/views/projects/commits/_commit_list.html.haml b/app/views/projects/commits/_commit_list.html.haml
index 8f8eb2c3d5a..6ed65d07202 100644
--- a/app/views/projects/commits/_commit_list.html.haml
+++ b/app/views/projects/commits/_commit_list.html.haml
@@ -1,9 +1,10 @@
-- commits, hidden = limited_commits(@commits)
+- commits = @commits
+- hidden = @hidden_commit_count
 - commits = Commit.decorate(commits, @project)
 
 .card
   .card-header
-    Commits (#{@commits.count})
+    Commits (#{@total_commit_count})
   - if hidden > 0
     %ul.content-list
       - commits.each do |commit|
diff --git a/app/views/projects/commits/_commits.html.haml b/app/views/projects/commits/_commits.html.haml
index ac6852751be..ec05ff50f25 100644
--- a/app/views/projects/commits/_commits.html.haml
+++ b/app/views/projects/commits/_commits.html.haml
@@ -2,7 +2,8 @@
 - project = local_assigns.fetch(:project) { merge_request&.project }
 - ref = local_assigns.fetch(:ref) { merge_request&.source_branch }
 
-- commits, hidden = limited_commits(@commits)
+- commits = @commits
+- hidden = @hidden_commit_count
 
 - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits|
   %li.commit-header.js-commit-header{ data: { day: day } }
diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml
index f7a5d85500f..d5c4134dee2 100644
--- a/app/views/projects/merge_requests/creations/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml
@@ -33,7 +33,7 @@
             %li.commits-tab.new-tab
               = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tabvue'} do
                 Commits
-                %span.badge.badge-pill= @commits.size
+                %span.badge.badge-pill= @total_commit_count
             - if @pipelines.any?
               %li.builds-tab
                 = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tabvue'} do
diff --git a/changelogs/unreleased/sh-limit-commit-renderering.yml b/changelogs/unreleased/sh-limit-commit-renderering.yml
new file mode 100644
index 00000000000..c44c67bcc90
--- /dev/null
+++ b/changelogs/unreleased/sh-limit-commit-renderering.yml
@@ -0,0 +1,5 @@
+---
+title: Speed up diff comparisons by limiting number of commit messages rendered
+merge_request: 21335
+author:
+type: performance
diff --git a/spec/controllers/projects/merge_requests/creations_controller_spec.rb b/spec/controllers/projects/merge_requests/creations_controller_spec.rb
index d8995f98575..f8c37c0a676 100644
--- a/spec/controllers/projects/merge_requests/creations_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests/creations_controller_spec.rb
@@ -29,6 +29,55 @@ describe Projects::MergeRequests::CreationsController do
         expect(response).to be_success
       end
     end
+
+    context 'merge request with some commits' do
+      render_views
+
+      let(:large_diff_params) do
+        {
+          namespace_id: fork_project.namespace.to_param,
+          project_id: fork_project,
+          merge_request: {
+            source_branch: 'master',
+            target_branch: 'fix'
+          }
+        }
+      end
+
+      describe 'with artificial limits' do
+        before do
+          # Load MergeRequestdiff so stub_const won't override it with its own definition
+          # See https://github.com/rspec/rspec-mocks/issues/1079
+          stub_const("#{MergeRequestDiff}::COMMITS_SAFE_SIZE", 2)
+        end
+
+        it 'limits total commits' do
+          get :new, large_diff_params
+
+          expect(response).to be_success
+
+          total = assigns(:total_commit_count)
+          expect(assigns(:commits)).to be_an Array
+          expect(total).to be > 0
+          expect(assigns(:hidden_commit_count)).to be > 0
+          expect(response).to have_gitlab_http_status(200)
+          expect(response.body).to match %r(<span class="commits-count">2 commits</span>)
+        end
+      end
+
+      it 'shows total commits' do
+        get :new, large_diff_params
+
+        expect(response).to be_success
+
+        total = assigns(:total_commit_count)
+        expect(assigns(:commits)).to be_an Array
+        expect(total).to be > 0
+        expect(assigns(:hidden_commit_count)).to eq(0)
+        expect(response).to have_gitlab_http_status(200)
+        expect(response.body).to match %r(<span class="commits-count">#{total} commits</span>)
+      end
+    end
   end
 
   describe 'GET diffs' do
diff --git a/spec/views/projects/merge_requests/_commits.html.haml_spec.rb b/spec/views/projects/merge_requests/_commits.html.haml_spec.rb
index b1c6565c08a..a7628548de6 100644
--- a/spec/views/projects/merge_requests/_commits.html.haml_spec.rb
+++ b/spec/views/projects/merge_requests/_commits.html.haml_spec.rb
@@ -20,6 +20,7 @@ describe 'projects/merge_requests/_commits.html.haml' do
 
     assign(:merge_request, merge_request)
     assign(:commits, merge_request.commits)
+    assign(:hidden_commit_count, 0)
   end
 
   it 'shows commits from source project' do
@@ -30,4 +31,16 @@ describe 'projects/merge_requests/_commits.html.haml' do
 
     expect(rendered).to have_link(href: href)
   end
+
+  context 'when there are hidden commits' do
+    before do
+      assign(:hidden_commit_count, 1)
+    end
+
+    it 'shows notice about omitted commits' do
+      render
+
+      expect(rendered).to match(/1 additional commit has been omitted to prevent performance issues/)
+    end
+  end
 end
diff --git a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
index 9ab105c3238..8befae39d3a 100644
--- a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
+++ b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
@@ -9,6 +9,8 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
 
     assign(:merge_request, merge_request)
     assign(:commits, merge_request.commits)
+    assign(:hidden_commit_count, 0)
+    assign(:total_commit_count, merge_request.commits.count)
     assign(:project, merge_request.target_project)
 
     allow(view).to receive(:can?).and_return(true)
@@ -29,4 +31,17 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
       expect(rendered).not_to have_text('Builds')
     end
   end
+
+  context 'when there are hidden commits' do
+    before do
+      assign(:pipelines, Ci::Pipeline.none)
+      assign(:hidden_commit_count, 2)
+    end
+
+    it 'shows notice about omitted commits' do
+      render
+
+      expect(rendered).to match(/2 additional commits have been omitted to prevent performance issues/)
+    end
+  end
 end