Render commit range reference with short shas, link to full shas.

app/models/commit_range.rb

@@ -2,12 +2,12 @@
 #
 # Examples:
 #
-#   range = CommitRange.new('f3f85602...e86e1013')
+#   range = CommitRange.new('f3f85602...e86e1013', project)
 #   range.exclude_start?  # => false
 #   range.reference_title # => "Commits f3f85602 through e86e1013"
 #   range.to_s            # => "f3f85602...e86e1013"
 #
-#   range = CommitRange.new('f3f856029bc5f966c5a7ee24cf7efefdd20e6019..e86e1013709735be5bb767e2b228930c543f25ae')
+#   range = CommitRange.new('f3f856029bc5f966c5a7ee24cf7efefdd20e6019..e86e1013709735be5bb767e2b228930c543f25ae', project)
 #   range.exclude_start?  # => true
 #   range.reference_title # => "Commits f3f85602^ through e86e1013"
 #   range.to_param        # => {from: "f3f856029bc5f966c5a7ee24cf7efefdd20e6019^", to: "e86e1013709735be5bb767e2b228930c543f25ae"}
@@ -21,7 +21,7 @@ class CommitRange
   include ActiveModel::Conversion
   include Referable
 
-  attr_reader :sha_from, :notation, :sha_to
+  attr_reader :commit_from, :notation, :commit_to
 
   # Optional Project model
   attr_accessor :project
@@ -53,17 +53,22 @@ class CommitRange
   # project      - An optional Project model.
   #
   # Raises ArgumentError if `range_string` does not match `PATTERN`.
-  def initialize(range_string, project = nil)
+  def initialize(range_string, project)
+    @project = project
+
     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)
+    ref_from, @notation, ref_to = range_string.split(/(\.{2,3})/, 2)
 
-    @project = project
+    @exclude_start = @notation == '..'
+    if project.valid_repo?
+      @commit_from = project.commit(ref_from)
+      @commit_to   = project.commit(ref_to)
+    end
   end
 
   def inspect
@@ -71,15 +76,16 @@ class CommitRange
   end
 
   def to_s
-    "#{sha_from[0..7]}#{notation}#{sha_to[0..7]}"
+    sha_from + notation + sha_to
   end
 
+  alias_method :id, :to_s
+
   def to_reference(from_project = nil)
-    # Not using to_s because we want the full SHAs
-    reference = sha_from + notation + sha_to
+    reference = Commit.truncate_sha(sha_from) + notation + Commit.truncate_sha(sha_to)
 
     if cross_project_reference?(from_project)
-      reference = project.to_reference + '@' + reference
+      reference = project.to_reference + self.class.reference_prefix + reference
     end
 
     reference
@@ -87,14 +93,14 @@ class CommitRange
 
   # Returns a String for use in a link's title attribute
   def reference_title
-    "Commits #{suffixed_sha_from} through #{sha_to}"
+    "Commits #{sha_start} 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 }
+    { from: sha_start, to: sha_to }
   end
 
   def exclude_start?
@@ -105,28 +111,42 @@ class CommitRange
   # 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?
+  def valid_commits?
+    commit_start.present? && commit_end.present?
   end
 
   def persisted?
     true
   end
 
-  def commit_from
-    @commit_from ||= project.repository.commit(suffixed_sha_from)
+  def sha_from
+    return nil unless @commit_from
+
+    @commit_from.id
   end
 
-  def commit_to
-    @commit_to ||= project.repository.commit(sha_to)
+  def sha_to
+    return nil unless @commit_to
+
+    @commit_to.id
+  end
+
+  def sha_start
+    return nil unless sha_from
+
+    exclude_start? ? sha_from + '^' : sha_from
   end
 
-  private
+  def commit_start
+    return nil unless sha_start
 
-  def suffixed_sha_from
-    sha_from + (exclude_start? ? '^' : '')
+    if exclude_start?
+      @commit_start ||= project.commit(sha_start)
+    else
+      commit_from
+    end
   end
+
+  alias_method :sha_end, :sha_to
+  alias_method :commit_end, :commit_to
 end

spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb

@@ -8,8 +8,8 @@ module Gitlab::Markdown
     let(:commit1) { project.commit }
     let(:commit2) { project.commit("HEAD~2") }
 
-    let(:range)  { CommitRange.new("#{commit1.id}...#{commit2.id}") }
-    let(:range2) { CommitRange.new("#{commit1.id}..#{commit2.id}") }
+    let(:range)  { CommitRange.new("#{commit1.id}...#{commit2.id}", project) }
+    let(:range2) { CommitRange.new("#{commit1.id}..#{commit2.id}", project) }
 
     it 'requires project context' do
       expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
@@ -53,7 +53,7 @@ module Gitlab::Markdown
       it 'links with adjacent text' do
         doc = filter("See (#{reference}.)")
 
