Merge branch 'id-highlight-diff-conflicts-backend' into 'master'

Highlight merge request conflicts displayed in diff

See merge request gitlab-org/gitlab!47003

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -34,6 +34,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       environment: environment,
       merge_request: @merge_request,
       diff_view: diff_view,
+      merge_ref_head_diff: render_merge_ref_head_diff?,
       pagination_data: diffs.pagination_data
     }
 
@@ -67,7 +68,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       render: ->(partial, locals) { view_to_html_string(partial, locals) }
     }
 
-    options = additional_attributes.merge(diff_view: Feature.enabled?(:unified_diff_lines, @merge_request.project, default_enabled: true) ? "inline" : diff_view)
+    options = additional_attributes.merge(
+      diff_view: Feature.enabled?(:unified_diff_lines, @merge_request.project, default_enabled: true) ? "inline" : diff_view,
+      merge_ref_head_diff: render_merge_ref_head_diff?
+    )
 
     if @merge_request.project.context_commits_enabled?
       options[:context_commits] = @merge_request.recent_context_commits
@@ -116,7 +120,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       end
     end
 
-    if Gitlab::Utils.to_boolean(params[:diff_head]) && @merge_request.diffable_merge_ref?
+    if render_merge_ref_head_diff?
       return CompareService.new(@project, @merge_request.merge_ref_head.sha)
         .execute(@project, @merge_request.target_branch)
     end
@@ -158,6 +162,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
     @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request)
   end
 
+  def render_merge_ref_head_diff?
+    Gitlab::Utils.to_boolean(params[:diff_head]) && @merge_request.diffable_merge_ref?
+  end
+
   def note_positions
     @note_positions ||= Gitlab::Diff::PositionCollection.new(renderable_notes.map(&:position))
   end

app/helpers/diff_helper.rb

@@ -252,4 +252,18 @@ module DiffHelper
 
     "...#{path[-(max - 3)..-1]}"
   end
+
+  def code_navigation_path(diffs)
+    Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha)
+  end
+
+  def conflicts
+    return unless options[:merge_ref_head_diff]
+
+    conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request) # rubocop:disable CodeReuse/ServiceClass
+
+    return unless conflicts_service.can_be_resolved_in_ui?
+
+    conflicts_service.conflicts.files.index_by(&:our_path)
+  end
 end

app/serializers/diff_file_entity.rb

@@ -3,6 +3,7 @@
 class DiffFileEntity < DiffFileBaseEntity
   include CommitsHelper
   include IconsHelper
+  include Gitlab::Utils::StrongMemoize
 
   expose :added_lines
   expose :removed_lines
@@ -54,12 +55,17 @@ class DiffFileEntity < DiffFileBaseEntity
 
   # Used for inline diffs
   expose :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, options) { inline_diff_view?(options, diff_file) && diff_file.text? } do |diff_file|
-    diff_file.diff_lines_for_serializer
+    file = conflict_file(options, diff_file) || diff_file
+    file.diff_lines_for_serializer
   end
 
   expose :is_fully_expanded do |diff_file|
+    if conflict_file(options, diff_file)
+      false
+    else
       diff_file.fully_expanded?
     end
+  end
 
   # Used for parallel diffs
   expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, options) { parallel_diff_view?(options, diff_file) && diff_file.text? }
@@ -79,4 +85,10 @@ class DiffFileEntity < DiffFileBaseEntity
     # If nothing is present, inline will be the default.
     options.fetch(:diff_view, :inline).to_sym == :inline
   end
+
+  def conflict_file(options, diff_file)
+    strong_memoize(:conflict_file) do
+      options[:conflicts] && options[:conflicts][diff_file.new_path]
+    end
+  end
 end

app/serializers/diffs_entity.rb

@@ -71,7 +71,7 @@ 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)))
+      options.merge(submodule_links: submodule_links, code_navigation_path: code_navigation_path(diffs), conflicts: conflicts))
   end
 
   expose :merge_request_diffs, using: MergeRequestDiffEntity, if: -> (_, options) { options[:merge_request_diffs]&.any? } do |diffs|
@@ -88,10 +88,6 @@ class DiffsEntity < Grape::Entity
 
   private
 
-  def code_navigation_path(diffs)
-    Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha)
-  end
-
   def commit_ids
     @commit_ids ||= merge_request.recent_commits.map(&:id)
   end

app/serializers/paginated_diff_entity.rb

@@ -7,6 +7,7 @@
 #
 class PaginatedDiffEntity < Grape::Entity
   include RequestAwareEntity
+  include DiffHelper
 
   expose :diff_files do |diffs, options|
     submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
