diff options
author | Cindy Pallares 🦉 <cindy@gitlab.com> | 2018-11-15 06:02:47 +0300 |
---|---|---|
committer | Cindy Pallares 🦉 <cindy@gitlab.com> | 2018-11-15 06:02:47 +0300 |
commit | 27d48d3d65e9ab44c73bcc0937540492f4236981 (patch) | |
tree | a9a4a61a9f5d8877442f263a614143ff3adc6c7c | |
parent | 3b3c915ca2945b40e5fc5c190962a693f59ce2aa (diff) | |
parent | d24f6ca066158bb955f2316a7de16e45d309689b (diff) |
Merge branch '11-5-stable-prepare-rc9' into '11-5-stable'
Prepare 11.5 RC9 release
See merge request gitlab-org/gitlab-ce!23061
35 files changed, 1409 insertions, 100 deletions
diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index a7eea2c1449..e651c197968 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/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, diff --git a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js index c23d4c484a5..89dcff74d0e 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js +++ b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js @@ -135,10 +135,6 @@ export default class FilteredSearchVisualTokens { } static updateUserTokenAppearance(tokenValueContainer, tokenValueElement, tokenValue) { - if (tokenValue === 'none') { - return Promise.resolve(); - } - const username = tokenValue.replace(/^@/, ''); return ( UsersCache.retrieve(username) @@ -184,7 +180,12 @@ export default class FilteredSearchVisualTokens { const tokenValueElement = tokenValueContainer.querySelector('.value'); tokenValueElement.innerText = tokenValue; + if (tokenValue === 'none' || tokenValue === 'any') { + return; + } + const tokenType = tokenName.toLowerCase(); + if (tokenType === 'label') { FilteredSearchVisualTokens.updateLabelTokenColor(tokenValueContainer, tokenValue); } else if (tokenType === 'author' || tokenType === 'assignee') { diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 45b921a2fbb..97b3f696139 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -241,7 +241,7 @@ table { .discussion-reply-holder { background-color: $white-light; padding: 10px 16px; - border-radius: 0 0 $border-radius-default $border-radius-default; + border-radius: 0 0 3px 3px; &.is-replying { padding-bottom: $gl-padding; diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 35e01c9c807..e4f59778d1e 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -28,7 +28,7 @@ $note-form-margin-left: 72px; } .main-notes-list { - @include vertical-line(39px); + @include vertical-line(36px); } .notes { @@ -499,6 +499,10 @@ $note-form-margin-left: 72px; @include vertical-line(52px); } + .notes_content { + background-color: $white-light; + } + .discussion-reply-holder { border-top: 1px solid $border-color; } @@ -910,3 +914,23 @@ $note-form-margin-left: 72px; } } } + +//This needs to be deleted when Snippet/Commit comments are convered to Vue +// See https://gitlab.com/gitlab-org/gitlab-ce/issues/53918#note_117038785 +.unstyled-comments { + + .discussion-header { + padding: $gl-padding; + border-bottom: 1px solid $border-color; + } + + .note-wrapper.outlined { + margin: 0; + border: 0; + border-radius: 0; + } + + .discussion-form-container { + padding: $gl-padding; + } +} diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index c02ec407262..0718658cd48 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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, expanded_diff_line_type, nil, nil, nil) diff_line.rich_text = line diff_line end @@ -132,6 +132,11 @@ class Projects::BlobController < Projects::ApplicationController render json: DiffLineSerializer.new.represent(@lines) end + def expanded_diff_line_type + # Context lines can't receive comments. + Feature.enabled?(:comment_in_any_diff_line, @project) ? nil : 'context' + end + def add_match_line return unless @form.unfold? diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 5307cd0720a..b3d77335c2a 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/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 = renderable_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 @@ -108,4 +114,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs) @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request) end + + def renderable_notes + define_diff_comment_vars unless @notes + + @notes + end end diff --git a/app/helpers/profiles_helper.rb b/app/helpers/profiles_helper.rb index 42f9a1213e9..df318de740a 100644 --- a/app/helpers/profiles_helper.rb +++ b/app/helpers/profiles_helper.rb @@ -7,7 +7,7 @@ module ProfilesHelper [ [s_("Profiles|Use a private email - %{email}").html_safe % { email: private_email }, Gitlab::PrivateCommitEmail::TOKEN], - verified_emails + *verified_emails ] end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 5f59e4832db..c32008aa9c7 100644 --- a/app/models/diff_note.rb +++ b/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? diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb index b47d8f3f63a..c64b2e99b52 100644 --- a/app/services/merge_requests/reload_diffs_service.rb +++ b/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. diff --git a/app/services/notes/base_service.rb b/app/services/notes/base_service.rb new file mode 100644 index 00000000000..c1260837c12 --- /dev/null +++ b/app/services/notes/base_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Notes + class BaseService < ::BaseService + def clear_noteable_diffs_cache(note) + if note.is_a?(DiffNote) && + note.discussion_first_note? && + note.position.unfolded_diff?(project.repository) + note.noteable.diffs.clear_cache + end + end + end +end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 049e6c5a871..e03789e3ca9 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -1,7 +1,7 @@ # 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? diff --git a/app/services/notes/destroy_service.rb b/app/services/notes/destroy_service.rb index 64e9accd97f..fa0c2c5c86b 100644 --- a/app/services/notes/destroy_service.rb +++ b/app/services/notes/destroy_service.rb @@ -1,11 +1,13 @@ # 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 diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index 1765251c93d..10187129a33 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -1,12 +1,12 @@ - expanded = discussion.expanded? -%li.note.note-discussion.timeline-entry +%li.note.note-discussion.timeline-entry.unstyled-comments .timeline-entry-inner - .timeline-icon - = link_to user_path(discussion.author) do - = image_tag avatar_icon_for_user(discussion.author), class: "avatar s40" .timeline-content .discussion.js-toggle-container{ data: { discussion_id: discussion.id } } .discussion-header + .timeline-icon + = link_to user_path(discussion.author) do + = image_tag avatar_icon_for_user(discussion.author), class: "avatar s40" .discussion-actions %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button", class: ("js-toggle-lazy-diff" unless expanded) } - if expanded diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 82693ec832e..69914fccc48 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -7,10 +7,10 @@ .event-title.d-flex.flex-wrap = inline_event_icon(event) %span.event-type.d-inline-block.append-right-4.pushed #{event.action_name} #{event.ref_type} - %span + %span.append-right-4 - commits_link = project_commits_path(project, event.ref_name) - should_link = event.tag? ? project.repository.tag_exists?(event.ref_name) : project.repository.branch_exists?(event.ref_name) - = link_to_if should_link, event.ref_name, commits_link, class: 'ref-name append-right-4' + = link_to_if should_link, event.ref_name, commits_link, class: 'ref-name' = render "events/event_scope", event: event diff --git a/changelogs/unreleased/53636-fix-rendering-of-any-user-filter.yml b/changelogs/unreleased/53636-fix-rendering-of-any-user-filter.yml new file mode 100644 index 00000000000..a59a276a334 --- /dev/null +++ b/changelogs/unreleased/53636-fix-rendering-of-any-user-filter.yml @@ -0,0 +1,5 @@ +--- +title: Fix rendering of filter bar tokens for special values +merge_request: 22865 +author: Heinrich Lee Yu +type: fixed diff --git a/changelogs/unreleased/54002-activity-feed-missing-padding-in-event-note-when-a-branch-is-deleted.yml b/changelogs/unreleased/54002-activity-feed-missing-padding-in-event-note-when-a-branch-is-deleted.yml new file mode 100644 index 00000000000..9f4f104a12c --- /dev/null +++ b/changelogs/unreleased/54002-activity-feed-missing-padding-in-event-note-when-a-branch-is-deleted.yml @@ -0,0 +1,5 @@ +--- +title: Adds margin after a deleted branch name in the activity feed. +merge_request: 23038 +author: +type: fixed diff --git a/changelogs/unreleased/dm-commit-email-select-options.yml b/changelogs/unreleased/dm-commit-email-select-options.yml new file mode 100644 index 00000000000..90d5c8cf0c6 --- /dev/null +++ b/changelogs/unreleased/dm-commit-email-select-options.yml @@ -0,0 +1,5 @@ +--- +title: Fix bug causing not all emails to show up in commit email selectbox +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/osw-comment-on-any-line-on-diffs.yml b/changelogs/unreleased/osw-comment-on-any-line-on-diffs.yml new file mode 100644 index 00000000000..e25d64a89d7 --- /dev/null +++ b/changelogs/unreleased/osw-comment-on-any-line-on-diffs.yml @@ -0,0 +1,5 @@ +--- +title: Allow commenting on any diff line in Merge Requests +merge_request: 22914 +author: +type: added diff --git a/doc/topics/git/troubleshooting_git.md b/doc/topics/git/troubleshooting_git.md index 8555c5e91ea..d1729d70158 100644 --- a/doc/topics/git/troubleshooting_git.md +++ b/doc/topics/git/troubleshooting_git.md @@ -78,5 +78,20 @@ git push In case you're running an older version of Git (< 2.9), consider upgrading to >= 2.9 (see [Broken pipe when pushing to Git repository][Broken-Pipe]). +## Timeout during git push/pull + +If pulling/pushing from/to your repository ends up taking more than 50 seconds, +a timeout will be issued with a log of the number of operations performed +and their respective timings, like the example below: + +``` +remote: Running checks for branch: master +remote: Scanning for LFS objects... (153ms) +remote: Calculating new repository size... (cancelled after 729ms) +``` + +This could be used to further investigate what operation is performing poorly +and provide GitLab with more information on how to improve the service. + [SSH troubleshooting]: ../../ssh/README.md#troubleshooting "SSH Troubleshooting" [Broken-Pipe]: https://stackoverflow.com/questions/19120120/broken-pipe-when-pushing-to-git-repository/36971469#36971469 "StackOverflow: 'Broken pipe when pushing to Git repository'" diff --git a/doc/user/project/repository/index.md b/doc/user/project/repository/index.md index 6d822d3f7f2..1710bba2fd0 100644 --- a/doc/user/project/repository/index.md +++ b/doc/user/project/repository/index.md @@ -53,17 +53,35 @@ To get started with the command line, please read through the Use GitLab's [file finder](../../../workflow/file_finder.md) to search for files in a repository. +### Supported markup languages and extensions + +GitLab supports a number of markup languages (sometimes called [lightweight +markup languages](https://en.wikipedia.org/wiki/Lightweight_markup_language)) +that you can use for the content of your files in a repository. They are mostly +used for documentation purposes. + +Just pick the right extension for your files and GitLab will render them +according to the markup language. + +| Markup language | Extensions | +| --------------- | ---------- | +| Plain text | `txt` | +| [Markdown](../../markdown.md) | `mdown`, `mkd`, `mkdn`, `md`, `markdown` | +| [reStructuredText](http://docutils.sourceforge.net/rst.html) | `rst` | +| [Asciidoc](https://asciidoctor.org/docs/what-is-asciidoc/) | `adoc`, `ad`, `asciidoc` | +| [Textile](https://txstyle.org/) | `textile` | +| [rdoc](http://rdoc.sourceforge.net/doc/index.html) | `rdoc` | +| [Orgmode](https://orgmode.org/) | `org` | +| [creole](http://www.wikicreole.org/) | `creole` | +| [Mediawiki](https://www.mediawiki.org/wiki/MediaWiki) | `wiki`, `mediawiki` | + ### Repository README and index files When a `README` or `index` file is present in a repository, its contents will be automatically pre-rendered by GitLab without opening it. -They can either be plain text or have an extension of a supported markup language: - -- Asciidoc: `README.adoc` or `index.adoc` -- Markdown: `README.md` or `index.md` -- reStructuredText: `README.rst` or `index.rst` -- Text: `README.txt` or `index.txt` +They can either be plain text or have an extension of a +[supported markup language](#supported-markup-languages-and-extensions): Some things to note about precedence: @@ -75,10 +93,6 @@ Some things to note about precedence: precedence over `README.md`, and `README.rst` will take precedence over `README`. -NOTE: **Note:** -`index` files without an extension will not automatically pre-render. You'll -have to explicitly open them to see their contents. - ### Jupyter Notebook files > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/2508) in GitLab 9.1 diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index fb117baca9e..84595f8afd7 100644 --- a/lib/gitlab/diff/file.rb +++ b/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 diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 5b67cd46c48..70063071ee7 100644 --- a/lib/gitlab/diff/line.rb +++ b/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 diff --git a/lib/gitlab/diff/lines_unfolder.rb b/lib/gitlab/diff/lines_unfolder.rb new file mode 100644 index 00000000000..9306b7e16a2 --- /dev/null +++ b/lib/gitlab/diff/lines_unfolder.rb @@ -0,0 +1,235 @@ +# 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 diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index f967494199e..7bfab2d808f 100644 --- a/lib/gitlab/diff/position.rb +++ b/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) diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb index bc88e6450f5..135925c007f 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb @@ -10,7 +10,7 @@ module QA let(:key_title) { "key for ssh tests #{Time.now.to_f}" } let(:ssh_key) do - Factory::Resource::SSHKey.fabricate! do |resource| + Resource::SSHKey.fabricate! do |resource| resource.title = key_title end end @@ -38,7 +38,7 @@ module QA it 'user pushes to the repository' do # Create a project to push to - project = Factory::Resource::Project.fabricate! do |project| + project = Resource::Project.fabricate! do |project| project.name = 'git-protocol-project' end diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb index 28f7e4634a5..74771abde71 100644 --- a/spec/controllers/projects/blob_controller_spec.rb +++ b/spec/controllers/projects/blob_controller_spec.rb @@ -141,6 +141,28 @@ describe Projects::BlobController do expect(lines.first).to have_key('rich_text') end + context 'comment in any diff line feature flag' do + it 'renders context lines when feature disabled' do + stub_feature_flags(comment_in_any_diff_line: false) + + do_get(since: 1, to: 5, offset: 10, from_merge_request: true) + lines = JSON.parse(response.body) + all_context = lines.all? { |line| line['type'] == 'context' } + + expect(all_context).to be(true) + end + + it 'renders unchanged lines when feature enabled' do + stub_feature_flags(comment_in_any_diff_line: true) + + do_get(since: 1, to: 5, offset: 10, from_merge_request: true) + lines = JSON.parse(response.body) + all_unchanged = lines.all? { |line| line['type'].nil? } + + expect(all_unchanged).to be(true) + end + end + context 'when rendering match lines' do it 'adds top match line when "since" is less than 1' do do_get(since: 5, to: 10, offset: 10, from_merge_request: true) @@ -157,7 +179,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 +199,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 diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index fa148715855..51b78d3e7d1 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/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 diff --git a/spec/helpers/profiles_helper_spec.rb b/spec/helpers/profiles_helper_spec.rb index 9a2372de69f..8e336469c27 100644 --- a/spec/helpers/profiles_helper_spec.rb +++ b/spec/helpers/profiles_helper_spec.rb @@ -4,12 +4,17 @@ describe ProfilesHelper do describe '#commit_email_select_options' do it 'returns an array with private commit email along with all the verified emails' do user = create(:user) + create(:email, user: user) + confirmed_email1 = create(:email, :confirmed, user: user) + confirmed_email2 = create(:email, :confirmed, user: user) + private_email = user.private_commit_email - verified_emails = user.verified_emails - [private_email] emails = [ ["Use a private email - #{private_email}", Gitlab::PrivateCommitEmail::TOKEN], - verified_emails + user.email, + confirmed_email1.email, + confirmed_email2.email ] expect(helper.commit_email_select_options(user)).to match_array(emails) diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index fed04cbaed8..8821cde76f4 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -98,7 +98,7 @@ describe('DiffsStoreMutations', () => { it('should call utils.addContextLines with proper params', () => { const options = { lineNumbers: { oldLineNumber: 1, newLineNumber: 2 }, - contextLines: [{ oldLine: 1 }], + contextLines: [{ oldLine: 1, newLine: 1, lineCode: 'ff9200_1_1', discussions: [] }], fileHash: 'ff9200', params: { bottom: true, @@ -110,7 +110,7 @@ describe('DiffsStoreMutations', () => { parallelDiffLines: [], }; const state = { diffFiles: [diffFile] }; - const lines = [{ oldLine: 1 }]; + const lines = [{ oldLine: 1, newLine: 1 }]; const findDiffFileSpy = spyOnDependency(mutations, 'findDiffFile').and.returnValue(diffFile); const removeMatchLineSpy = spyOnDependency(mutations, 'removeMatchLine'); diff --git a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js index 0c1d5f5b0b4..4f561df7943 100644 --- a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js +++ b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js @@ -754,6 +754,50 @@ describe('Filtered Search Visual Tokens', () => { expect(updateLabelTokenColorSpy.calls.count()).toBe(0); expect(updateUserTokenAppearanceSpy.calls.count()).toBe(0); }); + + it('does not update user token appearance for `none` filter', () => { + const { tokenNameElement } = findElements(authorToken); + + const tokenName = tokenNameElement.innerText; + const tokenValue = 'none'; + + subject.renderVisualTokenValue(authorToken, tokenName, tokenValue); + + expect(updateUserTokenAppearanceSpy.calls.count()).toBe(0); + }); + + it('does not update user token appearance for `any` filter', () => { + const { tokenNameElement } = findElements(authorToken); + + const tokenName = tokenNameElement.innerText; + const tokenValue = 'any'; + + subject.renderVisualTokenValue(authorToken, tokenName, tokenValue); + + expect(updateUserTokenAppearanceSpy.calls.count()).toBe(0); + }); + + it('does not update label token color for `none` filter', () => { + const { tokenNameElement } = findElements(bugLabelToken); + + const tokenName = tokenNameElement.innerText; + const tokenValue = 'none'; + + subject.renderVisualTokenValue(bugLabelToken, tokenName, tokenValue); + + expect(updateLabelTokenColorSpy.calls.count()).toBe(0); + }); + + it('does not update label token color for `any` filter', () => { + const { tokenNameElement } = findElements(bugLabelToken); + + const tokenName = tokenNameElement.innerText; + const tokenValue = 'any'; + + subject.renderVisualTokenValue(bugLabelToken, tokenName, tokenValue); + + expect(updateLabelTokenColorSpy.calls.count()).toBe(0); + }); }); describe('updateUserTokenAppearance', () => { @@ -763,19 +807,6 @@ describe('Filtered Search Visual Tokens', () => { spyOn(UsersCache, 'retrieve').and.callFake(username => usersCacheSpy(username)); }); - it('ignores special value "none"', done => { - usersCacheSpy = username => { - expect(username).toBe('none'); - done.fail('Should not resolve "none"!'); - }; - const { tokenValueContainer, tokenValueElement } = findElements(authorToken); - - subject - .updateUserTokenAppearance(tokenValueContainer, tokenValueElement, 'none') - .then(done) - .catch(done.fail); - }); - it('ignores error if UsersCache throws', done => { spyOn(window, 'Flash'); const dummyError = new Error('Earth rotated backwards'); diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 2f51642b58e..3417896e259 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -41,6 +41,52 @@ describe Gitlab::Diff::File do end end + describe '#unfold_diff_lines' do + let(:unfolded_lines) { double('expanded-lines') } + let(:unfolder) { instance_double(Gitlab::Diff::LinesUnfolder) } + let(:position) { instance_double(Gitlab::Diff::Position, old_line: 10) } + + before do + allow(Gitlab::Diff::LinesUnfolder).to receive(:new) { unfolder } + end + + context 'when unfold required' do + before do + allow(unfolder).to receive(:unfold_required?) { true } + allow(unfolder).to receive(:unfolded_diff_lines) { unfolded_lines } + end + + it 'changes @unfolded to true' do + diff_file.unfold_diff_lines(position) + + expect(diff_file).to be_unfolded + end + + it 'updates @diff_lines' do + diff_file.unfold_diff_lines(position) + + expect(diff_file.diff_lines).to eq(unfolded_lines) + end + end + + context 'when unfold not required' do + before do + allow(unfolder).to receive(:unfold_required?) { false } + end + + it 'keeps @unfolded false' do + diff_file.unfold_diff_lines(position) + + expect(diff_file).not_to be_unfolded + end + + it 'does not update @diff_lines' do + expect { diff_file.unfold_diff_lines(position) } + .not_to change(diff_file, :diff_lines) + end + end + end + describe '#mode_changed?' do it { expect(diff_file.mode_changed?).to be_falsey } end diff --git a/spec/lib/gitlab/diff/lines_unfolder_spec.rb b/spec/lib/gitlab/diff/lines_unfolder_spec.rb new file mode 100644 index 00000000000..8e00c8e0e30 --- /dev/null +++ b/spec/lib/gitlab/diff/lines_unfolder_spec.rb @@ -0,0 +1,750 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Diff::LinesUnfolder do + let(:raw_diff) do + <<-DIFF.strip_heredoc + @@ -7,9 +7,6 @@ + "tags": ["devel", "development", "nightly"], + "desktop-file-name-prefix": "(Development) ", + "finish-args": [ + - "--share=ipc", "--socket=x11", + - "--socket=wayland", + - "--talk-name=org.gnome.OnlineAccounts", + "--talk-name=org.freedesktop.Tracker1", + "--filesystem=home", + "--talk-name=org.gtk.vfs", "--talk-name=org.gtk.vfs.*", + @@ -62,7 +59,7 @@ + }, + { + "name": "gnome-desktop", + - "config-opts": ["--disable-debug-tools", "--disable-udev"], + + "config-opts": ["--disable-debug-tools", "--disable-"], + "sources": [ + { + "type": "git", + @@ -83,11 +80,6 @@ + "buildsystem": "meson", + "builddir": true, + "name": "nautilus", + - "config-opts": [ + - "-Denable-desktop=false", + - "-Denable-selinux=false", + - "--libdir=/app/lib" + - ], + "sources": [ + { + "type": "git", + DIFF + end + + let(:raw_old_blob) do + <<-BLOB.strip_heredoc + { + "app-id": "org.gnome.Nautilus", + "runtime": "org.gnome.Platform", + "runtime-version": "master", + "sdk": "org.gnome.Sdk", + "command": "nautilus", + "tags": ["devel", "development", "nightly"], + "desktop-file-name-prefix": "(Development) ", + "finish-args": [ + "--share=ipc", "--socket=x11", + "--socket=wayland", + "--talk-name=org.gnome.OnlineAccounts", + "--talk-name=org.freedesktop.Tracker1", + "--filesystem=home", + "--talk-name=org.gtk.vfs", "--talk-name=org.gtk.vfs.*", + "--filesystem=xdg-run/dconf", "--filesystem=~/.config/dconf:ro", + "--talk-name=ca.desrt.dconf", "--env=DCONF_USER_CONFIG_DIR=.config/dconf" + ], + "cleanup": [ "/include", "/share/bash-completion" ], + "modules": [ + { + "name": "exiv2", + "sources": [ + { + "type": "archive", + "url": "http://exiv2.org/builds/exiv2-0.26-trunk.tar.gz", + "sha256": "c75e3c4a0811bf700d92c82319373b7a825a2331c12b8b37d41eb58e4f18eafb" + }, + { + "type": "shell", + "commands": [ + "cp -f /usr/share/gnu-config/config.sub ./config/", + "cp -f /usr/share/gnu-config/config.guess ./config/" + ] + } + ] + }, + { + "name": "gexiv2", + "config-opts": [ "--disable-introspection" ], + "sources": [ + { + "type": "git", + "url": "https://git.gnome.org/browse/gexiv2" + } + ] + }, + { + "name": "tracker", + "cleanup": [ "/bin", "/etc", "/libexec" ], + "config-opts": [ "--disable-miner-apps", "--disable-static", + "--disable-tracker-extract", "--disable-tracker-needle", + "--disable-tracker-preferences", "--disable-artwork", + "--disable-tracker-writeback", "--disable-miner-user-guides", + "--with-bash-completion-dir=no" ], + "sources": [ + { + "type": "git", + "url": "https://git.gnome.org/browse/tracker" + } + ] + }, + { + "name": "gnome-desktop", + "config-opts": ["--disable-debug-tools", "--disable-udev"], + "sources": [ + { + "type": "git", + "url": "https://git.gnome.org/browse/gnome-desktop" + } + ] + }, + { + "name": "gnome-autoar", + "sources": [ + { + "type": "git", + "url": "https://git.gnome.org/browse/gnome-autoar" + } + ] + }, + { + "buildsystem": "meson", + "builddir": true, + "name": "nautilus", + "config-opts": [ + "-Denable-desktop=false", + "-Denable-selinux=false", + "--libdir=/app/lib" + ], + "sources": [ + { + "type": "git", + "url": "https://gitlab.gnome.org/GNOME/nautilus.git" + } + ] + } + ] + }, + { + "app-id": "foo", + "runtime": "foo", + "runtime-version": "foo", + "sdk": "foo", + "command": "foo", + "tags": ["foo", "bar", "kux"], + "desktop-file-name-prefix": "(Foo) ", + { + "buildsystem": "meson", + "builddir": true, + "name": "nautilus", + "sources": [ + { + "type": "git", + "url": "https://gitlab.gnome.org/GNOME/nautilus.git" + } + ] + } + }, + { + "app-id": "foo", + "runtime": "foo", + "runtime-version": "foo", + "sdk": "foo", + "command": "foo", + "tags": ["foo", "bar", "kux"], + "desktop-file-name-prefix": "(Foo) ", + { + "buildsystem": "meson", + "builddir": true, + "name": "nautilus", + "sources": [ + { + "type": "git", + "url": "https://gitlab.gnome.org/GNOME/nautilus.git" + } + ] + } + } + BLOB + end + + let(:project) { create(:project) } + + let(:old_blob) { Gitlab::Git::Blob.new(data: raw_old_blob) } + + let(:diff) do + Gitlab::Git::Diff.new(diff: raw_diff, + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + a_mode: "100644", + b_mode: "100644", + new_file: false, + renamed_file: false, + deleted_file: false, + too_large: false) + end + + let(:diff_file) do + Gitlab::Diff::File.new(diff, repository: project.repository) + end + + before do + allow(old_blob).to receive(:load_all_data!) + allow(diff_file).to receive(:old_blob) { old_blob } + end + + subject { described_class.new(diff_file, position) } + + context 'position requires a middle expansion and new match lines' do + let(:position) do + Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + head_sha: "1487062132228de836236c522fe52fed4980a46c", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + position_type: "text", + old_line: 43, + new_line: 40) + end + + context 'blob lines' do + let(:expected_blob_lines) do + [[40, 40, " \"config-opts\": [ \"--disable-introspection\" ],"], + [41, 41, " \"sources\": ["], + [42, 42, " {"], + [43, 43, " \"type\": \"git\","], + [44, 44, " \"url\": \"https://git.gnome.org/browse/gexiv2\""], + [45, 45, " }"], + [46, 46, " ]"]] + end + + it 'returns the extracted blob lines correctly' do + extracted_lines = subject.blob_lines + + expect(extracted_lines.size).to eq(7) + + extracted_lines.each_with_index do |line, i| + expect([line.old_line, line.new_line, line.text]).to eq(expected_blob_lines[i]) + end + end + end + + context 'diff lines' do + let(:expected_diff_lines) do + [[7, 7, "@@ -7,9 +7,6 @@"], + [7, 7, " \"tags\": [\"devel\", \"development\", \"nightly\"],"], + [8, 8, " \"desktop-file-name-prefix\": \"(Development) \","], + [9, 9, " \"finish-args\": ["], + [10, 10, "- \"--share=ipc\", \"--socket=x11\","], + [11, 10, "- \"--socket=wayland\","], + [12, 10, "- \"--talk-name=org.gnome.OnlineAccounts\","], + [13, 10, " \"--talk-name=org.freedesktop.Tracker1\","], + [14, 11, " \"--filesystem=home\","], + [15, 12, " \"--talk-name=org.gtk.vfs\", \"--talk-name=org.gtk.vfs.*\","], + + # New match line + [40, 37, "@@ -40,7+37,7 @@"], + + # Injected blob lines + [40, 37, " \"config-opts\": [ \"--disable-introspection\" ],"], + [41, 38, " \"sources\": ["], + [42, 39, " {"], + [43, 40, " \"type\": \"git\","], # comment + [44, 41, " \"url\": \"https://git.gnome.org/browse/gexiv2\""], + [45, 42, " }"], + [46, 43, " ]"], + # end + + # Second match line + [62, 59, "@@ -62,7+59,7 @@"], + + [62, 59, " },"], + [63, 60, " {"], + [64, 61, " \"name\": \"gnome-desktop\","], + [65, 62, "- \"config-opts\": [\"--disable-debug-tools\", \"--disable-udev\"],"], + [66, 62, "+ \"config-opts\": [\"--disable-debug-tools\", \"--disable-\"],"], + [66, 63, " \"sources\": ["], + [67, 64, " {"], + [68, 65, " \"type\": \"git\","], + [83, 80, "@@ -83,11 +80,6 @@"], + [83, 80, " \"buildsystem\": \"meson\","], + [84, 81, " \"builddir\": true,"], + [85, 82, " \"name\": \"nautilus\","], + [86, 83, "- \"config-opts\": ["], + [87, 83, "- \"-Denable-desktop=false\","], + [88, 83, "- \"-Denable-selinux=false\","], + [89, 83, "- \"--libdir=/app/lib\""], + [90, 83, "- ],"], + [91, 83, " \"sources\": ["], + [92, 84, " {"], + [93, 85, " \"type\": \"git\","]] + end + + it 'return merge of blob lines with diff lines correctly' do + new_diff_lines = subject.unfolded_diff_lines + + expected_diff_lines.each_with_index do |expected_line, i| + line = new_diff_lines[i] + + expect([line.old_pos, line.new_pos, line.text]) + .to eq([expected_line[0], expected_line[1], expected_line[2]]) + end + end + + it 'merged lines have correct line codes' do + new_diff_lines = subject.unfolded_diff_lines + + new_diff_lines.each_with_index do |line, i| + old_pos, new_pos = expected_diff_lines[i][0], expected_diff_lines[i][1] + + unless line.type == 'match' + expect(line.line_code).to eq(Gitlab::Git.diff_line_code(diff_file.file_path, new_pos, old_pos)) + end + end + end + end + end + + context 'position requires a middle expansion and no top match line' do + let(:position) do + Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + head_sha: "1487062132228de836236c522fe52fed4980a46c", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + position_type: "text", + old_line: 16, + new_line: 17) + end + + context 'blob lines' do + let(:expected_blob_lines) do + [[16, 16, " \"--filesystem=xdg-run/dconf\", \"--filesystem=~/.config/dconf:ro\","], + [17, 17, " \"--talk-name=ca.desrt.dconf\", \"--env=DCONF_USER_CONFIG_DIR=.config/dconf\""], + [18, 18, " ],"], + [19, 19, " \"cleanup\": [ \"/include\", \"/share/bash-completion\" ],"]] + end + + it 'returns the extracted blob lines correctly' do + extracted_lines = subject.blob_lines + + expect(extracted_lines.size).to eq(4) + + extracted_lines.each_with_index do |line, i| + expect([line.old_line, line.new_line, line.text]).to eq(expected_blob_lines[i]) + end + end + end + + context 'diff lines' do + let(:expected_diff_lines) do + [[7, 7, "@@ -7,9 +7,6 @@"], + [7, 7, " \"tags\": [\"devel\", \"development\", \"nightly\"],"], + [8, 8, " \"desktop-file-name-prefix\": \"(Development) \","], + [9, 9, " \"finish-args\": ["], + [10, 10, "- \"--share=ipc\", \"--socket=x11\","], + [11, 10, "- \"--socket=wayland\","], + [12, 10, "- \"--talk-name=org.gnome.OnlineAccounts\","], + [13, 10, " \"--talk-name=org.freedesktop.Tracker1\","], + [14, 11, " \"--filesystem=home\","], + [15, 12, " \"--talk-name=org.gtk.vfs\", \"--talk-name=org.gtk.vfs.*\","], + # No new match needed + + # Injected blob lines + [16, 13, " \"--filesystem=xdg-run/dconf\", \"--filesystem=~/.config/dconf:ro\","], + [17, 14, " \"--talk-name=ca.desrt.dconf\", \"--env=DCONF_USER_CONFIG_DIR=.config/dconf\""], + [18, 15, " ],"], + [19, 16, " \"cleanup\": [ \"/include\", \"/share/bash-completion\" ],"], + # end + + # Second match line + [62, 59, "@@ -62,4+59,4 @@"], + + [62, 59, " },"], + [63, 60, " {"], + [64, 61, " \"name\": \"gnome-desktop\","], + [65, 62, "- \"config-opts\": [\"--disable-debug-tools\", \"--disable-udev\"],"], + [66, 62, "+ \"config-opts\": [\"--disable-debug-tools\", \"--disable-\"],"], + [66, 63, " \"sources\": ["], + [67, 64, " {"], + [68, 65, " \"type\": \"git\","], + [83, 80, "@@ -83,11 +80,6 @@"], + [83, 80, " \"buildsystem\": \"meson\","], + [84, 81, " \"builddir\": true,"], + [85, 82, " \"name\": \"nautilus\","], + [86, 83, "- \"config-opts\": ["], + [87, 83, "- \"-Denable-desktop=false\","], + [88, 83, "- \"-Denable-selinux=false\","], + [89, 83, "- \"--libdir=/app/lib\""], + [90, 83, "- ],"], + [91, 83, " \"sources\": ["], + [92, 84, " {"], + [93, 85, " \"type\": \"git\","]] + end + + it 'return merge of blob lines with diff lines correctly' do + new_diff_lines = subject.unfolded_diff_lines + + expected_diff_lines.each_with_index do |expected_line, i| + line = new_diff_lines[i] + + expect([line.old_pos, line.new_pos, line.text]) + .to eq([expected_line[0], expected_line[1], expected_line[2]]) + end + end + + it 'merged lines have correct line codes' do + new_diff_lines = subject.unfolded_diff_lines + + new_diff_lines.each_with_index do |line, i| + old_pos, new_pos = expected_diff_lines[i][0], expected_diff_lines[i][1] + + unless line.type == 'match' + expect(line.line_code).to eq(Gitlab::Git.diff_line_code(diff_file.file_path, new_pos, old_pos)) + end + end + end + end + end + + context 'position requires a middle expansion and no bottom match line' do + let(:position) do + Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + head_sha: "1487062132228de836236c522fe52fed4980a46c", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + position_type: "text", + old_line: 82, + new_line: 79) + end + + context 'blob lines' do + let(:expected_blob_lines) do + [[79, 79, " }"], + [80, 80, " ]"], + [81, 81, " },"], + [82, 82, " {"]] + end + + it 'returns the extracted blob lines correctly' do + extracted_lines = subject.blob_lines + + expect(extracted_lines.size).to eq(4) + + extracted_lines.each_with_index do |line, i| + expect([line.old_line, line.new_line, line.text]).to eq(expected_blob_lines[i]) + end + end + end + + context 'diff lines' do + let(:expected_diff_lines) do + [[7, 7, "@@ -7,9 +7,6 @@"], + [7, 7, " \"tags\": [\"devel\", \"development\", \"nightly\"],"], + [8, 8, " \"desktop-file-name-prefix\": \"(Development) \","], + [9, 9, " \"finish-args\": ["], + [10, 10, "- \"--share=ipc\", \"--socket=x11\","], + [11, 10, "- \"--socket=wayland\","], + [12, 10, "- \"--talk-name=org.gnome.OnlineAccounts\","], + [13, 10, " \"--talk-name=org.freedesktop.Tracker1\","], + [14, 11, " \"--filesystem=home\","], + [15, 12, " \"--talk-name=org.gtk.vfs\", \"--talk-name=org.gtk.vfs.*\","], + [62, 59, "@@ -62,7 +59,7 @@"], + [62, 59, " },"], + [63, 60, " {"], + [64, 61, " \"name\": \"gnome-desktop\","], + [65, 62, "- \"config-opts\": [\"--disable-debug-tools\", \"--disable-udev\"],"], + [66, 62, "+ \"config-opts\": [\"--disable-debug-tools\", \"--disable-\"],"], + [66, 63, " \"sources\": ["], + [67, 64, " {"], + [68, 65, " \"type\": \"git\","], + + # New top match line + [79, 76, "@@ -79,4+76,4 @@"], + + # Injected blob lines + [79, 76, " }"], + [80, 77, " ]"], + [81, 78, " },"], + [82, 79, " {"], + # end + + # No new second match line + [83, 80, " \"buildsystem\": \"meson\","], + [84, 81, " \"builddir\": true,"], + [85, 82, " \"name\": \"nautilus\","], + [86, 83, "- \"config-opts\": ["], + [87, 83, "- \"-Denable-desktop=false\","], + [88, 83, "- \"-Denable-selinux=false\","], + [89, 83, "- \"--libdir=/app/lib\""], + [90, 83, "- ],"], + [91, 83, " \"sources\": ["], + [92, 84, " {"], + [93, 85, " \"type\": \"git\","]] + end + + it 'return merge of blob lines with diff lines correctly' do + new_diff_lines = subject.unfolded_diff_lines + + expected_diff_lines.each_with_index do |expected_line, i| + line = new_diff_lines[i] + + expect([line.old_pos, line.new_pos, line.text]) + .to eq([expected_line[0], expected_line[1], expected_line[2]]) + end + end + + it 'merged lines have correct line codes' do + new_diff_lines = subject.unfolded_diff_lines + + new_diff_lines.each_with_index do |line, i| + old_pos, new_pos = expected_diff_lines[i][0], expected_diff_lines[i][1] + + unless line.type == 'match' + expect(line.line_code).to eq(Gitlab::Git.diff_line_code(diff_file.file_path, new_pos, old_pos)) + end + end + end + end + end + + context 'position requires a short top expansion' do + let(:position) do + Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + head_sha: "1487062132228de836236c522fe52fed4980a46c", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + position_type: "text", + old_line: 6, + new_line: 6) + end + + context 'blob lines' do + let(:expected_blob_lines) do + [[3, 3, " \"runtime\": \"org.gnome.Platform\","], + [4, 4, " \"runtime-version\": \"master\","], + [5, 5, " \"sdk\": \"org.gnome.Sdk\","], + [6, 6, " \"command\": \"nautilus\","]] + end + + it 'returns the extracted blob lines correctly' do + extracted_lines = subject.blob_lines + + expect(extracted_lines.size).to eq(4) + + extracted_lines.each_with_index do |line, i| + expect([line.old_line, line.new_line, line.text]).to eq(expected_blob_lines[i]) + end + end + end + + context 'diff lines' do + let(:expected_diff_lines) do + # New match line + [[3, 3, "@@ -3,4+3,4 @@"], + + # Injected blob lines + [3, 3, " \"runtime\": \"org.gnome.Platform\","], + [4, 4, " \"runtime-version\": \"master\","], + [5, 5, " \"sdk\": \"org.gnome.Sdk\","], + [6, 6, " \"command\": \"nautilus\","], + # end + [7, 7, " \"tags\": [\"devel\", \"development\", \"nightly\"],"], + [8, 8, " \"desktop-file-name-prefix\": \"(Development) \","], + [9, 9, " \"finish-args\": ["], + [10, 10, "- \"--share=ipc\", \"--socket=x11\","], + [11, 10, "- \"--socket=wayland\","], + [12, 10, "- \"--talk-name=org.gnome.OnlineAccounts\","], + [13, 10, " \"--talk-name=org.freedesktop.Tracker1\","], + [14, 11, " \"--filesystem=home\","], + [15, 12, " \"--talk-name=org.gtk.vfs\", \"--talk-name=org.gtk.vfs.*\","], + [62, 59, "@@ -62,7 +59,7 @@"], + [62, 59, " },"], + [63, 60, " {"], + [64, 61, " \"name\": \"gnome-desktop\","], + [65, 62, "- \"config-opts\": [\"--disable-debug-tools\", \"--disable-udev\"],"], + [66, 62, "+ \"config-opts\": [\"--disable-debug-tools\", \"--disable-\"],"], + [66, 63, " \"sources\": ["], + [67, 64, " {"], + [68, 65, " \"type\": \"git\","], + [83, 80, "@@ -83,11 +80,6 @@"], + [83, 80, " \"buildsystem\": \"meson\","], + [84, 81, " \"builddir\": true,"], + [85, 82, " \"name\": \"nautilus\","], + [86, 83, "- \"config-opts\": ["], + [87, 83, "- \"-Denable-desktop=false\","], + [88, 83, "- \"-Denable-selinux=false\","], + [89, 83, "- \"--libdir=/app/lib\""], + [90, 83, "- ],"], + [91, 83, " \"sources\": ["], + [92, 84, " {"], + [93, 85, " \"type\": \"git\","]] + end + + it 'return merge of blob lines with diff lines correctly' do + new_diff_lines = subject.unfolded_diff_lines + + expected_diff_lines.each_with_index do |expected_line, i| + line = new_diff_lines[i] + + expect([line.old_pos, line.new_pos, line.text]) + .to eq([expected_line[0], expected_line[1], expected_line[2]]) + end + end + + it 'merged lines have correct line codes' do + new_diff_lines = subject.unfolded_diff_lines + + new_diff_lines.each_with_index do |line, i| + old_pos, new_pos = expected_diff_lines[i][0], expected_diff_lines[i][1] + + unless line.type == 'match' + expect(line.line_code).to eq(Gitlab::Git.diff_line_code(diff_file.file_path, new_pos, old_pos)) + end + end + end + end + end + + context 'position sits between two match lines (no expasion needed)' do + let(:position) do + Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + head_sha: "1487062132228de836236c522fe52fed4980a46c", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + position_type: "text", + old_line: 64, + new_line: 61) + end + + context 'diff lines' do + it 'returns nil' do + expect(subject.unfolded_diff_lines).to be_nil + end + end + end + + context 'position requires bottom expansion and new match lines' do + let(:position) do + Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19", + head_sha: "1487062132228de836236c522fe52fed4980a46c", + old_path: "build-aux/flatpak/org.gnome.Nautilus.json", + new_path: "build-aux/flatpak/org.gnome.Nautilus.json", + position_type: "text", + old_line: 107, + new_line: 99) + end + + context 'blob lines' do + let(:expected_blob_lines) do + [[104, 104, " \"sdk\": \"foo\","], + [105, 105, " \"command\": \"foo\","], + [106, 106, " \"tags\": [\"foo\", \"bar\", \"kux\"],"], + [107, 107, " \"desktop-file-name-prefix\": \"(Foo) \","], + [108, 108, " {"], + [109, 109, " \"buildsystem\": \"meson\","], + [110, 110, " \"builddir\": true,"]] + end + + it 'returns the extracted blob lines correctly' do + extracted_lines = subject.blob_lines + + expect(extracted_lines.size).to eq(7) + + extracted_lines.each_with_index do |line, i| + expect([line.old_line, line.new_line, line.text]).to eq(expected_blob_lines[i]) + end + end + end + + context 'diff lines' do + let(:expected_diff_lines) do + [[7, 7, "@@ -7,9 +7,6 @@"], + [7, 7, " \"tags\": [\"devel\", \"development\", \"nightly\"],"], + [8, 8, " \"desktop-file-name-prefix\": \"(Development) \","], + [9, 9, " \"finish-args\": ["], + [10, 10, "- \"--share=ipc\", \"--socket=x11\","], + [11, 10, "- \"--socket=wayland\","], + [12, 10, "- \"--talk-name=org.gnome.OnlineAccounts\","], + [13, 10, " \"--talk-name=org.freedesktop.Tracker1\","], + [14, 11, " \"--filesystem=home\","], + [15, 12, " \"--talk-name=org.gtk.vfs\", \"--talk-name=org.gtk.vfs.*\","], + [62, 59, "@@ -62,7 +59,7 @@"], + [62, 59, " },"], + [63, 60, " {"], + [64, 61, " \"name\": \"gnome-desktop\","], + [65, 62, "- \"config-opts\": [\"--disable-debug-tools\", \"--disable-udev\"],"], + [66, 62, "+ \"config-opts\": [\"--disable-debug-tools\", \"--disable-\"],"], + [66, 63, " \"sources\": ["], + [67, 64, " {"], + [68, 65, " \"type\": \"git\","], + [83, 80, "@@ -83,11 +80,6 @@"], + [83, 80, " \"buildsystem\": \"meson\","], + [84, 81, " \"builddir\": true,"], + [85, 82, " \"name\": \"nautilus\","], + [86, 83, "- \"config-opts\": ["], + [87, 83, "- \"-Denable-desktop=false\","], + [88, 83, "- \"-Denable-selinux=false\","], + [89, 83, "- \"--libdir=/app/lib\""], + [90, 83, "- ],"], + [91, 83, " \"sources\": ["], + [92, 84, " {"], + [93, 85, " \"type\": \"git\","], + # New match line + [104, 96, "@@ -104,7+96,7 @@"], + + # Injected blob lines + [104, 96, " \"sdk\": \"foo\","], + [105, 97, " \"command\": \"foo\","], + [106, 98, " \"tags\": [\"foo\", \"bar\", \"kux\"],"], + [107, 99, " \"desktop-file-name-prefix\": \"(Foo) \","], + [108, 100, " {"], + [109, 101, " \"buildsystem\": \"meson\","], + [110, 102, " \"builddir\": true,"]] + # end + end + + it 'return merge of blob lines with diff lines correctly' do + new_diff_lines = subject.unfolded_diff_lines + + expected_diff_lines.each_with_index do |expected_line, i| + line = new_diff_lines[i] + + expect([line.old_pos, line.new_pos, line.text]) + .to eq([expected_line[0], expected_line[1], expected_line[2]]) + end + end + + it 'merged lines have correct line codes' do + new_diff_lines = subject.unfolded_diff_lines + + new_diff_lines.each_with_index do |line, i| + old_pos, new_pos = expected_diff_lines[i][0], expected_diff_lines[i][1] + + unless line.type == 'match' + expect(line.line_code).to eq(Gitlab::Git.diff_line_code(diff_file.file_path, new_pos, old_pos)) + end + end + end + end + end +end diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb index 546c9f277c5..5acd01828cb 100644 --- a/spec/services/merge_requests/reload_diffs_service_spec.rb +++ b/spec/services/merge_requests/reload_diffs_service_spec.rb @@ -31,32 +31,11 @@ describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_cachin end context 'cache clearing' do - before do - allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true) - allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true) - end - - it 'retrieves the diff files to cache the highlighted result' do - new_diff = merge_request.create_merge_request_diff - cache_key = new_diff.diffs_collection.cache_key - - expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff) - expect(Rails.cache).to receive(:read).with(cache_key).and_call_original - expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original - - subject.execute - end - it 'clears the cache for older diffs on the merge request' do old_diff = merge_request.merge_request_diff old_cache_key = old_diff.diffs_collection.cache_key - new_diff = merge_request.create_merge_request_diff - new_cache_key = new_diff.diffs_collection.cache_key - expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff) expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original - expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original - expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original subject.execute end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index b1290fd0d47..80b015d4cd0 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -57,6 +57,57 @@ describe Notes::CreateService do end end + context 'noteable highlight cache clearing' do + let(:project_with_repo) { create(:project, :repository) } + let(:merge_request) do + create(:merge_request, source_project: project_with_repo, + target_project: project_with_repo) + end + + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: merge_request.diff_refs) + end + + let(:new_opts) do + opts.merge(in_reply_to_discussion_id: nil, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: position.to_h) + end + + before do + allow_any_instance_of(Gitlab::Diff::Position) + .to receive(:unfolded_diff?) { true } + end + + it 'clears noteable diff cache when it was unfolded for the note position' do + expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear) + + described_class.new(project_with_repo, user, new_opts).execute + end + + it 'does not clear cache when note is not the first of the discussion' do + prev_note = + create(:diff_note_on_merge_request, noteable: merge_request, + project: project_with_repo) + reply_opts = + opts.merge(in_reply_to_discussion_id: prev_note.discussion_id, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: position.to_h) + + expect(merge_request).not_to receive(:diffs) + + described_class.new(project_with_repo, user, reply_opts).execute + end + end + context 'note diff file' do let(:project_with_repo) { create(:project, :repository) } let(:merge_request) do diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb index 64445be560e..b1f4e87e8ea 100644 --- a/spec/services/notes/destroy_service_spec.rb +++ b/spec/services/notes/destroy_service_spec.rb @@ -21,5 +21,38 @@ describe Notes::DestroyService do expect { described_class.new(project, user).execute(note) } .to change { user.todos_pending_count }.from(1).to(0) end + + context 'noteable highlight cache clearing' do + let(:repo_project) { create(:project, :repository) } + let(:merge_request) do + create(:merge_request, source_project: repo_project, + target_project: repo_project) + end + + let(:note) do + create(:diff_note_on_merge_request, project: repo_project, + noteable: merge_request) + end + + before do + allow(note.position).to receive(:unfolded_diff?) { true } + end + + it 'clears noteable diff cache when it was unfolded for the note position' do + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + described_class.new(repo_project, user).execute(note) + end + + it 'does not clear cache when note is not the first of the discussion' do + reply_note = create(:diff_note_on_merge_request, in_reply_to: note, + project: repo_project, + noteable: merge_request) + + expect(merge_request).not_to receive(:diffs) + + described_class.new(repo_project, user).execute(reply_note) + end + end end end |