Allow missing side when listing conflicts on diffs

In https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3648,
support for listing conflicts even when there's a missing side has
been added to ListConflictFiles RPC. This only happens when
`allow_tree_conflicts` is set to `true` in the RPC request.

This allows gitlab-rails to use that parameter when displaying
MR diffs with conflicts. The `allow_tree_conflicts` will be set
to `true` only when `display_merge_conflicts_in_diff` is enabled.
Otherwise, it'll be set to `false`.

Gemfile

@@ -472,7 +472,7 @@ end
 gem 'spamcheck', '~> 0.1.0'
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 14.1.0.pre.rc3'
+gem 'gitaly', '~> 14.1.0.pre.rc4'
 
 # KAS GRPC protocol definitions
 gem 'kas-grpc', '~> 0.0.2'

Gemfile.lock

@@ -460,7 +460,7 @@ GEM
       rails (>= 3.2.0)
     git (1.7.0)
       rchardet (~> 1.8)
-    gitaly (14.1.0.pre.rc3)
+    gitaly (14.1.0.pre.rc4)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab (4.16.1)
@@ -531,8 +531,8 @@ GEM
       signet (~> 0.12)
     google-cloud-env (1.5.0)
       faraday (>= 0.17.3, < 2.0)
-    google-protobuf (3.17.1)
-    googleapis-common-protos-types (1.0.6)
+    google-protobuf (3.17.3)
+    googleapis-common-protos-types (1.1.0)
       google-protobuf (~> 3.14)
     googleauth (0.14.0)
       faraday (>= 0.17.3, < 2.0)
@@ -1482,7 +1482,7 @@ DEPENDENCIES
   gettext (~> 3.3)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 14.1.0.pre.rc3)
+  gitaly (~> 14.1.0.pre.rc4)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 2.2.2)

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -38,7 +38,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       merge_request: @merge_request,
       diff_view: diff_view,
       merge_ref_head_diff: render_merge_ref_head_diff?,
-      pagination_data: diffs.pagination_data
+      pagination_data: diffs.pagination_data,
+      allow_tree_conflicts: display_merge_conflicts_in_diff?
     }
 
     if diff_options_hash[:paths].blank? && Feature.enabled?(:diffs_batch_render_cached, project, default_enabled: :yaml)
@@ -82,7 +83,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     options = additional_attributes.merge(
       diff_view: "inline",
-      merge_ref_head_diff: render_merge_ref_head_diff?
+      merge_ref_head_diff: render_merge_ref_head_diff?,
+      allow_tree_conflicts: display_merge_conflicts_in_diff?
     )
 
     if @merge_request.project.context_commits_enabled?
@@ -213,4 +215,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
     Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
       .track_mr_diffs_single_file_action(merge_request: @merge_request, user: current_user)
   end
+
+  def display_merge_conflicts_in_diff?
+    Feature.enabled?(:display_merge_conflicts_in_diff, @merge_request.project)
+  end
 end

app/helpers/diff_helper.rb

@@ -273,14 +273,14 @@ module DiffHelper
     Gitlab::CodeNavigationPath.new(merge_request.project, merge_request.diff_head_sha)
   end
 
-  def conflicts
+  def conflicts(allow_tree_conflicts: false)
     return unless options[:merge_ref_head_diff]
 
-    conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request) # rubocop:disable CodeReuse/ServiceClass
+    conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request, allow_tree_conflicts: allow_tree_conflicts) # rubocop:disable CodeReuse/ServiceClass
 
-    return unless conflicts_service.can_be_resolved_in_ui?
+    return unless allow_tree_conflicts || conflicts_service.can_be_resolved_in_ui?
 
-    conflicts_service.conflicts.files.index_by(&:our_path)
+    conflicts_service.conflicts.files.index_by(&:path)
   end
 
   def log_overflow_limits(diff_files:, collection_overflow:)

app/serializers/diffs_entity.rb

