diff --git a/app/controllers/projects/merge_requests/diffs_stream_controller.rb b/app/controllers/projects/merge_requests/diffs_stream_controller.rb index 7a824f96f79b7308a84d548963ab4ae36277da32..84f6c3bff5bb2033ef067f5c3ae342bc4a4fb012 100644 --- a/app/controllers/projects/merge_requests/diffs_stream_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_stream_controller.rb @@ -13,21 +13,47 @@ def diffs stream_headers offset = params[:offset].to_i - view = helpers.diff_view - - @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff| - response.stream.write( - render_to_string( - ::RapidDiffs::DiffFileComponent.new(diff_file: diff, parallel_view: view == :parallel), - layout: false - ) - ) + + # NOTE: This is a temporary flag to test out the new diff_blobs + if !!ActiveModel::Type::Boolean.new.cast(params[:diff_blobs]) + stream_diff_blobs(offset) + else + stream_diff_files(offset) end + rescue StandardError => e + Gitlab::AppLogger.error("Error streaming diffs: #{e.message}") response.stream.write e.message ensure response.stream.close end + + private + + def view + helpers.diff_view + end + + def stream_diff_blobs(offset) + @merge_request.diffs_for_streaming(offset_index: offset) do |diff_files_batch| + diff_files_batch.each do |diff_file| + response.stream.write(render_diff_file(diff_file)) + end + end + end + + def stream_diff_files(offset) + @merge_request.diffs_for_streaming(offset_index: offset).diff_files.each do |diff_file| + response.stream.write(render_diff_file(diff_file)) + end + end + + def render_diff_file(diff_file) + render_to_string( + ::RapidDiffs::DiffFileComponent.new(diff_file: diff_file, parallel_view: view == :parallel), + layout: false + ) + end end end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0b6504b3b460ea50d40d22584c2842a38bcd9729..93ec04308668089865565dd2822374168490aedb 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -849,10 +849,16 @@ def raw_diffs(*args) compare.present? ? compare.raw_diffs(*args) : merge_request_diff.raw_diffs(*args) end - def diffs_for_streaming(diff_options = {}) + def diffs_for_streaming(diff_options = {}, &) diff = diffable_merge_ref? ? merge_head_diff : merge_request_diff - diff.diffs(diff_options) + offset = diff_options[:offset_index].to_i || 0 + + if block_given? + source_project.repository.diffs_by_changed_paths(diff.diff_refs, offset, &) + else + diff.diffs(diff_options) + end end def diffs(diff_options = {}) diff --git a/lib/gitlab/git/changed_path.rb b/lib/gitlab/git/changed_path.rb index 6883a35ff35168828e9a83dcde8af1e6d7219198..4c2ffcf699b0950825af318a35244a9bb317228d 100644 --- a/lib/gitlab/git/changed_path.rb +++ b/lib/gitlab/git/changed_path.rb @@ -3,15 +3,16 @@ module Gitlab module Git class ChangedPath - attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id + attr_reader :status, :path, :old_mode, :new_mode, :new_blob_id, :old_blob_id, :old_path - def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_id: nil) + def initialize(status:, path:, old_mode:, new_mode:, new_blob_id: nil, old_blob_id: nil, old_path: nil) @status = status @path = path @old_mode = old_mode @new_mode = new_mode @old_blob_id = old_blob_id @new_blob_id = new_blob_id + @old_path = old_path.presence || @path end def new_file? diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 4d3df2dc0a95320220b6d019fd5c8fe5fb24b3f6..b0843af27f868ad88de3f5a497700c7eab2576b5 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -536,13 +536,13 @@ def diff_stats(left_id, right_id) empty_diff_stats end - def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil) + def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil, find_renames: false) processed_objects = treeish_objects.compact return [] if processed_objects.empty? wrapped_gitaly_errors do - gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode) + gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode, find_renames: find_renames) end rescue CommandError, TypeError, NoRepository [] @@ -1259,8 +1259,52 @@ def detect_generated_files(base, head, changed_paths) end # rubocop: enable CodeReuse/ActiveRecord + def diffs_by_changed_paths(diff_refs, offset, batch_size = 30) + changed_paths = find_changed_paths( + [Gitlab::Git::DiffTree.new(diff_refs.base_sha, diff_refs.head_sha)], + find_renames: true + ) + + changed_paths.drop(offset).each_slice(batch_size) do |batched_changed_paths| + blob_pairs = batched_changed_paths.map do |changed_path| + Gitaly::DiffBlobsRequest::BlobPair.new( + left_blob: changed_path.old_blob_id, + right_blob: changed_path.new_blob_id + ) + end + + yield diff_files_by_blob_pairs(blob_pairs, batched_changed_paths, diff_refs) + end + end + private + def diff_files_by_blob_pairs(blob_pairs, changed_paths, diff_refs) + diff_blobs = diff_blobs(blob_pairs, patch_bytes_limit: Gitlab::Git::Diff.patch_hard_limit_bytes) + + changed_diff_blobs = diff_blobs.zip(changed_paths) + + changed_diff_blobs.map do |diff_blob, changed_path| + diff = Gitlab::Git::Diff.new({ + diff: diff_blob.patch, + too_large: diff_blob.over_patch_bytes_limit, + new_path: changed_path.path, + old_path: changed_path.old_path, + a_mode: changed_path.old_mode, + b_mode: changed_path.new_mode, + new_file: changed_path.status == :ADDED, + renamed_file: changed_path.status == :RENAMED, + deleted_file: changed_path.status == :DELETED + }) + + Gitlab::Diff::File.new( + diff, + repository: container.repository, + diff_refs: diff_refs + ) + end + end + def check_blobs_generated(base, head, changed_paths) wrapped_gitaly_errors do gitaly_analysis_client.check_blobs_generated(base, head, changed_paths) diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 34a6de540418eb0d61d9c5e7212d9de5059801a1..c7285f14baa5c7db9d3b7828dd0ac9c3d0873322 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -251,8 +251,8 @@ def diff_stats(left_commit_sha, right_commit_sha) # else # defaults to :include_merges behavior # ['foo_bar.rb', 'bar_baz.rb'], # - def find_changed_paths(objects, merge_commit_diff_mode: nil) - request = find_changed_paths_request(objects, merge_commit_diff_mode) + def find_changed_paths(objects, merge_commit_diff_mode: nil, find_renames: false) + request = find_changed_paths_request(objects, merge_commit_diff_mode, find_renames) return [] if request.nil? @@ -262,6 +262,7 @@ def find_changed_paths(objects, merge_commit_diff_mode: nil) Gitlab::Git::ChangedPath.new( status: path.status, path: EncodingHelper.encode!(path.path), + old_path: EncodingHelper.encode!(path.old_path), old_mode: path.old_mode.to_s(8), new_mode: path.new_mode.to_s(8), old_blob_id: path.old_blob_id, @@ -647,7 +648,7 @@ def call_find_commit(revision) response.commit end - def find_changed_paths_request(objects, merge_commit_diff_mode) + def find_changed_paths_request(objects, merge_commit_diff_mode, find_renames) diff_mode = MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] if Feature.enabled?(:merge_commit_diff_modes) requests = objects.filter_map do |object| @@ -667,7 +668,7 @@ def find_changed_paths_request(objects, merge_commit_diff_mode) return if requests.blank? - Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: requests, merge_commit_diff_mode: diff_mode) + Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: requests, merge_commit_diff_mode: diff_mode, find_renames: find_renames) end def path_error_message(path_error) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 36b51de7060c37650a94f47c04ad0abdebd4df78..580697fd5fb839572d00e9dc72b2eba1101978c1 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1740,7 +1740,7 @@ def create_commit(blobs) let(:commit_1_files) do [ Gitlab::Git::ChangedPath.new( - status: :ADDED, path: "files/executables/ls", old_mode: "0", new_mode: "100755", + status: :ADDED, path: "files/executables/ls", old_path: '', old_mode: "0", new_mode: "100755", old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: 'c84acd1ff0b844201312052f9bb3b7259eb2e177' ) ] @@ -1749,7 +1749,7 @@ def create_commit(blobs) let(:commit_2_files) do [ Gitlab::Git::ChangedPath.new( - status: :ADDED, path: "bar/branch-test.txt", old_mode: "0", new_mode: "100644", + status: :ADDED, path: "bar/branch-test.txt", old_path: '', old_mode: "0", new_mode: "100644", old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '93e123ac8a3e6a0b600953d7598af629dec7b735' ) ] @@ -1758,11 +1758,11 @@ def create_commit(blobs) let(:commit_3_files) do [ Gitlab::Git::ChangedPath.new( - status: :MODIFIED, path: ".gitmodules", old_mode: "100644", new_mode: "100644", + status: :MODIFIED, path: ".gitmodules", old_path: '', old_mode: "100644", new_mode: "100644", old_blob_id: 'fdaada1754989978413d618ee1fb1c0469d6a664', new_blob_id: '0792c58905eff3432b721f8c4a64363d8e28d9ae' ), Gitlab::Git::ChangedPath.new( - status: :ADDED, path: "gitlab-shell", old_mode: "0", new_mode: "160000", + status: :ADDED, path: "gitlab-shell", old_path: '', old_mode: "0", new_mode: "160000", old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '79bceae69cb5750d6567b223597999bfa91cb3b9' ) ] @@ -1771,15 +1771,15 @@ def create_commit(blobs) let(:initial_commit_files) do [ Gitlab::Git::ChangedPath.new( - status: :ADDED, path: ".gitignore", old_mode: "0", new_mode: "100644", + status: :ADDED, path: ".gitignore", old_path: '', old_mode: "0", new_mode: "100644", old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '470ad2fcf1e33798f1afc5781d08e60c40f51e7a' ), Gitlab::Git::ChangedPath.new( - status: :ADDED, path: "LICENSE", old_mode: "0", new_mode: "100644", + status: :ADDED, path: "LICENSE", old_path: '', old_mode: "0", new_mode: "100644", old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '50b27c6518be44c42c4d87966ae2481ce895624c' ), Gitlab::Git::ChangedPath.new( - status: :ADDED, path: "README.md", old_mode: "0", new_mode: "100644", + status: :ADDED, path: "README.md", old_path: '', old_mode: "0", new_mode: "100644", old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: 'faaf198af3a36dbf41961466703cc1d47c61d051' ) ] @@ -1816,7 +1816,7 @@ def create_commit(blobs) expect(collection.to_a).to be_empty end - describe 'merge_commit_diff_mode argument' do + describe 'optional arguments' do let(:gitaly_commit_client) { double('Gitlab::GitalyClient::CommitService') } before do @@ -1829,24 +1829,38 @@ def create_commit(blobs) repository.find_changed_paths(['sha']) end - it 'defaults to nil' do + it 'returns default values' do expect(gitaly_commit_client) .to have_received(:find_changed_paths) - .with(['sha'], merge_commit_diff_mode: nil) + .with(['sha'], merge_commit_diff_mode: nil, find_renames: false) end end - context 'when included' do - let(:passed_value) { 'foobar' } + context 'merge_commit_diff_mode is given' do + let(:merge_commit_diff_mode) { 'foobar' } before do - repository.find_changed_paths(['sha'], merge_commit_diff_mode: passed_value) + repository.find_changed_paths(['sha'], merge_commit_diff_mode: merge_commit_diff_mode) end it 'passes the value on to the commit client' do expect(gitaly_commit_client) .to have_received(:find_changed_paths) - .with(['sha'], merge_commit_diff_mode: passed_value) + .with(['sha'], merge_commit_diff_mode: merge_commit_diff_mode, find_renames: false) + end + end + + context 'find_names is included' do + let(:find_renames) { true } + + before do + repository.find_changed_paths(['sha'], find_renames: find_renames) + end + + it 'passes the value on to the commit client' do + expect(gitaly_commit_client) + .to have_received(:find_changed_paths) + .with(['sha'], merge_commit_diff_mode: nil, find_renames: find_renames) end end end @@ -3015,6 +3029,156 @@ def create_remote_branch(remote_name, branch_name, source_branch_name) end end + describe '#diffs_by_changed_path' do + let(:changed_paths) do + [ + Gitlab::Git::ChangedPath.new( + status: :ADDED, path: "added_file.rb", old_path: '', old_mode: "0", new_mode: "100644", + old_blob_id: '0000000000000000000000000000000000000000', new_blob_id: '470ad2fcf1e33798f1afc5781d08e60c40f51e7a' + ), + Gitlab::Git::ChangedPath.new( + status: :RENAMED, path: "renamed_file.rb", old_path: 'original_file.rb', old_mode: "100644", new_mode: "100644", + old_blob_id: '93e123ac8a3e6a0b600953d7598af629dec7b735', new_blob_id: '50b27c6518be44c42c4d87966ae2481ce895624c' + ), + Gitlab::Git::ChangedPath.new( + status: :MODIFIED, path: "modified_file.rb", old_path: '', old_mode: "100644", new_mode: "100644", + old_blob_id: 'fdaada1754989978413d618ee1fb1c0469d6a664', new_blob_id: '0792c58905eff3432b721f8c4a64363d8e28d9ae' + ), + Gitlab::Git::ChangedPath.new( + status: :DELETED, path: "deleted_file.rb", old_path: '', old_mode: "100644", new_mode: "0", + old_blob_id: '79bceae69cb5750d6567b223597999bfa91cb3b9', new_blob_id: '0000000000000000000000000000000000000000' + ) + ] + end + + let(:diff_refs) do + Gitlab::Diff::DiffRefs.new( + base_sha: "913c66a37b4a45b9769037c55c2d238bd0942d2e", + head_sha: "874797c3a73b60d2187ed6e2fcabd289ff75171e" + ) + end + + let(:blob_pairs) do + changed_paths.map do |changed_path| + Gitaly::DiffBlobsRequest::BlobPair.new( + left_blob: changed_path.old_blob_id, + right_blob: changed_path.new_blob_id + ) + end + end + + let(:diff_blobs) do + changed_paths.map do |changed_path| + Gitlab::GitalyClient::DiffBlob.new( + left_blob_id: changed_path.old_blob_id, + right_blob_id: changed_path.new_blob_id, + patch: 'some content' + ) + end + end + + before do + allow(repository).to receive(:find_changed_paths) + .once + .with([kind_of(Gitlab::Git::DiffTree)], find_renames: true) + .and_return(changed_paths) + end + + context 'diff file count is smaller than the default batch size' do + before do + allow(repository).to receive(:diff_blobs) + .once + .with(blob_pairs, anything) + .and_return(diff_blobs) + end + + it 'returns diff filess in batches' do + diff_files = [] + + repository.diffs_by_changed_paths(diff_refs, 0) do |batch| + diff_files << batch + end + + expect(diff_files.count).to eq 1 + + first_batch = diff_files[0] + + expect(first_batch.count).to eq 4 + expect(first_batch[0].new_path).to eq 'added_file.rb' + expect(first_batch[0].new_file?).to eq true + expect(first_batch[1].new_path).to eq 'renamed_file.rb' + expect(first_batch[1].renamed_file?).to eq true + expect(first_batch[2].new_path).to eq 'modified_file.rb' + expect(first_batch[2].new_file?).to eq false + expect(first_batch[3].new_path).to eq 'deleted_file.rb' + expect(first_batch[3].deleted_file?).to eq true + end + end + + context 'offset is given' do + before do + allow(repository).to receive(:diff_blobs) + .once + .with(blob_pairs[3..], anything) + .and_return(diff_blobs[3..]) + end + + it 'returns diff filess in batches' do + diff_files = [] + + repository.diffs_by_changed_paths(diff_refs, 3) do |batch| + diff_files << batch + end + + expect(diff_files.count).to eq 1 + + first_batch = diff_files[0] + + expect(first_batch.count).to eq 1 + expect(first_batch[0].new_path).to eq 'deleted_file.rb' + expect(first_batch[0].deleted_file?).to eq true + end + end + + context 'batch_size is given' do + before do + allow(repository).to receive(:diff_blobs) + .once + .with(blob_pairs[..1], anything) + .and_return(diff_blobs[..1]) + + allow(repository).to receive(:diff_blobs) + .once + .with(blob_pairs[2..], anything) + .and_return(diff_blobs[2..]) + end + + it 'returns diff filess in batches' do + diff_files = [] + + repository.diffs_by_changed_paths(diff_refs, 0, 2) do |batch| + diff_files << batch + end + + expect(diff_files.count).to eq 2 + + first_batch = diff_files[0] + second_batch = diff_files[1] + + expect(first_batch.count).to eq 2 + expect(first_batch[0].new_path).to eq 'added_file.rb' + expect(first_batch[0].new_file?).to eq true + expect(first_batch[1].new_path).to eq 'renamed_file.rb' + expect(first_batch[1].renamed_file?).to eq true + expect(second_batch.count).to eq 2 + expect(second_batch[0].new_path).to eq 'modified_file.rb' + expect(second_batch[0].new_file?).to eq false + expect(second_batch[1].new_path).to eq 'deleted_file.rb' + expect(second_batch[1].deleted_file?).to eq true + end + end + end + describe '#gitaly_diff_client' do it 'instantiates a new DiffService class' do expect(repository.gitaly_diff_client).to be_a(Gitlab::GitalyClient::DiffService) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index 9f212598310a4f5b4f31d6bbcbb937301f040ef7..79eaec7e9993ae431f4b7dd9fd4313eaeee3d11c 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -169,6 +169,7 @@ describe '#find_changed_paths' do let(:mapped_merge_commit_diff_mode) { described_class::MERGE_COMMIT_DIFF_MODES[merge_commit_diff_mode] } + let(:find_renames) { false } let(:commits) do %w[ ade1c0b4b116209ed2a9958436b26f89085ec383 @@ -204,47 +205,47 @@ let(:changed_paths) do [ { - path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/foo.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391" }, { - path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', + path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/foo.lfs', old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007" }, { - path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/bar.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1" }, { - path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', + path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/foo.lfs', old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007" }, { - path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/bar.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1" }, { - path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', + path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/bar.lfs', old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1", new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba" }, { - path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', + path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/bar.lfs', old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1", new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba" }, { - path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9" }, { - path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9" } @@ -260,32 +261,32 @@ let(:changed_paths) do [ { - path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/foo.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/foo.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391" }, { - path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', + path: 'files/locked/foo.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/foo.lfs', old_blob_id: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", new_blob_id: "3eac02ca74e5b8e5df01dbdfdd7a9905c5e12007" }, { - path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/bar.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/bar.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1" }, { - path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', + path: 'files/locked/bar.lfs', status: 'MODIFIED', old_mode: '100644', new_mode: '100644', old_path: 'files/locked/bar.lfs', old_blob_id: "ea6c0a2142103f2d9157c1a9d50cc708032ec4a1", new_blob_id: "9d8e9599c93013dee199bfdc13e8365c11652bba" }, { - path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9" }, { - path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', + path: 'files/locked/baz.lfs', status: 'ADDED', old_mode: '0', new_mode: '100644', old_path: 'files/locked/baz.lfs', old_blob_id: "0000000000000000000000000000000000000000", new_blob_id: "dd1a523861a19addf2cce888119a07560be334b9" } @@ -304,7 +305,8 @@ .to have_received(:new).with( repository: repository_message, requests: requests, - merge_commit_diff_mode: mapped_merge_commit_diff_mode + merge_commit_diff_mode: mapped_merge_commit_diff_mode, + find_renames: find_renames ) end end @@ -397,6 +399,67 @@ end end + context 'when renamed file exists' do + let(:branch) { 'gitaly-rename-test' } + let(:treeish_objects) { [repository.commit(branch)] } + + subject(:find_changed_paths) do + described_class + .new(repository) + .find_changed_paths(treeish_objects, find_renames: find_renames) + .as_json + end + + context 'when find_renames is true' do + let(:find_renames) { true } + + it 'detects renamed file and includes old_path' do + expected_changed_paths = [ + { + "new_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb", + "new_mode" => "100644", + "old_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb", + "old_mode" => "100644", + "old_path" => "CHANGELOG", + "path" => "CHANGELOG.md", + "status" => "RENAMED" + } + ] + + expect(find_changed_paths).to eq expected_changed_paths + end + end + + context 'when find_renames is false' do + let(:find_renames) { false } + + it 'does not detect renamed file' do + expected_changed_paths = [ + { + "new_blob_id" => "0000000000000000000000000000000000000000", + "new_mode" => "0", + "old_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb", + "old_mode" => "100644", + "old_path" => "CHANGELOG", + "path" => "CHANGELOG", + "status" => "DELETED" + }, + { + "new_blob_id" => "53855584db773c3df5b5f61f72974cb298822fbb", + "new_mode" => "100644", + "old_blob_id" => "0000000000000000000000000000000000000000", + "old_mode" => "0", + "old_path" => "CHANGELOG.md", + "path" => "CHANGELOG.md", + "status" => "ADDED" + } + ] + + expect(find_changed_paths).to eq expected_changed_paths + end + end + end + context 'when all requested objects are invalid' do it 'does not send RPC request' do expect_any_instance_of(Gitaly::DiffService::Stub).not_to receive(:find_changed_paths) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 95c8a2c736c2ac617655b290807870d0665ff583..8ef7220667569bbaa4e32c07f8d9a433b423342a 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -6986,6 +6986,36 @@ def transition! expect(merge_request.diffs_for_streaming).to eq(['HEAD diff']) end end + + context 'when block is given' do + let(:diff_refs) { instance_double(Gitlab::Diff::DiffRefs) } + let(:expected_block) { proc {} } + let(:repository) { merge_request.source_project.repository } + + before do + allow(base_diff).to receive(:diff_refs).and_return(diff_refs) + end + + it 'calls diffs_by_changed_paths with given offset' do + expect(repository).to receive(:diffs_by_changed_paths).with(diff_refs, 0) do |_, &block| + expect(block).to be(expected_block) + end + + merge_request.diffs_for_streaming(&expected_block) + end + + context 'when offset_index is given' do + let(:offset) { 5 } + + it 'calls diffs_by_changed_paths with given offset' do + expect(repository).to receive(:diffs_by_changed_paths).with(diff_refs, offset) do |_, &block| + expect(block).to be(expected_block) + end + + merge_request.diffs_for_streaming({ offset_index: offset }, &expected_block) + end + end + end end describe '#merge_exclusive_lease' do diff --git a/spec/requests/projects/merge_requests/diffs_stream_spec.rb b/spec/requests/projects/merge_requests/diffs_stream_spec.rb index 68288ea9cf0b0d8a3c3b4ba8e16f0712065d6fab..b6b78ca28fb35e100bd5066873b5f7a4dcaf45c7 100644 --- a/spec/requests/projects/merge_requests/diffs_stream_spec.rb +++ b/spec/requests/projects/merge_requests/diffs_stream_spec.rb @@ -24,6 +24,8 @@ def go(**extra_params) let_it_be_with_reload(:merge_request) do create( :merge_request_with_diffs, + source_branch: 'expand-collapse-files', + target_branch: 'master', target_project: project, source_project: project ) @@ -91,6 +93,33 @@ def go(**extra_params) expect(response).to have_gitlab_http_status(:not_found) end end + + context 'with diffs_blob option' do + context 'when offset is not given' do + it 'streams all diffs' do + go(diff_blobs: true) + + expect(response).to have_gitlab_http_status(:success) + expect(response.body).to include(*file_identifier_hashes(merge_request.merge_request_diff)) + end + end + + context 'when offset is given' do + let(:offset) { 5 } + + it 'streams diffs except the offset' do + go(diff_blobs: true, offset: offset) + + diff_files = merge_request.merge_request_diff.diffs.diff_files.to_a + offset_file_identifier_hashes = diff_files.take(offset).map(&:file_identifier_hash) + remaining_file_identifier_hashes = diff_files.slice(offset..).map(&:file_identifier_hash) + + expect(response).to have_gitlab_http_status(:success) + expect(response.body).not_to include(*offset_file_identifier_hashes) + expect(response.body).to include(*remaining_file_identifier_hashes) + end + end + end end def file_identifier_hashes(diff)