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.

app/controllers/concerns/renders_commits.rb

 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
 

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.

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

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

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

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)

app/views/projects/commits/_commit_list.html.haml

-- 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|

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 } }

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

changelogs/unreleased/sh-limit-commit-renderering.yml

+---
+title: Speed up diff comparisons by limiting number of commit messages rendered
+merge_request: 21335
+author:
+type: performance

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

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

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