@@ -71,7 +71,12 @@ class DiffsEntity < Grape::Entity
     submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
 
     DiffFileEntity.represent(diffs.diff_files,
-      options.merge(submodule_links: submodule_links, code_navigation_path: code_navigation_path(diffs), conflicts: conflicts))
+      options.merge(
+        submodule_links: submodule_links,
+        code_navigation_path: code_navigation_path(diffs),
+        conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
+      )
+    )
   end
 
   expose :merge_request_diffs, using: MergeRequestDiffEntity, if: -> (_, options) { options[:merge_request_diffs]&.any? } do |diffs|

app/serializers/paginated_diff_entity.rb

@@ -17,7 +17,7 @@ class PaginatedDiffEntity < Grape::Entity
       options.merge(
         submodule_links: submodule_links,
         code_navigation_path: code_navigation_path(diffs),
-        conflicts: conflicts
+        conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
       )
     )
   end

app/services/merge_requests/conflicts/base_service.rb

@@ -3,10 +3,11 @@
 module MergeRequests
   module Conflicts
     class BaseService
-      attr_reader :merge_request
+      attr_reader :merge_request, :params
 
-      def initialize(merge_request)
+      def initialize(merge_request, params = {})
         @merge_request = merge_request
+        @params = params
       end
     end
   end

app/services/merge_requests/conflicts/list_service.rb

@@ -23,7 +23,11 @@ module MergeRequests
       end
 
       def conflicts
-        @conflicts ||= Gitlab::Conflict::FileCollection.new(merge_request)
+        @conflicts ||=
+          Gitlab::Conflict::FileCollection.new(
+            merge_request,
+            allow_tree_conflicts: params[:allow_tree_conflicts]
+          )
       end
     end
   end

lib/gitlab/conflict/file.rb

@@ -23,7 +23,7 @@ module Gitlab
       # 'raw' holds the Gitlab::Git::Conflict::File that this instance wraps
       attr_reader :raw
 
-      delegate :type, :content, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
+      delegate :type, :content, :path, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
 
       def initialize(raw, merge_request:)
         @raw = raw

lib/gitlab/conflict/file_collection.rb

@@ -7,14 +7,14 @@ module Gitlab
 
       attr_reader :merge_request, :resolver
 
-      def initialize(merge_request)
+      def initialize(merge_request, allow_tree_conflicts: false)
         our_commit = merge_request.source_branch_head.raw
         their_commit = merge_request.target_branch_head.raw
         @target_repo = merge_request.target_project.repository
         @source_repo = merge_request.source_project.repository.raw
         @our_commit_id = our_commit.id
         @their_commit_id = their_commit.id
-        @resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id)
+        @resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id, allow_tree_conflicts: allow_tree_conflicts)
         @merge_request = merge_request
       end
 

lib/gitlab/git/conflict/file.rb

@@ -94,6 +94,15 @@ module Gitlab
 
           resolution
         end
+
+        def path
+          # There are conflict scenarios (e.g. file is removed on source) wherein
+          # our_path will be blank/nil. Since we are indexing them by path in
+          # `#conflicts` helper and we want to match the diff file to a conflict
+          # in `DiffFileEntity#highlighted_diff_lines`, we need to fallback to
+          # their_path (this is the path on target).
+          our_path.presence || their_path
+        end
       end
     end
   end

lib/gitlab/git/conflict/resolver.rb

@@ -9,15 +9,16 @@ module Gitlab
         ConflictSideMissing = Class.new(StandardError)
         ResolutionError = Class.new(StandardError)
 
-        def initialize(target_repository, our_commit_oid, their_commit_oid)
+        def initialize(target_repository, our_commit_oid, their_commit_oid, allow_tree_conflicts: false)
           @target_repository = target_repository
           @our_commit_oid = our_commit_oid
           @their_commit_oid = their_commit_oid
+          @allow_tree_conflicts = allow_tree_conflicts
         end
 
         def conflicts
           @conflicts ||= wrapped_gitaly_errors do
