Merge branch 'osw-delete-non-latest-mr-diff-files-upon-merge' into 'master'

Delete non-latest merge request diff files upon MRs merge

Closes #37639

See merge request gitlab-org/gitlab-ce!19670

2 files changed, 32 insertions, 12 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 3df1130a6e2..f112c06e26f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -378,6 +378,10 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def non_latest_diffs
+    merge_request_diffs.where.not(id: merge_request_diff.id)
+  end
+
   def diff_size
     # Calling `merge_request_diff.diffs.real_size` will also perform
     # highlighting, which we don't need here.
@@ -619,18 +623,7 @@ class MergeRequest < ActiveRecord::Base
   def reload_diff(current_user = nil)
     return unless open?
 
-    old_diff_refs = self.diff_refs
-    new_diff = create_merge_request_diff
-
-    MergeRequests::MergeRequestDiffCacheService.new.execute(self, new_diff)
-
-    new_diff_refs = self.diff_refs
-
-    update_diff_discussion_positions(
-      old_diff_refs: old_diff_refs,
-      new_diff_refs: new_diff_refs,
-      current_user: current_user
-    )
+    MergeRequests::ReloadDiffsService.new(self, current_user).execute
   end
 
   def check_if_can_be_merged
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 06aa67c600f..3d72c447b4b 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -3,6 +3,7 @@ class MergeRequestDiff < ActiveRecord::Base
   include Importable
   include ManualInverseAssociation
   include IgnorableColumn
+  include EachBatch
 
   # Don't display more than 100 commits at once
   COMMITS_SAFE_SIZE = 100
@@ -17,8 +18,14 @@ class MergeRequestDiff < ActiveRecord::Base
   has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) }
 
   state_machine :state, initial: :empty do
+    event :clean do
+      transition any => :without_files
+    end
+
     state :collected
     state :overflow
+    # Diff files have been deleted by the system
+    state :without_files
     # Deprecated states: these are no longer used but these values may still occur
     # in the database.
     state :timeout
@@ -27,6 +34,7 @@ class MergeRequestDiff < ActiveRecord::Base
     state :overflow_diff_lines_limit
   end
 
+  scope :with_files, -> { without_states(:without_files, :empty) }
   scope :viewable, -> { without_state(:empty) }
   scope :by_commit_sha, ->(sha) do
     joins(:merge_request_diff_commits).where(merge_request_diff_commits: { sha: sha }).reorder(nil)
@@ -42,6 +50,10 @@ class MergeRequestDiff < ActiveRecord::Base
     find_by(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha)
   end
 
+  def viewable?
+    collected? || without_files? || overflow?
+  end
+
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
@@ -170,6 +182,21 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def diffs(diff_options = nil)
+    if without_files? && comparison = diff_refs.compare_in(project)
+      # It should fetch the repository when diffs are cleaned by the system.
+      # We don't keep these for storage overload purposes.
+      # See https://gitlab.com/gitlab-org/gitlab-ce/issues/37639
+      comparison.diffs(diff_options)
+    else
+      diffs_collection(diff_options)
+    end
+  end
+
+  # Should always return the DB persisted diffs collection
+  # (e.g. Gitlab::Diff::FileCollection::MergeRequestDiff.
+  # It's useful when trying to invalidate old caches through
+  # FileCollection::MergeRequestDiff#clear_cache!
+  def diffs_collection(diff_options = nil)
     Gitlab::Diff::FileCollection::MergeRequestDiff.new(self, diff_options: diff_options)
   end