Merge branch 'represent-all-changes-in-MR' into 'master'

Model files newly returned from gitaly

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124907



Merged-by: Kerri Miller <kerrizor@kerrizor.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Kerri Miller <kerrizor@kerrizor.com>
Reviewed-by: Peter Leitzen <pleitzen@gitlab.com>
Co-authored-by: johnwparent <john.parent@kitware.com>

config/feature_flags/development/collect_all_diff_paths.yml

+---
+name: collect_all_diff_paths
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124907
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/421460
+milestone: '16.4'
+type: development
+group: "group::gitaly"
+default_enabled: false

lib/gitlab/git/diff.rb

@@ -81,7 +81,7 @@ def between(repo, head, base, options = {}, *paths)
         #    exceeded
         def filter_diff_options(options, default_options = {})
           allowed_options = [:ignore_whitespace_change, :max_files, :max_lines,
-                             :limits, :expanded]
+                             :limits, :expanded, :collect_all_paths]
 
           if default_options
             actual_defaults = default_options.dup
@@ -126,6 +126,10 @@ def patch_safe_limit_bytes(limit = patch_hard_limit_bytes)
           limit / 10
         end
 
+        def collect_patch_overage?
+          !!Feature.enabled?(:collect_all_diff_paths)
+        end
+
         # Returns the limit for a single diff file (patch).
         #
         # Patches surpassing this limit shouldn't be persisted in the database
@@ -198,9 +202,13 @@ def too_large?
       # This is used by `to_hash` and `init_from_hash`.
       alias_method :too_large, :too_large?
 
-      def too_large!
+      def prune!
         @diff = ''
         @line_count = 0
+      end
+
+      def too_large!
+        prune!
         @too_large = true
       end
 
@@ -211,11 +219,19 @@ def collapsed?
       end
 
       def collapse!
-        @diff = ''
-        @line_count = 0
+        prune!
         @collapsed = true
       end
 
+      def overflow?
+        return @overflow if defined?(@overflow)
+
+        # If overflow is not defined, we're
+        # not recieveing a diff from Gitaly
+        # and overflow has no meaning
+        false
+      end
+
       def json_safe_diff
         return @diff unless detect_binary?(@diff)
 
@@ -248,7 +264,7 @@ def init_from_hash(hash)
       end
 
       def init_from_gitaly(gitaly_diff)
-        @diff = gitaly_diff.respond_to?(:patch) ? encode!(gitaly_diff.patch) : ''
+        @diff = gitaly_diff.try(:patch).present? ? encode!(gitaly_diff.patch) : ''
         @new_path = encode!(gitaly_diff.to_path.dup)
         @old_path = encode!(gitaly_diff.from_path.dup)
         @a_mode = gitaly_diff.old_mode.to_s(8)
@@ -257,11 +273,19 @@ def init_from_gitaly(gitaly_diff)
         @renamed_file = gitaly_diff.from_path != gitaly_diff.to_path
         @deleted_file = gitaly_diff.to_id == BLANK_SHA
         @too_large = gitaly_diff.too_large if gitaly_diff.respond_to?(:too_large)
+        gitaly_overflow = gitaly_diff.try(:overflow_marker)
+        @overflow = Diff.collect_patch_overage? && gitaly_overflow
 
         collapse! if gitaly_diff.respond_to?(:collapsed) && gitaly_diff.collapsed
+        # Diffs exceeding limits returned from gitaly when "collect_all_paths" are enabled
+        # are already pruned, but should be "collapsed" as they have no content
+        @collapsed = true if @overflow
       end
 
       def prune_diff_if_eligible
+        # If we have overflow, diffs are already pruned, retain line counts
+        return if overflow?
+
         if too_large?
           ::Gitlab::Metrics.add_event(:patch_hard_limit_bytes_hit)
 

lib/gitlab/git/diff_collection.rb

@@ -13,6 +13,10 @@ def self.default_limits
         { max_files: ::Commit.diff_safe_max_files, max_lines: ::Commit.diff_safe_max_lines }
       end
 
+      def self.collect_all_paths?(collect_all_paths)
+        Gitlab::Git::Diff.collect_patch_overage? ? collect_all_paths : false
+      end
+
       def self.limits(options = {})
         limits = {}
         defaults = default_limits
@@ -25,7 +29,7 @@ def self.limits(options = {})
         limits[:safe_max_bytes] = limits[:safe_max_files] * 5.kilobytes # Average 5 KB per file
         limits[:max_patch_bytes] = Gitlab::Git::Diff.patch_hard_limit_bytes
         limits[:max_patch_bytes_for_file_extension] = options.fetch(:max_patch_bytes_for_file_extension, {})
-
+        limits[:collect_all_paths] = collect_all_paths?(options.fetch(:collect_all_paths, false))
         limits
       end
 
@@ -164,7 +168,12 @@ def each_gitaly_patch
 
           if raw.overflow_marker
             @overflow = true
-            break
+            # If we're requesting patches with `collect_all_paths` enabled, then
+            # Once we hit the overflow marker, gitlay has still returned diffs, just without
+            # patches, only metadata
+            unless @limits[:collect_all_paths]
+              break
+            end
           end
 
           yield @array[i] = diff

spec/lib/gitlab/git/diff_spec.rb

@@ -131,6 +131,31 @@
           expect(diff.diff).to be_utf8
         end
       end
+
+      context 'using a diff that it too large but collecting all paths' do
+        let(:gitaly_diff) do
+          Gitlab::GitalyClient::Diff.new(
+            from_path: '.gitmodules',
+            to_path: '.gitmodules',
+            old_mode: 0100644,
+            new_mode: 0100644,
+            from_id: '0792c58905eff3432b721f8c4a64363d8e28d9ae',
+            to_id: 'efd587ccb47caf5f31fc954edb21f0a713d9ecc3',
+            overflow_marker: true,
+            collapsed: false,
+            too_large: false,
+            patch: ''
+          )
+        end
+
+        let(:diff) { described_class.new(gitaly_diff) }
+
+        it 'is already pruned and collapsed but not too large' do
+          expect(diff.diff).to be_empty
+          expect(diff).not_to be_too_large
+          expect(diff).to be_collapsed
+        end
+      end
     end
 
     context 'using a Gitaly::CommitDelta' do