From 2c2422d54e4b12471dbc25dbc90cbbffe4fb1c2b Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 29 Jun 2018 14:25:35 +0100
Subject: [PATCH] Fix MR diffs created with gitaly_diff_between enabled

When we save merge request diffs to the database, we need to expand the diff
before doing so. That's so that we can expand diffs (within the normal limits)
without hitting the repository, but just by going to the database.

This is done implicitly - diffs are expanded unless we say otherwise. However,
we have another option we can pass, that lets us enforce diff size limits, that
defaults to true.

Prior to this commit:

- The Rugged code path defaulted to setting `expanded: true` and
  `enforce_limits: true`.
- The Gitaly code path defaulted to setting `expanded: false` and
  `enforce_limits: true`.

This was introduced by eb36fa17a6ae5cda8950904b5a52e6aa365ae591, which
implemented the initial feature. Since then, if the `gitaly_diff_between`
feature flag was enabled, MRs would have diffs that could not be expanded in
some cases, with no fix other than to disable the feature flag and force push to
the MR to refresh the diff in the database.
---
 changelogs/unreleased/fix-gitaly-mr-creation-limits.yml | 5 +++++
 lib/gitlab/gitaly_client/commit_service.rb              | 2 +-
 spec/lib/gitlab/gitaly_client/commit_service_spec.rb    | 4 ++--
 spec/models/merge_request_diff_spec.rb                  | 7 +++++++
 4 files changed, 15 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/fix-gitaly-mr-creation-limits.yml

diff --git a/changelogs/unreleased/fix-gitaly-mr-creation-limits.yml b/changelogs/unreleased/fix-gitaly-mr-creation-limits.yml
new file mode 100644
index 00000000000..e903f2e5277
--- /dev/null
+++ b/changelogs/unreleased/fix-gitaly-mr-creation-limits.yml
@@ -0,0 +1,5 @@
+---
+title: Fix merge request diffs when created with gitaly_diff_between enabled
+merge_request:
+author:
+type: fixed
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index c9c414e5d33..d979ba0eb14 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -368,7 +368,7 @@ module Gitlab
       def call_commit_diff(request_params, options = {})
         request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false)
         request_params[:enforce_limits] = options.fetch(:limits, true)
-        request_params[:collapse_diffs] = request_params[:enforce_limits] || !options.fetch(:expanded, true)
+        request_params[:collapse_diffs] = !options.fetch(:expanded, true)
         request_params.merge!(Gitlab::Git::DiffCollection.collection_limits(options).to_h)
 
         request = Gitaly::CommitDiffRequest.new(request_params)
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index 7951cbe7b1d..54f2ea33f90 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -17,7 +17,7 @@ describe Gitlab::GitalyClient::CommitService do
           repository: repository_message,
           left_commit_id: 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660',
           right_commit_id: commit.id,
-          collapse_diffs: true,
+          collapse_diffs: false,
           enforce_limits: true,
           **Gitlab::Git::DiffCollection.collection_limits.to_h
         )
@@ -35,7 +35,7 @@ describe Gitlab::GitalyClient::CommitService do
           repository: repository_message,
           left_commit_id: Gitlab::Git::EMPTY_TREE_ID,
           right_commit_id: initial_commit.id,
-          collapse_diffs: true,
+          collapse_diffs: false,
           enforce_limits: true,
           **Gitlab::Git::DiffCollection.collection_limits.to_h
         )
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 48c01fc4d4e..ccc3ff861c5 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -153,6 +153,13 @@ describe MergeRequestDiff do
       expect(mr_diff.empty?).to be_truthy
     end
 
+    it 'expands collapsed diffs before saving' do
+      mr_diff = create(:merge_request, source_branch: 'expand-collapse-lines', target_branch: 'master').merge_request_diff
+      diff_file = mr_diff.merge_request_diff_files.find_by(new_path: 'expand-collapse/file-5.txt')
+
+      expect(diff_file.diff).not_to be_empty
+    end
+
     it 'saves binary diffs correctly' do
       path = 'files/images/icn-time-tracking.pdf'
       mr_diff = create(:merge_request, source_branch: 'add-pdf-text-binary', target_branch: 'master').merge_request_diff