From 0d741c84bc221e048228bd66af96eb81ca8eb97c Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Thu, 8 Aug 2024 21:58:03 +1000
Subject: [PATCH 1/7] Try diff_blobs for streaming

---
 .../merge_requests/diffs_stream_controller.rb | 69 +++++++++++++++++--
 lib/gitlab/git/changed_path.rb                |  5 +-
 lib/gitlab/git/repository.rb                  |  4 +-
 lib/gitlab/gitaly_client/commit_service.rb    | 11 +--
 4 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/app/controllers/projects/merge_requests/diffs_stream_controller.rb b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
index 7a824f96f79b73..51e356a8a0e9f7 100644
--- a/app/controllers/projects/merge_requests/diffs_stream_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
@@ -7,6 +7,7 @@ class DiffsStreamController < Projects::MergeRequests::ApplicationController
 
       urgency :low, [:diffs]
 
+      # rubocop: disable Metrics/AbcSize -- WIP
       def diffs
         return render_404 unless ::Feature.enabled?(:rapid_diffs, current_user, type: :wip)
 
@@ -15,13 +16,68 @@ def diffs
         offset = params[:offset].to_i
         view = helpers.diff_view
 
-        @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff|
-          response.stream.write(
-            render_to_string(
-              ::RapidDiffs::DiffFileComponent.new(diff_file: diff, parallel_view: view == :parallel),
-              layout: false
+        if params[:diff_blobs]
+          changed_paths = project
+            .repository
+            .find_changed_paths([
+              Gitlab::Git::DiffTree.new(
+                @merge_request.diff_refs.base_sha,
+                @merge_request.diff_refs.head_sha
+              )
+            ], find_renames: true)
+
+          changed_paths.each_slice(50) do |changed_paths_slice|
+            blob_ids = changed_paths_slice.map do |changed_path|
+              Gitaly::DiffBlobsRequest::BlobPair.new(
+                left_blob: changed_path.old_blob_id,
+                right_blob: changed_path.new_blob_id
+              )
+            end
+
+            diff_blobs = project
+              .repository
+              .diff_blobs(blob_ids, patch_bytes_limit: Gitlab::Git::Diff.patch_hard_limit_bytes)
+
+            diff_blobs.each do |blob|
+              changed_path = changed_paths_slice.find do |cp|
+                blob.left_blob_id == cp.old_blob_id && blob.right_blob_id == cp.new_blob_id
+              end
+
+              diff_attrs = {
+                diff: blob.patch,
+                new_path: changed_path.path,
+                old_path: changed_path.old_path,
+                a_mode: changed_path.old_mode,
+                b_mode: changed_path.new_mode,
+                new_file: changed_path.status == :ADDED,
+                renamed_file: changed_path.status == :RENAMED,
+                deleted_file: changed_path.status == :DELETED,
+                too_large: blob.over_patch_bytes_limit
+              }
+
+              diff = Gitlab::Git::Diff.new(diff_attrs)
+              diff_file = Gitlab::Diff::File.new(
+                diff,
+                repository: project.repository,
+                diff_refs: @merge_request.diff_refs
+              )
+
+              response.stream.write(
+                render_to_string(
+                  ::RapidDiffs::DiffFileComponent.new(diff_file: diff_file),
+                  layout: false
+                )
+              )
+            end
+          end
+        else
+          @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff|
+            response.stream.write(
+              render_to_string(
+                ::RapidDiffs::DiffFileComponent.new(diff_file: diff, parallel_view: view == :parallel),
+                layout: false
             )
-          )
+          end
         end
       rescue StandardError => e
         response.stream.write e.message
@@ -29,5 +85,6 @@ def diffs
         response.stream.close
       end
     end
+    # rubocop: enable Metrics/AbcSize
   end
 end
diff --git a/lib/gitlab/git/changed_path.rb b/lib/gitlab/git/changed_path.rb
index 6883a35ff35168..b7b48bbe7d5121 100644
--- a/lib/gitlab/git/changed_path.rb
+++ b/lib/gitlab/git/changed_path.rb
@@ -3,15 +3,16 @@
 module Gitlab
   module Git
     class ChangedPath
-      attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id
+      attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id, :old_path
 
-      def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_id: nil)
+      def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_id: nil, old_path: nil)
         @status = status
         @path = path
         @old_mode = old_mode
         @new_mode = new_mode
         @old_blob_id = old_blob_id
         @new_blob_id = new_blob_id
+        @old_path = old_path
       end
 
       def new_file?
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 4d3df2dc0a9532..6f08cfd455c8c1 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -536,13 +536,13 @@ def diff_stats(left_id, right_id)
         empty_diff_stats
       end
 
-      def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil)
+      def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil, find_renames: false)
         processed_objects = treeish_objects.compact
 
         return [] if processed_objects.empty?
 
         wrapped_gitaly_errors do
-          gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode)
+          gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode, find_renames: find_renames)
         end
       rescue CommandError, TypeError, NoRepository
         []
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 34a6de540418eb..7870a05fb2a1ea 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -251,8 +251,8 @@ def diff_stats(left_commit_sha, right_commit_sha)
       # else # defaults to :include_merges behavior
       #   ['foo_bar.rb', 'bar_baz.rb'],
       #
-      def find_changed_paths(objects, merge_commit_diff_mode: nil)
-        request = find_changed_paths_request(objects, merge_commit_diff_mode)
+      def find_changed_paths(objects, merge_commit_diff_mode: nil, find_renames: false)
+        request = find_changed_paths_request(objects, merge_commit_diff_mode, find_renames)
 
         return [] if request.nil?
 
