Commit be8a320b authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

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.
parent 5af7fd59
...@@ -54,7 +54,20 @@ class DiffNote < Note ...@@ -54,7 +54,20 @@ class DiffNote < Note
end end
def diff_file 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 end
def diff_line def diff_line
......
---
title: Use persisted diff data instead fetching Git on discussions
merge_request:
author:
type: performance
...@@ -36,6 +36,8 @@ module Gitlab ...@@ -36,6 +36,8 @@ module Gitlab
private private
def decorate_diff!(diff) 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) Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
end end
end end
......
...@@ -85,12 +85,35 @@ describe DiffNote do ...@@ -85,12 +85,35 @@ describe DiffNote do
end end
describe "#diff_file" do describe "#diff_file" do
it "returns the correct diff file" do context 'when the discussion was created in the diff' do
diff_file = subject.diff_file it 'returns correct diff file' do
diff_file = subject.diff_file
expect(diff_file.old_path).to eq(position.old_path) expect(diff_file.old_path).to eq(position.old_path)
expect(diff_file.new_path).to eq(position.new_path) expect(diff_file.new_path).to eq(position.new_path)
expect(diff_file.diff_refs).to eq(position.diff_refs) 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
end end
......
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