-            gitaly_conflicts_client(@target_repository).list_conflict_files.to_a
+            gitaly_conflicts_client(@target_repository).list_conflict_files(allow_tree_conflicts: @allow_tree_conflicts).to_a
           rescue GRPC::FailedPrecondition => e
             raise Gitlab::Git::Conflict::Resolver::ConflictSideMissing, e.message
           end

lib/gitlab/gitaly_client/conflicts_service.rb

@@ -14,11 +14,12 @@ module Gitlab
         @their_commit_oid = their_commit_oid
       end
 
-      def list_conflict_files
+      def list_conflict_files(allow_tree_conflicts: false)
         request = Gitaly::ListConflictFilesRequest.new(
           repository: @gitaly_repo,
           our_commit_oid: @our_commit_oid,
-          their_commit_oid: @their_commit_oid
+          their_commit_oid: @their_commit_oid,
+          allow_tree_conflicts: allow_tree_conflicts
         )
         response = GitalyClient.call(@repository.storage, :conflicts_service, :list_conflict_files, request, timeout: GitalyClient.long_timeout)
         GitalyClient::ConflictFilesStitcher.new(response, @gitaly_repo)

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -471,6 +471,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         merge_request: merge_request,
         diff_view: :inline,
         merge_ref_head_diff: nil,
+        allow_tree_conflicts: true,
         pagination_data: {
           total_pages: nil
         }.merge(pagination_data)
@@ -589,6 +590,21 @@ RSpec.describe Projects::MergeRequests::DiffsController do
       it_behaves_like 'successful request'
     end
 
+    context 'when display_merge_conflicts_in_diff is disabled' do
+      before do
+        stub_feature_flags(display_merge_conflicts_in_diff: false)
+      end
+
+      subject { go }
+
+      it_behaves_like 'serializes diffs with expected arguments' do
+        let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+        let(:expected_options) { collection_arguments(total_pages: 20).merge(allow_tree_conflicts: false) }
+      end
+
+      it_behaves_like 'successful request'
+    end
+
     it_behaves_like 'forked project with submodules'
     it_behaves_like 'cached diff collection'
 

spec/lib/gitlab/conflict/file_spec.rb

@@ -17,6 +17,10 @@ RSpec.describe Gitlab::Conflict::File do
   let(:raw_conflict_file) { Gitlab::Git::Conflict::File.new(repository, our_commit.oid, rugged_conflict, raw_conflict_content) }
   let(:conflict_file) { described_class.new(raw_conflict_file, merge_request: merge_request) }
 
+  describe 'delegates' do
+    it { expect(conflict_file).to delegate_method(:path).to(:raw) }
+  end
+
   describe '#resolve_lines' do
     let(:section_keys) { conflict_file.sections.map { |section| section[:id] }.compact }
 

spec/lib/gitlab/git/conflict/file_spec.rb

@@ -48,4 +48,18 @@ RSpec.describe Gitlab::Git::Conflict::File do
       end
     end
   end
+
+  describe '#path' do
+    it 'returns our_path' do
+      expect(valid_content.path).to eq(conflict[:ours][:path])
+    end
+
+    context 'when our_path is not present' do
+      let(:conflict) { { ancestor: { path: 'ancestor' }, theirs: { path: 'theirs', mode: 33188 }, ours: { path: '', mode: 0 } } }
+
+      it 'returns their_path' do
+        expect(valid_content.path).to eq(conflict[:theirs][:path])
+      end
+    end
+  end
 end

spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb

@@ -15,18 +15,31 @@ RSpec.describe Gitlab::GitalyClient::ConflictsService do
   end
 
   describe '#list_conflict_files' do
+    let(:allow_tree_conflicts) { false }
     let(:request) do
       Gitaly::ListConflictFilesRequest.new(
-        repository: target_gitaly_repository, our_commit_oid: our_commit_oid,
-        their_commit_oid: their_commit_oid
+        repository: target_gitaly_repository,
+        our_commit_oid: our_commit_oid,
+        their_commit_oid: their_commit_oid,
+        allow_tree_conflicts: allow_tree_conflicts
       )
     end
 
