diff options
| -rw-r--r-- | app/assets/javascripts/diff_notes/diff_notes_bundle.js | 2 | ||||
| -rw-r--r-- | app/controllers/concerns/renders_notes.rb | 11 | ||||
| -rw-r--r-- | app/controllers/projects/merge_requests/diffs_controller.rb | 5 | ||||
| -rw-r--r-- | app/models/commit.rb | 3 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 21 | ||||
| -rw-r--r-- | app/services/system_note_service.rb | 14 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml | 8 | ||||
| -rw-r--r-- | lib/banzai/filter/commit_reference_filter.rb | 38 | ||||
| -rw-r--r-- | lib/banzai/object_renderer.rb | 13 | ||||
| -rw-r--r-- | lib/gitlab/diff/diff_refs.rb | 22 | ||||
| -rw-r--r-- | lib/gitlab/git.rb | 12 | ||||
| -rw-r--r-- | lib/gitlab/git/commit.rb | 1 | ||||
| -rw-r--r-- | spec/controllers/projects/commit_controller_spec.rb | 2 | ||||
| -rw-r--r-- | spec/lib/gitlab/git_spec.rb | 25 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 2 | ||||
| -rw-r--r-- | spec/services/system_note_service_spec.rb | 11 |
16 files changed, 105 insertions, 85 deletions
diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js b/app/assets/javascripts/diff_notes/diff_notes_bundle.js index 2f22361d6d2..e0422057090 100644 --- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js @@ -16,7 +16,7 @@ import './components/diff_note_avatars'; import './components/new_issue_for_discussion'; $(() => { - const projectPathHolder = document.querySelector('.merge-request') || document.querySelector('.commit-box') + const projectPathHolder = document.querySelector('.merge-request') || document.querySelector('.commit-box'); const projectPath = projectPathHolder.dataset.projectPath; const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn'; diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb index a35313c917c..824ad06465c 100644 --- a/app/controllers/concerns/renders_notes.rb +++ b/app/controllers/concerns/renders_notes.rb @@ -3,7 +3,7 @@ module RendersNotes preload_noteable_for_regular_notes(notes) preload_max_access_for_authors(notes, @project) preload_first_time_contribution_for_authors(noteable, notes) - Notes::RenderService.new(current_user).execute(notes, @project, noteable_context(noteable)) + Notes::RenderService.new(current_user).execute(notes, @project) notes end @@ -26,13 +26,4 @@ module RendersNotes notes.each {|n| n.specialize_for_first_contribution!(noteable)} end - - def noteable_context(noteable) - case noteable - when MergeRequest - { merge_request: noteable } - else - {} - end - end end diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 07bf9db5a34..fe8525a488c 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -21,8 +21,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic private def define_diff_vars - @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff.order_id_desc - + @merge_request_diffs = @merge_request.merge_request_diffs.viewable.order_id_desc @compare = commit || find_merge_request_diff_compare return render_404 unless @compare @@ -31,7 +30,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic def commit return nil unless commit_id = params[:commit_id].presence - return nil unless @merge_request.all_commit_shas.include?(commit_id) + return nil unless @merge_request.all_commits.exists?(sha: commit_id) @commit ||= @project.commit(commit_id) end diff --git a/app/models/commit.rb b/app/models/commit.rb index 6b28d290f99..307e4fcedfe 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -1,3 +1,4 @@ +# coding: utf-8 class Commit extend ActiveModel::Naming extend Gitlab::Cache::RequestCache @@ -25,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_FILES = 1000 DIFF_HARD_LIMIT_LINES = 50000 - MIN_SHA_LENGTH = 7 + MIN_SHA_LENGTH = Gitlab::Git::Commit::MIN_SHA_LENGTH COMMIT_SHA_PATTERN = /\h{#{MIN_SHA_LENGTH},40}/.freeze def banzai_render_context(field) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 949d42f865c..22a79da9879 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -649,6 +649,7 @@ class MergeRequest < ActiveRecord::Base .to_sql Note.from("(#{union}) #{Note.table_name}") + .includes(:noteable) end alias_method :discussion_notes, :related_notes @@ -920,16 +921,13 @@ class MergeRequest < ActiveRecord::Base def all_pipelines return Ci::Pipeline.none unless source_project + commit_shas = all_commits.unscope(:limit).select(:sha) @all_pipelines ||= source_project.pipelines - .where(sha: all_commit_shas, ref: source_branch) + .where(sha: commit_shas, ref: source_branch) .order(id: :desc) end - # Note that this could also return SHA from now dangling commits - # - def all_commit_shas - return commit_shas unless persisted? - + def all_commits diffs_relation = merge_request_diffs # MySQL doesn't support LIMIT in a subquery. @@ -938,8 +936,15 @@ class MergeRequest < ActiveRecord::Base MergeRequestDiffCommit .where(merge_request_diff: diffs_relation) .limit(10_000) - .pluck('sha') - .uniq + end + + # Note that this could also return SHA from now dangling commits + # + def all_commit_shas + @all_commit_shas ||= begin + return commit_shas unless persisted? + all_commits.pluck(:sha).uniq + end end def merge_commit diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 5f8a1bf07e2..30a5aab13bf 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -23,7 +23,7 @@ module SystemNoteService body = "added #{commits_text}\n\n" body << existing_commit_summary(noteable, existing_commits, oldrev) - body << new_commit_summary(noteable, new_commits).join("\n") + body << new_commit_summary(new_commits).join("\n") body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})" create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count)) @@ -486,9 +486,9 @@ module SystemNoteService # new_commits - Array of new Commit objects # # Returns an Array of Strings - def new_commit_summary(merge_request, new_commits) + def new_commit_summary(new_commits) new_commits.collect do |commit| - "* [#{commit.short_id}](#{merge_request_commit_url(merge_request, commit)}) - #{escape_html(commit.title)}" + "* #{commit.short_id} - #{escape_html(commit.title)}" end end @@ -668,12 +668,4 @@ module SystemNoteService start_sha: oldrev ) end - - def merge_request_commit_url(merge_request, commit) - url_helpers.diffs_project_merge_request_url( - merge_request.target_project, - merge_request, - commit_id: commit - ) - end end diff --git a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml index e4a1dc786b9..faabba5fc35 100644 --- a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml +++ b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml @@ -11,7 +11,7 @@ comparing two versions of the diff - else viewing an old version of the diff - .pull-right - = link_to diffs_project_merge_request_path(@project, @merge_request), class: 'btn btn-sm' do - Show latest version - = "of the diff" if @commit + .pull-right + = link_to diffs_project_merge_request_path(@project, @merge_request), class: 'btn btn-sm' do + Show latest version + = "of the diff" if @commit diff --git a/lib/banzai/filter/commit_reference_filter.rb b/lib/banzai/filter/commit_reference_filter.rb index f4e0c3111f5..c202d71072e 100644 --- a/lib/banzai/filter/commit_reference_filter.rb +++ b/lib/banzai/filter/commit_reference_filter.rb @@ -22,19 +22,29 @@ module Banzai end end + def referenced_merge_request_commit_shas + return [] unless noteable.is_a?(MergeRequest) + + @referenced_merge_request_commit_shas ||= begin + referenced_shas = references_per_project.values.reduce(:|).to_a + noteable.all_commit_shas.select do |sha| + referenced_shas.any? { |ref| Gitlab::Git.shas_eql?(sha, ref) } + end + end + end + def url_for_object(commit, project) h = Gitlab::Routing.url_helpers - noteable = context[:merge_request] || context[:noteable] - - if noteable.is_a?(MergeRequest) && - noteable.all_commit_shas.include?(commit.id) - # the internal shas are in the context? - # why not preload in the object?, just make sure we have the same ref - # in all the rendering - h.diffs_project_merge_request_url(project, noteable, commit_id: commit.id) + if referenced_merge_request_commit_shas.include?(commit.id) + h.diffs_project_merge_request_url(project, + noteable, + commit_id: commit.id, + only_path: only_path?) else - h.project_commit_url(project, commit, only_path: context[:only_path]) + h.project_commit_url(project, + commit, + only_path: only_path?) end end @@ -48,6 +58,16 @@ module Banzai extras end + + private + + def noteable + context[:noteable] + end + + def only_path? + context[:only_path] + end end end end diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index 29c4e60f70c..0bf9a8d66bc 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -17,11 +17,11 @@ module Banzai # project - A Project to use for redacting Markdown. # user - The user viewing the Markdown/HTML documents, if any. - # context - A Hash containing extra attributes to use during redaction - def initialize(project, user = nil, context = {}) + # redaction_context - A Hash containing extra attributes to use during redaction + def initialize(project, user = nil, redaction_context = {}) @project = project @user = user - @context = base_context.merge(context) + @redaction_context = base_context.merge(redaction_context) end # Renders and redacts an Array of objects. @@ -48,8 +48,7 @@ module Banzai pipeline = HTML::Pipeline.new([]) objects.map do |object| - context = context_for(object, attribute) - pipeline.to_document(Banzai.render_field(object, attribute, context)) + pipeline.to_document(Banzai.render_field(object, attribute)) end end @@ -74,7 +73,7 @@ module Banzai # Returns a Banzai context for the given object and attribute. def context_for(object, attribute) - @context.merge(object.banzai_render_context(attribute)) + @redaction_context.merge(object.banzai_render_context(attribute)) end def base_context @@ -86,7 +85,7 @@ module Banzai end def save_options - return {} unless @context[:xhtml] + return {} unless @redaction_context[:xhtml] { save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML } end end diff --git a/lib/gitlab/diff/diff_refs.rb b/lib/gitlab/diff/diff_refs.rb index c98eefbce25..88e0db830f6 100644 --- a/lib/gitlab/diff/diff_refs.rb +++ b/lib/gitlab/diff/diff_refs.rb @@ -13,9 +13,9 @@ module Gitlab def ==(other) other.is_a?(self.class) && - shas_equal?(base_sha, other.base_sha) && - shas_equal?(start_sha, other.start_sha) && - shas_equal?(head_sha, other.head_sha) + Git.shas_eql?(base_sha, other.base_sha) && + Git.shas_eql?(start_sha, other.start_sha) && + Git.shas_eql?(head_sha, other.head_sha) end alias_method :eql?, :== @@ -47,22 +47,6 @@ module Gitlab CompareService.new(project, head_sha).execute(project, start_sha, straight: straight) end end - - private - - def shas_equal?(sha1, sha2) - return true if sha1 == sha2 - return false if sha1.nil? || sha2.nil? - return false unless sha1.class == sha2.class - - length = [sha1.length, sha2.length].min - - # If either of the shas is below the minimum length, we cannot be sure - # that they actually refer to the same commit because of hash collision. - return false if length < Commit::MIN_SHA_LENGTH - - sha1[0, length] == sha2[0, length] - end end end end diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 1f31cdbc96d..1f7c35cafaa 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -70,6 +70,18 @@ module Gitlab def diff_line_code(file_path, new_line_position, old_line_position) "#{Digest::SHA1.hexdigest(file_path)}_#{old_line_position}_#{new_line_position}" end + + def shas_eql?(sha1, sha2) + return false if sha1.nil? || sha2.nil? + return false unless sha1.class == sha2.class + + # If either of the shas is below the minimum length, we cannot be sure + # that they actually refer to the same commit because of hash collision. + length = [sha1.length, sha2.length].min + return false if length < Gitlab::Git::Commit::MIN_SHA_LENGTH + + sha1[0, length] == sha2[0, length] + end end end end diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 8900e2d7afe..e90b158fb34 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -6,6 +6,7 @@ module Gitlab attr_accessor :raw_commit, :head + MIN_SHA_LENGTH = 7 SERIALIZE_KEYS = [ :id, :message, :parent_ids, :authored_date, :author_name, :author_email, diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index a5b603d6bff..fa8533bc3d9 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -338,7 +338,7 @@ describe Projects::CommitController do context 'when the commit does not exist' do before do - diff_for_path(id: commit.id.succ, old_path: existing_path, new_path: existing_path) + diff_for_path(id: commit.id.reverse, old_path: existing_path, new_path: existing_path) end it 'returns a 404' do diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb index 494dfe0e595..ce15057dd7d 100644 --- a/spec/lib/gitlab/git_spec.rb +++ b/spec/lib/gitlab/git_spec.rb @@ -38,4 +38,29 @@ describe Gitlab::Git do expect(described_class.ref_name(utf8_invalid_ref)).to eq("an_invalid_ref_å") end end + + describe '.shas_eql?' do + using RSpec::Parameterized::TableSyntax + + where(:sha1, :sha2, :result) do + sha = RepoHelpers.sample_commit.id + short_sha = sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH] + too_short_sha = sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH - 1] + + [ + [sha, sha, true], + [sha, short_sha, true], + [sha, sha.reverse, false], + [sha, too_short_sha, false], + [sha, nil, false] + ] + end + + with_them do + it { expect(described_class.shas_eql?(sha1, sha2)).to eq(result) } + it 'is commutative' do + expect(described_class.shas_eql?(sha2, sha1)).to eq(result) + end + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 71fbb82184c..30a5a3bbff7 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -967,7 +967,7 @@ describe MergeRequest do end shared_examples 'returning all SHA' do - it 'returns all SHA from all merge_request_diffs' do + it 'returns all SHAs from all merge_request_diffs' do expect(subject.merge_request_diffs.size).to eq(2) expect(subject.all_commit_shas).to match_array(all_commit_shas) end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 148f81b6a58..47412110b4b 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -690,20 +690,11 @@ describe SystemNoteService do end describe '.new_commit_summary' do - let(:merge_request) { create(:merge_request, :simple, target_project: project, source_project: project) } - it 'escapes HTML titles' do commit = double(title: '<pre>This is a test</pre>', short_id: '12345678') escaped = '<pre>This is a test</pre>' - expect(described_class.new_commit_summary(merge_request, [commit])).to all(match(%r[- #{escaped}])) - end - - it 'contains the MR diffs commit url' do - commit = merge_request.commits.last - url = %r[/merge_requests/#{merge_request.iid}/diffs\?commit_id=#{commit.id}] - - expect(described_class.new_commit_summary(merge_request, [commit])).to all(match(url)) + expect(described_class.new_commit_summary([commit])).to all(match(%r[- #{escaped}])) end end |