@@ -265,7 +265,8 @@ def find_changed_paths(objects, merge_commit_diff_mode: nil)
               old_mode: path.old_mode.to_s(8),
               new_mode: path.new_mode.to_s(8),
               old_blob_id: path.old_blob_id,
-              new_blob_id: path.new_blob_id
+              new_blob_id: path.new_blob_id,
+              old_path: EncodingHelper.encode!(path.old_path)
             )
           end
         end
@@ -647,7 +648,7 @@ def call_find_commit(revision)
         response.commit
       end
 
-      def find_changed_paths_request(objects, merge_commit_diff_mode)
+      def find_changed_paths_request(objects, merge_commit_diff_mode, find_renames)
         diff_mode = MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] if Feature.enabled?(:merge_commit_diff_modes)
 
         requests = objects.filter_map do |object|
@@ -667,7 +668,7 @@ def find_changed_paths_request(objects, merge_commit_diff_mode)
 
         return if requests.blank?
 
-        Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: requests, merge_commit_diff_mode: diff_mode)
+        Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: requests, merge_commit_diff_mode: diff_mode, find_renames: find_renames)
       end
 
       def path_error_message(path_error)
-- 
GitLab


From b6b5ba802c9f13e1086731f55f6821b87ec136b9 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Fri, 16 Aug 2024 14:47:40 +1000
Subject: [PATCH 2/7] Use path when old_path is blank

---
 lib/gitlab/git/changed_path.rb             | 7 ++++++-
 lib/gitlab/gitaly_client/commit_service.rb | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/git/changed_path.rb b/lib/gitlab/git/changed_path.rb
index b7b48bbe7d5121..9ee65653c56324 100644
--- a/lib/gitlab/git/changed_path.rb
+++ b/lib/gitlab/git/changed_path.rb
@@ -3,7 +3,7 @@
 module Gitlab
   module Git
     class ChangedPath
-      attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id, :old_path
+      attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id
 
       def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_id: nil, old_path: nil)
         @status = status
@@ -19,6 +19,11 @@ def new_file?
         status == :ADDED
       end
 
+      # NOTE: old_path may not always be provided so fallback to the path
+      def old_path
+        @old_path.presence || @path
+      end
+
       def submodule_change?
         # The file mode 160000 represents a "Gitlink" or a git submodule.
         # The first two digits can be used to distinguish it from regular files.
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 7870a05fb2a1ea..c7285f14baa5c7 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -262,11 +262,11 @@ def find_changed_paths(objects, merge_commit_diff_mode: nil, find_renames: false
             Gitlab::Git::ChangedPath.new(
               status: path.status,
               path: EncodingHelper.encode!(path.path),
+              old_path: EncodingHelper.encode!(path.old_path),
               old_mode: path.old_mode.to_s(8),
               new_mode: path.new_mode.to_s(8),
               old_blob_id: path.old_blob_id,
-              new_blob_id: path.new_blob_id,
-              old_path: EncodingHelper.encode!(path.old_path)
+              new_blob_id: path.new_blob_id
             )
           end
         end
-- 
GitLab


From 4cf3b5b7f6fa6f60f3881a63eb4d8bab7c47016f Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Wed, 21 Aug 2024 13:41:30 +1000
Subject: [PATCH 3/7] Fix fails specs and add find_renames specs

---
 spec/lib/gitlab/git/repository_spec.rb        | 42 +++++---
 .../gitaly_client/commit_service_spec.rb      | 95 +++++++++++++++----
 2 files changed, 107 insertions(+), 30 deletions(-)

diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 36b51de7060c37..6421750b45e377 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1740,7 +1740,7 @@ def create_commit(blobs)
     let(:commit_1_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "files/executables/ls", old_mode: "0", new_mode: "100755",
+          status: :ADDED, path: "files/executables/ls", old_mode: "0", new_mode: "100755", old_path: '',
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: 'c84acd1ff0b844201312052f9bb3b7259eb2e177'
         )
       ]
@@ -1749,7 +1749,7 @@ def create_commit(blobs)
     let(:commit_2_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "bar/branch-test.txt", old_mode: "0", new_mode: "100644",
+          status: :ADDED, path: "bar/branch-test.txt", old_mode: "0", new_mode: "100644", old_path: '',
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '93e123ac8a3e6a0b600953d7598af629dec7b735'
         )
       ]
@@ -1758,11 +1758,11 @@ def create_commit(blobs)
     let(:commit_3_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :MODIFIED, path: ".gitmodules", old_mode: "100644", new_mode: "100644",
+          status: :MODIFIED, path: ".gitmodules", old_mode: "100644", new_mode: "100644", old_path: '',
           old_blob_id: 'fdaada1754989978413d618ee1fb1c0469d6a664', new_blob_id: '0792c58905eff3432b721f8c4a64363d8e28d9ae'
         ),
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "gitlab-shell", old_mode: "0", new_mode: "160000",
+          status: :ADDED, path: "gitlab-shell", old_mode: "0", new_mode: "160000", old_path: '',
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '79bceae69cb5750d6567b223597999bfa91cb3b9'
         )
       ]
@@ -1771,15 +1771,15 @@ def create_commit(blobs)
     let(:initial_commit_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: ".gitignore", old_mode: "0", new_mode: "100644",
+          status: :ADDED, path: ".gitignore", old_mode: "0", new_mode: "100644", old_path: '',
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '470ad2fcf1e33798f1afc5781d08e60c40f51e7a'
         ),
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "LICENSE", old_mode: "0", new_mode: "100644",
+          status: :ADDED, path: "LICENSE", old_mode: "0", new_mode: "100644", old_path: '',
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '50b27c6518be44c42c4d87966ae2481ce895624c'
         ),
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "README.md", old_mode: "0", new_mode: "100644",
+          status: :ADDED, path: "README.md", old_mode: "0", new_mode: "100644", old_path: '',
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: 'faaf198af3a36dbf41961466703cc1d47c61d051'
         )
       ]
