Commit d6cdc4e6 authored by Robert May's avatar Robert May

Merge branch '281171-conflict-type-diffs-batch' into 'master'

Return conflict_type property in diffs_batch.json endpoint

See merge request gitlab-org/gitlab!66719
parents a6bfebb3 7410ad5f
......@@ -57,8 +57,14 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
def diffs_metadata
diffs = @compare.diffs(diff_options)
options = additional_attributes.merge(
only_context_commits: show_only_context_commits?,
merge_ref_head_diff: render_merge_ref_head_diff?,
allow_tree_conflicts: display_merge_conflicts_in_diff?
)
render json: DiffsMetadataSerializer.new(project: @merge_request.project, current_user: current_user)
.represent(diffs, additional_attributes.merge(only_context_commits: show_only_context_commits?))
.represent(diffs, options)
end
private
......
# frozen_string_literal: true
module DiffFileConflictType
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
included do
expose :conflict_type do |diff_file, options|
conflict_file = conflict_file(options, diff_file)
next unless conflict_file
conflict_file.conflict_type(diff_file)
end
end
private
def conflict_file(options, diff_file)
strong_memoize(:conflict_file) do
options[:conflicts] && options[:conflicts][diff_file.new_path]
end
end
end
# frozen_string_literal: true
class DiffFileEntity < DiffFileBaseEntity
include DiffFileConflictType
include CommitsHelper
include IconsHelper
include Gitlab::Utils::StrongMemoize
......@@ -88,10 +89,4 @@ class DiffFileEntity < DiffFileBaseEntity
# If nothing is present, inline will be the default.
options.fetch(:diff_view, :inline).to_sym
end
def conflict_file(options, diff_file)
strong_memoize(:conflict_file) do
options[:conflicts] && options[:conflicts][diff_file.new_path]
end
end
end
# frozen_string_literal: true
class DiffFileMetadataEntity < Grape::Entity
include DiffFileConflictType
expose :added_lines
expose :removed_lines
expose :new_path
......
......@@ -2,8 +2,13 @@
class DiffsMetadataEntity < DiffsEntity
unexpose :diff_files
expose :diff_files, using: DiffFileMetadataEntity do |diffs, _|
diffs.raw_diff_files(sorted: true)
expose :diff_files do |diffs, options|
DiffFileMetadataEntity.represent(
diffs.raw_diff_files(sorted: true),
options.merge(
conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
)
)
end
expose :conflict_resolution_path do |_, options|
......
......@@ -23,7 +23,7 @@ module Gitlab
# 'raw' holds the Gitlab::Git::Conflict::File that this instance wraps
attr_reader :raw
delegate :type, :content, :path, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
delegate :type, :content, :path, :ancestor_path, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
def initialize(raw, merge_request:)
@raw = raw
......@@ -227,6 +227,26 @@ module Gitlab
new_path: our_path)
end
def conflict_type(diff_file)
if ancestor_path.present?
if our_path.present? && their_path.present?
:both_modified
elsif their_path.blank?
:modified_source_removed_target
else
:modified_target_removed_source
end
else
if our_path.present? && their_path.present?
:both_added
elsif their_path.blank?
diff_file.renamed_file? ? :renamed_same_file : :removed_target_renamed_source
else
:removed_source_renamed_target
end
end
end
private
def map_raw_lines(raw_lines)
......
......@@ -6,13 +6,14 @@ module Gitlab
class File
UnsupportedEncoding = Class.new(StandardError)
attr_reader :their_path, :our_path, :our_mode, :repository, :commit_oid
attr_reader :ancestor_path, :their_path, :our_path, :our_mode, :repository, :commit_oid
attr_accessor :raw_content
def initialize(repository, commit_oid, conflict, raw_content)
@repository = repository
@commit_oid = commit_oid
@ancestor_path = conflict[:ancestor][:path]
@their_path = conflict[:theirs][:path]
@our_path = conflict[:ours][:path]
@our_mode = conflict[:ours][:mode]
......
......@@ -43,6 +43,7 @@ module Gitlab
def conflict_from_gitaly_file_header(header)
{
ancestor: { path: header.ancestor_path },
ours: { path: header.our_path, mode: header.our_mode },
theirs: { path: header.their_path }
}
......
......@@ -141,6 +141,24 @@ RSpec.describe Projects::MergeRequests::DiffsController do
end
describe 'GET diffs_metadata' do
shared_examples_for 'serializes diffs metadata with expected arguments' do
it 'returns success' do
subject
expect(response).to have_gitlab_http_status(:ok)
end
it 'serializes paginated merge request diff collection' do
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(collection), expected_options)
.and_call_original
end
subject
end
end
def go(extra_params = {})
params = {
namespace_id: project.namespace.to_param,
......@@ -179,32 +197,25 @@ RSpec.describe Projects::MergeRequests::DiffsController do
end
context 'with valid diff_id' do
it 'returns success' do
go(diff_id: merge_request.merge_request_diff.id)
expect(response).to have_gitlab_http_status(:ok)
end
it 'serializes diffs metadata with expected arguments' do
expected_options = {
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true,
only_context_commits: false
}
subject { go(diff_id: merge_request.merge_request_diff.id) }
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff), expected_options)
.and_call_original
it_behaves_like 'serializes diffs metadata with expected arguments' do
let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiff }
let(:expected_options) do
{
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true,
only_context_commits: false,
allow_tree_conflicts: true,
merge_ref_head_diff: false
}
end
go(diff_id: merge_request.merge_request_diff.id)
end
end
......@@ -261,62 +272,75 @@ RSpec.describe Projects::MergeRequests::DiffsController do
end
context 'with MR regular diff params' do
it 'returns success' do
go
subject { go }
expect(response).to have_gitlab_http_status(:ok)
it_behaves_like 'serializes diffs metadata with expected arguments' do
let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiff }
let(:expected_options) do
{
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true,
only_context_commits: false,
allow_tree_conflicts: true,
merge_ref_head_diff: nil
}
end
end
end
it 'serializes diffs metadata with expected arguments' do
expected_options = {
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true,
only_context_commits: false
}
context 'with commit param' do
subject { go(commit_id: merge_request.diff_head_sha) }
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff), expected_options)
.and_call_original
it_behaves_like 'serializes diffs metadata with expected arguments' do
let(:collection) { Gitlab::Diff::FileCollection::Commit }
let(:expected_options) do
{
environment: nil,
merge_request: merge_request,
merge_request_diff: nil,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: merge_request.diff_head_commit,
latest_diff: nil,
only_context_commits: false,
allow_tree_conflicts: true,
merge_ref_head_diff: nil
}
end
go
end
end
context 'with commit param' do
it 'returns success' do
go(commit_id: merge_request.diff_head_sha)
context 'when display_merge_conflicts_in_diff is disabled' do
subject { go }
expect(response).to have_gitlab_http_status(:ok)
before do
stub_feature_flags(display_merge_conflicts_in_diff: false)
end
it 'serializes diffs metadata with expected arguments' do
expected_options = {
environment: nil,
merge_request: merge_request,
merge_request_diff: nil,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: merge_request.diff_head_commit,
latest_diff: nil,
only_context_commits: false
}
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(Gitlab::Diff::FileCollection::Commit), expected_options)
.and_call_original
it_behaves_like 'serializes diffs metadata with expected arguments' do
let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiff }
let(:expected_options) do
{
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true,
only_context_commits: false,
allow_tree_conflicts: false,
merge_ref_head_diff: nil
}
end
go(commit_id: merge_request.diff_head_sha)
end
end
end
......
......@@ -18,7 +18,15 @@ RSpec.describe Gitlab::Conflict::File do
let(:conflict_file) { described_class.new(raw_conflict_file, merge_request: merge_request) }
describe 'delegates' do
it { expect(conflict_file).to delegate_method(:type).to(:raw) }
it { expect(conflict_file).to delegate_method(:content).to(:raw) }
it { expect(conflict_file).to delegate_method(:path).to(:raw) }
it { expect(conflict_file).to delegate_method(:ancestor_path).to(:raw) }
it { expect(conflict_file).to delegate_method(:their_path).to(:raw) }
it { expect(conflict_file).to delegate_method(:our_path).to(:raw) }
it { expect(conflict_file).to delegate_method(:our_mode).to(:raw) }
it { expect(conflict_file).to delegate_method(:our_blob).to(:raw) }
it { expect(conflict_file).to delegate_method(:repository).to(:raw) }
end
describe '#resolve_lines' do
......@@ -328,4 +336,27 @@ RSpec.describe Gitlab::Conflict::File do
end
end
end
describe '#conflict_type' do
using RSpec::Parameterized::TableSyntax
let(:rugged_conflict) { { ancestor: { path: ancestor_path }, theirs: { path: their_path }, ours: { path: our_path } } }
let(:diff_file) { double(renamed_file?: renamed_file?) }
subject(:conflict_type) { conflict_file.conflict_type(diff_file) }
where(:ancestor_path, :their_path, :our_path, :renamed_file?, :result) do
'/ancestor/path' | '/their/path' | '/our/path' | false | :both_modified
'/ancestor/path' | '' | '/our/path' | false | :modified_source_removed_target
'/ancestor/path' | '/their/path' | '' | false | :modified_target_removed_source
'' | '/their/path' | '/our/path' | false | :both_added
'' | '' | '/our/path' | false | :removed_target_renamed_source
'' | '' | '/our/path' | true | :renamed_same_file
'' | '/their/path' | '' | false | :removed_source_renamed_target
end
with_them do
it { expect(conflict_type).to eq(result) }
end
end
end
......@@ -3,7 +3,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Git::Conflict::File do
let(:conflict) { { theirs: { path: 'foo', mode: 33188 }, ours: { path: 'foo', mode: 33188 } } }
let(:conflict) { { ancestor: { path: 'ancestor' }, theirs: { path: 'foo', mode: 33188 }, ours: { path: 'foo', mode: 33188 } } }
let(:invalid_content) { described_class.new(nil, nil, conflict, (+"a\xC4\xFC").force_encoding(Encoding::ASCII_8BIT)) }
let(:valid_content) { described_class.new(nil, nil, conflict, (+"Espa\xC3\xB1a").force_encoding(Encoding::ASCII_8BIT)) }
......
......@@ -9,22 +9,37 @@ RSpec.describe Gitlab::GitalyClient::ConflictFilesStitcher do
target_repository = target_project.repository.raw
target_gitaly_repository = target_repository.gitaly_repository
ancestor_path_1 = 'ancestor/path/1'
our_path_1 = 'our/path/1'
their_path_1 = 'their/path/1'
our_mode_1 = 0744
commit_oid_1 = 'f00'
content_1 = 'content of the first file'
ancestor_path_2 = 'ancestor/path/2'
our_path_2 = 'our/path/2'
their_path_2 = 'their/path/2'
our_mode_2 = 0600
commit_oid_2 = 'ba7'
content_2 = 'content of the second file'
header_1 = double(repository: target_gitaly_repository, commit_oid: commit_oid_1,
our_path: our_path_1, their_path: their_path_1, our_mode: our_mode_1)
header_2 = double(repository: target_gitaly_repository, commit_oid: commit_oid_2,
our_path: our_path_2, their_path: their_path_2, our_mode: our_mode_2)
header_1 = double(
repository: target_gitaly_repository,
commit_oid: commit_oid_1,
ancestor_path: ancestor_path_1,
our_path: our_path_1,
their_path: their_path_1,
our_mode: our_mode_1
)
header_2 = double(
repository: target_gitaly_repository,
commit_oid: commit_oid_2,
ancestor_path: ancestor_path_2,
our_path: our_path_2,
their_path: their_path_2,
our_mode: our_mode_2
)
messages = [
double(files: [double(header: header_1), double(header: nil, content: content_1[0..5])]),
......@@ -39,6 +54,7 @@ RSpec.describe Gitlab::GitalyClient::ConflictFilesStitcher do
expect(conflict_files.size).to be(2)
expect(conflict_files[0].content).to eq(content_1)
expect(conflict_files[0].ancestor_path).to eq(ancestor_path_1)
expect(conflict_files[0].their_path).to eq(their_path_1)
expect(conflict_files[0].our_path).to eq(our_path_1)
expect(conflict_files[0].our_mode).to be(our_mode_1)
......@@ -46,6 +62,7 @@ RSpec.describe Gitlab::GitalyClient::ConflictFilesStitcher do
expect(conflict_files[0].commit_oid).to eq(commit_oid_1)
expect(conflict_files[1].content).to eq(content_2)
expect(conflict_files[1].ancestor_path).to eq(ancestor_path_2)
expect(conflict_files[1].their_path).to eq(their_path_2)
expect(conflict_files[1].our_path).to eq(our_path_2)
expect(conflict_files[1].our_mode).to be(our_mode_2)
......
......@@ -82,7 +82,7 @@ RSpec.describe DiffFileEntity do
describe '#is_fully_expanded' do
context 'file with a conflict' do
let(:options) { { conflicts: { diff_file.new_path => double(diff_lines_for_serializer: []) } } }
let(:options) { { conflicts: { diff_file.new_path => double(diff_lines_for_serializer: [], conflict_type: :both_modified) } } }
it 'returns false' do
expect(diff_file).not_to receive(:fully_expanded?)
......@@ -90,4 +90,6 @@ RSpec.describe DiffFileEntity do
end
end
end
it_behaves_like 'diff file with conflict_type'
end
......@@ -4,8 +4,9 @@ require 'spec_helper'
RSpec.describe DiffFileMetadataEntity do
let(:merge_request) { create(:merge_request_with_diffs) }
let(:raw_diff_file) { merge_request.merge_request_diff.diffs.raw_diff_files.first }
let(:entity) { described_class.new(raw_diff_file) }
let(:diff_file) { merge_request.merge_request_diff.diffs.raw_diff_files.first }
let(:options) { {} }
let(:entity) { described_class.new(diff_file, options) }
context 'as json' do
subject { entity.as_json }
......@@ -20,8 +21,11 @@ RSpec.describe DiffFileMetadataEntity do
:deleted_file,
:submodule,
:file_identifier_hash,
:file_hash
:file_hash,
:conflict_type
)
end
it_behaves_like 'diff file with conflict_type'
end
end
......@@ -93,7 +93,7 @@ RSpec.describe DiffsEntity do
let(:diff_file_without_conflict) { diff_files.to_a[-2] }
let(:resolvable_conflicts) { true }
let(:conflict_file) { double(path: diff_file_with_conflict.new_path) }
let(:conflict_file) { double(path: diff_file_with_conflict.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
let(:merge_ref_head_diff) { true }
......
......@@ -9,12 +9,17 @@ RSpec.describe DiffsMetadataEntity do
let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
let(:merge_request_diffs) { merge_request.merge_request_diffs }
let(:merge_request_diff) { merge_request_diffs.last }
let(:options) { {} }
let(:entity) do
described_class.new(merge_request_diff.diffs,
request: request,
merge_request: merge_request,
merge_request_diffs: merge_request_diffs)
described_class.new(
merge_request_diff.diffs,
options.merge(
request: request,
merge_request: merge_request,
merge_request_diffs: merge_request_diffs
)
)
end
context 'as json' do
......@@ -38,20 +43,61 @@ RSpec.describe DiffsMetadataEntity do
end
describe 'diff_files' do
it 'returns diff files metadata' do
raw_diff_files = merge_request_diff.diffs.raw_diff_files
let!(:raw_diff_files) { merge_request_diff.diffs.raw_diff_files }
before do
expect_next_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff) do |instance|
# Use lightweight version instead. Several methods delegate to it, so putting a 5
# calls limit.
expect(instance).to receive(:raw_diff_files).at_most(5).times.and_call_original
expect(instance).not_to receive(:diff_files)
end
end
it 'returns diff files metadata' do
payload = DiffFileMetadataEntity.represent(raw_diff_files).as_json
expect(subject[:diff_files]).to eq(payload)
end
context 'when merge_ref_head_diff and allow_tree_conflicts options are set' do
let(:conflict_file) { double(path: raw_diff_files.first.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: false) }
before do
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
context 'when merge_ref_head_diff is true and allow_tree_conflicts is false' do
let(:options) { { merge_ref_head_diff: true, allow_tree_conflicts: false } }
it 'returns diff files metadata without conflicts' do
payload = DiffFileMetadataEntity.represent(raw_diff_files).as_json
expect(subject[:diff_files]).to eq(payload)
end
end
context 'when merge_ref_head_diff is false and allow_tree_conflicts is true' do
let(:options) { { merge_ref_head_diff: false, allow_tree_conflicts: true } }
it 'returns diff files metadata without conflicts' do
payload = DiffFileMetadataEntity.represent(raw_diff_files).as_json
expect(subject[:diff_files]).to eq(payload)
end
end
context 'when merge_ref_head_diff and allow_tree_conflicts are true' do
let(:options) { { merge_ref_head_diff: true, allow_tree_conflicts: true } }
it 'returns diff files metadata with conflicts' do
payload = DiffFileMetadataEntity.represent(raw_diff_files, conflicts: { conflict_file.path => conflict_file }).as_json
expect(subject[:diff_files]).to eq(payload)
end
end
end
end
end
end
......@@ -36,7 +36,7 @@ RSpec.describe PaginatedDiffEntity do
let(:diff_file_without_conflict) { diff_files.first }
let(:resolvable_conflicts) { true }
let(:conflict_file) { double(path: diff_file_with_conflict.new_path) }
let(:conflict_file) { double(path: diff_file_with_conflict.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
let(:merge_ref_head_diff) { true }
......
......@@ -63,3 +63,19 @@ end
RSpec.shared_examples 'diff file discussion entity' do
it_behaves_like 'diff file base entity'
end
RSpec.shared_examples 'diff file with conflict_type' do
describe '#conflict_type' do
it 'returns nil by default' do
expect(subject[:conflict_type]).to be_nil
end
context 'when there is matching conflict file' do
let(:options) { { conflicts: { diff_file.new_path => double(diff_lines_for_serializer: [], conflict_type: :both_modified) } } }
it 'returns false' do
expect(subject[:conflict_type]).to eq(:both_modified)
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