+    shared_examples_for 'listing conflicts' do
       it 'sends an RPC request' do
         expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
           .with(request, kind_of(Hash)).and_return([].to_enum)
 
-      client.list_conflict_files
+        client.list_conflict_files(allow_tree_conflicts: allow_tree_conflicts)
+      end
+    end
+
+    it_behaves_like 'listing conflicts'
+
+    context 'when allow_tree_conflicts is set to true' do
+      let(:allow_tree_conflicts) { true }
+
+      it_behaves_like 'listing conflicts'
     end
   end
 

spec/requests/projects/merge_requests/diffs_spec.rb

@@ -33,6 +33,7 @@ RSpec.describe 'Merge Requests Diffs' do
         merge_request: merge_request,
         diff_view: :inline,
         merge_ref_head_diff: nil,
+        allow_tree_conflicts: true,
         pagination_data: {
           total_pages: nil
         }.merge(pagination_data)

spec/serializers/diffs_entity_spec.rb

@@ -9,8 +9,14 @@ RSpec.describe DiffsEntity do
 
   let(:request) { EntityRequest.new(project: project, current_user: user) }
   let(:merge_request_diffs) { merge_request.merge_request_diffs }
+  let(:allow_tree_conflicts) { false }
   let(:options) do
-    { request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs }
+    {
+      request: request,
+      merge_request: merge_request,
+      merge_request_diffs: merge_request_diffs,
+      allow_tree_conflicts: allow_tree_conflicts
+    }
   end
 
   let(:entity) do
@@ -87,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(our_path: diff_file_with_conflict.new_path) }
+      let(:conflict_file) { double(path: diff_file_with_conflict.new_path) }
       let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
 
       let(:merge_ref_head_diff) { true }
@@ -123,6 +129,18 @@ RSpec.describe DiffsEntity do
 
           subject
         end
+
+        context 'when allow_tree_conflicts is set to true' do
+          let(:allow_tree_conflicts) { true }
+
+          it 'conflicts are still highlighted' do
+            expect(conflict_file).to receive(:diff_lines_for_serializer)
+            expect(diff_file_with_conflict).not_to receive(:diff_lines_for_serializer)
+            expect(diff_file_without_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
+
+            subject
+          end
+        end
       end
     end
   end

spec/serializers/paginated_diff_entity_spec.rb

@@ -7,11 +7,13 @@ RSpec.describe PaginatedDiffEntity do
   let(:request) { double('request', current_user: user) }
   let(:merge_request) { create(:merge_request) }
   let(:diff_batch) { merge_request.merge_request_diff.diffs_in_batch(2, 3, diff_options: nil) }
+  let(:allow_tree_conflicts) { false }
   let(:options) do
     {
       request: request,
       merge_request: merge_request,
-      pagination_data: diff_batch.pagination_data
+      pagination_data: diff_batch.pagination_data,
+      allow_tree_conflicts: allow_tree_conflicts
     }
   end
 
@@ -34,7 +36,7 @@ RSpec.describe PaginatedDiffEntity do
     let(:diff_file_without_conflict) { diff_files.first }
 
     let(:resolvable_conflicts) { true }
-    let(:conflict_file) { double(our_path: diff_file_with_conflict.new_path) }
+    let(:conflict_file) { double(path: diff_file_with_conflict.new_path) }
     let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
 
     let(:merge_ref_head_diff) { true }
@@ -70,6 +72,18 @@ RSpec.describe PaginatedDiffEntity do
 
         subject
       end
+
+      context 'when allow_tree_conflicts is set to true' do
+        let(:allow_tree_conflicts) { true }
+
+        it 'conflicts are still highlighted' do
+          expect(conflict_file).to receive(:diff_lines_for_serializer)
+          expect(diff_file_with_conflict).not_to receive(:diff_lines_for_serializer)
+          expect(diff_file_without_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
+
+          subject
+        end
+      end
     end
   end
 end