@@ -1816,7 +1816,7 @@ def create_commit(blobs)
       expect(collection.to_a).to be_empty
     end
 
-    describe 'merge_commit_diff_mode argument' do
+    describe 'optional arguments' do
       let(:gitaly_commit_client) { double('Gitlab::GitalyClient::CommitService') }
 
       before do
@@ -1829,24 +1829,38 @@ def create_commit(blobs)
           repository.find_changed_paths(['sha'])
         end
 
-        it 'defaults to nil' do
+        it 'returns default values' do
           expect(gitaly_commit_client)
             .to have_received(:find_changed_paths)
-            .with(['sha'], merge_commit_diff_mode: nil)
+            .with(['sha'], merge_commit_diff_mode: nil, find_renames: false)
         end
       end
 
-      context 'when included' do
-        let(:passed_value) { 'foobar' }
+      context 'merge_commit_diff_mode is given' do
+        let(:merge_commit_diff_mode) { 'foobar' }
 
         before do
-          repository.find_changed_paths(['sha'], merge_commit_diff_mode: passed_value)
+          repository.find_changed_paths(['sha'], merge_commit_diff_mode: merge_commit_diff_mode)
         end
 
         it 'passes the value on to the commit client' do
           expect(gitaly_commit_client)
             .to have_received(:find_changed_paths)
-            .with(['sha'], merge_commit_diff_mode: passed_value)
+            .with(['sha'], merge_commit_diff_mode: merge_commit_diff_mode, find_renames: false)
+        end
+      end
+
+      context 'find_names is included' do
+        let(:find_renames) { true }
+
+        before do
+          repository.find_changed_paths(['sha'], find_renames: find_renames)
+        end
+
+        it 'passes the value on to the commit client' do
+          expect(gitaly_commit_client)
+            .to have_received(:find_changed_paths)
+            .with(['sha'], merge_commit_diff_mode: nil, find_renames: find_renames)
         end
       end
     end
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index 9f212598310a4f..17f8be07d53d2b 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -169,6 +169,7 @@
 
   describe '#find_changed_paths' do
     let(:mapped_merge_commit_diff_mode) { described_class::MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] }
