Merge branch 'ambiguous-sha' into 'master'

Fix ambiguous sha problem with mentioned commit

Before: write in database only 6 chars of commit sha. This cause to `Ambiguous SHA1 prefix` exception.

- [x] write full commit sha in db.
- [x] Standardise usage of sha truncation: 8 characters everywhere.
- [x] prevent exception when ambiguous sha requested in markdown

Fixes #1644

See merge request !1171

app/helpers/commits_helper.rb

@@ -120,4 +120,8 @@ module CommitsHelper
                                        class: 'commit-short-id')
     end
   end
+
+  def truncate_sha(sha)
+    Commit.truncate_sha(sha)
+  end
 end

app/models/commit.rb

@@ -19,13 +19,24 @@ class Commit
 
   class << self
     def decorate(commits)
-      commits.map { |c| self.new(c) }
+      commits.map do |commit|
+        if commit.kind_of?(Commit)
+          commit
+        else
+          self.new(commit)
+        end
+      end
     end
 
     # Calculate number of lines to render for diffs
     def diff_line_count(diffs)
       diffs.reduce(0) { |sum, d| sum + d.diff.lines.count }
     end
+
+    # Truncate sha to 8 characters
+    def truncate_sha(sha)
+      sha[0..7]
+    end
   end
 
   attr_accessor :raw
@@ -111,7 +122,7 @@ class Commit
 
   # Mentionable override.
   def gfm_reference
-    "commit #{sha[0..5]}"
+    "commit #{id}"
   end
 
   def method_missing(m, *args, &block)
@@ -124,6 +135,11 @@ class Commit
     super
   end
 
+  # Truncate sha to 8 characters
+  def short_id
+    @raw.short_id(7)
+  end
+
   def parents
     @parents ||= Commit.decorate(super)
   end

app/models/event.rb

@@ -266,7 +266,7 @@ class Event < ActiveRecord::Base
   end
 
   def note_short_commit_id
-    note_commit_id[0..8]
+    Commit.truncate_sha(note_commit_id)
   end
 
   def note_commit?

app/models/merge_request_diff.rb

@@ -55,7 +55,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def last_commit_short_sha
-    @last_commit_short_sha ||= last_commit.sha[0..10]
+    @last_commit_short_sha ||= last_commit.short_id
   end
 
   private

app/models/repository.rb

@@ -30,6 +30,8 @@ class Repository
     commit = Gitlab::Git::Commit.find(raw_repository, id)
     commit = Commit.new(commit) if commit
     commit
+  rescue Rugged::OdbError => ex
+    nil
   end
 
   def commits(ref, path = nil, limit = nil, offset = nil, skip_merges = false)

app/views/events/_commit.html.haml

@@ -1,5 +1,5 @@
 %li.commit
   .commit-row-title
-    = link_to commit[:id][0..8], project_commit_path(project, commit[:id]), class: "commit_short_id", alt: ''
+    = link_to truncate_sha(commit[:id]), project_commit_path(project, commit[:id]), class: "commit_short_id", alt: ''
      
     = gfm event_commit_title(commit[:message]), project

app/views/events/_event_push.atom.haml

@@ -2,7 +2,7 @@
   - event.commits.first(15).each do |commit|
     %p
       %strong= commit[:author][:name]
-      = link_to "(##{commit[:id][0...8]})", project_commit_path(event.project, id: commit[:id])
+      = link_to "(##{truncate_sha(commit[:id])})", project_commit_path(event.project, id: commit[:id])
       %i
         at
         = commit[:timestamp].to_time.to_s(:short)

app/views/events/event/_push.html.haml

@@ -22,4 +22,4 @@
           - if event.commits_count > 2
             %span ... and #{event.commits_count - 2} more commits.
           = link_to project_compare_path(event.project, from: event.commit_from, to: event.commit_to) do
-            %strong Compare → #{event.commit_from[0..7]}...#{event.commit_to[0..7]}
+            %strong Compare → #{truncate_sha(event.commit_from)}...#{truncate_sha(event.commit_to)}

app/views/projects/blame/show.html.haml

@@ -15,7 +15,7 @@
           %tr
             %td.blame-commit
               %span.commit
-                = link_to commit.short_id(8), project_commit_path(@project, commit), class: "commit_short_id"
+                = link_to commit.short_id, project_commit_path(@project, commit), class: "commit_short_id"
                  
                 = commit_author_link(commit, avatar: true, size: 16)
                  

app/views/projects/commit/_commit_box.html.haml

@@ -35,7 +35,7 @@
 .commit-info-row
   %span.cgray= pluralize(@commit.parents.count, "parent")
   - @commit.parents.each do |parent|
-    = link_to parent.id[0...10], project_commit_path(@project, parent)
+    = link_to parent.short_id, project_commit_path(@project, parent)
 
 - if @branches.any?
   .commit-info-row

app/views/projects/commits/_commit.html.haml

@@ -1,6 +1,6 @@
 %li.commit.js-toggle-container
   .commit-row-title
-    = link_to commit.short_id(8), project_commit_path(project, commit), class: "commit_short_id"
+    = link_to commit.short_id, project_commit_path(project, commit), class: "commit_short_id"
      
     %span.str-truncated
       = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message"

app/views/projects/commits/_inline_commit.html.haml

@@ -1,6 +1,6 @@
 %li.commit.inline-commit
   .commit-row-title
-    = link_to commit.short_id(8), project_commit_path(project, commit), class: "commit_short_id"
+    = link_to commit.short_id, project_commit_path(project, commit), class: "commit_short_id"
      
     %span.str-truncated
       = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message"

