Incorporate Gitaly's CommitService.FindCommit RPCgitaly-find-commit

9 files changed, 76 insertions, 26 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 3045db06af1..bd9adeaf613 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -769,7 +769,7 @@ class Repository
       index = Gitlab::Git::Index.new(raw_repository)
 
       if start_commit
-        index.read_tree(start_commit.raw_commit.tree)
+        index.read_tree(start_commit.rugged_commit.tree)
         parents = [start_commit.sha]
       else
         parents = []
diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb
index 1611eba31da..d671867e7c7 100644
--- a/lib/gitlab/conflict/file_collection.rb
+++ b/lib/gitlab/conflict/file_collection.rb
@@ -77,8 +77,8 @@ EOM
 
       def initialize(merge_request, project)
         @merge_request = merge_request
-        @our_commit = merge_request.source_branch_head.raw.raw_commit
-        @their_commit = merge_request.target_branch_head.raw.raw_commit
+        @our_commit = merge_request.source_branch_head.raw.rugged_commit
+        @their_commit = merge_request.target_branch_head.raw.rugged_commit
         @project = project
       end
     end
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index c3eb3fc44f5..9256663f454 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -14,7 +14,7 @@ module Gitlab
 
       attr_accessor *SERIALIZE_KEYS # rubocop:disable Lint/AmbiguousOperator
 
-      delegate :tree, to: :raw_commit
+      delegate :tree, to: :rugged_commit
 
       def ==(other)
         return false unless other.is_a?(Gitlab::Git::Commit)
@@ -50,19 +50,29 @@ module Gitlab
         #
         # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/321
         def find(repo, commit_id = "HEAD")
+          # Already a commit?
           return commit_id if commit_id.is_a?(Gitlab::Git::Commit)
+
+          # A rugged reference?
+          commit_id = Gitlab::Git::Ref.dereference_object(commit_id)
           return decorate(repo, commit_id) if commit_id.is_a?(Rugged::Commit)
 
-          obj = if commit_id.is_a?(String)
-                  repo.rev_parse_target(commit_id)
-                else
-                  Gitlab::Git::Ref.dereference_object(commit_id)
-                end
+          # Some weird thing?
+          return nil unless commit_id.is_a?(String)
+
+          commit = repo.gitaly_migrate(:find_commit) do |is_enabled|
+            if is_enabled
+              repo.gitaly_commit_client.find_commit(commit_id)
+            else
+              obj = repo.rev_parse_target(commit_id)
 
-          return nil unless obj.is_a?(Rugged::Commit)
+              obj.is_a?(Rugged::Commit) ? obj : nil
+            end
+          end
 
-          decorate(repo, obj)
-        rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError, Gitlab::Git::Repository::NoRepository
+          decorate(repo, commit) if commit
+        rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError,
+               Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository
           nil
         end
 
@@ -273,11 +283,11 @@ module Gitlab
         break_rewrites = options[:break_rewrites]
         actual_options = Gitlab::Git::Diff.filter_diff_options(options)
 
-        diff = if raw_commit.parents.empty?
-                raw_commit.diff(actual_options.merge(reverse: true))
-              else
-                raw_commit.parents[0].diff(raw_commit, actual_options)
-              end
+        diff = if rugged_commit.parents.empty?
+                 rugged_commit.diff(actual_options.merge(reverse: true))
+               else
+                 rugged_commit.parents[0].diff(rugged_commit, actual_options)
+               end
 
         diff.find_similar!(break_rewrites: break_rewrites)
         diff
@@ -340,7 +350,7 @@ module Gitlab
 
       def to_patch(options = {})
         begin
-          raw_commit.to_mbox(options)
+          rugged_commit.to_mbox(options)
         rescue Rugged::InvalidError => ex
           if ex.message =~ /commit \w+ is a merge commit/i
             'Patch format is not currently supported for merge commits.'
@@ -388,6 +398,14 @@ module Gitlab
         encode! @committer_email
       end
 
+      def rugged_commit
+        @rugged_commit ||= if raw_commit.is_a?(Rugged::Commit)
+                             raw_commit
+                           else
+                             @repository.rev_parse_target(id)
+                           end
+      end
+
       private
 
       def init_from_hash(hash)
@@ -421,10 +439,10 @@ module Gitlab
         # subject from the message to make it clearer when there's one
         # available but not the other.
         @message = (commit.body.presence || commit.subject).dup
-        @authored_date = Time.at(commit.author.date.seconds)
+        @authored_date = Time.at(commit.author.date.seconds).utc
         @author_name = commit.author.name.dup
         @author_email = commit.author.email.dup