@@ -15,7 +16,8 @@ class PaginatedDiffEntity < Grape::Entity
       diffs.diff_files,
       options.merge(
         submodule_links: submodule_links,
-        code_navigation_path: code_navigation_path(diffs)
+        code_navigation_path: code_navigation_path(diffs),
+        conflicts: conflicts
       )
     )
   end
@@ -41,10 +43,6 @@ class PaginatedDiffEntity < Grape::Entity
 
   private
 
-  def code_navigation_path(diffs)
-    Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha)
-  end
-
   %i[current_page next_page total_pages].each do |method|
     define_method method do
       pagination_data[method]

config/feature_flags/development/display_merge_conflicts_in_diff.yml

 ---
 name: display_merge_conflicts_in_diff
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45008
-rollout_issue_url: 
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/277097
 milestone: '13.5'
 type: development
 group: group::source code

lib/gitlab/conflict/file.rb

@@ -9,6 +9,11 @@ module Gitlab
 
       CONTEXT_LINES = 3
 
+      CONFLICT_TYPES = {
+        "old" => "conflict_marker_their",
+        "new" => "conflict_marker_our"
+      }.freeze
+
       attr_reader :merge_request
 
       # 'raw' holds the Gitlab::Git::Conflict::File that this instance wraps
@@ -46,6 +51,34 @@ module Gitlab
         end
       end
 
+      def diff_lines_for_serializer
+        # calculate sections and highlight lines before changing types
+        sections && highlight_lines!
+
+        sections.flat_map do |section|
+          if section[:conflict]
+            lines = []
+
+            initial_type = nil
+            section[:lines].each do |line|
+              if line.type != initial_type
+                lines << create_separator_line(line)
+                initial_type = line.type
+              end
+
+              line.type = CONFLICT_TYPES[line.type]
+              lines << line
+            end
+
+            lines << create_separator_line(lines.last)
+
+            lines
+          else
+            section[:lines]
+          end
+        end
+      end
+
       def sections
         return @sections if @sections
 
@@ -93,9 +126,15 @@ module Gitlab
 
               lines = tail_lines
             elsif conflict_before
-              # We're at the end of the file (no conflicts after), so just remove extra
-              # trailing lines.
+              # We're at the end of the file (no conflicts after)
+              number_of_trailing_lines = lines.size
+
+              # Remove extra trailing lines
               lines = lines.first(CONTEXT_LINES)
+
+              if number_of_trailing_lines > CONTEXT_LINES
+                lines << create_match_line(lines.last)
+              end
             end
           end
 
@@ -117,6 +156,10 @@ module Gitlab
         Gitlab::Diff::Line.new('', 'match', line.index, line.old_pos, line.new_pos)
       end
 
+      def create_separator_line(line)
+        Gitlab::Diff::Line.new('', 'conflict_marker', line.index, nil, nil)
+      end
+
       # Any line beginning with a letter, an underscore, or a dollar can be used in a
       # match line header. Only context sections can contain match lines, as match lines
       # have to exist in both versions of the file.

lib/gitlab/diff/line.rb

@@ -8,9 +8,9 @@ module Gitlab
       #
       SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze
 
-      attr_reader :line_code, :type, :old_pos, :new_pos
+      attr_reader :line_code, :old_pos, :new_pos
       attr_writer :rich_text
-      attr_accessor :text, :index
+      attr_accessor :text, :index, :type
 
       def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil)
         @text, @type, @index = text, type, index

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -383,6 +383,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         environment: nil,
         merge_request: merge_request,
         diff_view: :inline,