app/views/projects/tree/_submodule_item.html.haml

@@ -7,8 +7,8 @@
     @
     %span.monospace
       - if commit.nil?
-        #{submodule_item.id[0..10]}
+        #{truncate_sha(submodule_item.id)}
       - else
-        = link_to "#{submodule_item.id[0..10]}", commit
+        = link_to "#{truncate_sha(submodule_item.id)}", commit
   %td
   %td.hidden-xs

app/views/projects/wikis/history.html.haml

@@ -17,7 +17,7 @@
       %tr
         %td
           = link_to project_wiki_path(@project, @page, version_id: commit.id) do
-            = commit.id[0..10]
+            = truncate_sha(commit.id)
         %td
           = commit.author.name
         %td

app/views/search/results/_note.html.haml

@@ -10,7 +10,7 @@
         = project.name_with_namespace
       ·
       = link_to project_commit_path(project, note.commit_id, anchor: dom_id(note)) do
-        Commit #{note.commit_id[0..8]}
+        Commit #{truncate_sha(note.commit_id)}
     - else
       = link_to project do
         = project.name_with_namespace

features/steps/project/commits/commits.rb

@@ -8,7 +8,7 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
     commit = @project.repository.commit
     page.should have_content(@project.name)
     page.should have_content(commit.message[0..20])
-    page.should have_content(commit.id.to_s[0..5])
+    page.should have_content(commit.short_id)
   end
 
   step 'I click atom feed link' do

features/steps/project/merge_requests.rb

@@ -111,7 +111,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
 
   step 'I click on the commit in the merge request' do
     within '.mr-commits' do
-      click_link sample_commit.id[0..8]
+      click_link Commit.truncate_sha(sample_commit.id)
     end
   end
 

spec/helpers/gitlab_markdown_helper_spec.rb

@@ -60,7 +60,7 @@ describe GitlabMarkdownHelper do
       end
 
       it "should link using a short id" do
-        actual = "Backported from #{commit.short_id(6)}"
+        actual = "Backported from #{commit.short_id}"
         gfm(actual).should match(expected)
       end
 

spec/models/commit_spec.rb

@@ -75,7 +75,7 @@ eos
   it_behaves_like 'a mentionable' do
     let(:subject) { commit }
     let(:mauthor) { create :user, email: commit.author_email }
-    let(:backref_text) { "commit #{subject.sha[0..5]}" }
+    let(:backref_text) { "commit #{subject.id}" }
     let(:set_mentionable_text) { ->(txt){ subject.stub(safe_message: txt) } }
 
     # Include the subject in the repository stub.

spec/models/note_spec.rb

@@ -228,7 +228,7 @@ describe Note do
 
       it { should be_valid }
       its(:noteable) { should == issue }
-      its(:note) { should == "_mentioned in commit #{commit.sha[0..5]}_" }
+      its(:note) { should == "_mentioned in commit #{commit.sha}_" }
     end
 
     context 'merge request from an issue' do
@@ -267,7 +267,7 @@ describe Note do
       its(:noteable_type) { should == "Commit" }
       its(:noteable_id) { should be_nil }
       its(:commit_id) { should == commit.id }
-      its(:note) { should == "_mentioned in commit #{parent_commit.id[0...6]}_" }
+      its(:note) { should == "_mentioned in commit #{parent_commit.id}_" }
     end
   end
 

spec/support/mentionable_shared_examples.rb

@@ -30,15 +30,15 @@ def common_mentionable_setup
     "!#{mentioned_mr.iid}, " +
     "#{ext_proj.path_with_namespace}##{ext_issue.iid}, " +
     "#{ext_proj.path_with_namespace}!#{ext_mr.iid}, " +
-    "#{ext_proj.path_with_namespace}@#{ext_commit.id[0..5]}, " +
-    "#{mentioned_commit.sha[0..5]} and itself as #{backref_text}"
+    "#{ext_proj.path_with_namespace}@#{ext_commit.short_id}, " +
+    "#{mentioned_commit.sha[0..10]} and itself as #{backref_text}"
   end
 
   before do
     # Wire the project's repository to return the mentioned commit, and +nil+ for any
     # unrecognized commits.
-    commitmap = { '123456' => mentioned_commit }
-    extra_commits.each { |c| commitmap[c.sha[0..5]] = c }
+    commitmap = { '1234567890a' => mentioned_commit }
+    extra_commits.each { |c| commitmap[c.short_id] = c }
     mproject.repository.stub(:commit) { |sha| commitmap[sha] }
     set_mentionable_text.call(ref_string)
   end
@@ -54,7 +54,6 @@ shared_examples 'a mentionable' do
   it "extracts references from its reference property" do
     # De-duplicate and omit itself
     refs = subject.references(mproject)
-
     refs.should have(6).items
     refs.should include(mentioned_issue)
     refs.should include(mentioned_mr)
@@ -90,7 +89,7 @@ shared_examples 'an editable mentionable' do
 
   it 'creates new cross-reference notes when the mentionable text is edited' do
     new_text = "still mentions ##{mentioned_issue.iid}, " +
-      "#{mentioned_commit.sha[0..5]}, " +
+      "#{mentioned_commit.sha[0..10]}, " +
       "#{ext_issue.iid}, " +
       "new refs: ##{other_issue.iid}, " +
       "#{ext_proj.path_with_namespace}##{other_ext_issue.iid}"