Merge branch 'osw-comment-on-any-line-on-diffs' into 'master'

Comment on any expanded diff line on MRs

See merge request gitlab-org/gitlab-ce!22398

app/assets/javascripts/diffs/components/diff_line_gutter_content.vue

@@ -55,11 +55,6 @@ export default {
       required: false,
       default: false,
     },
-    isContextLine: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
     isHover: {
       type: Boolean,
       required: false,
@@ -81,7 +76,6 @@ export default {
         this.showCommentButton &&
         this.isHover &&
         !this.isMatchLine &&
-        !this.isContextLine &&
         !this.isMetaLine &&
         !this.hasDiscussions
       );

app/assets/javascripts/diffs/components/diff_table_cell.vue

@@ -3,7 +3,6 @@ import { mapGetters } from 'vuex';
 import DiffLineGutterContent from './diff_line_gutter_content.vue';
 import {
   MATCH_LINE_TYPE,
-  CONTEXT_LINE_TYPE,
   EMPTY_CELL_TYPE,
   OLD_LINE_TYPE,
   OLD_NO_NEW_LINE_TYPE,
@@ -71,9 +70,6 @@ export default {
     isMatchLine() {
       return this.line.type === MATCH_LINE_TYPE;
     },
-    isContextLine() {
-      return this.line.type === CONTEXT_LINE_TYPE;
-    },
     isMetaLine() {
       const { type } = this.line;
@@ -88,11 +84,7 @@ export default {
         [type]: type,
         [LINE_UNFOLD_CLASS_NAME]: this.isMatchLine,
         [LINE_HOVER_CLASS_NAME]:
-          this.isLoggedIn &&
-          this.isHover &&
-          !this.isMatchLine &&
-          !this.isContextLine &&
-          !this.isMetaLine,
+          this.isLoggedIn && this.isHover && !this.isMatchLine && !this.isMetaLine,
       };
     },
     lineNumber() {

app/assets/javascripts/diffs/components/inline_diff_table_row.vue

@@ -4,8 +4,6 @@ import DiffTableCell from './diff_table_cell.vue';
 import {
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
-  CONTEXT_LINE_TYPE,
-  CONTEXT_LINE_CLASS_NAME,
   PARALLEL_DIFF_VIEW_TYPE,
   LINE_POSITION_LEFT,
   LINE_POSITION_RIGHT,
@@ -41,13 +39,9 @@ export default {
   },
   computed: {
     ...mapGetters('diffs', ['isInlineView']),
-    isContextLine() {
-      return this.line.type === CONTEXT_LINE_TYPE;
-    },
     classNameMap() {
       return {
         [this.line.type]: this.line.type,
-        [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
         [PARALLEL_DIFF_VIEW_TYPE]: this.isParallelView,
       };
     },

app/assets/javascripts/diffs/components/parallel_diff_table_row.vue

@@ -5,8 +5,6 @@ import DiffTableCell from './diff_table_cell.vue';
 import {
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
-  CONTEXT_LINE_TYPE,
-  CONTEXT_LINE_CLASS_NAME,
   OLD_NO_NEW_LINE_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
   NEW_NO_NEW_LINE_TYPE,
@@ -43,12 +41,8 @@ export default {
     };
   },
   computed: {
-    isContextLine() {
-      return this.line.left && this.line.left.type === CONTEXT_LINE_TYPE;
-    },
     classNameMap() {
       return {
-        [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
         [PARALLEL_DIFF_VIEW_TYPE]: true,
       };
     },

app/assets/javascripts/diffs/constants.js

@@ -3,7 +3,6 @@ export const PARALLEL_DIFF_VIEW_TYPE = 'parallel';
 export const MATCH_LINE_TYPE = 'match';
 export const OLD_NO_NEW_LINE_TYPE = 'old-nonewline';
 export const NEW_NO_NEW_LINE_TYPE = 'new-nonewline';
-export const CONTEXT_LINE_TYPE = 'context';
 export const EMPTY_CELL_TYPE = 'empty-cell';
 export const COMMENT_FORM_TYPE = 'commentForm';
 export const DIFF_NOTE_TYPE = 'DiffNote';
@@ -22,7 +21,6 @@ export const LINE_SIDE_RIGHT = 'right-side';
 export const DIFF_VIEW_COOKIE_NAME = 'diff_view';
 export const LINE_HOVER_CLASS_NAME = 'is-over';
 export const LINE_UNFOLD_CLASS_NAME = 'unfold js-unfold';
-export const CONTEXT_LINE_CLASS_NAME = 'diff-expanded';
 export const UNFOLD_COUNT = 20;
 export const COUNT_OF_AVATARS_IN_GUTTER = 3;

app/assets/javascripts/diffs/store/mutations.js

@@ -65,7 +65,13 @@ export default {
     const { highlightedDiffLines, parallelDiffLines } = diffFile;
     removeMatchLine(diffFile, lineNumbers, bottom);
-    const lines = addLineReferences(contextLines, lineNumbers, bottom);
+
+    const lines = addLineReferences(contextLines, lineNumbers, bottom).map(line => ({
+      ...line,
+      lineCode: line.lineCode || `${fileHash}_${line.oldLine}_${line.newLine}`,
+      discussions: line.discussions || [],
+    }));
+
     addContextLines({
       inlineLines: highlightedDiffLines,
       parallelLines: parallelDiffLines,

app/controllers/projects/blob_controller.rb

@@ -122,7 +122,7 @@ class Projects::BlobController < Projects::ApplicationController
     @lines.map! do |line|
       # These are marked as context lines but are loaded from blobs.
       # We also have context lines loaded from diffs in other places.
-      diff_line = Gitlab::Diff::Line.new(line, 'context', nil, nil, nil)
+      diff_line = Gitlab::Diff::Line.new(line, nil, nil, nil, nil)
       diff_line.rich_text = line
       diff_line
     end

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -22,6 +22,12 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   def render_diffs
     @environment = @merge_request.environments_for(current_user).last
+    notes_grouped_by_path = @notes.group_by { |note| note.position.file_path }
+
+    @diffs.diff_files.each do |diff_file|
+      notes = notes_grouped_by_path.fetch(diff_file.file_path, [])
+      notes.each { |note| diff_file.unfold_diff_lines(note.position) }
+    end
     @diffs.write_cache

app/models/diff_note.rb

@@ -66,6 +66,10 @@ class DiffNote < Note
     self.original_position.diff_refs == diff_refs
   end
+  def discussion_first_note?
+    self == discussion.first_note
+  end
+
   private
   def enqueue_diff_file_creation_job
@@ -78,26 +82,33 @@ class DiffNote < Note
   end
   def should_create_diff_file?
-    on_text? && note_diff_file.nil? && self == discussion.first_note
+    on_text? && note_diff_file.nil? && discussion_first_note?
   end
   def fetch_diff_file
-    if note_diff_file
-      diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
-      Gitlab::Diff::File.new(diff,
-                             repository: project.repository,
-                             diff_refs: original_position.diff_refs)
-    elsif 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(original_position.diff_options).diff_files.first
-    else
-      original_position.diff_file(self.project.repository)
-    end
+    file =
+      if note_diff_file
+        diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
+        Gitlab::Diff::File.new(diff,
+                               repository: project.repository,
+                               diff_refs: original_position.diff_refs)
+      elsif 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(original_position.diff_options).diff_files.first
+      else
+        original_position.diff_file(self.project.repository)
+      end
+
+    # Since persisted diff files already have its content "unfolded"
+    # there's no need to make it pass through the unfolding process.
+    file&.unfold_diff_lines(position) unless note_diff_file
+
+    file
   end
   def supported?

app/services/merge_requests/reload_diffs_service.rb

@@ -29,10 +29,6 @@ module MergeRequests
     # rubocop: disable CodeReuse/ActiveRecord
     def clear_cache(new_diff)
-      # Executing the iteration we cache highlighted diffs for each diff file of
-      # MergeRequestDiff.
-      cacheable_collection(new_diff).write_cache
-
       # Remove cache for all diffs on this MR. Do not use the association on the
       # model, as that will interfere with other actions happening when
       # reloading the diff.

app/services/notes/base_service.rb

+# frozen_string_literal: true
+
+module Notes
+  class BaseService < ::BaseService
+    def clear_noteable_diffs_cache(note)
+      noteable = note.noteable
+
+      if note.is_a?(DiffNote) &&
+          note.discussion_first_note? &&
+          note.position.unfolded_diff?(project.repository)
+        noteable.diffs.clear_cache
+      end
+    end
+  end
+end

app/services/notes/create_service.rb

 # frozen_string_literal: true
 module Notes
-  class CreateService < ::BaseService
+  class CreateService < ::Notes::BaseService
     def execute
       merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
@@ -35,6 +35,7 @@ module Notes
       if !only_commands && note.save
         todo_service.new_note(note, current_user)
+        clear_noteable_diffs_cache(note)
       end
       if command_params.present?

app/services/notes/destroy_service.rb

 # frozen_string_literal: true
 module Notes
-  class DestroyService < BaseService
+  class DestroyService < ::Notes::BaseService
     def execute(note)
       TodoService.new.destroy_target(note) do |note|
         note.destroy
       end
+
+      clear_noteable_diffs_cache(note)
     end
   end
 end

changelogs/unreleased/osw-comment-on-any-line-on-diffs.yml

+---
+title: Allow commenting on any diff line in Merge Requests
+merge_request: 22398
+author:
+type: added

lib/gitlab/diff/file.rb

@@ -26,6 +26,7 @@ module Gitlab
         @repository = repository
         @diff_refs = diff_refs
         @fallback_diff_refs = fallback_diff_refs
+        @unfolded = false
         # Ensure items are collected in the the batch
         new_blob_lazy
@@ -135,6 +136,24 @@ module Gitlab
           Gitlab::Diff::Parser.new.parse(raw_diff.each_line, diff_file: self).to_a
       end
+      # Changes diff_lines according to the given position. That is,
+      # it checks whether the position requires blob lines into the diff
+      # in order to be presented.
+      def unfold_diff_lines(position)
+        return unless position
+
+        unfolder = Gitlab::Diff::LinesUnfolder.new(self, position)
+
+        if unfolder.unfold_required?
+          @diff_lines = unfolder.unfolded_diff_lines
+          @unfolded = true
+        end
+      end
+
+      def unfolded?
+        @unfolded
+      end
+
       def highlighted_diff_lines
         @highlighted_diff_lines ||=
           Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight

lib/gitlab/diff/line.rb

@@ -3,9 +3,9 @@ module Gitlab
     class Line
       SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze
-      attr_reader :line_code, :type, :index, :old_pos, :new_pos
+      attr_reader :line_code, :type, :old_pos, :new_pos
       attr_writer :rich_text
-      attr_accessor :text
+      attr_accessor :text, :index
       def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil)
         @text, @type, @index = text, type, index
@@ -19,7 +19,14 @@ module Gitlab
       end
       def self.init_from_hash(hash)
-        new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code], rich_text: hash[:rich_text])
+        new(hash[:text],
+            hash[:type],
+            hash[:index],
+            hash[:old_pos],
+            hash[:new_pos],
+            parent_file: hash[:parent_file],
+            line_code: hash[:line_code],
+            rich_text: hash[:rich_text])
       end
       def to_hash

lib/gitlab/diff/lines_unfolder.rb

+# frozen_string_literal: true
+
+# Given a position, calculates which Blob lines should be extracted, treated and
+# injected in the current diff file lines in order to present a "unfolded" diff.
+module Gitlab
+  module Diff
+    class LinesUnfolder
+      include Gitlab::Utils::StrongMemoize
+
+      UNFOLD_CONTEXT_SIZE = 3
+
+      def initialize(diff_file, position)
+        @diff_file = diff_file
+        @blob = diff_file.old_blob
+        @position = position
+        @generate_top_match_line = true
+        @generate_bottom_match_line = true
+
+        # These methods update `@generate_top_match_line` and
+        # `@generate_bottom_match_line`.
+        @from_blob_line = calculate_from_blob_line!
+        @to_blob_line = calculate_to_blob_line!
+      end
+
+      # Returns merged diff lines with required blob lines with correct
+      # positions.
+      def unfolded_diff_lines
+        strong_memoize(:unfolded_diff_lines) do
+          next unless unfold_required?
+
+          merged_diff_with_blob_lines
+        end
+      end
+
+      # Returns the extracted lines from the old blob which should be merged
+      # with the current diff lines.
+      def blob_lines
+        strong_memoize(:blob_lines) do
+          # Blob lines, unlike diffs, doesn't start with an empty space for
+          # unchanged line, so the parsing and highlighting step can get fuzzy
+          # without the following change.
+          line_prefix = ' '
+          blob_as_diff_lines = @blob.data.each_line.map { |line| "#{line_prefix}#{line}" }
+
+          lines = Gitlab::Diff::Parser.new.parse(blob_as_diff_lines, diff_file: @diff_file).to_a
+
+          from = from_blob_line - 1
+          to = to_blob_line - 1
+
+          lines[from..to]
+        end
+      end
+
+      def unfold_required?
+        strong_memoize(:unfold_required) do
+          next false unless @diff_file.text?
+          next false unless @position.unchanged?
+          next false if @diff_file.new_file? || @diff_file.deleted_file?
+          next false unless @position.old_line
+          # Invalid position (MR import scenario)
+          next false if @position.old_line > @blob.lines.size
+          next false if @diff_file.diff_lines.empty?
+          next false if @diff_file.line_for_position(@position)
+          next false unless unfold_line
+
+          true
+        end
+      end
+
+      private
+
+      attr_reader :from_blob_line, :to_blob_line
+
+      def merged_diff_with_blob_lines
+        lines = @diff_file.diff_lines
+        match_line = unfold_line
+        insert_index = bottom? ? -1 : match_line.index
+
+        lines -= [match_line] unless bottom?
+
+        lines.insert(insert_index, *blob_lines_with_matches)
+
+        # The inserted blob lines have invalid indexes, so we need
+        # to reindex them.
+        reindex(lines)
+
+        lines
+      end
+
+      # Returns 'unchanged' blob lines with recalculated `old_pos` and
+      # `new_pos` and the recalculated new match line (needed if we for instance
+      # we unfolded once, but there are still folded lines).
+      def blob_lines_with_matches
+        old_pos = from_blob_line
+        new_pos = from_blob_line + offset
+
+        new_blob_lines = []
+
+        new_blob_lines.push(top_blob_match_line) if top_blob_match_line
+
+        blob_lines.each do |line|
+          new_blob_lines << Gitlab::Diff::Line.new(line.text, line.type, nil, old_pos, new_pos,
+                                                   parent_file: @diff_file)
+
+          old_pos += 1
+          new_pos += 1
+        end
+
+        new_blob_lines.push(bottom_blob_match_line) if bottom_blob_match_line
+
+        new_blob_lines
+      end
+
+      def reindex(lines)
+        lines.each_with_index { |line, i| line.index = i }
+      end
+
+      def top_blob_match_line
+        strong_memoize(:top_blob_match_line) do
+          next unless @generate_top_match_line
+
+          old_pos = from_blob_line
+          new_pos = from_blob_line + offset
+
+          build_match_line(old_pos, new_pos)
+        end
+      end
+
+      def bottom_blob_match_line
+        strong_memoize(:bottom_blob_match_line) do
+          # The bottom line match addition is already handled on
+          # Diff::File#diff_lines_for_serializer
+          next if bottom?
+          next unless @generate_bottom_match_line
+
+          position = line_after_unfold_position.old_pos
+
+          old_pos = position
+          new_pos = position + offset
+
+          build_match_line(old_pos, new_pos)
+        end
+      end
+
+      def build_match_line(old_pos, new_pos)
+        blob_lines_length = blob_lines.length
+        old_line_ref = [old_pos, blob_lines_length].join(',')
+        new_line_ref = [new_pos, blob_lines_length].join(',')
+        new_match_line_str = "@@ -#{old_line_ref}+#{new_line_ref} @@"
+
+        Gitlab::Diff::Line.new(new_match_line_str, 'match', nil, old_pos, new_pos)
+      end
+
+      # Returns the first line position that should be extracted
+      # from `blob_lines`.
+      def calculate_from_blob_line!
+        return unless unfold_required?
+
+        from = comment_position - UNFOLD_CONTEXT_SIZE
+
+        # There's no line before the match if it's in the top-most
+        # position.
+        prev_line_number = line_before_unfold_position&.old_pos || 0
+
+        if from <= prev_line_number + 1
+          @generate_top_match_line = false
+          from = prev_line_number + 1
+        end
+
+        from
+      end
+
+      # Returns the last line position that should be extracted
+      # from `blob_lines`.
+      def calculate_to_blob_line!
+        return unless unfold_required?
+
+        to = comment_position + UNFOLD_CONTEXT_SIZE
+
+        return to if bottom?
+
+        next_line_number = line_after_unfold_position.old_pos
+
+        if to >= next_line_number - 1
+          @generate_bottom_match_line = false
+          to = next_line_number - 1
+        end
+
+        to
+      end
+
+      def offset
+        unfold_line.new_pos - unfold_line.old_pos
+      end
+
+      def line_before_unfold_position
+        return unless index = unfold_line&.index
+
+        @diff_file.diff_lines[index - 1] if index > 0
+      end
+
+      def line_after_unfold_position
+        return unless index = unfold_line&.index
+
+        @diff_file.diff_lines[index + 1] if index >= 0
+      end
+
+      def bottom?
+        strong_memoize(:bottom) do
+          @position.old_line > last_line.old_pos
+        end
+      end
+
+      # Returns the line which needed to be expanded in order to send a comment
+      # in `@position`.
+      def unfold_line
+        strong_memoize(:unfold_line) do
+          next last_line if bottom?
+
+          @diff_file.diff_lines.find do |line|
+            line.old_pos > comment_position && line.type == 'match'
+          end
+        end
+      end
+
+      def comment_position
+        @position.old_line
+      end
+
+      def last_line
+        @diff_file.diff_lines.last
+      end
+    end
+  end
+end

lib/gitlab/diff/position.rb

@@ -101,6 +101,10 @@ module Gitlab
         @diff_refs ||= DiffRefs.new(base_sha: base_sha, start_sha: start_sha, head_sha: head_sha)
       end
+      def unfolded_diff?(repository)
+        diff_file(repository)&.unfolded?
+      end
+
       def diff_file(repository)
         return @diff_file if defined?(@diff_file)
@@ -134,7 +138,13 @@ module Gitlab
         return unless diff_refs.complete?
         return unless comparison = diff_refs.compare_in(repository.project)
-        comparison.diffs(diff_options).diff_files.first
+        file = comparison.diffs(diff_options).diff_files.first
+
+        # We need to unfold diff lines according to the position in order
+        # to correctly calculate the line code and trace position changes.
+        file&.unfold_diff_lines(self)
+
+        file
       end
       def get_formatter_class(type)

spec/controllers/projects/blob_controller_spec.rb

@@ -157,7 +157,7 @@ describe Projects::BlobController do
             match_line = JSON.parse(response.body).first
-            expect(match_line['type']).to eq('context')
+            expect(match_line['type']).to be_nil
           end
           it 'adds bottom match line when "t"o is less than blob size' do
@@ -177,7 +177,7 @@ describe Projects::BlobController do
             match_line = JSON.parse(response.body).last
-            expect(match_line['type']).to eq('context')
+            expect(match_line['type']).to be_nil
           end
         end
       end

spec/features/merge_request/user_posts_diff_notes_spec.rb

@@ -85,12 +85,13 @@ describe 'Merge request > User posts diff notes', :js do
       # `.line_holder` will be an unfolded line.
       let(:line_holder) { first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder') }
-      it 'does not allow commenting on the left side' do
-        should_not_allow_commenting(line_holder, 'left')
+      it 'allows commenting on the left side' do
+        should_allow_commenting(line_holder, 'left')
       end
-      it 'does not allow commenting on the right side' do
-        should_not_allow_commenting(line_holder, 'right')
+      it 'allows commenting on the right side' do
+        # Automatically shifts comment box to left side.
+        should_allow_commenting(line_holder, 'right')
       end
     end
   end
@@ -147,8 +148,8 @@ describe 'Merge request > User posts diff notes', :js do
       # `.line_holder` will be an unfolded line.
       let(:line_holder) { first('.line_holder[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') }
-      it 'does not allow commenting' do
-        should_not_allow_commenting line_holder
+      it 'allows commenting' do
+        should_allow_commenting line_holder
       end
     end