+        merge_ref_head_diff: nil,
         pagination_data: {
           current_page: nil,
           next_page: nil,

spec/lib/gitlab/conflict/file_spec.rb

@@ -93,6 +93,51 @@ RSpec.describe Gitlab::Conflict::File do
     end
   end
 
+  describe '#diff_lines_for_serializer' do
+    let(:diff_line_types) { conflict_file.diff_lines_for_serializer.map(&:type) }
+
+    it 'assigns conflict types to the diff lines' do
+      expect(diff_line_types[4]).to eq('conflict_marker')
+      expect(diff_line_types[5..10]).to eq(['conflict_marker_our'] * 6)
+      expect(diff_line_types[11]).to eq('conflict_marker')
+      expect(diff_line_types[12..17]).to eq(['conflict_marker_their'] * 6)
+      expect(diff_line_types[18]).to eq('conflict_marker')
+
+      expect(diff_line_types[19..24]).to eq([nil] * 6)
+
+      expect(diff_line_types[25]).to eq('conflict_marker')
+      expect(diff_line_types[26..27]).to eq(['conflict_marker_our'] * 2)
+      expect(diff_line_types[28]).to eq('conflict_marker')
+      expect(diff_line_types[29..30]).to eq(['conflict_marker_their'] * 2)
+      expect(diff_line_types[31]).to eq('conflict_marker')
+    end
+
+    it 'does not add a match line to the end of the section' do
+      expect(diff_line_types.last).to eq(nil)
+    end
+
+    context 'when there are unchanged trailing lines' do
+      let(:rugged_conflict) { index.conflicts.first }
+      let(:raw_conflict_content) { index.merge_file('files/ruby/popen.rb')[:data] }
+
+      it 'assign conflict types and adds match line to the end of the section' do
+        expect(diff_line_types).to eq([
+          'match',
+          nil, nil, nil,
+          "conflict_marker",
+          "conflict_marker_our",
+          "conflict_marker",
+          "conflict_marker_their",
+          "conflict_marker_their",
+          "conflict_marker_their",
+          "conflict_marker",
+          nil, nil, nil,
+          "match"
+        ])
+      end
+    end
+  end
+
   describe '#sections' do
     it 'only inserts match lines when there is a gap between sections' do
       conflict_file.sections.each_with_index do |section, i|

spec/serializers/diff_file_entity_spec.rb

@@ -69,4 +69,15 @@ RSpec.describe DiffFileEntity do
       end
     end
   end
+
+  describe '#is_fully_expanded' do
+    context 'file with a conflict' do
+      let(:options) { { conflicts: { diff_file.new_path => double(diff_lines_for_serializer: []) } } }
+
+      it 'returns false' do
+        expect(diff_file).not_to receive(:fully_expanded?)
+        expect(subject[:is_fully_expanded]).to eq(false)
+      end
+    end
+  end
 end

spec/serializers/diffs_entity_spec.rb

@@ -8,9 +8,12 @@ RSpec.describe DiffsEntity do
   let(:request) { EntityRequest.new(project: project, current_user: user) }
   let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
   let(:merge_request_diffs) { merge_request.merge_request_diffs }
+  let(:options) do
+    { request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs }
+  end
 
   let(:entity) do
-    described_class.new(merge_request_diffs.first.diffs, request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs)
+    described_class.new(merge_request_diffs.first.diffs, options)
   end
 
   context 'as json' do
@@ -68,5 +71,50 @@ RSpec.describe DiffsEntity do
         end
       end
     end
+
+    context 'when there are conflicts' do
+      let(:diff_files) { merge_request_diffs.first.diffs.diff_files }
+      let(:diff_file_with_conflict) { diff_files.to_a.last }
+      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(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
+
+      let(:merge_ref_head_diff) { true }
+      let(:options) { super().merge(merge_ref_head_diff: merge_ref_head_diff) }
+
+      before do
+        allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
+      end
+
+      it 'conflicts are 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
+
+      context 'merge ref head diff is not chosen to be displayed' do
+        let(:merge_ref_head_diff) { false }
+
+        it 'conflicts are not calculated' do
+          expect(MergeRequests::Conflicts::ListService).not_to receive(:new)
+        end
+      end
+
+      context 'when conflicts cannot be resolved' do
+        let(:resolvable_conflicts) { false }
+
+        it 'conflicts are not highlighted' do
+          expect(conflict_file).not_to receive(:diff_lines_for_serializer)
+          expect(diff_file_with_conflict).to receive(:diff_lines_for_serializer).twice  # for highlighted_diff_lines and is_fully_expanded
+          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

@@ -31,4 +31,50 @@ RSpec.describe PaginatedDiffEntity do
       total_pages: 7
     )
   end
+
+  context 'when there are conflicts' do
+    let(:diff_batch) { merge_request.merge_request_diff.diffs_in_batch(7, 3, diff_options: nil) }
+    let(:diff_files) { diff_batch.diff_files.to_a }
+    let(:diff_file_with_conflict) { diff_files.last }
+    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(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
+
+    let(:merge_ref_head_diff) { true }
+    let(:options) { super().merge(merge_ref_head_diff: merge_ref_head_diff) }
+
+    before do
+      allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
+    end
+
+    it 'conflicts are 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
+
+    context 'merge ref head diff is not chosen to be displayed' do
+      let(:merge_ref_head_diff) { false }
+
+      it 'conflicts are not calculated' do
+        expect(MergeRequests::Conflicts::ListService).not_to receive(:new)
+      end
+    end
+
+    context 'when conflicts cannot be resolved' do
+      let(:resolvable_conflicts) { false }
+
+      it 'conflicts are not highlighted' do
+        expect(conflict_file).not_to receive(:diff_lines_for_serializer)
+        expect(diff_file_with_conflict).to receive(:diff_lines_for_serializer).twice  # for highlighted_diff_lines and is_fully_expanded
+        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