-        exp = Regexp.escape(range.to_s)
+        exp = Regexp.escape(range.to_reference)
         expect(doc.to_html).to match(/\(<a.+>#{exp}<\/a>\.\)/)
       end
 
@@ -62,6 +62,7 @@ module Gitlab::Markdown
 
         expect(project).to receive(:valid_repo?).and_return(true)
         expect(project.repository).to receive(:commit).with(commit1.id.reverse)
+        expect(project.repository).to receive(:commit).with(commit2.id)
         expect(filter(act).to_html).to eq exp
       end
 

spec/models/commit_range_spec.rb

@@ -7,50 +7,56 @@ describe CommitRange do
     it { is_expected.to include_module(Referable) }
   end
 
-  let(:sha_from) { 'f3f85602' }
-  let(:sha_to)   { 'e86e1013' }
+  let!(:project) { create(:project, :public) }
+  let!(:commit1) { project.commit("HEAD~2") }
+  let!(:commit2) { project.commit }
 
-  let(:range)  { described_class.new("#{sha_from}...#{sha_to}") }
-  let(:range2) { described_class.new("#{sha_from}..#{sha_to}") }
+  let(:sha_from) { commit1.short_id }
+  let(:sha_to)   { commit2.short_id }
+
+  let(:full_sha_from) { commit1.id }
+  let(:full_sha_to)   { commit2.id }
+
+  let(:range)  { described_class.new("#{sha_from}...#{sha_to}", project) }
+  let(:range2) { described_class.new("#{sha_from}..#{sha_to}", project) }
 
   it 'raises ArgumentError when given an invalid range string' do
-    expect { described_class.new("Foo") }.to raise_error(ArgumentError)
+    expect { described_class.new("Foo", project) }.to raise_error(ArgumentError)
   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]}"
+      expect(range.to_s).to eq "#{full_sha_from}...#{full_sha_to}"
     end
 
     it 'is correct for two-dot syntax' do
-      expect(range2.to_s).to eq "#{sha_from[0..7]}..#{sha_to[0..7]}"
+      expect(range2.to_s).to eq "#{full_sha_from}..#{full_sha_to}"
     end
   end
 
   describe '#to_reference' do
-    let(:project) { double('project', to_reference: 'namespace1/project') }
+    let(:cross) { create(:project) }
 
-    before do
-      range.project = project
+    it 'returns a String reference to the object' do
+      expect(range.to_reference).to eq "#{sha_from}...#{sha_to}"
     end
 
     it 'returns a String reference to the object' do
-      expect(range.to_reference).to eq range.to_s
+      expect(range2.to_reference).to eq "#{sha_from}..#{sha_to}"
     end
 
     it 'supports a cross-project reference' do
-      cross = double('project')
-      expect(range.to_reference(cross)).to eq "#{project.to_reference}@#{range.to_s}"
+      expect(range.to_reference(cross)).to eq "#{project.to_reference}@#{sha_from}...#{sha_to}"
     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}"
+      expect(range.reference_title).to eq "Commits #{full_sha_from} through #{full_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}"
+      expect(range2.reference_title).to eq "Commits #{full_sha_from}^ through #{full_sha_to}"
     end
   end
 
@@ -60,11 +66,11 @@ describe CommitRange do
     end
 
     it 'includes the correct values for a three-dot range' do
-      expect(range.to_param).to eq({ from: sha_from, to: sha_to })
+      expect(range.to_param).to eq({ from: full_sha_from, to: full_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 })
+      expect(range2.to_param).to eq({ from: full_sha_from + '^', to: full_sha_to })
     end
   end
 
@@ -79,64 +85,37 @@ describe CommitRange do
   end
 
   describe '#valid_commits?' do
-    context 'without a project' do
-      it 'returns nil' do
-        expect(range.valid_commits?).to be_nil
+    context 'with a valid repo' do
+      before do
+        expect(project).to receive(:valid_repo?).and_return(true)
       end
-    end
 
-    it 'accepts an optional project argument' do
-      project1 = double('project1').as_null_object
-      project2 = double('project2').as_null_object
+      it 'is false when `sha_from` is invalid' do
+        expect(project).to receive(:commit).with(sha_from).and_return(nil)
+        expect(project).to receive(:commit).with(sha_to).and_call_original
 
-      # 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
+        expect(range).not_to be_valid_commits
+      end
 
-      # project2 gets passed to `valid_commits?`
-      expect(project2).to receive(:present?).and_return(false)
+      it 'is false when `sha_to` is invalid' do
+        expect(project).to receive(:commit).with(sha_from).and_call_original
+        expect(project).to receive(:commit).with(sha_to).and_return(nil)
 
-      range.valid_commits?(project2)
-    end
+        expect(range).not_to be_valid_commits
+      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
+      it 'is true when both `sha_from` and `sha_to` are valid' do
+        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
+    context 'without a valid repo' do
+      before do
+        expect(project).to receive(:valid_repo?).and_return(false)
+      end
 
-        it 'returns false' do
-          expect(range).not_to be_valid_commits
-        end
+      it 'returns false' do
+        expect(range).not_to be_valid_commits
       end
     end
   end