Merge branch 'rs-refactor-reference-extractor' into 'master'

Refactor ReferenceExtractor

See merge request !557

app/models/commit_range.rb

+# CommitRange makes it easier to work with commit ranges
+#
+# Examples:
+#
+#   range = CommitRange.new('f3f85602...e86e1013')
+#   range.exclude_start?  # => false
+#   range.reference_title # => "Commits f3f85602 through e86e1013"
+#   range.to_s            # => "f3f85602...e86e1013"
+#
+#   range = CommitRange.new('f3f856029bc5f966c5a7ee24cf7efefdd20e6019..e86e1013709735be5bb767e2b228930c543f25ae')
+#   range.exclude_start?  # => true
+#   range.reference_title # => "Commits f3f85602^ through e86e1013"
+#   range.to_param        # => {from: "f3f856029bc5f966c5a7ee24cf7efefdd20e6019^", to: "e86e1013709735be5bb767e2b228930c543f25ae"}
+#   range.to_s            # => "f3f85602..e86e1013"
+#
+#   # Assuming `project` is a Project with a repository containing both commits:
+#   range.project = project
+#   range.valid_commits? # => true
+#
+class CommitRange
+  include ActiveModel::Conversion
+
+  attr_reader :sha_from, :notation, :sha_to
+
+  # Optional Project model
+  attr_accessor :project
+
+  # See `exclude_start?`
+  attr_reader :exclude_start
+
+  # The beginning and ending SHA sums can be between 6 and 40 hex characters,
+  # and the range selection can be double- or triple-dot.
+  PATTERN = /\h{6,40}\.{2,3}\h{6,40}/
+
+  # Initialize a CommitRange
+  #
+  # range_string - The String commit range.
+  # project      - An optional Project model.
+  #
+  # Raises ArgumentError if `range_string` does not match `PATTERN`.
+  def initialize(range_string, project = nil)
+    range_string.strip!
+
+    unless range_string.match(/\A#{PATTERN}\z/)
+      raise ArgumentError, "invalid CommitRange string format: #{range_string}"
+    end
+
+    @exclude_start = !range_string.include?('...')
+    @sha_from, @notation, @sha_to = range_string.split(/(\.{2,3})/, 2)
+
+    @project = project
+  end
+
+  def inspect
+    %(#<#{self.class}:#{object_id} #{to_s}>)
+  end
+
+  def to_s
+    "#{sha_from[0..7]}#{notation}#{sha_to[0..7]}"
+  end
+
+  # Returns a String for use in a link's title attribute
+  def reference_title
+    "Commits #{suffixed_sha_from} through #{sha_to}"
+  end
+
+  # Return a Hash of parameters for passing to a URL helper
+  #
+  # See `namespace_project_compare_url`
+  def to_param
+    { from: suffixed_sha_from, to: sha_to }
+  end
+
+  def exclude_start?
+    exclude_start
+  end
+
+  # Check if both the starting and ending commit IDs exist in a project's
+  # repository
+  #
+  # project - An optional Project to check (default: `project`)
+  def valid_commits?(project = project)
+    return nil   unless project.present?
+    return false unless project.valid_repo?
+
+    commit_from.present? && commit_to.present?
+  end
+
+  def persisted?
+    true
+  end
+
+  def commit_from
+    @commit_from ||= project.repository.commit(suffixed_sha_from)
+  end
+
+  def commit_to
+    @commit_to ||= project.repository.commit(sha_to)
+  end
+
+  private
+
+  def suffixed_sha_from
+    sha_from + (exclude_start? ? '^' : '')
+  end
+end

lib/gitlab/markdown/commit_range_reference_filter.rb

@@ -32,11 +32,8 @@ module Gitlab
 
       # Pattern used to extract commit range references from text
       #
-      # The beginning and ending SHA1 sums can be between 6 and 40 hex
-      # characters, and the range selection can be double- or triple-dot.
-      #
       # This pattern supports cross-project references.
-      COMMIT_RANGE_PATTERN = /(#{PROJECT_PATTERN}@)?(?<commit_range>\h{6,40}\.{2,3}\h{6,40})/
+      COMMIT_RANGE_PATTERN = /(#{PROJECT_PATTERN}@)?(?<commit_range>#{CommitRange::PATTERN})/
 
       def call
         replace_text_nodes_matching(COMMIT_RANGE_PATTERN) do |content|
@@ -53,52 +50,34 @@ module Gitlab
       # links have `gfm` and `gfm-commit_range` class names attached for
       # styling.
       def commit_range_link_filter(text)
-        self.class.references_in(text) do |match, commit_range, project_ref|
+        self.class.references_in(text) do |match, id, project_ref|
           project = self.project_from_ref(project_ref)
 
-          from_id, to_id = split_commit_range(commit_range)
+          range = CommitRange.new(id, project)
+
+          if range.valid_commits?
+            push_result(:commit_range, range)
 
-          if valid_range?(project, from_id, to_id)
-            url = url_for_commit_range(project, from_id, to_id)
+            url = url_for_commit_range(project, range)
 
-            title = "Commits #{from_id} through #{to_id}"
+            title = range.reference_title
             klass = reference_class(:commit_range)
 
             project_ref += '@' if project_ref
 
             %(<a href="#{url}"
                  title="#{title}"
-                 class="#{klass}">#{project_ref}#{commit_range}</a>)
+                 class="#{klass}">#{project_ref}#{range}</a>)
           else
             match
           end
         end
       end
 
-      def split_commit_range(range)
-        from_id, to_id = range.split(/\.{2,3}/, 2)
-        from_id << "^" if range !~ /\.{3}/
-
-        [from_id, to_id]
-      end
-
-      def commit(id)
-        unless @commit_map[id]
-          @commit_map[id] = project.commit(id)
-        end
-
-        @commit_map[id]
-      end
-
-      def valid_range?(project, from_id, to_id)
-        project && project.valid_repo? && commit(from_id) && commit(to_id)
-      end
-
-      def url_for_commit_range(project, from_id, to_id)
+      def url_for_commit_range(project, range)
         h = Rails.application.routes.url_helpers
         h.namespace_project_compare_url(project.namespace, project,
-                                        from: from_id, to: to_id,
-                                        only_path: context[:only_path])
+                                        range.to_param.merge(only_path: context[:only_path]))
       end
     end
   end

lib/gitlab/markdown/commit_reference_filter.rb

@@ -48,6 +48,8 @@ module Gitlab
           project = self.project_from_ref(project_ref)
 
           if commit = commit_from_ref(project, commit_ref)
+            push_result(:commit, commit)
+
             url = url_for_commit(project, commit)
 
             title = escape_once(commit.link_title)
@@ -57,7 +59,7 @@ module Gitlab
 
             %(<a href="#{url}"
                  title="#{title}"
-                 class="#{klass}">#{project_ref}#{commit_ref}</a>)
+                 class="#{klass}">#{project_ref}#{commit.short_id}</a>)
           else
             match
           end

lib/gitlab/markdown/issue_reference_filter.rb

@@ -48,6 +48,9 @@ module Gitlab
           project = self.project_from_ref(project_ref)
 
           if project && project.issue_exists?(issue)
+            # FIXME (rspeicher): Law of Demeter
+            push_result(:issue, project.issues.where(iid: issue).first)
+
             url = url_for_issue(issue, project, only_path: context[:only_path])
 
             title = escape_once("Issue: #{title_for_issue(issue, project)}")

lib/gitlab/markdown/label_reference_filter.rb

@@ -52,11 +52,13 @@ module Gitlab
           params = label_params(id, name)
 
           if label = project.labels.find_by(params)
-            url = url_for_label(project, label)
+            push_result(:label, label)
 
+            url = url_for_label(project, label)
             klass = reference_class(:label)
 
-            %(<a href="#{url}" class="#{klass}">#{render_colored_label(label)}</a>)
+            %(<a href="#{url}"
+                 class="#{klass}">#{render_colored_label(label)}</a>)
           else
             match
           end

lib/gitlab/markdown/merge_request_reference_filter.rb

@@ -48,6 +48,8 @@ module Gitlab
           project = self.project_from_ref(project_ref)
 
           if project && merge_request = project.merge_requests.find_by(iid: id)
+            push_result(:merge_request, merge_request)
+
             title = escape_once("Merge Request: #{merge_request.title}")
             klass = reference_class(:merge_request)
 

lib/gitlab/markdown/reference_filter.rb

@@ -12,7 +12,15 @@ module Gitlab
     #   :reference_class    - Custom CSS class added to reference links.
     #   :only_path          - Generate path-only links.
     #
+    # Results:
+    #   :references - A Hash of references that were found and replaced.
     class ReferenceFilter < HTML::Pipeline::Filter
+      def initialize(*args)
+        super
+
+        result[:references] = Hash.new { |hash, type| hash[type] = [] }
+      end
+
       def escape_once(html)
         ERB::Util.html_escape_once(html)
       end
@@ -29,6 +37,16 @@ module Gitlab
         context[:project]
       end
 
+      # Add a reference to the pipeline's result Hash
+      #
+      # type  - Singular Symbol reference type (e.g., :issue, :user, etc.)
+      # value - Object to add
+      def push_result(type, *values)
+        return if values.empty?
+
+        result[:references][type].push(*values)
+      end
+
       def reference_class(type)
         "gfm gfm-#{type} #{context[:reference_class]}".strip
       end

lib/gitlab/markdown/snippet_reference_filter.rb

@@ -48,6 +48,8 @@ module Gitlab
           project = self.project_from_ref(project_ref)
 
           if project && snippet = project.snippets.find_by(id: id)
+            push_result(:snippet, snippet)
+
             title = escape_once("Snippet: #{snippet.title}")
             klass = reference_class(:snippet)
 

lib/gitlab/markdown/user_reference_filter.rb

@@ -44,18 +44,25 @@ module Gitlab
           klass = reference_class(:project_member)
 
           if user == 'all'
+            # FIXME (rspeicher): Law of Demeter
+            push_result(:user, *project.team.members.flatten)
+
             url = link_to_all(project)
 
             %(<a href="#{url}" class="#{klass}">@#{user}</a>)
           elsif namespace = Namespace.find_by(path: user)
             if namespace.is_a?(Group)
               if user_can_reference_group?(namespace)
+                push_result(:user, *namespace.users)
+
                 url = group_url(user, only_path: context[:only_path])
                 %(<a href="#{url}" class="#{klass}">@#{user}</a>)
               else
                 match
               end
             else
+              push_result(:user, namespace.owner)
+
               url = user_url(user, only_path: context[:only_path])
               %(<a href="#{url}" class="#{klass}">@#{user}</a>)
             end

lib/gitlab/reference_extractor.rb

@@ -8,151 +8,70 @@ module Gitlab
       @current_user = current_user
     end
 
-    def can?(user, action, subject)
-      Ability.abilities.allowed?(user, action, subject)
-    end
-
     def analyze(text)
-      text = text.dup
-
-      # Remove preformatted/code blocks so that references are not included
-      text.gsub!(/^```.*?^```/m, '')
-      text.gsub!(/[^`]`[^`]*?`[^`]/, '')
-
-      @references = Hash.new { |hash, type| hash[type] = [] }
-      parse_references(text)
+      @_text = text.dup
     end
 
-    # Given a valid project, resolve the extracted identifiers of the requested type to
-    # model objects.
-
     def users
-      references[:user].uniq.map do |project, identifier|
-        if identifier == "all"
-          project.team.members.flatten
-        elsif namespace = Namespace.find_by(path: identifier)
-          if namespace.is_a?(Group)
-            namespace.users if can?(current_user, :read_group, namespace)
-          else
-            namespace.owner
-          end
-        end
-      end.flatten.compact.uniq
+      result = pipeline_result(:user)
+      result.uniq
     end
 
     def labels
-      references[:label].uniq.map do |project, identifier|
-        project.labels.where(id: identifier).first
-      end.compact.uniq
+      result = pipeline_result(:label)
+      result.uniq
     end
 
     def issues
-      references[:issue].uniq.map do |project, identifier|
-        if project.default_issues_tracker?
-          project.issues.where(iid: identifier).first
-        end
-      end.compact.uniq
+      # TODO (rspeicher): What about external issues?
+
+      result = pipeline_result(:issue)
+      result.uniq
     end
 
     def merge_requests
-      references[:merge_request].uniq.map do |project, identifier|
-        project.merge_requests.where(iid: identifier).first
-      end.compact.uniq
+      result = pipeline_result(:merge_request)
+      result.uniq
     end
 
     def snippets
-      references[:snippet].uniq.map do |project, identifier|
-        project.snippets.where(id: identifier).first
-      end.compact.uniq
+      result = pipeline_result(:snippet)
+      result.uniq
     end
 
     def commits
-      references[:commit].uniq.map do |project, identifier|
-        repo = project.repository
-        repo.commit(identifier) if repo
-      end.compact.uniq
+      result = pipeline_result(:commit)
+      result.uniq
     end
 
     def commit_ranges
-      references[:commit_range].uniq.map do |project, identifier|
-        repo = project.repository
-        if repo
-          from_id, to_id = identifier.split(/\.{2,3}/, 2)
-          [repo.commit(from_id), repo.commit(to_id)]
-        end
-      end.compact.uniq
+      result = pipeline_result(:commit_range)
+      result.uniq
     end
 
     private
 
-    NAME_STR = Gitlab::Regex::NAMESPACE_REGEX_STR
-    PROJ_STR = "(?<project>#{NAME_STR}/#{NAME_STR})"
-
-    REFERENCE_PATTERN = %r{
-      (?<prefix>\W)?                         # Prefix
-      (                                      # Reference
-         @(?<user>#{NAME_STR})               # User name
-        |~(?<label>\d+)                      # Label ID
-        |(?<issue>([A-Z\-]+-)\d+)            # JIRA Issue ID
-        |#{PROJ_STR}?\#(?<issue>([a-zA-Z\-]+-)?\d+) # Issue ID
-        |#{PROJ_STR}?!(?<merge_request>\d+)  # MR ID
-        |\$(?<snippet>\d+)                   # Snippet ID
-        |(#{PROJ_STR}@)?(?<commit_range>[\h]{6,40}\.{2,3}[\h]{6,40}) # Commit range
-        |(#{PROJ_STR}@)?(?<commit>[\h]{6,40}) # Commit ID
-      )
-      (?<suffix>\W)?                         # Suffix
-    }x.freeze
-
-    TYPES = %i(user issue label merge_request snippet commit commit_range).freeze
-
-    def parse_references(text, project = @project)
-      # parse reference links
-      text.gsub!(REFERENCE_PATTERN) do |match|
-        type = TYPES.detect { |t| $~[t].present? }
-
-        actual_project = project
-        project_prefix = nil
-        project_path = $LAST_MATCH_INFO[:project]
-        if project_path
-          actual_project = ::Project.find_with_namespace(project_path)
-          actual_project = nil unless can?(current_user, :read_project, actual_project)
-          project_prefix = project_path
-        end
-
-        parse_result($LAST_MATCH_INFO, type,
-                     actual_project, project_prefix) || match
-      end
-    end
-
-    # Called from #parse_references.  Attempts to build a gitlab reference
-    # link.  Returns nil if +type+ is nil, if the match string is an HTML
-    # entity, if the reference is invalid, or if the matched text includes an
-    # invalid project path.
-    def parse_result(match_info, type, project, project_prefix)
-      prefix = match_info[:prefix]
-      suffix = match_info[:suffix]
-
-      return nil if html_entity?(prefix, suffix) || type.nil?
-      return nil if project.nil? && !project_prefix.nil?
-
-      identifier = match_info[type]
-      ref_link = reference_link(type, identifier, project, project_prefix)
-
-      if ref_link
-        "#{prefix}#{ref_link}#{suffix}"
-      else
-        nil
-      end
-    end
-
-    # Return true if the +prefix+ and +suffix+ indicate that the matched string
-    # is an HTML entity like &amp;
-    def html_entity?(prefix, suffix)
-      prefix && suffix && prefix[0] == '&' && suffix[-1] == ';'
-    end
-
-    def reference_link(type, identifier, project, _)
-      references[type] << [project, identifier]
+    # Instantiate and call HTML::Pipeline with a single reference filter type,
+    # returning the result
+    #
+    # filter_type - Symbol reference type (e.g., :commit, :issue, etc.)
+    #
+    # Returns the results Array for the requested filter type
+    def pipeline_result(filter_type)
+      klass  = filter_type.to_s.camelize + 'ReferenceFilter'
+      filter = "Gitlab::Markdown::#{klass}".constantize
+
+      context = {
+        project: project,
+        current_user: current_user,
+        # We don't actually care about the links generated
+        only_path: true
+      }
+
+      pipeline = HTML::Pipeline.new([filter], context)
+      result = pipeline.call(@_text)
+
+      result[:references][filter_type]
     end
   end
 end

spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb

@@ -42,13 +42,17 @@ module Gitlab::Markdown
         reference = "#{commit1.short_id}...#{commit2.id}"
         reference2 = "#{commit1.id}...#{commit2.short_id}"
 
-        expect(filter("See #{reference}").css('a').first.text).to eq reference
-        expect(filter("See #{reference2}").css('a').first.text).to eq reference2
+        exp = commit1.short_id + '...' + commit2.short_id
+
+        expect(filter("See #{reference}").css('a').first.text).to eq exp
+        expect(filter("See #{reference2}").css('a').first.text).to eq exp
       end
 
       it 'links with adjacent text' do
         doc = filter("See (#{reference}.)")
-        expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(reference)}<\/a>\.\)/)
+
+        exp = Regexp.escape("#{commit1.short_id}...#{commit2.short_id}")
+        expect(doc.to_html).to match(/\(<a.+>#{exp}<\/a>\.\)/)
       end
 
       it 'ignores invalid commit IDs' do
@@ -81,6 +85,11 @@ module Gitlab::Markdown
         expect(link).not_to match %r(https?://)
         expect(link).to eq urls.namespace_project_compare_url(project.namespace, project, from: commit1.id, to: commit2.id, only_path: true)
       end
+
+      it 'adds to the results hash' do
+        result = pipeline_result("See #{reference}")
+        expect(result[:references][:commit_range]).not_to be_empty
+      end
     end
 
     context 'cross-project reference' do
@@ -102,7 +111,9 @@ module Gitlab::Markdown
 
         it 'links with adjacent text' do
           doc = filter("Fixed (#{reference}.)")
-          expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(reference)}<\/a>\.\)/)
+
+          exp = Regexp.escape("#{project2.path_with_namespace}@#{commit1.short_id}...#{commit2.short_id}")
+          expect(doc.to_html).to match(/\(<a.+>#{exp}<\/a>\.\)/)
         end
 
         it 'ignores invalid commit IDs on the referenced project' do
@@ -112,6 +123,11 @@ module Gitlab::Markdown
           exp = act = "Fixed #{project2.path_with_namespace}##{commit1.id}...#{commit2.id.reverse}"
           expect(filter(act).to_html).to eq exp
         end
+
+        it 'adds to the results hash' do
+          result = pipeline_result("See #{reference}")
+          expect(result[:references][:commit_range]).not_to be_empty
+        end
       end
 
       context 'when user cannot access reference' do

spec/lib/gitlab/markdown/commit_reference_filter_spec.rb

@@ -27,15 +27,23 @@ module Gitlab::Markdown
         it "links to a valid reference of #{size} characters" do
           doc = filter("See #{reference[0...size]}")
 
-          expect(doc.css('a').first.text).to eq reference[0...size]
+          expect(doc.css('a').first.text).to eq commit.short_id
           expect(doc.css('a').first.attr('href')).
             to eq urls.namespace_project_commit_url(project.namespace, project, reference)
         end
       end
 
+      it 'always uses the short ID as the link text' do
+        doc = filter("See #{commit.id}")
+        expect(doc.text).to eq "See #{commit.short_id}"
+
+        doc = filter("See #{commit.id[0...6]}")
+        expect(doc.text).to eq "See #{commit.short_id}"
+      end
+
       it 'links with adjacent text' do
         doc = filter("See (#{reference}.)")
-        expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(reference)}<\/a>\.\)/)
+        expect(doc.to_html).to match(/\(<a.+>#{commit.short_id}<\/a>\.\)/)
       end
 
       it 'ignores invalid commit IDs' do
@@ -55,7 +63,7 @@ module Gitlab::Markdown
         allow_any_instance_of(Commit).to receive(:title).and_return(%{"></a>whatever<a title="})
 
         doc = filter("See #{reference}")
-        expect(doc.text).to eq "See #{commit.id}"
+        expect(doc.text).to eq "See #{commit.short_id}"
       end
 
       it 'includes default classes' do
@@ -75,6 +83,11 @@ module Gitlab::Markdown
         expect(link).not_to match %r(https?://)
         expect(link).to eq urls.namespace_project_commit_url(project.namespace, project, reference, only_path: true)
       end
+
+      it 'adds to the results hash' do
+        result = pipeline_result("See #{reference}")
+        expect(result[:references][:commit]).not_to be_empty
+      end
     end
 
     context 'cross-project reference' do
@@ -95,13 +108,20 @@ module Gitlab::Markdown
 
         it 'links with adjacent text' do
           doc = filter("Fixed (#{reference}.)")
-          expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(reference)}<\/a>\.\)/)
+
+          exp = Regexp.escape(project2.path_with_namespace)
+          expect(doc.to_html).to match(/\(<a.+>#{exp}@#{commit.short_id}<\/a>\.\)/)
         end
 
         it 'ignores invalid commit IDs on the referenced project' do
           exp = act = "Committed #{project2.path_with_namespace}##{commit.id.reverse}"
           expect(filter(act).to_html).to eq exp
         end
+
+        it 'adds to the results hash' do
+          result = pipeline_result("See #{reference}")
+          expect(result[:references][:commit]).not_to be_empty
+        end
       end
 
       context 'when user cannot access reference' do

spec/lib/gitlab/markdown/issue_reference_filter_spec.rb

@@ -34,7 +34,7 @@ module Gitlab::Markdown
       end
 
       it 'links to a valid reference' do
-        doc = filter("See #{reference}")
+        doc = filter("Fixed #{reference}")
 
         expect(doc.css('a').first.attr('href')).
           to eq helper.url_for_issue(issue.iid, project)
@@ -81,6 +81,11 @@ module Gitlab::Markdown
         expect(link).not_to match %r(https?://)
         expect(link).to eq helper.url_for_issue(issue.iid, project, only_path: true)
       end
+
+      it 'adds to the results hash' do
+        result = pipeline_result("Fixed #{reference}")
+        expect(result[:references][:issue]).to eq [issue]
+      end
     end
 
     context 'cross-project reference' do
@@ -117,6 +122,11 @@ module Gitlab::Markdown
 
           expect(filter(act).to_html).to eq exp
         end
+
+        it 'adds to the results hash' do
+          result = pipeline_result("Fixed #{reference}")
+          expect(result[:references][:issue]).to eq [issue]
+        end
       end
 
       context 'when user cannot access reference' do

spec/lib/gitlab/markdown/label_reference_filter_spec.rb

@@ -39,6 +39,11 @@ module Gitlab::Markdown
       expect(link).to eq urls.namespace_project_issues_url(project.namespace, project, label_name: label.name, only_path: true)
     end
 
+    it 'adds to the results hash' do
+      result = pipeline_result("Label #{reference}")
+      expect(result[:references][:label]).to eq [label]
+    end
+
     describe 'label span element' do
       it 'includes default classes' do
         doc = filter("Label #{reference}")

spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb

@@ -69,6 +69,11 @@ module Gitlab::Markdown
         expect(link).not_to match %r(https?://)
         expect(link).to eq urls.namespace_project_merge_request_url(project.namespace, project, merge, only_path: true)
       end
+
+      it 'adds to the results hash' do
+        result = pipeline_result("Merge #{reference}")
+        expect(result[:references][:merge_request]).to eq [merge]
+      end
     end
 
     context 'cross-project reference' do
@@ -98,6 +103,11 @@ module Gitlab::Markdown
 
           expect(filter(act).to_html).to eq exp
         end
+
+        it 'adds to the results hash' do
+          result = pipeline_result("Merge #{reference}")
+          expect(result[:references][:merge_request]).to eq [merge]
+        end
       end
 
       context 'when user cannot access reference' do

spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb

@@ -68,6 +68,11 @@ module Gitlab::Markdown
         expect(link).not_to match %r(https?://)
         expect(link).to eq urls.namespace_project_snippet_url(project.namespace, project, snippet, only_path: true)
       end
+
+      it 'adds to the results hash' do
+        result = pipeline_result("Snippet #{reference}")
+        expect(result[:references][:snippet]).to eq [snippet]
+      end
     end
 
     context 'cross-project reference' do
@@ -96,6 +101,11 @@ module Gitlab::Markdown
 
           expect(filter(act).to_html).to eq exp
         end
+
+        it 'adds to the results hash' do
+          result = pipeline_result("Snippet #{reference}")
+          expect(result[:references][:snippet]).to eq [snippet]
+        end
       end
 
       context 'when user cannot access reference' do

spec/lib/gitlab/markdown/user_reference_filter_spec.rb

@@ -24,9 +24,29 @@ module Gitlab::Markdown
       end
     end
 
+    context 'mentioning @all' do
+      before do
+        project.team << [project.creator, :developer]
+      end
+
+      it 'supports a special @all mention' do
+        doc = filter("Hey @all")
+        expect(doc.css('a').length).to eq 1
+        expect(doc.css('a').first.attr('href'))
+          .to eq urls.namespace_project_url(project.namespace, project)
+      end
+
+      it 'adds to the results hash' do
+        result = pipeline_result('Hey @all')
+        expect(result[:references][:user]).to eq [project.creator]
+      end
+    end
+
     context 'mentioning a user' do
+      let(:reference) { "@#{user.username}" }
+
       it 'links to a User' do
-        doc = filter("Hey @#{user.username}")
+        doc = filter("Hey #{reference}")
         expect(doc.css('a').first.attr('href')).to eq urls.user_url(user)
       end
 
@@ -45,22 +65,45 @@ module Gitlab::Markdown
         doc = filter("Hey @#{user.username}")
         expect(doc.css('a').length).to eq 1
       end
+
+      it 'adds to the results hash' do
+        result = pipeline_result("Hey #{reference}")
+        expect(result[:references][:user]).to eq [user]
+      end
     end
 
     context 'mentioning a group' do
       let(:group) { create(:group) }
       let(:user)  { create(:user) }
 
-      it 'links to a Group that the current user can read' do
+      let(:reference) { "@#{group.name}" }
+
+      context 'that the current user can read' do
+        before do
           group.add_user(user, Gitlab::Access::DEVELOPER)
+        end
 
-        doc = filter("Hey @#{group.name}", current_user: user)
+        it 'links to the Group' do
+          doc = filter("Hey #{reference}", current_user: user)
           expect(doc.css('a').first.attr('href')).to eq urls.group_url(group)
         end
 
-      it 'ignores references to a Group that the current user cannot read' do
-        doc = filter("Hey @#{group.name}", current_user: user)
-        expect(doc.to_html).to eq "Hey @#{group.name}"
+        it 'adds to the results hash' do
+          result = pipeline_result("Hey #{reference}", current_user: user)
+          expect(result[:references][:user]).to eq group.users
+        end
+      end
+
+      context 'that the current user cannot read' do
+        it 'ignores references to the Group' do
+          doc = filter("Hey #{reference}", current_user: user)
+          expect(doc.to_html).to eq "Hey #{reference}"
+        end
+
+        it 'does not add to the results hash' do
+          result = pipeline_result("Hey #{reference}", current_user: user)
+          expect(result[:references][:user]).to eq []
+        end
       end
     end
 
@@ -70,13 +113,6 @@ module Gitlab::Markdown
       expect(doc.to_html).to match(/\(<a.+>@#{user.username}<\/a>\.\)/)
     end
 
-    it 'supports a special @all mention' do
-      doc = filter("Hey @all")
-      expect(doc.css('a').length).to eq 1
-      expect(doc.css('a').first.attr('href'))
-        .to eq urls.namespace_project_url(project.namespace, project)
-    end
-
     it 'includes default classes' do
       doc = filter("Hey @#{user.username}")
       expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-project_member'

spec/lib/gitlab/reference_extractor_spec.rb

@@ -4,80 +4,6 @@ describe Gitlab::ReferenceExtractor do
   let(:project) { create(:project) }
   subject { Gitlab::ReferenceExtractor.new(project, project.creator) }
 
-  it 'extracts username references' do
-    subject.analyze('this contains a @user reference')
-    expect(subject.references[:user]).to eq([[project, 'user']])
-  end
-
-  it 'extracts issue references' do
-    subject.analyze('this one talks about issue #1234')
-    expect(subject.references[:issue]).to eq([[project, '1234']])
-  end
-
-  it 'extracts JIRA issue references' do
-    subject.analyze('this one talks about issue JIRA-1234')
-    expect(subject.references[:issue]).to eq([[project, 'JIRA-1234']])
-  end
-
-  it 'extracts merge request references' do
-    subject.analyze("and here's !43, a merge request")
-    expect(subject.references[:merge_request]).to eq([[project, '43']])
-  end
-
-  it 'extracts snippet ids' do
-    subject.analyze('snippets like $12 get extracted as well')
-    expect(subject.references[:snippet]).to eq([[project, '12']])
-  end
-
-  it 'extracts commit shas' do
-    subject.analyze('commit shas 98cf0ae3 are pulled out as Strings')
-    expect(subject.references[:commit]).to eq([[project, '98cf0ae3']])
-  end
-
-  it 'extracts commit ranges' do
-    subject.analyze('here you go, a commit range: 98cf0ae3...98cf0ae4')
-    expect(subject.references[:commit_range]).to eq([[project, '98cf0ae3...98cf0ae4']])
-  end
-
-  it 'extracts multiple references and preserves their order' do
-    subject.analyze('@me and @you both care about this')
-    expect(subject.references[:user]).to eq([
-      [project, 'me'],
-      [project, 'you']
-    ])
-  end
-
-  it 'leaves the original note unmodified' do
-    text = 'issue #123 is just the worst, @user'
-    subject.analyze(text)
-    expect(text).to eq('issue #123 is just the worst, @user')
-  end
-
-  it 'extracts no references for <pre>..</pre> blocks' do
-    subject.analyze("<pre>def puts '#1 issue'\nend\n</pre>```")
-    expect(subject.issues).to be_blank
-  end
-
-  it 'extracts no references for <code>..</code> blocks' do
-    subject.analyze("<code>def puts '!1 request'\nend\n</code>```")
-    expect(subject.merge_requests).to be_blank
-  end
-
-  it 'extracts no references for code blocks with language' do
-    subject.analyze("this code:\n```ruby\ndef puts '#1 issue'\nend\n```")
-    expect(subject.issues).to be_blank
-  end
-
-  it 'extracts issue references for invalid code blocks' do
-    subject.analyze('test: ```this one talks about issue #1234```')
-    expect(subject.references[:issue]).to eq([[project, '1234']])
-  end
-
-  it 'handles all possible kinds of references' do
-    accessors = described_class::TYPES.map { |t| "#{t}s".to_sym }
-    expect(subject).to respond_to(*accessors)
-  end
-
   it 'accesses valid user objects' do
     @u_foo = create(:user, username: 'foo')
     @u_bar = create(:user, username: 'bar')
@@ -139,12 +65,12 @@ describe Gitlab::ReferenceExtractor do
     earlier_commit = project.commit('master~2')
 
     subject.analyze("this references commits #{earlier_commit.sha[0..6]}...#{commit.sha[0..6]}")
+
     extracted = subject.commit_ranges
     expect(extracted.size).to eq(1)
-    expect(extracted[0][0].sha).to eq(earlier_commit.sha)
-    expect(extracted[0][0].message).to eq(earlier_commit.message)
-    expect(extracted[0][1].sha).to eq(commit.sha)
-    expect(extracted[0][1].message).to eq(commit.message)
+    expect(extracted.first).to be_kind_of(CommitRange)
+    expect(extracted.first.commit_from).to eq earlier_commit
+    expect(extracted.first.commit_to).to eq commit
   end
 
   context 'with a project with an underscore' do

spec/models/commit_range_spec.rb

+require 'spec_helper'
+
+describe CommitRange do
+  let(:sha_from) { 'f3f85602' }
+  let(:sha_to)   { 'e86e1013' }
+
+  let(:range)  { described_class.new("#{sha_from}...#{sha_to}") }
+  let(:range2) { described_class.new("#{sha_from}..#{sha_to}") }
+
+  it 'raises ArgumentError when given an invalid range string' do
+    expect { described_class.new("Foo") }.to raise_error
+  end
+
+  describe '#to_s' do
+    it 'is correct for three-dot syntax' do
+      expect(range.to_s).to eq "#{sha_from[0..7]}...#{sha_to[0..7]}"
+    end
+
+    it 'is correct for two-dot syntax' do
+      expect(range2.to_s).to eq "#{sha_from[0..7]}..#{sha_to[0..7]}"
+    end
+  end
+
+  describe '#reference_title' do
+    it 'returns the correct String for three-dot ranges' do
+      expect(range.reference_title).to eq "Commits #{sha_from} through #{sha_to}"
+    end
+
+    it 'returns the correct String for two-dot ranges' do
+      expect(range2.reference_title).to eq "Commits #{sha_from}^ through #{sha_to}"
+    end
+  end
+
+  describe '#to_param' do
+    it 'includes the correct keys' do
+      expect(range.to_param.keys).to eq %i(from to)
+    end
+
+    it 'includes the correct values for a three-dot range' do
+      expect(range.to_param).to eq({from: sha_from, to: sha_to})
+    end
+
+    it 'includes the correct values for a two-dot range' do
+      expect(range2.to_param).to eq({from: sha_from + '^', to: sha_to})
+    end
+  end
+
+  describe '#exclude_start?' do
+    it 'is false for three-dot ranges' do
+      expect(range.exclude_start?).to eq false
+    end
+
+    it 'is true for two-dot ranges' do
+      expect(range2.exclude_start?).to eq true
+    end
+  end
+
+  describe '#valid_commits?' do
+    context 'without a project' do
+      it 'returns nil' do
+        expect(range.valid_commits?).to be_nil
+      end
+    end
+
+    it 'accepts an optional project argument' do
+      project1 = double('project1').as_null_object
+      project2 = double('project2').as_null_object
+
+      # project1 gets assigned through the accessor, but ignored when not given
+      # as an argument to `valid_commits?`
+      expect(project1).not_to receive(:present?)
+      range.project = project1
+
+      # project2 gets passed to `valid_commits?`
+      expect(project2).to receive(:present?).and_return(false)
+
+      range.valid_commits?(project2)
+    end
+
+    context 'with a project' do
+      let(:project) { double('project', repository: double('repository')) }
+
+      context 'with a valid repo' do
+        before do
+          expect(project).to receive(:valid_repo?).and_return(true)
+          range.project = project
+        end
+
+        it 'is false when `sha_from` is invalid' do
+          expect(project.repository).to receive(:commit).with(sha_from).and_return(false)
+          expect(project.repository).not_to receive(:commit).with(sha_to)
+          expect(range).not_to be_valid_commits
+        end
+
+        it 'is false when `sha_to` is invalid' do
+          expect(project.repository).to receive(:commit).with(sha_from).and_return(true)
+          expect(project.repository).to receive(:commit).with(sha_to).and_return(false)
+          expect(range).not_to be_valid_commits
+        end
+
+        it 'is true when both `sha_from` and `sha_to` are valid' do
+          expect(project.repository).to receive(:commit).with(sha_from).and_return(true)
+          expect(project.repository).to receive(:commit).with(sha_to).and_return(true)
+          expect(range).to be_valid_commits
+        end
+      end
+
+      context 'without a valid repo' do
+        before do
+          expect(project).to receive(:valid_repo?).and_return(false)
+          range.project = project
+        end
+
+        it 'returns false' do
+          expect(range).not_to be_valid_commits
+        end
+      end
+    end
+  end
+end

spec/support/reference_filter_spec_helper.rb

@@ -35,6 +35,20 @@ module ReferenceFilterSpecHelper
     described_class.call(html, contexts)
   end
 
+  # Run text through HTML::Pipeline with the current filter and return the
+  # result Hash
+  #
+  # body     - String text to run through the pipeline
+  # contexts - Hash context for the filter. (default: {project: project})
+  #
+  # Returns the Hash of the pipeline result
+  def pipeline_result(body, contexts = {})
+    contexts.reverse_merge!(project: project)
+
+    pipeline = HTML::Pipeline.new([described_class], contexts)
+    pipeline.call(body)
+  end
+
   def allow_cross_reference!
     allow_any_instance_of(described_class).
       to receive(:user_can_reference_project?).and_return(true)