+    let(:find_renames) { false }
     let(:commits) do
       %w[
         ade1c0b4b116209ed2a9958436b26f89085ec383
@@ -204,47 +205,47 @@
       let(:changed_paths) do
         [
           {
-            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
           },
           {
-            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644',
+            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
             old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
             new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1"
           },
           {
-            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644',
+            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
             old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
             new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644',
+            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
             old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1",
             new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644',
+            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
             old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1",
             new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           }
@@ -260,32 +261,32 @@
       let(:changed_paths) do
         [
           {
-            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
           },
           {
-            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644',
+            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
             old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
             new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644',
+            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
             old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1",
             new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           }
@@ -304,7 +305,8 @@
           .to have_received(:new).with(
             repository: repository_message,
             requests: requests,
-            merge_commit_diff_mode: mapped_merge_commit_diff_mode
+            merge_commit_diff_mode: mapped_merge_commit_diff_mode,
+            find_renames: find_renames
           )
       end
     end
@@ -397,6 +399,67 @@
       end
     end
 
+    context 'when renamed file exists' do
+      let(:branch) { 'gitaly-rename-test' }
+      let(:treeish_objects) { [repository.commit(branch)] }
+
+      subject(:find_changed_paths) do
+        described_class
+          .new(repository)
+          .find_changed_paths(treeish_objects, find_renames: find_renames)
+          .as_json
+      end
+
+      context 'when find_renames is true' do
+        let(:find_renames) { true }
+
+        it 'detects renamed file and includes old_path' do
+          expected_changed_paths = [
+            {
+              "new_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb",
+              "new_mode" => "100644",
+              "old_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb",
+              "old_mode" => "100644",
+              "old_path" => "CHANGELOG",
+              "path" => "CHANGELOG.md",
+              "status" => "RENAMED"
+            }
+          ]
+
+          expect(find_changed_paths).to eq expected_changed_paths
+        end
+      end
+
+      context 'when find_renames is false' do
+        let(:find_renames) { false }
+
+        it 'does not detect renamed file' do
+          expected_changed_paths = [
+            {
+              "new_blob_id" => "0000000000000000000000000000000000000000",
+              "new_mode" => "0",
+              "old_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb",
+              "old_mode" => "100644",
+              "old_path" => "",
+              "path" => "CHANGELOG",
+              "status" => "DELETED"
+            },
+            {
+              "new_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb",
+              "new_mode" => "100644",
+              "old_blob_id" => "0000000000000000000000000000000000000000",
+              "old_mode" => "0",
+              "old_path" => "",
+              "path" => "CHANGELOG.md",
+              "status" => "ADDED"
+            }
+          ]
+
+          expect(find_changed_paths).to eq expected_changed_paths
+        end
+      end
+    end
+
     context 'when all requested objects are invalid' do
       it 'does not send RPC request' do
         expect_any_instance_of(Gitaly::DiffService::Stub).not_to receive(:find_changed_paths)
-- 
GitLab


From fa30b2dce8d4a299f7f8dfc64218f65fb9e2bd97 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Wed, 21 Aug 2024 19:19:55 +1000
Subject: [PATCH 4/7] Refactor some code and support offset

---
 .../merge_requests/diffs_stream_controller.rb | 57 ++-----------------
 app/models/merge_request.rb                   |  8 ++-
 lib/gitlab/git/repository.rb                  | 49 ++++++++++++++++
 3 files changed, 61 insertions(+), 53 deletions(-)

diff --git a/app/controllers/projects/merge_requests/diffs_stream_controller.rb b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
index 51e356a8a0e9f7..01332f2dde2eff 100644
--- a/app/controllers/projects/merge_requests/diffs_stream_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
@@ -7,7 +7,6 @@ class DiffsStreamController < Projects::MergeRequests::ApplicationController
 
       urgency :low, [:diffs]
 
-      # rubocop: disable Metrics/AbcSize -- WIP
       def diffs
         return render_404 unless ::Feature.enabled?(:rapid_diffs, current_user, type: :wip)
 
@@ -17,58 +16,15 @@ def diffs
         view = helpers.diff_view
 
         if params[:diff_blobs]
-          changed_paths = project
-            .repository
-            .find_changed_paths([
-              Gitlab::Git::DiffTree.new(
-                @merge_request.diff_refs.base_sha,
-                @merge_request.diff_refs.head_sha
-              )
-            ], find_renames: true)
-
-          changed_paths.each_slice(50) do |changed_paths_slice|
-            blob_ids = changed_paths_slice.map do |changed_path|
-              Gitaly::DiffBlobsRequest::BlobPair.new(
-                left_blob: changed_path.old_blob_id,
-                right_blob: changed_path.new_blob_id
+          @merge_request.diffs_for_streaming(offset_index: offset) do |diff_files_batch|
+            rendered_diff_files = diff_files_batch.map do |diff_file|
+              render_to_string(
+                ::RapidDiffs::DiffFileComponent.new(diff_file: diff_file),
+                layout: false
               )
             end
 
-            diff_blobs = project
-              .repository
-              .diff_blobs(blob_ids, patch_bytes_limit: Gitlab::Git::Diff.patch_hard_limit_bytes)
-
-            diff_blobs.each do |blob|
-              changed_path = changed_paths_slice.find do |cp|
-                blob.left_blob_id == cp.old_blob_id && blob.right_blob_id == cp.new_blob_id
-              end
-
-              diff_attrs = {
-                diff: blob.patch,
-                new_path: changed_path.path,
-                old_path: changed_path.old_path,
-                a_mode: changed_path.old_mode,
-                b_mode: changed_path.new_mode,
-                new_file: changed_path.status == :ADDED,
-                renamed_file: changed_path.status == :RENAMED,
-                deleted_file: changed_path.status == :DELETED,
-                too_large: blob.over_patch_bytes_limit
-              }
-
-              diff = Gitlab::Git::Diff.new(diff_attrs)
-              diff_file = Gitlab::Diff::File.new(
-                diff,
-                repository: project.repository,
-                diff_refs: @merge_request.diff_refs
-              )
-
-              response.stream.write(
-                render_to_string(
-                  ::RapidDiffs::DiffFileComponent.new(diff_file: diff_file),
-                  layout: false
-                )
-              )
-            end
+            response.stream.write(rendered_diff_files.join)
           end
         else
           @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff|
@@ -85,6 +41,5 @@ def diffs
         response.stream.close
       end
     end
-    # rubocop: enable Metrics/AbcSize
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 0b6504b3b460ea..8fc8c8836a3008 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -849,10 +849,14 @@ def raw_diffs(*args)
     compare.present? ? compare.raw_diffs(*args) : merge_request_diff.raw_diffs(*args)
   end
 
-  def diffs_for_streaming(diff_options = {})
+  def diffs_for_streaming(diff_options = {}, &)
     diff = diffable_merge_ref? ? merge_head_diff : merge_request_diff
 
-    diff.diffs(diff_options)
+    if block_given?
+      source_project.repository.diffs_by_changed_paths(diff.diff_refs, diff_options[:offset_index], &)
+    else
+      diff.diffs(diff_options)
+    end
   end
 
   def diffs(diff_options = {})
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 6f08cfd455c8c1..04b1fb7cee3403 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1259,8 +1259,57 @@ def detect_generated_files(base, head, changed_paths)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
+      def diffs_by_changed_paths(diff_refs, offset, batch_size = 50)
+        changed_paths = find_changed_paths(
+          [Gitlab::Git::DiffTree.new(diff_refs.base_sha, diff_refs.head_sha)],
+          find_renames: true
+        )
+
+        changed_paths = changed_paths.drop(offset) if offset.present?
+
+        changed_paths.each_slice(batch_size) do |batched_changed_paths|
+          blob_pairs = batched_changed_paths.map do |changed_path|
+            Gitaly::DiffBlobsRequest::BlobPair.new(
+              left_blob: changed_path.old_blob_id,
+              right_blob: changed_path.new_blob_id
+            )
+          end
+
+          yield diff_files_by_blob_pairs(blob_pairs, batched_changed_paths, diff_refs)
+        end
+      end
+
       private
 
+      def diff_files_by_blob_pairs(blob_pairs, changed_paths, diff_refs)
+        diff_blobs = diff_blobs(blob_pairs, patch_bytes_limit: Gitlab::Git::Diff.patch_hard_limit_bytes)
+
+        diff_blobs.map do |diff_blob|
+          changed_path = changed_paths.find do |changed_path|
+            diff_blob.left_blob_id == changed_path.old_blob_id &&
+              diff_blob.right_blob_id == changed_path.new_blob_id
+          end
+
+          diff = Gitlab::Git::Diff.new({
+            diff: diff_blob.patch,
+            too_large: diff_blob.over_patch_bytes_limit,
+            new_path: changed_path.path,
+            old_path: changed_path.old_path,
+            a_mode: changed_path.old_mode,
+            b_mode: changed_path.new_mode,
+            new_file: changed_path.status == :ADDED,
+            renamed_file: changed_path.status == :RENAMED,
+            deleted_file: changed_path.status == :DELETED
+          })
+
+          Gitlab::Diff::File.new(
+            diff,
+            repository: container.repository,
+            diff_refs: diff_refs
+          )
+        end
+      end
+
       def check_blobs_generated(base, head, changed_paths)
         wrapped_gitaly_errors do
           gitaly_analysis_client.check_blobs_generated(base, head, changed_paths)
-- 
GitLab


From f8fa0c840c4cc72370bc1124e02e5d943a16b0b1 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Fri, 23 Aug 2024 18:19:54 +1000
Subject: [PATCH 5/7] Refactor and add some more specs

---
 .../merge_requests/diffs_stream_controller.rb |  47 +++--
 lib/gitlab/git/repository.rb                  |  11 +-
 spec/lib/gitlab/git/repository_spec.rb        | 164 +++++++++++++++++-
 spec/models/merge_request_spec.rb             |  11 ++
 .../merge_requests/diffs_stream_spec.rb       |  29 ++++
 5 files changed, 229 insertions(+), 33 deletions(-)

diff --git a/app/controllers/projects/merge_requests/diffs_stream_controller.rb b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
index 01332f2dde2eff..e51f4ebb0624ee 100644
--- a/app/controllers/projects/merge_requests/diffs_stream_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
@@ -15,31 +15,42 @@ def diffs
         offset = params[:offset].to_i
         view = helpers.diff_view
 
-        if params[:diff_blobs]
-          @merge_request.diffs_for_streaming(offset_index: offset) do |diff_files_batch|
-            rendered_diff_files = diff_files_batch.map do |diff_file|
-              render_to_string(
-                ::RapidDiffs::DiffFileComponent.new(diff_file: diff_file),
-                layout: false
-              )
-            end
-
-            response.stream.write(rendered_diff_files.join)
-          end
+        # NOTE: This is a temporary flag to test out the new diff_blobs
+        if !!ActiveModel::Type::Boolean.new.cast(params[:diff_blobs])
+          stream_diff_blobs(offset)
         else
-          @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff|
-            response.stream.write(
-              render_to_string(
-                ::RapidDiffs::DiffFileComponent.new(diff_file: diff, parallel_view: view == :parallel),
-                layout: false
-            )
-          end
+          stream_diff_files(offset)
         end
+
       rescue StandardError => e
+        Gitlab::AppLogger.error("Error streaming diffs: #{e.message}")
         response.stream.write e.message
       ensure
         response.stream.close
       end
+
+      private
+
+      def stream_diff_blobs(offset)
+        @merge_request.diffs_for_streaming(offset_index: offset) do |diff_files_batch|
+          diff_files_batch.each do |diff_file|
+            response.stream.write(render_diff_file(diff_file))
+          end
+        end
+      end
+
+      def stream_diff_files(offset)
+        @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff_file|
+          response.stream.write(render_diff_file(diff_file))
+        end
+      end
+
+      def render_diff_file(diff_file)
+        render_to_string(
+          ::RapidDiffs::DiffFileComponent.new(diff_file: diff_file, parallel_view: view == :parallel),
+          layout: false
+        )
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 04b1fb7cee3403..f663516d63c3ff 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1265,9 +1265,7 @@ def diffs_by_changed_paths(diff_refs, offset, batch_size = 50)
           find_renames: true
         )
 
-        changed_paths = changed_paths.drop(offset) if offset.present?
-
-        changed_paths.each_slice(batch_size) do |batched_changed_paths|
+        changed_paths.drop(offset).each_slice(batch_size) do |batched_changed_paths|
           blob_pairs = batched_changed_paths.map do |changed_path|
             Gitaly::DiffBlobsRequest::BlobPair.new(
               left_blob: changed_path.old_blob_id,
@@ -1284,12 +1282,9 @@ def diffs_by_changed_paths(diff_refs, offset, batch_size = 50)
       def diff_files_by_blob_pairs(blob_pairs, changed_paths, diff_refs)
         diff_blobs = diff_blobs(blob_pairs, patch_bytes_limit: Gitlab::Git::Diff.patch_hard_limit_bytes)
 
-        diff_blobs.map do |diff_blob|
-          changed_path = changed_paths.find do |changed_path|
-            diff_blob.left_blob_id == changed_path.old_blob_id &&
-              diff_blob.right_blob_id == changed_path.new_blob_id
-          end
+        changed_diff_blobs = diff_blobs.zip(changed_paths)
 
+        changed_diff_blobs.map do |diff_blob, changed_path|
           diff = Gitlab::Git::Diff.new({
             diff: diff_blob.patch,
             too_large: diff_blob.over_patch_bytes_limit,
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 6421750b45e377..580697fd5fb839 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1740,7 +1740,7 @@ def create_commit(blobs)
     let(:commit_1_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "files/executables/ls", old_mode: "0", new_mode: "100755", old_path: '',
+          status: :ADDED, path: "files/executables/ls", old_path: '', old_mode: "0", new_mode: "100755",
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: 'c84acd1ff0b844201312052f9bb3b7259eb2e177'
         )
       ]
@@ -1749,7 +1749,7 @@ def create_commit(blobs)
     let(:commit_2_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "bar/branch-test.txt", old_mode: "0", new_mode: "100644", old_path: '',
+          status: :ADDED, path: "bar/branch-test.txt", old_path: '', old_mode: "0", new_mode: "100644",
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '93e123ac8a3e6a0b600953d7598af629dec7b735'
         )
       ]
@@ -1758,11 +1758,11 @@ def create_commit(blobs)
     let(:commit_3_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :MODIFIED, path: ".gitmodules", old_mode: "100644", new_mode: "100644", old_path: '',
+          status: :MODIFIED, path: ".gitmodules", old_path: '', old_mode: "100644", new_mode: "100644",
           old_blob_id: 'fdaada1754989978413d618ee1fb1c0469d6a664', new_blob_id: '0792c58905eff3432b721f8c4a64363d8e28d9ae'
         ),
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "gitlab-shell", old_mode: "0", new_mode: "160000", old_path: '',
+          status: :ADDED, path: "gitlab-shell", old_path: '', old_mode: "0", new_mode: "160000",
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '79bceae69cb5750d6567b223597999bfa91cb3b9'
         )
       ]
@@ -1771,15 +1771,15 @@ def create_commit(blobs)
     let(:initial_commit_files) do
       [
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: ".gitignore", old_mode: "0", new_mode: "100644", old_path: '',
+          status: :ADDED, path: ".gitignore", old_path: '', old_mode: "0", new_mode: "100644",
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '470ad2fcf1e33798f1afc5781d08e60c40f51e7a'
         ),
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "LICENSE", old_mode: "0", new_mode: "100644", old_path: '',
+          status: :ADDED, path: "LICENSE", old_path: '', old_mode: "0", new_mode: "100644",
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '50b27c6518be44c42c4d87966ae2481ce895624c'
         ),
         Gitlab::Git::ChangedPath.new(
-          status: :ADDED, path: "README.md", old_mode: "0", new_mode: "100644", old_path: '',
+          status: :ADDED, path: "README.md", old_path: '', old_mode: "0", new_mode: "100644",
           old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: 'faaf198af3a36dbf41961466703cc1d47c61d051'
         )
       ]
@@ -3029,6 +3029,156 @@ def create_remote_branch(remote_name, branch_name, source_branch_name)
     end
   end
 
+  describe '#diffs_by_changed_path' do
+    let(:changed_paths) do
+      [
+        Gitlab::Git::ChangedPath.new(
+          status: :ADDED, path: "added_file.rb", old_path: '', old_mode: "0", new_mode: "100644",
+          old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '470ad2fcf1e33798f1afc5781d08e60c40f51e7a'
+        ),
+        Gitlab::Git::ChangedPath.new(
+          status: :RENAMED, path: "renamed_file.rb", old_path: 'original_file.rb', old_mode: "100644", new_mode: "100644",
+          old_blob_id: '93e123ac8a3e6a0b600953d7598af629dec7b735', new_blob_id: '50b27c6518be44c42c4d87966ae2481ce895624c'
+        ),
+        Gitlab::Git::ChangedPath.new(
+          status: :MODIFIED, path: "modified_file.rb", old_path: '', old_mode: "100644", new_mode: "100644",
+          old_blob_id: 'fdaada1754989978413d618ee1fb1c0469d6a664', new_blob_id: '0792c58905eff3432b721f8c4a64363d8e28d9ae'
+        ),
+        Gitlab::Git::ChangedPath.new(
+          status: :DELETED, path: "deleted_file.rb", old_path: '', old_mode: "100644", new_mode: "0",
+          old_blob_id: '79bceae69cb5750d6567b223597999bfa91cb3b9', new_blob_id: '0000000000000000000000000000000000000000'
+        )
+      ]
+    end
+
+    let(:diff_refs) do
+      Gitlab::Diff::DiffRefs.new(
+        base_sha: "913c66a37b4a45b9769037c55c2d238bd0942d2e",
+        head_sha: "874797c3a73b60d2187ed6e2fcabd289ff75171e"
+      )
+    end
+
+    let(:blob_pairs) do
+      changed_paths.map do |changed_path|
+        Gitaly::DiffBlobsRequest::BlobPair.new(
+          left_blob: changed_path.old_blob_id,
+          right_blob: changed_path.new_blob_id
+        )
+      end
+    end
+
+    let(:diff_blobs) do
+      changed_paths.map do |changed_path|
+        Gitlab::GitalyClient::DiffBlob.new(
+          left_blob_id: changed_path.old_blob_id,
+          right_blob_id: changed_path.new_blob_id,
+          patch: 'some content'
+        )
+      end
+    end
+
+    before do
+      allow(repository).to receive(:find_changed_paths)
+        .once
+        .with([kind_of(Gitlab::Git::DiffTree)], find_renames: true)
+        .and_return(changed_paths)
+    end
+
+    context 'diff file count is smaller than the default batch size' do
+      before do
+        allow(repository).to receive(:diff_blobs)
+          .once
+          .with(blob_pairs, anything)
+          .and_return(diff_blobs)
+      end
+
+      it 'returns diff filess in batches' do
+        diff_files = []
+
+        repository.diffs_by_changed_paths(diff_refs, 0) do |batch|
+          diff_files << batch
+        end
+
+        expect(diff_files.count).to eq 1
+
+        first_batch = diff_files[0]
+
+        expect(first_batch.count).to eq 4
+        expect(first_batch[0].new_path).to eq 'added_file.rb'
+        expect(first_batch[0].new_file?).to eq true
+        expect(first_batch[1].new_path).to eq 'renamed_file.rb'
+        expect(first_batch[1].renamed_file?).to eq true
+        expect(first_batch[2].new_path).to eq 'modified_file.rb'
+        expect(first_batch[2].new_file?).to eq false
+        expect(first_batch[3].new_path).to eq 'deleted_file.rb'
+        expect(first_batch[3].deleted_file?).to eq true
+      end
+    end
+
+    context 'offset is given' do
+      before do
+        allow(repository).to receive(:diff_blobs)
+          .once
+          .with(blob_pairs[3..], anything)
+          .and_return(diff_blobs[3..])
+      end
+
+      it 'returns diff filess in batches' do
+        diff_files = []
+
+        repository.diffs_by_changed_paths(diff_refs, 3) do |batch|
+          diff_files << batch
+        end
+
+        expect(diff_files.count).to eq 1
+
+        first_batch = diff_files[0]
+
+        expect(first_batch.count).to eq 1
+        expect(first_batch[0].new_path).to eq 'deleted_file.rb'
+        expect(first_batch[0].deleted_file?).to eq true
+      end
+    end
+
+    context 'batch_size is given' do
+      before do
+        allow(repository).to receive(:diff_blobs)
+          .once
+          .with(blob_pairs[..1], anything)
+          .and_return(diff_blobs[..1])
+
+        allow(repository).to receive(:diff_blobs)
+          .once
+          .with(blob_pairs[2..], anything)
+          .and_return(diff_blobs[2..])
+      end
+
+      it 'returns diff filess in batches' do
+        diff_files = []
+
+        repository.diffs_by_changed_paths(diff_refs, 0, 2) do |batch|
+          diff_files << batch
+        end
+
+        expect(diff_files.count).to eq 2
+
+        first_batch = diff_files[0]
+        second_batch = diff_files[1]
+
+        expect(first_batch.count).to eq 2
+        expect(first_batch[0].new_path).to eq 'added_file.rb'
+        expect(first_batch[0].new_file?).to eq true
+        expect(first_batch[1].new_path).to eq 'renamed_file.rb'
+        expect(first_batch[1].renamed_file?).to eq true
+        expect(second_batch.count).to eq 2
+        expect(second_batch[0].new_path).to eq 'modified_file.rb'
+        expect(second_batch[0].new_file?).to eq false
+        expect(second_batch[1].new_path).to eq 'deleted_file.rb'
+        expect(second_batch[1].deleted_file?).to eq true
+      end
+    end
+  end
+
   describe '#gitaly_diff_client' do
     it 'instantiates a new DiffService class' do
       expect(repository.gitaly_diff_client).to be_a(Gitlab::GitalyClient::DiffService)
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 95c8a2c736c2ac..3352e01bead6db 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -6986,6 +6986,17 @@ def transition!
         expect(merge_request.diffs_for_streaming).to eq(['HEAD diff'])
       end
     end
+
+    context 'when block is given' do
+      it 'calls diffs_by_changed_paths' do
+        expect(base_diff).to receive(:diff_refs)
+        expect_next_instance_of(Repository) do |repository|
+          expect(repository).to receive(:diffs_by_changed_paths)
+        end
+
+        merge_request.diffs_for_streaming { |_| 'do something' }
+      end
+    end
   end
 
   describe '#merge_exclusive_lease' do
diff --git a/spec/requests/projects/merge_requests/diffs_stream_spec.rb b/spec/requests/projects/merge_requests/diffs_stream_spec.rb
index 68288ea9cf0b0d..b6b78ca28fb35e 100644
--- a/spec/requests/projects/merge_requests/diffs_stream_spec.rb
+++ b/spec/requests/projects/merge_requests/diffs_stream_spec.rb
@@ -24,6 +24,8 @@ def go(**extra_params)
     let_it_be_with_reload(:merge_request) do
       create(
         :merge_request_with_diffs,
+        source_branch: 'expand-collapse-files',
+        target_branch: 'master',
         target_project: project,
         source_project: project
       )
@@ -91,6 +93,33 @@ def go(**extra_params)
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
+
+    context 'with diffs_blob option' do
+      context 'when offset is not given' do
+        it 'streams all diffs' do
+          go(diff_blobs: true)
+
+          expect(response).to have_gitlab_http_status(:success)
+          expect(response.body).to include(*file_identifier_hashes(merge_request.merge_request_diff))
+        end
+      end
+
+      context 'when offset is given' do
+        let(:offset) { 5 }
+
+        it 'streams diffs except the offset' do
+          go(diff_blobs: true, offset: offset)
+
+          diff_files = merge_request.merge_request_diff.diffs.diff_files.to_a
+          offset_file_identifier_hashes = diff_files.take(offset).map(&:file_identifier_hash)
+          remaining_file_identifier_hashes = diff_files.slice(offset..).map(&:file_identifier_hash)
+
+          expect(response).to have_gitlab_http_status(:success)
+          expect(response.body).not_to include(*offset_file_identifier_hashes)
+          expect(response.body).to include(*remaining_file_identifier_hashes)
+        end
+      end
+    end
   end
 
   def file_identifier_hashes(diff)
-- 
GitLab


From 8098b5e9778c348e58e9a833d370a5d14b3153f8 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Tue, 3 Sep 2024 17:07:49 +1000
Subject: [PATCH 6/7] Address review comments and fix specs

---
 app/models/merge_request.rb       |  4 +++-
 lib/gitlab/git/repository.rb      |  2 +-
 spec/models/merge_request_spec.rb | 29 ++++++++++++++++++++++++-----
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 8fc8c8836a3008..93ec0430866808 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -852,8 +852,10 @@ def raw_diffs(*args)
   def diffs_for_streaming(diff_options = {}, &)
     diff = diffable_merge_ref? ? merge_head_diff : merge_request_diff
 
+    offset = diff_options[:offset_index].to_i || 0
+
     if block_given?
-      source_project.repository.diffs_by_changed_paths(diff.diff_refs, diff_options[:offset_index], &)
+      source_project.repository.diffs_by_changed_paths(diff.diff_refs, offset, &)
     else
       diff.diffs(diff_options)
     end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index f663516d63c3ff..b0843af27f868a 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1259,7 +1259,7 @@ def detect_generated_files(base, head, changed_paths)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      def diffs_by_changed_paths(diff_refs, offset, batch_size = 50)
+      def diffs_by_changed_paths(diff_refs, offset, batch_size = 30)
         changed_paths = find_changed_paths(
           [Gitlab::Git::DiffTree.new(diff_refs.base_sha, diff_refs.head_sha)],
           find_renames: true
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 3352e01bead6db..8ef7220667569b 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -6988,13 +6988,32 @@ def transition!
     end
 
     context 'when block is given' do
-      it 'calls diffs_by_changed_paths' do
-        expect(base_diff).to receive(:diff_refs)
-        expect_next_instance_of(Repository) do |repository|
-          expect(repository).to receive(:diffs_by_changed_paths)
+      let(:diff_refs) { instance_double(Gitlab::Diff::DiffRefs) }
+      let(:expected_block) { proc {} }
+      let(:repository) { merge_request.source_project.repository }
+
+      before do
+        allow(base_diff).to receive(:diff_refs).and_return(diff_refs)
+      end
+
+      it 'calls diffs_by_changed_paths with given offset' do
+        expect(repository).to receive(:diffs_by_changed_paths).with(diff_refs, 0) do |_, &block|
+          expect(block).to be(expected_block)
         end
 
-        merge_request.diffs_for_streaming { |_| 'do something' }
+        merge_request.diffs_for_streaming(&expected_block)
+      end
+
+      context 'when offset_index is given' do
+        let(:offset) { 5 }
+
+        it 'calls diffs_by_changed_paths with given offset' do
+          expect(repository).to receive(:diffs_by_changed_paths).with(diff_refs, offset) do |_, &block|
+            expect(block).to be(expected_block)
+          end
+
+          merge_request.diffs_for_streaming({ offset_index: offset }, &expected_block)
+        end
       end
     end
   end
-- 
GitLab


From c6642778926d4555c705536cb03ade5aff1fa0ac Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Wed, 4 Sep 2024 15:15:04 +1000
Subject: [PATCH 7/7] Fix failing specs

---
 .../merge_requests/diffs_stream_controller.rb |  5 ++-
 lib/gitlab/git/changed_path.rb                |  9 ++---
 .../gitaly_client/commit_service_spec.rb      | 34 +++++++++----------
 3 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/app/controllers/projects/merge_requests/diffs_stream_controller.rb b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
index e51f4ebb0624ee..84f6c3bff5bb20 100644
--- a/app/controllers/projects/merge_requests/diffs_stream_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_stream_controller.rb
@@ -13,7 +13,6 @@ def diffs
         stream_headers
 
         offset = params[:offset].to_i
-        view = helpers.diff_view
 
         # NOTE: This is a temporary flag to test out the new diff_blobs
         if !!ActiveModel::Type::Boolean.new.cast(params[:diff_blobs])
@@ -31,6 +30,10 @@ def diffs
 
       private
 
+      def view
+        helpers.diff_view
+      end
+
       def stream_diff_blobs(offset)
         @merge_request.diffs_for_streaming(offset_index: offset) do |diff_files_batch|
           diff_files_batch.each do |diff_file|
diff --git a/lib/gitlab/git/changed_path.rb b/lib/gitlab/git/changed_path.rb
index 9ee65653c56324..4c2ffcf699b095 100644
--- a/lib/gitlab/git/changed_path.rb
+++ b/lib/gitlab/git/changed_path.rb
@@ -3,7 +3,7 @@
 module Gitlab
   module Git
     class ChangedPath
-      attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id
+      attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id, :old_path
 
       def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_id: nil, old_path: nil)
         @status = status
@@ -12,18 +12,13 @@ def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_
         @new_mode = new_mode
         @old_blob_id = old_blob_id
         @new_blob_id = new_blob_id
-        @old_path = old_path
+        @old_path = old_path.presence || @path
       end
 
       def new_file?
         status == :ADDED
       end
 
-      # NOTE: old_path may not always be provided so fallback to the path
-      def old_path
-        @old_path.presence || @path
-      end
-
       def submodule_change?
         # The file mode 160000 represents a "Gitlink" or a git submodule.
         # The first two digits can be used to distinguish it from regular files.
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index 17f8be07d53d2b..79eaec7e9993ae 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -205,47 +205,47 @@
       let(:changed_paths) do
         [
           {
-            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/foo.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
           },
           {
-            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
+            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/foo.lfs',
             old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
             new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/bar.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1"
           },
           {
-            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
+            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/foo.lfs',
             old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
             new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/bar.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
+            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/bar.lfs',
             old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1",
             new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
+            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/bar.lfs',
             old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1",
             new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           }
@@ -261,32 +261,32 @@
       let(:changed_paths) do
         [
           {
-            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/foo.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
           },
           {
-            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
+            path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/foo.lfs',
             old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
             new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/bar.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1"
           },
           {
-            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: '',
+            path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/bar.lfs',
             old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1",
             new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           },
           {
-            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: '',
+            path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs',
             old_blob_id: "0000000000000000000000000000000000000000",
             new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9"
           }
@@ -440,7 +440,7 @@
               "new_mode" => "0",
               "old_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb",
               "old_mode" => "100644",
-              "old_path" => "",
+              "old_path" => "CHANGELOG",
               "path" => "CHANGELOG",
               "status" => "DELETED"
             },
@@ -449,7 +449,7 @@
               "new_mode" => "100644",
               "old_blob_id" => "0000000000000000000000000000000000000000",
               "old_mode" => "0",
-              "old_path" => "",
+              "old_path" => "CHANGELOG.md",
               "path" => "CHANGELOG.md",
               "status" => "ADDED"
             }
-- 
GitLab