-        @committed_date = Time.at(commit.committer.date.seconds)
+        @committed_date = Time.at(commit.committer.date.seconds).utc
         @committer_name = commit.committer.name.dup
         @committer_email = commit.committer.email.dup
         @parent_ids = commit.parent_ids
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index c90ef282fdd..70177cd0fec 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -100,5 +100,9 @@ module Gitlab
       path = Rails.root.join(SERVER_VERSION_FILE)
       path.read.chomp
     end
+
+    def self.encode(s)
+      s.dup.force_encoding(Encoding::ASCII_8BIT)
+    end
   end
 end
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 793de65595f..fef77f43670 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -43,7 +43,7 @@ module Gitlab
         request = Gitaly::TreeEntryRequest.new(
           repository: @gitaly_repo,
           revision: ref,
-          path: path.dup.force_encoding(Encoding::ASCII_8BIT),
+          path: GitalyClient.encode(path),
           limit: limit.to_i
         )
 
@@ -99,8 +99,8 @@ module Gitlab
       def last_commit_for_path(revision, path)
         request = Gitaly::LastCommitForPathRequest.new(
           repository: @gitaly_repo,
-          revision: revision.force_encoding(Encoding::ASCII_8BIT),
-          path: path.to_s.force_encoding(Encoding::ASCII_8BIT)
+          revision: GitalyClient.encode(revision),
+          path: GitalyClient.encode(path.to_s)
         )
 
         gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request).commit
@@ -151,6 +151,17 @@ module Gitlab
         response.reduce("") { |memo, msg| memo << msg.data }
       end
 
+      def find_commit(revision)
+        request = Gitaly::FindCommitRequest.new(
+          repository: @gitaly_repo,
+          revision: GitalyClient.encode(revision)
+        )
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :find_commit, request)
+
+        response.commit
+      end
+
       private
 
       def commit_diff_request_params(commit, options = {})
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index 4a7e5f87ff9..c531d4b055f 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -66,6 +66,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
   describe "Commit info from gitaly commit" do
     let(:id) { 'f00' }
+    let(:parent_ids) { %w(b45 b46) }
     let(:subject) { "My commit".force_encoding('ASCII-8BIT') }
     let(:body) { subject + "My body".force_encoding('ASCII-8BIT') }
     let(:committer) do
@@ -88,7 +89,8 @@ describe Gitlab::Git::Commit, seed_helper: true do
         subject: subject,
         body: body,
         author: author,
-        committer: committer
+        committer: committer,
+        parent_ids: parent_ids
       )
     end
     let(:commit) { described_class.new(repository, gitaly_commit) }
@@ -102,6 +104,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
     it { expect(commit.author_name).to eq(author.name) }
     it { expect(commit.committer_name).to eq(committer.name) }
     it { expect(commit.committer_email).to eq(committer.email) }
+    it { expect(commit.parent_ids).to eq(parent_ids) }
 
     context 'no body' do
       let(:body) { "".force_encoding('ASCII-8BIT') }
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index f4a715e43d6..7fe698fcb18 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -112,4 +112,18 @@ describe Gitlab::GitalyClient::CommitService do
       client.tree_entries(repository, revision, path)
     end
   end
+
+  describe '#find_commit' do
+    let(:revision) { '4b825dc642cb6eb9a060e54bf8d69288fbee4904' }
+    it 'sends an RPC request' do
+      request = Gitaly::FindCommitRequest.new(
+        repository: repository_message, revision: revision
+      )
+
+      expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit)
+        .with(request, kind_of(Hash)).and_return(double(commit: nil))
+
+      described_class.new(repository).find_commit(revision)
+    end
+  end
 end
diff --git a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb
index cf2d5827306..e5793a3c0ee 100644
--- a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb
+++ b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb
@@ -6,7 +6,7 @@ require Rails.root.join('db', 'migrate', '20161124141322_migrate_process_commit_
 describe MigrateProcessCommitWorkerJobs do
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
-  let(:commit) { project.commit.raw.raw_commit }
+  let(:commit) { project.commit.raw.rugged_commit }
 
   describe 'Project' do
     describe 'find_including_path' do
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 1d38f8e5a6d..c18c635d811 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -189,7 +189,7 @@ eos
 
     it { expect(data).to be_a(Hash) }
     it { expect(data[:message]).to include('adds bar folder and branch-test text file to check Repository merged_to_root_ref method') }
-    it { expect(data[:timestamp]).to eq('2016-09-27T14:37:46+00:00') }
+    it { expect(data[:timestamp]).to eq('2016-09-27T14:37:46Z') }
     it { expect(data[:added]).to eq(["bar/branch-test.txt"]) }
     it { expect(data[:modified]).to eq([]) }
     it { expect(data[:removed]).to eq([]) }