Commit 2c2422d5 authored by Sean McGivern's avatar Sean McGivern

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 eb36fa17, 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.
parent 2dfddddd
---
title: Fix merge request diffs when created with gitaly_diff_between enabled
merge_request:
author:
type: fixed
...@@ -368,7 +368,7 @@ module Gitlab ...@@ -368,7 +368,7 @@ module Gitlab
def call_commit_diff(request_params, options = {}) def call_commit_diff(request_params, options = {})
request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false) request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false)
request_params[:enforce_limits] = options.fetch(:limits, true) 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_params.merge!(Gitlab::Git::DiffCollection.collection_limits(options).to_h)
request = Gitaly::CommitDiffRequest.new(request_params) request = Gitaly::CommitDiffRequest.new(request_params)
......
...@@ -17,7 +17,7 @@ describe Gitlab::GitalyClient::CommitService do ...@@ -17,7 +17,7 @@ describe Gitlab::GitalyClient::CommitService do
repository: repository_message, repository: repository_message,
left_commit_id: 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660', left_commit_id: 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660',
right_commit_id: commit.id, right_commit_id: commit.id,
collapse_diffs: true, collapse_diffs: false,
enforce_limits: true, enforce_limits: true,
**Gitlab::Git::DiffCollection.collection_limits.to_h **Gitlab::Git::DiffCollection.collection_limits.to_h
) )
...@@ -35,7 +35,7 @@ describe Gitlab::GitalyClient::CommitService do ...@@ -35,7 +35,7 @@ describe Gitlab::GitalyClient::CommitService do
repository: repository_message, repository: repository_message,
left_commit_id: Gitlab::Git::EMPTY_TREE_ID, left_commit_id: Gitlab::Git::EMPTY_TREE_ID,
right_commit_id: initial_commit.id, right_commit_id: initial_commit.id,
collapse_diffs: true, collapse_diffs: false,
enforce_limits: true, enforce_limits: true,
**Gitlab::Git::DiffCollection.collection_limits.to_h **Gitlab::Git::DiffCollection.collection_limits.to_h
) )
......
...@@ -153,6 +153,13 @@ describe MergeRequestDiff do ...@@ -153,6 +153,13 @@ describe MergeRequestDiff do
expect(mr_diff.empty?).to be_truthy expect(mr_diff.empty?).to be_truthy
end 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 it 'saves binary diffs correctly' do
path = 'files/images/icn-time-tracking.pdf' path = 'files/images/icn-time-tracking.pdf'
mr_diff = create(:merge_request, source_branch: 'add-pdf-text-binary', target_branch: 'master').merge_request_diff mr_diff = create(:merge_request, source_branch: 'add-pdf-text-binary', target_branch: 'master').merge_request_diff
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment