Cache the commit partials in MRs

This caches the rendered commit partials in MRs, using a
helper to calculate the required cache key. In the MR context
we don't need to cover all aspects of the view.

app/helpers/commits_helper.rb

@@ -148,6 +148,23 @@ module CommitsHelper
     end
   end
 
+  # This is used to calculate a cache key for the app/views/projects/commits/_commit.html.haml
+  # partial. It takes some of the same parameters as used in the partial and will hash the
+  # current pipeline status.
+  #
+  # This is currently configured only for use on the merge requests commits tab, and will
+  # require expansion for use elsewhere.
+  def commit_partial_cache_key(commit, ref:, request:)
+    [
+      commit,
+      commit.author,
+      ref,
+      hashed_pipeline_status(commit, ref),
+      { xhr: request.xhr? },
+      @path # referred to in #link_to_browse_code
+    ]
+  end
+
   protected
 
   # Private: Returns a link to a person. If the person has a matching user and
@@ -221,4 +238,14 @@ module CommitsHelper
       project_commit_path(project, commit)
     end
   end
+
+  private
+
+  def hashed_pipeline_status(commit, ref)
+    status = commit.status_for(ref)
+
+    return if status.nil?
+
+    Digest::SHA1.hexdigest(status.to_s)
+  end
 end

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

@@ -4,17 +4,15 @@
 
   EXCEPTION WARNING - see above `.vue` file for de-sync drift
 -#-----------------------------------------------------------------
-- view_details  = local_assigns.fetch(:view_details, false)
-- merge_request = local_assigns.fetch(:merge_request, nil)
-- project       = local_assigns.fetch(:project) { merge_request&.project }
-- ref           = local_assigns.fetch(:ref) { merge_request&.source_branch }
-- commit        = commit.present(current_user: current_user)
-- commit_status = commit.status_for(ref)
-- collapsible = local_assigns.fetch(:collapsible, true)
-- link_data_attrs = local_assigns.fetch(:link_data_attrs, {})
-
-- link = commit_path(project, commit, merge_request: merge_request)
-
+- view_details      = local_assigns.fetch(:view_details, false)
+- merge_request     = local_assigns.fetch(:merge_request, nil)
+- project           = local_assigns.fetch(:project) { merge_request&.project }
+- ref               = local_assigns.fetch(:ref) { merge_request&.source_branch }
+- commit            = commit.present(current_user: current_user)
+- commit_status     = commit.status_for(ref)
+- collapsible       = local_assigns.fetch(:collapsible, true)
+- link_data_attrs   = local_assigns.fetch(:link_data_attrs, {})
+- link              = commit_path(project, commit, merge_request: merge_request)
 - show_project_name = local_assigns.fetch(:show_project_name, false)
 
 %li{ class: ["commit flex-row", ("js-toggle-container" if collapsible)], id: "commit-#{commit.short_id}" }

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

@@ -3,8 +3,8 @@
 - ref = local_assigns.fetch(:ref) { merge_request&.source_branch }
 - can_update_merge_request = can?(current_user, :update_merge_request, @merge_request)
 
-- commits = @commits
-- context_commits = @context_commits
+- commits = @commits&.map { |commit| commit.present(current_user: current_user) }
+- context_commits = @context_commits&.map { |commit| commit.present(current_user: current_user) }
 - hidden = @hidden_commit_count
 
 - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, daily_commits|
@@ -14,7 +14,10 @@
 
   %li.commits-row{ data: { day: day } }
     %ul.content-list.commit-list.flex-list
-      = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }
+      - if Feature.enabled?(:cached_mr_commits, project, default_enabled: :yaml)
+        = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, request: request) }, expires_in: 1.day
+      - else
+        = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }
 
 - if context_commits.present?
   %li.commit-header.js-commit-header
@@ -25,7 +28,10 @@
 
   %li.commits-row
     %ul.content-list.commit-list.flex-list
-      = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }
+      - if Feature.enabled?(:cached_mr_commits, project, default_enabled: :yaml)
+        = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, request: request) }, expires_in: 1.day
+      - else
+        = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }
 
 - if hidden > 0
   %li.gl-alert.gl-alert-warning

config/feature_flags/development/cached_mr_commits.yml

+---
+name: cached_mr_commits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61617
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330968
+milestone: '13.12'
+type: development
+group: group::source code
+default_enabled: false

spec/helpers/commits_helper_spec.rb

@@ -289,4 +289,28 @@ RSpec.describe CommitsHelper do
       }
     end
   end
+
+  describe "#commit_partial_cache_key" do
+    subject { helper.commit_partial_cache_key(commit, ref: ref, request: request) }
+
+    let(:commit) { create(:commit).present(current_user: user) }
+    let(:commit_status) { create(:commit_status) }
+    let(:user) { create(:user) }
+    let(:ref) { "master" }
+    let(:request) { double(xhr?: true) }
+    let(:current_path) { "test" }
+
+    before do
+      expect(commit).to receive(:status_for).with(ref).and_return(commit_status)
+      assign(:path, current_path)
+    end
+
+    it { is_expected.to be_an(Array) }
+    it { is_expected.to include(commit) }
+    it { is_expected.to include(commit.author) }
+    it { is_expected.to include(ref) }
+    it { is_expected.to include({ xhr: true }) }
+    it { is_expected.to include(Digest::SHA1.hexdigest(commit_status.to_s)) }
+    it { is_expected.to include(current_path) }
+  end
 end