Merge branch...

Merge branch '31983-increase-merge-request-diff-file-size-limit-for-default-toggle-opening' into 'master'

Increase diff limits to 100 KB for collapse and 200 KB overall

Closes #31983

See merge request !11875

changelogs/unreleased/31983-increase-merge-request-diff-file-size-limit-for-default-toggle-opening.yml

+---
+title: Increase individual diff collapse limit to 100 KB, and render limit to 200 KB
+merge_request:
+author:

lib/feature.rb

@@ -27,6 +27,18 @@ class Feature
       all.map(&:name).include?(feature.name)
     end
 
+    def enabled?(key)
+      get(key).enabled?
+    end
+
+    def enable(key)
+      get(key).enable
+    end
+
+    def disable(key)
+      get(key).disable
+    end
+
     private
 
     def flipper

lib/gitlab/git/diff.rb

@@ -20,13 +20,25 @@ module Gitlab
       # We need this accessor because of `to_hash` and `init_from_hash`
       attr_accessor :too_large
 
+      class << self
         # The maximum size of a diff to display.
-      SIZE_LIMIT = 100.kilobytes
+        def size_limit
+          if Feature.enabled?('gitlab_git_diff_size_limit_increase')
+            200.kilobytes
+          else
+            100.kilobytes
+          end
+        end
 
         # The maximum size before a diff is collapsed.
-      COLLAPSE_LIMIT = 10.kilobytes
+        def collapse_limit
+          if Feature.enabled?('gitlab_git_diff_size_limit_increase')
+            100.kilobytes
+          else
+            10.kilobytes
+          end
+        end
 
-      class << self
         def between(repo, head, base, options = {}, *paths)
           straight = options.delete(:straight) || false
 
@@ -231,7 +243,7 @@ module Gitlab
 
       def too_large?
         if @too_large.nil?
-          @too_large = @diff.bytesize >= SIZE_LIMIT
+          @too_large = @diff.bytesize >= self.class.size_limit
         else
           @too_large
         end
@@ -246,7 +258,7 @@ module Gitlab
       def collapsed?
         return @collapsed if defined?(@collapsed)
 
-        @collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT
+        @collapsed = !expanded && @diff.bytesize >= self.class.collapse_limit
       end
 
       def collapse!
@@ -318,14 +330,14 @@ module Gitlab
           hunk.each_line do |line|
             size += line.content.bytesize
 
-            if size >= SIZE_LIMIT
+            if size >= self.class.size_limit
               too_large!
               return true
             end
           end
         end
 
-        if !expanded && size >= COLLAPSE_LIMIT
+        if !expanded && size >= self.class.collapse_limit
           collapse!
           return true
         end

spec/features/expand_collapse_diffs_spec.rb

@@ -5,6 +5,11 @@ feature 'Expand and collapse diffs', js: true, feature: true do
   let(:project) { create(:project, :repository) }
 
   before do
+    # Set the limits to those when these specs were written, to avoid having to
+    # update the test repo every time we change them.
+    allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes)
+    allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(10.kilobytes)
+
     login_as :admin
 
     # Ensure that undiffable.md is in .gitattributes
@@ -62,18 +67,6 @@ feature 'Expand and collapse diffs', js: true, feature: true do
       expect(small_diff).not_to have_selector('.nothing-here-block')
     end
 
-    it 'collapses large diffs by default' do
-      expect(large_diff).not_to have_selector('.code')
-      expect(large_diff).to have_selector('.nothing-here-block')
-    end
-
-    it 'collapses large diffs for renamed files by default' do
-      expect(large_diff_renamed).not_to have_selector('.code')
-      expect(large_diff_renamed).to have_selector('.nothing-here-block')
-      expect(large_diff_renamed).to have_selector('.js-file-title .deletion')
-      expect(large_diff_renamed).to have_selector('.js-file-title .addition')
-    end
-
     it 'shows non-renderable diffs as such immediately, regardless of their size' do
       expect(undiffable).not_to have_selector('.code')
       expect(undiffable).to have_selector('.nothing-here-block')

spec/features/projects/compare_spec.rb

@@ -24,6 +24,7 @@ describe "Compare", js: true do
       expect(find(".js-compare-to-dropdown .dropdown-toggle-text")).to have_content("binary-encoding")
 
       click_button "Compare"
+
       expect(page).to have_content "Commits"
     end
 

spec/lib/gitlab/git/diff_collection_spec.rb

@@ -341,7 +341,8 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
     end
 
     context 'when diff is quite large will collapse by default' do
-      let(:iterator) { [{ diff: 'a' * 20480 }] }
+      let(:iterator) { [{ diff: 'a' * (Gitlab::Git::Diff.collapse_limit + 1) }] }
+      let(:max_files) { 100 }
 
       context 'when no collapse is set' do
         let(:expanded) { true }

spec/lib/gitlab/git/diff_spec.rb

@@ -31,6 +31,36 @@ EOT
                                           [".gitmodules"]).patches.first
   end
 
+  describe 'size limit feature toggles' do
+    context 'when the feature gitlab_git_diff_size_limit_increase is enabled' do
+      before do
+        Feature.enable('gitlab_git_diff_size_limit_increase')
+      end
+
+      it 'returns 200 KB for size_limit' do
+        expect(described_class.size_limit).to eq(200.kilobytes)
+      end
+
+      it 'returns 100 KB for collapse_limit' do
+        expect(described_class.collapse_limit).to eq(100.kilobytes)
+      end
+    end
+
+    context 'when the feature gitlab_git_diff_size_limit_increase is disabled' do
+      before do
+        Feature.disable('gitlab_git_diff_size_limit_increase')
+      end
+
+      it 'returns 100 KB for size_limit' do
+        expect(described_class.size_limit).to eq(100.kilobytes)
+      end
+
+      it 'returns 10 KB for collapse_limit' do
+        expect(described_class.collapse_limit).to eq(10.kilobytes)
+      end
+    end
+  end
+
   describe '.new' do
     context 'using a Hash' do
       context 'with a small diff' do
@@ -47,7 +77,7 @@ EOT
 
       context 'using a diff that is too large' do
         it 'prunes the diff' do
-          diff = described_class.new(diff: 'a' * 204800)
+          diff = described_class.new(diff: 'a' * (described_class.size_limit + 1))
 
           expect(diff.diff).to be_empty
           expect(diff).to be_too_large
@@ -85,8 +115,8 @@ EOT
           # The patch total size is 200, with lines between 21 and 54.
           # This is a quick-and-dirty way to test this. Ideally, a new patch is
           # added to the test repo with a size that falls between the real limits.
-          stub_const("#{described_class}::SIZE_LIMIT", 150)
-          stub_const("#{described_class}::COLLAPSE_LIMIT", 100)
+          allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(150)
+          allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(100)
         end
 
         it 'prunes the diff as a large diff instead of as a collapsed diff' do
@@ -299,7 +329,7 @@ EOT
 
   describe '#collapsed?' do
     it 'returns true for a diff that is quite large' do
-      diff = described_class.new({ diff: 'a' * 20480 }, expanded: false)
+      diff = described_class.new({ diff: 'a' * (described_class.collapse_limit + 1) }, expanded: false)
 
       expect(diff).to be_collapsed
     end