diff options
author | Douwe Maan <douwe@selenight.nl> | 2018-03-02 19:57:21 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2018-09-30 00:36:15 +0300 |
commit | 05eb574ff4a433d69f281e668d0fa2d958c76584 (patch) | |
tree | 7aa356fad966519884ee2bfb16b9c407666b298d /app | |
parent | 227cc997fb107672e3293c56e0dcb1df72ad82d5 (diff) |
WIP: Download patch with code comments for unresolved discussionsdm-download-discussions-as-patch
Diffstat (limited to 'app')
13 files changed, 466 insertions, 12 deletions
diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index ad6e7cf501d..a4d988cf49c 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -1,5 +1,6 @@ <script> import { mapActions, mapGetters } from 'vuex'; +import Icon from '~/vue_shared/components/icon.vue'; import resolveSvg from 'icons/_icon_resolve_discussion.svg'; import resolvedSvg from 'icons/_icon_status_success_solid.svg'; import mrIssueSvg from 'icons/_icon_mr_issue.svg'; @@ -9,6 +10,9 @@ import discussionNavigation from '../mixins/discussion_navigation'; import tooltip from '../../vue_shared/directives/tooltip'; export default { + components: { + Icon, + }, directives: { tooltip, }, @@ -80,10 +84,10 @@ export default { </span> </div> <div - v-if="resolveAllDiscussionsIssuePath && !allResolved" class="btn-group" role="group"> <a + v-if="resolveAllDiscussionsIssuePath && !allResolved" v-tooltip :href="resolveAllDiscussionsIssuePath" :title="s__('Resolve all discussions in new issue')" @@ -91,12 +95,19 @@ export default { class="new-issue-for-discussion btn btn-default discussion-create-issue-btn"> <span v-html="mrIssueSvg"></span> </a> - </div> - <div - v-if="isLoggedIn && !allResolved" - class="btn-group" - role="group"> <button + v-if="!allResolved" + v-tooltip + data-target="#modal_download_as_patch" + data-toggle="modal" + :title="s__('Download unresolved discussions as code comments')" + data-container="body" + class="btn btn-default" + type="button"> + <icon name="download" /> + </button> + <button + v-if="isLoggedIn && !allResolved" v-tooltip title="Jump to first unresolved discussion" data-container="body" diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 97b131687d3..3efb4ab9f21 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -503,7 +503,7 @@ display: none; } -#modal_merge_info .modal-dialog { +.merge-request-modal .modal-dialog { .dark { margin-right: 40px; } diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index dfb69de650b..0881d168ef9 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -77,6 +77,26 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end end + def discussions + respond_to do |format| + format.json do + super + end + + format.patch do + return render_404 unless commit = commit_with_unresolved_discussions + + send_git_patch @project.repository, commit.diff_refs + end + + format.diff do + return render_404 unless commit = commit_with_unresolved_discussions + + send_git_diff @project.repository, commit.diff_refs + end + end + end + def commits # Get commits from repository # or from cache if already merged @@ -353,4 +373,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42441 Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42438') end + + def commit_with_unresolved_discussions + @commit_with_unresolved_discussions ||= Discussions::CommitWithUnresolvedDiscussionsService.new(project, current_user).execute(merge_request) + end end diff --git a/app/models/commit.rb b/app/models/commit.rb index 49c36ad9d3f..726c35c9b93 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -464,7 +464,7 @@ class Commit # We don't want to do anything for `Commit` model, so this is empty. end - WIP_REGEX = /\A\s*(((?i)(\[WIP\]|WIP:|WIP)\s|WIP$))|(fixup!|squash!)\s/.freeze + WIP_REGEX = /\A\s*(((?i)(\[WIP\]|WIP:|WIP)\s|WIP$))|(fixup!|squash!|FIXME:)\s/.freeze def work_in_progress? !!(title =~ WIP_REGEX) diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 00dec6bb92b..1c6a709af12 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -43,6 +43,10 @@ class LegacyDiffNote < Note self.line_code end + def position + @position ||= diff_file.position_for_line_code(self.line_code) if active? + end + # Check if this note is part of an "active" discussion # # This will always return true for anything except MergeRequest noteables, diff --git a/app/services/discussions/commit_with_unresolved_discussions_service.rb b/app/services/discussions/commit_with_unresolved_discussions_service.rb new file mode 100644 index 00000000000..550146125e3 --- /dev/null +++ b/app/services/discussions/commit_with_unresolved_discussions_service.rb @@ -0,0 +1,61 @@ +module Discussions + class CommitWithUnresolvedDiscussionsService < Discussions::BaseService + def execute(merge_request) + @merge_request = merge_request + + cached_commit || create_commit + end + + private + + def update_actions + @update_actions ||= UpdateActions.new(@merge_request) + end + + def cache_key + @cache_key ||= [ + 'merge-request', + @merge_request.id, + 'update-actions', + update_actions.cache_key + ] + end + + def cached_commit + return if Rails.env.development? + + commit_sha = Rails.cache.read(cache_key) + commit_sha && project.commit(commit_sha) + end + + def create_commit + return unless @merge_request.source_branch_exists? + + commit_sha = repository.multi_action( + @merge_request.author, + branch_name: nil, # We just want a commit, not a branch + message: commit_message, + start_branch_name: @merge_request.source_branch, + start_project: @merge_request.source_project, + actions: update_actions.actions + ) + return unless commit_sha + + Rails.cache.write(cache_key, commit_sha) + + project.commit(commit_sha) + end + + def commit_message + <<~MSG + FIXME: Add code comments for unresolved discussions + + This patch adds a code comment for every unresolved discussion on the + latest version of the diff of #{@merge_request.to_reference(full: true)}. + + This commit was automatically generated by GitLab. Don't forget to + remove it from the merge request source branch before it is merged. + MSG + end + end +end diff --git a/app/services/discussions/commit_with_unresolved_discussions_service/commenter.rb b/app/services/discussions/commit_with_unresolved_discussions_service/commenter.rb new file mode 100644 index 00000000000..1caaf5c30d7 --- /dev/null +++ b/app/services/discussions/commit_with_unresolved_discussions_service/commenter.rb @@ -0,0 +1,101 @@ +module Discussions + class CommitWithUnresolvedDiscussionsService + class Commenter + # Commenters can be strings or lambdas. + # Use a string for single-line comments: each line will be prefixed + # Use a lambda for multi-line comments: it will be called with the full text + COMMENTERS = { + double_slash: '// ', # Substitution applied to each line + double_stroke: '-- ', # Substitution applied to each line + pound: '# ', + percent: '% ', + quote: "'", + bang: '!', + semicolon: ';', + xml: lambda { |text| "<!--\n#{text}-->" } + }.freeze + + # Maps Rouge lexer (language) tags to commenters. + COMMENT_TYPES_BY_LANG = { + default: :double_slash, + + actionscript: :double_slash, + c: :double_slash, + csharp: :double_slash, + d: :double_slash, + cpp: :double_slash, + fsharp: :double_slash, + go: :double_slash, + php: :double_slash, + java: :double_slash, + kotlin: :double_slash, + javascript: :double_slash, + objective_c: :double_slash, + rust: :double_slash, + scala: :double_slash, + swift: :double_slash, + sass: :double_slash, + + markdown: :xml, + html: :xml, + xml: :xml, + + shell: :pound, + perl: :pound, + python: :pound, + powershell: :pound, + r: :pound, + ruby: :pound, + make: :pound, + elixir: :pound, + yaml: :pound, + + tex: :percent, + prolog: :percent, + matlab: :percent, + erlang: :percent, + + vb: :quote, + + fortran: :bang, + + lisp: :semicolon, + common_lisp: :semicolon, + clojure: :semicolon, + scheme: :semicolon, + + haskell: :double_stroke, + sql: :double_stroke, + lua: :double_stroke, + }.freeze + + attr_reader :format + + def initialize(comment_type = :default) + @format = COMMENTERS[comment_type] || COMMENTERS[:default] + end + + def self.for_lang(lang) + comment_type = COMMENT_TYPES_BY_LANG[lang] || COMMENT_TYPES_BY_LANG[:default] + new(comment_type) + end + + def self.for_blob(blob) + lexer = Gitlab::Highlight.new(blob.name, blob.data, repository: blob.project.repository).lexer + for_lang(lexer.tag.to_sym) + end + + def apply(text) + # Lambdas are used for multi-line comments, strings for single-line. See comment by the COMMENTERS constant. + if format.respond_to?(:call) + format.call(text) << "\n" + else + # With single line comments, we insert an extra newline at the end to + # create some distance between the comment and the next line of code. + text = "#{text}\n" + text.gsub(/^/, format) + end + end + end + end +end diff --git a/app/services/discussions/commit_with_unresolved_discussions_service/inserter.rb b/app/services/discussions/commit_with_unresolved_discussions_service/inserter.rb new file mode 100644 index 00000000000..9fb29a9e1f4 --- /dev/null +++ b/app/services/discussions/commit_with_unresolved_discussions_service/inserter.rb @@ -0,0 +1,35 @@ +module Discussions + class CommitWithUnresolvedDiscussionsService + class Inserter + attr_reader :blob + + def initialize(blob) + @blob = blob + end + + def insert(insertions) + commenter = Commenter.for_blob(blob) + + line_offset = 0 + insertions.sort.each do |insertion| + insertion_length = insertion.insert(lines, commenter, offset: line_offset) + line_offset += insertion_length + end + + lines.join + end + + private + + def lines + @lines ||= begin + blob.load_all_data! + + content = blob.data + content += "\n" unless content.end_with?("\n") + content.lines + end + end + end + end +end diff --git a/app/services/discussions/commit_with_unresolved_discussions_service/insertion.rb b/app/services/discussions/commit_with_unresolved_discussions_service/insertion.rb new file mode 100644 index 00000000000..f94d05bd806 --- /dev/null +++ b/app/services/discussions/commit_with_unresolved_discussions_service/insertion.rb @@ -0,0 +1,120 @@ +module Discussions + class CommitWithUnresolvedDiscussionsService + class Insertion + include Comparable + + attr_accessor :discussion, :override_preceding_lines + + def initialize(discussion) + @discussion = discussion + end + + def insert(target_lines, commenter, offset: 0) + line_index = line + offset + + insertion_lines = text(target_lines[0..line_index-1], commenter).lines + target_lines.insert(line_index, *insertion_lines) + + insertion_lines.length + end + + def line + position.removed? ? discussion.diff_line.new_pos - 1 : position.new_line + end + + def path + position.file_path + end + + def <=>(other) + sort_key <=> other.sort_key + end + + def sort_key + [ + line, + # Comments related to a preceding line should show up before comments + # related to a deleted + # line at the same location. + position.removed? ? 1 : 0, + discussion.created_at + ] + end + + private + + def text(preceding_lines, commenter) + text = base_text.dup + + text = commenter.apply(base_text) if commenter + + # We determine the indentation level of the insertion based the actual + # preceding lines, or the original preceding lines in case of deletion. + preceding_lines = removed_diff_lines if position.removed? + + text.gsub!(/^/, indentation(preceding_lines)) if preceding_lines + + text + end + + def base_text + sections = [] + + # When the discussion is on a deleted line, we include the preceding 5 + # deleted lines in the comment. + sections << removed_lines_text if position.removed? + + discussion.notes.each_with_index do |note, i| + sections << note_text(note, started: i == 0) + end + + text = sections.join("\n\n") + + text << "\n" unless text.end_with?("\n") + text + end + + def position + discussion.position + end + + def removed_diff_lines + @removed_diff_lines ||= + discussion + .truncated_diff_lines(highlight: false) + .reverse[0, 5] + .take_while(&:removed?) + .reverse + .map { |l| l.text[1..-1] } # Drop the - prefixes + end + + def removed_lines_text + heading = "Old #{"line".pluralize(removed_diff_lines.count)}:" + indented_removed_lines = removed_diff_lines.join("\n").strip_heredoc.gsub(/^/, ' ') + + [heading, indented_removed_lines].join("\n") + end + + def note_text(note, started: false) + byline = "FIXME: #{note.author.name} (#{note.author.to_reference}) " + byline << (started ? "started a discussion on the preceding line:" : "commented:") + byline << " (Resolved by #{note.resolved_by.try(:name)})" if note.resolved? + + comment = note.note.gsub(/^/, ' ') + + [byline, comment].join("\n") + end + + def indentation(preceding_lines) + # Find closest preceding non-blank line + preceding_line = preceding_lines.reverse.find(&:present?) + # Fall back on preceding line + preceding_line ||= preceding_lines.last + + return unless preceding_line + + preceding_line[/\A[\t ]*/] + end + end + end +end diff --git a/app/services/discussions/commit_with_unresolved_discussions_service/update_actions.rb b/app/services/discussions/commit_with_unresolved_discussions_service/update_actions.rb new file mode 100644 index 00000000000..9e48b9d94ee --- /dev/null +++ b/app/services/discussions/commit_with_unresolved_discussions_service/update_actions.rb @@ -0,0 +1,50 @@ +module Discussions + class CommitWithUnresolvedDiscussionsService + class UpdateActions + attr_reader :merge_request + + def initialize(merge_request) + @merge_request = merge_request + end + + def actions + insertions + .group_by(&:path) + .map do |path, insertions| + action_for(path, insertions) + end + end + + def cache_key + @cache_key ||= [ + @merge_request.diff_head_sha, + @merge_request.notes.diff_notes.count, + @merge_request.notes.diff_notes.maximum(:updated_at) + ] + end + + private + + def discussions + @discussions ||= @merge_request + .notes.diff_notes.inc_relations_for_view.fresh.discussions + .select { |d| d.on_text? && d.active? && !d.resolved? && !d.diff_file.deleted_file? && d.position } + end + + def insertions + discussions.map { |d| Insertion.new(d) } + end + + def action_for(path, insertions) + blob = @merge_request.project.repository.blob_at(@merge_request.diff_head_sha, path) + content = Inserter.new(blob).insert(insertions) + + { + action: :update, + file_path: path, + content: content + } + end + end + end +end diff --git a/app/views/projects/merge_requests/_download_as_patch_modal.html.haml b/app/views/projects/merge_requests/_download_as_patch_modal.html.haml new file mode 100644 index 00000000000..721cc8e65c2 --- /dev/null +++ b/app/views/projects/merge_requests/_download_as_patch_modal.html.haml @@ -0,0 +1,44 @@ +#modal_download_as_patch.merge-request-modal.modal{ tabindex: '-1' } + .modal-dialog.modal-lg + .modal-content + .modal-header + %h3.modal-title Download unresolved discussions as code comments + %button.close{ type: "button", "data-dismiss": "modal", "aria-label" => _('Close') } + %span{ "aria-hidden": true } × + .modal-body + %p + %strong Step 1. + Fetch and check out the branch for this merge request + = clipboard_button(target: "pre#download-as-patch-1", title: "Copy commands to clipboard") + %pre.dark#download-as-patch-1 + - if @merge_request.for_fork? + :preserve + git fetch #{h default_url_to_repo(@merge_request.source_project)} #{h @merge_request.source_branch} + git checkout -b #{h @merge_request.source_project_path}-#{h @merge_request.source_branch} FETCH_HEAD + - else + :preserve + git fetch origin + git checkout -b #{h @merge_request.source_branch} origin/#{h @merge_request.source_branch} + + %p + %strong Step 2. + Download patch that will add a code comment for every unresolved discussion on the latest version of the diff, and apply it to your working copy + + - url = discussions_project_merge_request_url(@project, @merge_request, format: :patch) + - if @project.public? + = clipboard_button(target: "pre#download-as-patch-2", title: "Copy commands to clipboard") + %pre.dark#download-as-patch-2 + :preserve + curl #{h url} | git apply + - else + - filename = "merge-request-#{@merge_request.iid}-discussions.patch" + %p + = link_to %Q{Download patch as "#{filename}"}, url, class: "btn", download: filename + = clipboard_button(target: "pre#download-as-patch-2", title: "Copy commands to clipboard") + %pre.dark#download-as-patch-2 + :preserve + git apply #{h filename} + + %p + %strong Step 3. + Find the newly added 'FIXME' comments and update the code as appropriate diff --git a/app/views/projects/merge_requests/_how_to_merge.html.haml b/app/views/projects/merge_requests/_how_to_merge_modal.html.haml index 15499c89ffb..d06bed9c15c 100644 --- a/app/views/projects/merge_requests/_how_to_merge.html.haml +++ b/app/views/projects/merge_requests/_how_to_merge_modal.html.haml @@ -1,4 +1,4 @@ -#modal_merge_info.modal{ tabindex: '-1' } +#modal_merge_info.merge-request-modal.modal{ tabindex: '-1' } .modal-dialog.modal-lg .modal-content .modal-header @@ -19,6 +19,7 @@ :preserve git fetch origin git checkout -b #{h @merge_request.source_branch} origin/#{h @merge_request.source_branch} + %p %strong Step 2. Review the changes locally @@ -38,6 +39,7 @@ git fetch origin git checkout origin/#{h @merge_request.target_branch} git merge --no-ff #{h @merge_request.source_branch} + %p %strong Step 4. Push the result of the merge to GitLab @@ -48,6 +50,7 @@ - unless @merge_request.can_be_merged_by?(current_user) %p Note that pushing to GitLab requires write access to this repository. + %p %strong Tip: = succeed '.' do diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index b23baa22d8b..bf5e5fb25c0 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -6,15 +6,16 @@ - page_description @merge_request.description - page_card_attributes @merge_request.card_attributes += render "projects/merge_requests/download_as_patch_modal" +- if @merge_request.source_branch_exists? + = render "projects/merge_requests/how_to_merge_modal" + .merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project) } } = render "projects/merge_requests/mr_title" .merge-request-details.issuable-details{ data: { id: @merge_request.project.id } } = render "projects/merge_requests/mr_box" - - if @merge_request.source_branch_exists? - = render "projects/merge_requests/how_to_merge" - -# haml-lint:disable InlineJavaScript :javascript window.gl = window.gl || {}; |