Allows to search within project by commit's hash

Was proposed in #24833

6 files changed, 179 insertions, 4 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 5d942cb0422..316bd2e512b 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -21,6 +21,9 @@ class Commit
   DIFF_HARD_LIMIT_FILES = 1000
   DIFF_HARD_LIMIT_LINES = 50000
 
+  # The SHA can be between 7 and 40 hex characters.
+  COMMIT_SHA_PATTERN = '\h{7,40}'
+
   class << self
     def decorate(commits, project)
       commits.map do |commit|
@@ -52,6 +55,10 @@ class Commit
     def from_hash(hash, project)
       new(Gitlab::Git::Commit.new(hash), project)
     end
+
+    def valid_hash?(key)
+      !!(/\A#{COMMIT_SHA_PATTERN}\z/ =~ key)
+    end
   end
 
   attr_accessor :raw
@@ -77,8 +84,6 @@ class Commit
 
   # Pattern used to extract commit references from text
   #
-  # The SHA can be between 7 and 40 hex characters.
-  #
   # This pattern supports cross-project references.
   def self.reference_pattern
     @reference_pattern ||= %r{
@@ -88,7 +93,7 @@ class Commit
   end
 
   def self.link_reference_pattern
-    @link_reference_pattern ||= super("commit", /(?<commit>\h{7,40})/)
+    @link_reference_pattern ||= super("commit", /(?<commit>#{COMMIT_SHA_PATTERN})/)
   end
 
   def to_reference(from_project = nil, full: false)
diff --git a/changelogs/unreleased/24833-Allow-to-search-by-commit-hash-within-project.yml b/changelogs/unreleased/24833-Allow-to-search-by-commit-hash-within-project.yml
new file mode 100644
index 00000000000..be66c370f36
--- /dev/null
+++ b/changelogs/unreleased/24833-Allow-to-search-by-commit-hash-within-project.yml
@@ -0,0 +1,4 @@
+---
+title: 'Allows to search within project by commit hash'
+merge_request: 
+author: YarNayar
diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb
index 6bdf3db9cb8..a66719706be 100644
--- a/lib/gitlab/project_search_results.rb
+++ b/lib/gitlab/project_search_results.rb
@@ -114,7 +114,25 @@ module Gitlab
     end
 
     def commits
-      @commits ||= project.repository.find_commits_by_message(query)
+      @commits ||= find_commits(query)
+    end
+
+    def find_commits(query)
+      return [] unless Ability.allowed?(@current_user, :download_code, @project)
+
+      commits = find_commits_by_message(query)
+      commit_by_sha = find_commit_by_sha(query)
+      commits << commit_by_sha if commit_by_sha && !commits.include?(commit_by_sha)
+      commits
+    end
+
+    def find_commits_by_message(query)
+      project.repository.find_commits_by_message(query)
+    end
+
+    def find_commit_by_sha(query)
+      key = query.strip
+      project.repository.commit(key) if Commit.valid_hash?(key)
     end
 
     def project_ids_relation
diff --git a/spec/features/search_spec.rb b/spec/features/search_spec.rb
index a05b83959fb..c64aeea0612 100644
--- a/spec/features/search_spec.rb
+++ b/spec/features/search_spec.rb
@@ -211,4 +211,19 @@ describe "Search", feature: true  do
       end
     end
   end
+
+  describe 'search for commits' do
+    before do
+      visit search_path(project_id: project.id)
+    end
+
+    it 'shows multiple matching commits' do
+      fill_in 'search', with: 'See merge request'
+
+      click_button 'Search'
+      click_link 'Commits'
+
+      expect(page).to have_selector('.commit-row-description', count: 9)
+    end
+  end
 end
diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb
index 14ee386dba6..d94eb52f838 100644
--- a/spec/lib/gitlab/project_search_results_spec.rb
+++ b/spec/lib/gitlab/project_search_results_spec.rb
@@ -178,4 +178,119 @@ describe Gitlab::ProjectSearchResults, lib: true do
       expect(results.objects('notes')).not_to include note
     end
   end
+
+  # Examples for commit access level test
+  #
+  # params:
+  # * search_phrase
+  # * commit
+  #
+  shared_examples 'access restricted commits' do
+    context 'when project is internal' do
+      let(:project) { create(:project, :internal) }
+
+      it 'does not search if user is not authenticated' do
+        commits = described_class.new(nil, project, search_phrase).objects('commits')
+
+        expect(commits).to be_empty
+      end
+
+      it 'searches if user is authenticated' do
+        commits = described_class.new(user, project, search_phrase).objects('commits')
+
+        expect(commits).to contain_exactly commit
+      end
+    end
+
+    context 'when project is private' do
+      let!(:creator) { create(:user, username: 'private-project-author') }
+      let!(:private_project) { create(:project, :private, creator: creator, namespace: creator.namespace) }
+      let(:team_master) do
+        user = create(:user, username: 'private-project-master')
+        private_project.team << [user, :master]
+        user
+      end
+      let(:team_reporter) do
+        user = create(:user, username: 'private-project-reporter')
+        private_project.team << [user, :reporter]
+        user
+      end
+
+      it 'does not show commit to stranger' do
+        commits = described_class.new(nil, private_project, search_phrase).objects('commits')
+
+        expect(commits).to be_empty
+      end
+
+      context 'team access' do
+        it 'shows commit to creator' do
+          commits = described_class.new(creator, private_project, search_phrase).objects('commits')
+
+          expect(commits).to contain_exactly commit
+        end
+
+        it 'shows commit to master' do
+          commits = described_class.new(team_master, private_project, search_phrase).objects('commits')
+
+          expect(commits).to contain_exactly commit
+        end
+
+        it 'shows commit to reporter' do
+          commits = described_class.new(team_reporter, private_project, search_phrase).objects('commits')
+
+          expect(commits).to contain_exactly commit
+        end
+      end
+    end
+  end
+
+  describe 'commit search' do
+    context 'by commit message' do
+      let(:project) { create(:project, :public) }
+      let(:commit) { project.repository.commit('59e29889be61e6e0e5e223bfa9ac2721d31605b8') }
+      let(:message) { 'Sorry, I did a mistake' }
+
+      it 'finds commit by message' do
+        commits = described_class.new(user, project, message).objects('commits')
+
+        expect(commits).to contain_exactly commit
+      end
+
+      it 'handles when no commit match' do
+        commits = described_class.new(user, project, 'not really an existing description').objects('commits')
+
+        expect(commits).to be_empty
+      end
+
+      it_behaves_like 'access restricted commits' do
+        let(:search_phrase) { message }
+        let(:commit) { project.repository.commit('59e29889be61e6e0e5e223bfa9ac2721d31605b8') }
+      end
+    end
+
+    context 'by commit hash' do
+      let(:project) { create(:project, :public) }
+      let(:commit) { project.repository.commit('0b4bc9a') }
+      commit_hashes = { short: '0b4bc9a', full: '0b4bc9a49b562e85de7cc9e834518ea6828729b9' }
+
+      commit_hashes.each do |type, commit_hash|
+        it "shows commit by #{type} hash id" do
+          commits = described_class.new(user, project, commit_hash).objects('commits')
+
+          expect(commits).to contain_exactly commit
+        end
+      end
+
+      it 'handles not existing commit hash correctly' do
+        commits = described_class.new(user, project, 'deadbeef').objects('commits')
+
+        expect(commits).to be_empty
+      end
+
+      it_behaves_like 'access restricted commits' do
+        let(:search_phrase) { '0b4bc9a49' }
+        let(:commit) { project.repository.commit('0b4bc9a') }
+      end
+    end
+  end
 end
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 0d425ab7fd4..b2202f0fd44 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -351,4 +351,22 @@ eos
       expect(commit).not_to be_work_in_progress
     end
   end
+
+  describe '.valid_hash?' do
+    it 'checks hash contents' do
+      expect(described_class.valid_hash?('abcdef01239ABCDEF')).to be true
+      expect(described_class.valid_hash?("abcdef01239ABCD\nEF")).to be false
+      expect(described_class.valid_hash?(' abcdef01239ABCDEF ')).to be false
+      expect(described_class.valid_hash?('Gabcdef01239ABCDEF')).to be false
+      expect(described_class.valid_hash?('gabcdef01239ABCDEF')).to be false
+      expect(described_class.valid_hash?('-abcdef01239ABCDEF')).to be false
+    end
+
+    it 'checks hash length' do
+      expect(described_class.valid_hash?('a' * 6)).to be false
+      expect(described_class.valid_hash?('a' * 7)).to be true
+      expect(described_class.valid_hash?('a' * 40)).to be true
+      expect(described_class.valid_hash?('a' * 41)).to be false
+    end
+  end
 end