add support for the commit reference filter

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';
 

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

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

app/models/commit.rb

+# 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)

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

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

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

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

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

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

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,

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

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

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

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 = '&lt;pre&gt;This is a test&lt;&#x2F;pre&gt;'
 
-      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