From be8a320bd8594fe42c2558d2eab471acdbdc7321 Mon Sep 17 00:00:00 2001
From: Oswaldo Ferreira <oswaldo@gitlab.com>
Date: Mon, 30 Apr 2018 18:44:37 -0300
Subject: [PATCH] Use persisted diff data instead fetching Git on discussions
Today, when fetching diffs of a note, we always go to Gitaly in order to diff between commits and return the diff of each discussion note. With this change we avoid doing that for notes on the "current version" of the MR.
---
app/models/diff_note.rb | 15 ++++++++-
...ed-highlighted-content-for-discussions.yml | 5 +++
lib/gitlab/diff/file_collection/base.rb | 2 ++
spec/models/diff_note_spec.rb | 33 ++++++++++++++++---
4 files changed, 49 insertions(+), 6 deletions(-)
create mode 100644 changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 15122cbc6935d..616a626419bf9 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -54,7 +54,20 @@ def on_image?
end
def diff_file
- @diff_file ||= self.original_position.diff_file(self.project.repository)
+ @diff_file ||=
+ begin
+ if created_at_diff?(noteable.diff_refs)
+ # We're able to use the already persisted diffs (Postgres) if we're
+ # presenting a "current version" of the MR discussion diff.
+ # So no need to make an extra Gitaly diff request for it.
+ # As an extra benefit, the returned `diff_file` already
+ # has `highlighted_diff_lines` data set from Redis on
+ # `Diff::FileCollection::MergeRequestDiff`.
+ noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
+ else
+ original_position.diff_file(self.project.repository)
+ end
+ end
end
def diff_line
diff --git a/changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml b/changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml
new file mode 100644
index 0000000000000..03a11a3038af3
--- /dev/null
+++ b/changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml
@@ -0,0 +1,5 @@
+---
+title: Use persisted diff data instead fetching Git on discussions
+merge_request:
+author:
+type: performance
diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb
index a6007ebf531b8..c79d8d3cb215f 100644
--- a/lib/gitlab/diff/file_collection/base.rb
+++ b/lib/gitlab/diff/file_collection/base.rb
@@ -36,6 +36,8 @@ def diff_file_with_new_path(new_path)
private
def decorate_diff!(diff)
+ return diff if diff.is_a?(File)
+
Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
end
end
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 2705421e540a4..fb51c0172ab9d 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -85,12 +85,35 @@
end
describe "#diff_file" do
- it "returns the correct diff file" do
- diff_file = subject.diff_file
+ context 'when the discussion was created in the diff' do
+ it 'returns correct diff file' do
+ diff_file = subject.diff_file
- expect(diff_file.old_path).to eq(position.old_path)
- expect(diff_file.new_path).to eq(position.new_path)
- expect(diff_file.diff_refs).to eq(position.diff_refs)
+ expect(diff_file.old_path).to eq(position.old_path)
+ expect(diff_file.new_path).to eq(position.new_path)
+ expect(diff_file.diff_refs).to eq(position.diff_refs)
+ end
+ end
+
+ context 'when discussion is outdated or not created in the diff' do
+ let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
+ let(:position) do
+ Gitlab::Diff::Position.new(
+ old_path: "files/ruby/popen.rb",
+ new_path: "files/ruby/popen.rb",
+ old_line: nil,
+ new_line: 14,
+ diff_refs: diff_refs
+ )
+ end
+
+ it 'returns the correct diff file' do
+ diff_file = subject.diff_file
+
+ expect(diff_file.old_path).to eq(position.old_path)
+ expect(diff_file.new_path).to eq(position.new_path)
+ expect(diff_file.diff_refs).to eq(position.diff_refs)
+ end
end
end
--
GitLab