Merge branch 'ee-gitaly-find-commit' into 'master'

[EE] Incorporate Gitaly's CommitService.FindCommit RPC

See merge request !2532

app/models/commit.rb

@@ -55,7 +55,8 @@ class Commit
     end
 
     def from_hash(hash, project)
-      new(Gitlab::Git::Commit.new(hash), project)
+      raw_commit = Gitlab::Git::Commit.new(project.repository.raw, hash)
+      new(raw_commit, project)
     end
 
     def valid_hash?(key)
@@ -320,21 +321,11 @@ class Commit
   end
 
   def raw_diffs(*args)
-    if Gitlab::GitalyClient.feature_enabled?(:commit_raw_diffs)
-      Gitlab::GitalyClient::CommitService.new(project.repository).diff_from_parent(self, *args)
-    else
     raw.diffs(*args)
   end
-  end
 
   def raw_deltas
-    @deltas ||= Gitlab::GitalyClient.migrate(:commit_deltas) do |is_enabled|
-      if is_enabled
-        Gitlab::GitalyClient::CommitService.new(project.repository).commit_deltas(self)
-      else
-        raw.deltas
-      end
-    end
+    @deltas ||= raw.deltas
   end
 
   def diffs(diff_options = nil)

app/models/merge_request_diff.rb

@@ -282,9 +282,7 @@ class MergeRequestDiff < ActiveRecord::Base
   def load_commits
     commits = st_commits.presence || merge_request_diff_commits
 
-    commits.map do |commit|
-      Commit.new(Gitlab::Git::Commit.new(commit.to_hash), merge_request.source_project)
-    end
+    commits.map { |commit| Commit.from_hash(commit.to_hash, project) }
   end
 
   def save_diffs

app/models/repository.rb

@@ -71,6 +71,8 @@ class Repository
     @raw_repository ||= initialize_raw_repository
   end
 
+  alias_method :raw, :raw_repository
+
   # Return absolute path to repository
   def path_to_repo
     @path_to_repo ||= File.expand_path(
@@ -774,7 +776,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 = []

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

lib/gitlab/cycle_analytics/plan_event_fetcher.rb

@@ -54,7 +54,7 @@ module Gitlab
       end
 
       def serialize_commit(event, commit, query)
-        commit = Commit.new(Gitlab::Git::Commit.new(commit.to_hash), @project)
+        commit = Commit.from_hash(commit.to_hash, @project)
 
         AnalyticsCommitSerializer.new(project: @project, total_time: event['total_time']).represent(commit)
       end

lib/gitlab/git/blame.rb

@@ -16,7 +16,7 @@ module Gitlab
       def each
         @blames.each do |blame|
           yield(
-            Gitlab::Git::Commit.new(blame.commit),
+            Gitlab::Git::Commit.new(@repo, blame.commit),
             blame.line
           )
         end

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)
-          return decorate(commit_id) if commit_id.is_a?(Rugged::Commit)
 
-          obj = if commit_id.is_a?(String)
-                  repo.rev_parse_target(commit_id)
+          # A rugged reference?
+          commit_id = Gitlab::Git::Ref.dereference_object(commit_id)
+          return decorate(repo, commit_id) if commit_id.is_a?(Rugged::Commit)
+
+          # 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
-                  Gitlab::Git::Ref.dereference_object(commit_id)
-                end
+              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(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
 
@@ -102,7 +112,7 @@ module Gitlab
             if is_enabled
               repo.gitaly_commit_client.between(base, head)
             else
-              repo.commits_between(base, head).map { |c| decorate(c) }
+              repo.rugged_commits_between(base, head).map { |c| decorate(repo, c) }
             end
           end
         rescue Rugged::ReferenceError
@@ -169,7 +179,7 @@ module Gitlab
           offset = actual_options[:skip]
           limit = actual_options[:max_count]
           walker.each(offset: offset, limit: limit) do |commit|
-            commits.push(decorate(commit))
+            commits.push(decorate(repo, commit))
           end
 
           walker.reset
@@ -183,27 +193,8 @@ module Gitlab
           Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options)
         end
 
-        def decorate(commit, ref = nil)
-          Gitlab::Git::Commit.new(commit, ref)
-        end
-
-        # Returns a diff object for the changes introduced by +rugged_commit+.
-        # If +rugged_commit+ doesn't have a parent, then the diff is between
-        # this commit and an empty repo.  See Repository#diff for the keys
-        # allowed in the +options+ hash.
-        def diff_from_parent(rugged_commit, options = {})
-          options ||= {}
-          break_rewrites = options[:break_rewrites]
-          actual_options = Gitlab::Git::Diff.filter_diff_options(options)
-
-          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
+        def decorate(repository, commit, ref = nil)
+          Gitlab::Git::Commit.new(repository, commit, ref)
         end
 
         # Returns the `Rugged` sorting type constant for one or more given
@@ -221,7 +212,7 @@ module Gitlab
         end
       end
 
-      def initialize(raw_commit, head = nil)
+      def initialize(repository, raw_commit, head = nil)
         raise "Nil as raw commit passed" unless raw_commit
 
         case raw_commit
@@ -229,12 +220,13 @@ module Gitlab
           init_from_hash(raw_commit)
         when Rugged::Commit
           init_from_rugged(raw_commit)
-        when Gitlab::GitalyClient::Commit
+        when Gitaly::GitCommit
           init_from_gitaly(raw_commit)
         else
           raise "Invalid raw commit type: #{raw_commit.class}"
         end
 
+        @repository = repository
         @head = head
       end
 
@@ -269,7 +261,7 @@ module Gitlab
       #
       # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/324
       def to_diff
-        diff_from_parent.patch
+        rugged_diff_from_parent.patch
       end
 
       # Returns a diff object for the changes from this commit's first parent.
@@ -277,11 +269,42 @@ module Gitlab
       # empty repo. See Repository#diff for keys allowed in the +options+
       # hash.
       def diff_from_parent(options = {})
-        Commit.diff_from_parent(raw_commit, options)
+        Gitlab::GitalyClient.migrate(:commit_raw_diffs) do |is_enabled|
+          if is_enabled
+            @repository.gitaly_commit_client.diff_from_parent(self, options)
+          else
+            rugged_diff_from_parent(options)
+          end
+        end
+      end
+
+      def rugged_diff_from_parent(options = {})
+        options ||= {}
+        break_rewrites = options[:break_rewrites]
+        actual_options = Gitlab::Git::Diff.filter_diff_options(options)
+
+        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
       end
 
       def deltas
-        @deltas ||= diff_from_parent.each_delta.map { |d| Gitlab::Git::Diff.new(d) }
+        @deltas ||= begin
+          deltas = Gitlab::GitalyClient.migrate(:commit_deltas) do |is_enabled|
+            if is_enabled
+              @repository.gitaly_commit_client.commit_deltas(self)
+            else
+              rugged_diff_from_parent.each_delta
+            end
+          end
+
+          deltas.map { |delta| Gitlab::Git::Diff.new(delta) }
+        end
       end
 
       def has_zero_stats?
@@ -309,14 +332,7 @@ module Gitlab
       end
 
       def parents
-        case raw_commit
-        when Rugged::Commit
-          raw_commit.parents.map { |c| Gitlab::Git::Commit.new(c) }
-        when Gitlab::GitalyClient::Commit
-          parent_ids.map { |oid| self.class.find(raw_commit.repository, oid) }.compact
-        else
-          raise NotImplementedError, "commit source doesn't support #parents"
-        end
+        parent_ids.map { |oid| self.class.find(@repository, oid) }.compact
       end
 
       # Get the gpg signature of this commit.
@@ -334,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.'
@@ -382,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)
@@ -415,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

lib/gitlab/git/commit_stats.rb

@@ -16,7 +16,7 @@ module Gitlab
         @deletions = 0
         @total = 0
 
-        diff = commit.diff_from_parent
+        diff = commit.rugged_diff_from_parent
 
         diff.each_patch do |p|
           # TODO: Use the new Rugged convenience methods when they're released

lib/gitlab/git/diff_collection.rb

@@ -28,7 +28,6 @@ module Gitlab
         @limits = self.class.collection_limits(options)
         @enforce_limits = !!options.fetch(:limits, true)
         @expanded = !!options.fetch(:expanded, true)
-        @from_gitaly = options.fetch(:from_gitaly, false)
 
         @line_count = 0
         @byte_count = 0
@@ -44,7 +43,7 @@ module Gitlab
         return if @iterator.nil?
 
         Gitlab::GitalyClient.migrate(:commit_raw_diffs) do |is_enabled|
-          if is_enabled && @from_gitaly
+          if is_enabled && @iterator.is_a?(Gitlab::GitalyClient::DiffStitcher)
             each_gitaly_patch(&block)
           else
             each_rugged_patch(&block)

lib/gitlab/git/repository.rb

@@ -58,11 +58,6 @@ module Gitlab
         end
       end
 
-      # Alias to old method for compatibility
-      def raw
-        rugged
-      end
-
       def rugged
         @rugged ||= circuit_breaker.perform do
           Rugged::Repository.new(path, alternates: alternate_object_directories)
@@ -303,7 +298,22 @@ module Gitlab
       #   )
       #
       def log(options)
-        raw_log(options).map { |c| Commit.decorate(c) }
+        default_options = {
+          limit: 10,
+          offset: 0,
+          path: nil,
+          follow: false,
+          skip_merges: false,
+          disable_walk: false,
+          after: nil,
+          before: nil
+        }
+
+        options = default_options.merge(options)
+        options[:limit] ||= 0
+        options[:offset] ||= 0
+
+        raw_log(options).map { |c| Commit.decorate(self, c) }
       end
 
       def count_commits(options)
@@ -330,7 +340,9 @@ module Gitlab
       # Return a collection of Rugged::Commits between the two revspec arguments.
       # See http://git-scm.com/docs/git-rev-parse.html#_specifying_revisions for
       # a detailed list of valid arguments.
-      def commits_between(from, to)
+      #
+      # Gitaly note: JV: to be deprecated in favor of Commit.between
+      def rugged_commits_between(from, to)
         walker = Rugged::Walker.new(rugged)
         walker.sorting(Rugged::SORT_NONE | Rugged::SORT_REVERSE)
 
@@ -714,20 +726,6 @@ module Gitlab
       end
 
       def raw_log(options)
-        default_options = {
-          limit: 10,
-          offset: 0,
-          path: nil,
-          follow: false,
-          skip_merges: false,
-          disable_walk: false,
-          after: nil,
-          before: nil
-        }
-
-        options = default_options.merge(options)
-        options[:limit] ||= 0
-        options[:offset] ||= 0
         actual_ref = options[:ref] || root_ref
         begin
           sha = sha_from_ref(actual_ref)

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

lib/gitlab/gitaly_client/commit.rb

-module Gitlab
-  module GitalyClient
-    class Commit
-      attr_reader :repository, :gitaly_commit
-
-      delegate :id, :subject, :body, :author, :committer, :parent_ids, to: :gitaly_commit
-
-      def initialize(repository, gitaly_commit)
-        @repository = repository
-        @gitaly_commit = gitaly_commit
-      end
-    end
-  end
-end

lib/gitlab/gitaly_client/commit_service.rb

@@ -29,22 +29,21 @@ module Gitlab
 
         request = Gitaly::CommitDiffRequest.new(request_params)
         response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request)
-        Gitlab::Git::DiffCollection.new(GitalyClient::DiffStitcher.new(response), options.merge(from_gitaly: true))
+        GitalyClient::DiffStitcher.new(response)
       end
 
       def commit_deltas(commit)
         request = Gitaly::CommitDeltaRequest.new(commit_diff_request_params(commit))
         response = GitalyClient.call(@repository.storage, :diff_service, :commit_delta, request)
-        response.flat_map do |msg|
-          msg.deltas.map { |d| Gitlab::Git::Diff.new(d) }
-        end
+
+        response.flat_map { |msg| msg.deltas }
       end
 
       def tree_entry(ref, path, limit = nil)
         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
         )
 
@@ -100,15 +99,14 @@ 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
         return unless gitaly_commit
 
-        commit = GitalyClient::Commit.new(@repository, gitaly_commit)
-        Gitlab::Git::Commit.new(commit)
+        Gitlab::Git::Commit.new(@repository, gitaly_commit)
       end
 
       def between(from, to)
@@ -153,10 +151,21 @@ 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 = {})
-        parent_id = commit.parents[0]&.id || EMPTY_TREE_ID
+        parent_id = commit.parent_ids.first || EMPTY_TREE_ID
 
         {
           repository: @gitaly_repo,
@@ -169,8 +178,7 @@ module Gitlab
       def consume_commits_response(response)
         response.flat_map do |message|
           message.commits.map do |gitaly_commit|
-            commit = GitalyClient::Commit.new(@repository, gitaly_commit)
-            Gitlab::Git::Commit.new(commit)
+            Gitlab::Git::Commit.new(@repository, gitaly_commit)
           end
         end
       end

lib/gitlab/gitaly_client/ref_service.rb

@@ -16,8 +16,7 @@ module Gitlab
 
         response.flat_map do |message|
           message.branches.map do |branch|
-            gitaly_commit = GitalyClient::Commit.new(@repository, branch.target)
-            target_commit = Gitlab::Git::Commit.decorate(gitaly_commit)
+            target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target)
             Gitlab::Git::Branch.new(@repository, branch.name, branch.target.id, target_commit)
           end
         end
@@ -102,8 +101,7 @@ module Gitlab
         response.flat_map do |message|
           message.tags.map do |gitaly_tag|
             if gitaly_tag.target_commit.present?
-              commit = GitalyClient::Commit.new(@repository, gitaly_tag.target_commit)
-              gitaly_commit = Gitlab::Git::Commit.new(commit)
+              gitaly_commit = Gitlab::Git::Commit.decorate(@repository, gitaly_tag.target_commit)
             end
 
             Gitlab::Git::Tag.new(
@@ -141,7 +139,7 @@ module Gitlab
           committer_email: response.commit_committer.email.dup
         }
 
-        Gitlab::Git::Commit.decorate(hash)
+        Gitlab::Git::Commit.decorate(@repository, hash)
       end
     end
   end

spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb

@@ -170,7 +170,7 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits do
     context 'when the merge request diffs are Rugged::Patch instances' do
       let(:commits) { merge_request_diff.commits.map(&:to_hash) }
       let(:first_commit) { merge_request.project.repository.commit(merge_request_diff.head_commit_sha) }
-      let(:diffs) { first_commit.diff_from_parent.patches }
+      let(:diffs) { first_commit.rugged_diff_from_parent.patches }
       let(:expected_diffs) { [] }
 
       include_examples 'updated MR diff'
@@ -179,7 +179,7 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits do
     context 'when the merge request diffs are Rugged::Diff::Delta instances' do
       let(:commits) { merge_request_diff.commits.map(&:to_hash) }
       let(:first_commit) { merge_request.project.repository.commit(merge_request_diff.head_commit_sha) }
-      let(:diffs) { first_commit.diff_from_parent.deltas }
+      let(:diffs) { first_commit.rugged_diff_from_parent.deltas }
       let(:expected_diffs) { [] }
 
       include_examples 'updated MR diff'

spec/lib/gitlab/git/commit_spec.rb

@@ -2,7 +2,7 @@ require "spec_helper"
 
 describe Gitlab::Git::Commit, seed_helper: true do
   let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
-  let(:commit) { Gitlab::Git::Commit.find(repository, SeedRepo::Commit::ID) }
+  let(:commit) { described_class.find(repository, SeedRepo::Commit::ID) }
   let(:rugged_commit) do
     repository.rugged.lookup(SeedRepo::Commit::ID)
   end
@@ -24,7 +24,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
       }
 
       @parents = [repo.head.target]
-      @gitlab_parents = @parents.map { |c| Gitlab::Git::Commit.decorate(c) }
+      @gitlab_parents = @parents.map { |c| described_class.decorate(repository, c) }
       @tree = @parents.first.tree
 
       sha = Rugged::Commit.create(
@@ -38,7 +38,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
       )
 
       @raw_commit = repo.lookup(sha)
-      @commit = Gitlab::Git::Commit.new(@raw_commit)
+      @commit = described_class.new(repository, @raw_commit)
     end
 
     it { expect(@commit.short_id).to eq(@raw_commit.oid[0..10]) }
@@ -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,10 +89,11 @@ 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(Gitlab::GitalyClient::Commit.new(repository, gitaly_commit)) }
+    let(:commit) { described_class.new(repository, gitaly_commit) }
 
     it { expect(commit.short_id).to eq(id[0..10]) }
     it { expect(commit.id).to eq(id) }
@@ -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') }
@@ -113,45 +116,45 @@ describe Gitlab::Git::Commit, seed_helper: true do
   context 'Class methods' do
     describe '.find' do
       it "should return first head commit if without params" do
-        expect(Gitlab::Git::Commit.last(repository).id).to eq(
-          repository.raw.head.target.oid
+        expect(described_class.last(repository).id).to eq(
+          repository.rugged.head.target.oid
         )
       end
 
       it "should return valid commit" do
-        expect(Gitlab::Git::Commit.find(repository, SeedRepo::Commit::ID)).to be_valid_commit
+        expect(described_class.find(repository, SeedRepo::Commit::ID)).to be_valid_commit
       end
 
       it "should return valid commit for tag" do
-        expect(Gitlab::Git::Commit.find(repository, 'v1.0.0').id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+        expect(described_class.find(repository, 'v1.0.0').id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
       end
 
       it "should return nil for non-commit ids" do
         blob = Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, "files/ruby/popen.rb")
-        expect(Gitlab::Git::Commit.find(repository, blob.id)).to be_nil
+        expect(described_class.find(repository, blob.id)).to be_nil
       end
 
       it "should return nil for parent of non-commit object" do
         blob = Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, "files/ruby/popen.rb")
-        expect(Gitlab::Git::Commit.find(repository, "#{blob.id}^")).to be_nil
+        expect(described_class.find(repository, "#{blob.id}^")).to be_nil
       end
 
       it "should return nil for nonexisting ids" do
-        expect(Gitlab::Git::Commit.find(repository, "+123_4532530XYZ")).to be_nil
+        expect(described_class.find(repository, "+123_4532530XYZ")).to be_nil
       end
 
       context 'with broken repo' do
         let(:repository) { Gitlab::Git::Repository.new('default', TEST_BROKEN_REPO_PATH) }
 
         it 'returns nil' do
-          expect(Gitlab::Git::Commit.find(repository, SeedRepo::Commit::ID)).to be_nil
+          expect(described_class.find(repository, SeedRepo::Commit::ID)).to be_nil
         end
       end
     end
 
     describe '.last_for_path' do
       context 'no path' do
-        subject { Gitlab::Git::Commit.last_for_path(repository, 'master') }
+        subject { described_class.last_for_path(repository, 'master') }
 
         describe '#id' do
           subject { super().id }
@@ -160,7 +163,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
       end
 
       context 'path' do
-        subject { Gitlab::Git::Commit.last_for_path(repository, 'master', 'files/ruby') }
+        subject { described_class.last_for_path(repository, 'master', 'files/ruby') }
 
         describe '#id' do
           subject { super().id }
@@ -169,7 +172,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
       end
 
       context 'ref + path' do
-        subject { Gitlab::Git::Commit.last_for_path(repository, SeedRepo::Commit::ID, 'encoding') }
+        subject { described_class.last_for_path(repository, SeedRepo::Commit::ID, 'encoding') }
 
         describe '#id' do
           subject { super().id }
@@ -181,7 +184,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
     describe '.where' do
       context 'path is empty string' do
         subject do
-          commits = Gitlab::Git::Commit.where(
+          commits = described_class.where(
             repo: repository,
             ref: 'master',
             path: '',
@@ -199,7 +202,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
       context 'path is nil' do
         subject do
-          commits = Gitlab::Git::Commit.where(
+          commits = described_class.where(
             repo: repository,
             ref: 'master',
             path: nil,
@@ -217,7 +220,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
       context 'ref is branch name' do
         subject do
-          commits = Gitlab::Git::Commit.where(
+          commits = described_class.where(
             repo: repository,
             ref: 'master',
             path: 'files',
@@ -237,7 +240,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
       context 'ref is commit id' do
         subject do
-          commits = Gitlab::Git::Commit.where(
+          commits = described_class.where(
             repo: repository,
             ref: "874797c3a73b60d2187ed6e2fcabd289ff75171e",
             path: 'files',
@@ -257,7 +260,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
       context 'ref is tag' do
         subject do
-          commits = Gitlab::Git::Commit.where(
+          commits = described_class.where(
             repo: repository,
             ref: 'v1.0.0',
             path: 'files',
@@ -278,7 +281,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
     describe '.between' do
       subject do
-        commits = Gitlab::Git::Commit.between(repository, SeedRepo::Commit::PARENT_ID, SeedRepo::Commit::ID)
+        commits = described_class.between(repository, SeedRepo::Commit::PARENT_ID, SeedRepo::Commit::ID)
         commits.map { |c| c.id }
       end
 
@@ -294,12 +297,12 @@ describe Gitlab::Git::Commit, seed_helper: true do
         it 'should return a return a collection of commits' do
           commits = described_class.find_all(repository)
 
-          expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) )
+          expect(commits).to all( be_a_kind_of(described_class) )
         end
 
         context 'max_count' do
           subject do
-            commits = Gitlab::Git::Commit.find_all(
+            commits = described_class.find_all(
               repository,
               max_count: 50
             )
@@ -322,7 +325,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
         context 'ref + max_count + skip' do
           subject do
-            commits = Gitlab::Git::Commit.find_all(
+            commits = described_class.find_all(
               repository,
               ref: 'master',
               max_count: 50,
@@ -374,7 +377,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
   end
 
   describe '#init_from_rugged' do
-    let(:gitlab_commit) { Gitlab::Git::Commit.new(rugged_commit) }
+    let(:gitlab_commit) { described_class.new(repository, rugged_commit) }
     subject { gitlab_commit }
 
     describe '#id' do
@@ -384,7 +387,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
   end
 
   describe '#init_from_hash' do
-    let(:commit) { Gitlab::Git::Commit.new(sample_commit_hash) }
+    let(:commit) { described_class.new(repository, sample_commit_hash) }
     subject { commit }
 
     describe '#id' do
@@ -451,7 +454,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
   end
 
   describe '#ref_names' do
-    let(:commit) { Gitlab::Git::Commit.find(repository, 'master') }
+    let(:commit) { described_class.find(repository, 'master') }
     subject { commit.ref_names(repository) }
 
     it 'has 1 element' do

spec/lib/gitlab/git/repository_spec.rb

@@ -22,7 +22,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
   describe "Respond to" do
     subject { repository }
 
-    it { is_expected.to respond_to(:raw) }
     it { is_expected.to respond_to(:rugged) }
     it { is_expected.to respond_to(:root_ref) }
     it { is_expected.to respond_to(:tags) }
@@ -506,17 +505,22 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe "#log" do
-    commit_with_old_name = nil
-    commit_with_new_name = nil
-    rename_commit = nil
+    let(:commit_with_old_name) do
+      Gitlab::Git::Commit.decorate(repository, @commit_with_old_name_id)
+    end
+    let(:commit_with_new_name) do
+      Gitlab::Git::Commit.decorate(repository, @commit_with_new_name_id)
+    end
+    let(:rename_commit) do
+      Gitlab::Git::Commit.decorate(repository, @rename_commit_id)
+    end
 
     before(:context) do
       # Add new commits so that there's a renamed file in the commit history
       repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH).rugged
-
-      commit_with_old_name = Gitlab::Git::Commit.decorate(new_commit_edit_old_file(repo))
-      rename_commit = Gitlab::Git::Commit.decorate(new_commit_move_file(repo))
-      commit_with_new_name = Gitlab::Git::Commit.decorate(new_commit_edit_new_file(repo))
+      @commit_with_old_name_id = new_commit_edit_old_file(repo)
+      @rename_commit_id = new_commit_move_file(repo)
+      @commit_with_new_name_id = new_commit_edit_new_file(repo)
     end
 
     after(:context) do
@@ -755,7 +759,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
       let(:options) { { ref: 'master', path: ['PROCESS.md', 'README.md'] } }
 
       def commit_files(commit)
-        commit.diff_from_parent.deltas.flat_map do |delta|
+        commit.rugged_diff_from_parent.deltas.flat_map do |delta|
           [delta.old_file[:path], delta.new_file[:path]].uniq.compact
         end
       end
@@ -771,13 +775,13 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe "#commits_between" do
+  describe "#rugged_commits_between" do
     context 'two SHAs' do
       let(:first_sha) { 'b0e52af38d7ea43cf41d8a6f2471351ac036d6c9' }
       let(:second_sha) { '0e50ec4d3c7ce42ab74dda1d422cb2cbffe1e326' }
 
       it 'returns the number of commits between' do
-        expect(repository.commits_between(first_sha, second_sha).count).to eq(3)
+        expect(repository.rugged_commits_between(first_sha, second_sha).count).to eq(3)
       end
     end
 
@@ -786,11 +790,11 @@ describe Gitlab::Git::Repository, seed_helper: true do
       let(:branch) { 'master' }
 
       it 'returns the number of commits between a sha and a branch' do
-        expect(repository.commits_between(sha, branch).count).to eq(5)
+        expect(repository.rugged_commits_between(sha, branch).count).to eq(5)
       end
 
       it 'returns the number of commits between a branch and a sha' do
-        expect(repository.commits_between(branch, sha).count).to eq(0) # sha is before branch
+        expect(repository.rugged_commits_between(branch, sha).count).to eq(0) # sha is before branch
       end
     end
 
@@ -799,7 +803,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
       let(:second_branch) { 'master' }
 
       it 'returns the number of commits between' do
-        expect(repository.commits_between(first_branch, second_branch).count).to eq(17)
+        expect(repository.rugged_commits_between(first_branch, second_branch).count).to eq(17)
       end
     end
   end

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::CommitService do
 
     context 'when a commit does not have a parent' do
       it 'sends an RPC request with empty tree ref as left commit' do
-        initial_commit = project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863')
+        initial_commit = project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863').raw
         request        = Gitaly::CommitDiffRequest.new(
           repository: repository_message,
           left_commit_id: '4b825dc642cb6eb9a060e54bf8d69288fbee4904',
@@ -46,18 +46,10 @@ describe Gitlab::GitalyClient::CommitService do
       end
     end
 
-    it 'returns a Gitlab::Git::DiffCollection' do
+    it 'returns a Gitlab::GitalyClient::DiffStitcher' do
       ret = client.diff_from_parent(commit)
 
-      expect(ret).to be_kind_of(Gitlab::Git::DiffCollection)
-    end
-
-    it 'passes options to Gitlab::Git::DiffCollection' do
-      options = { max_files: 31, max_lines: 13, from_gitaly: true }
-
-      expect(Gitlab::Git::DiffCollection).to receive(:new).with(kind_of(Enumerable), options)
-
-      client.diff_from_parent(commit, options)
+      expect(ret).to be_kind_of(Gitlab::GitalyClient::DiffStitcher)
     end
   end
 
@@ -120,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

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

spec/models/commit_spec.rb

@@ -33,7 +33,6 @@ describe Commit do
 
   describe '#to_reference' do
     let(:project) { create(:project, :repository, path: 'sample-project') }
-    let(:commit)  { project.commit }
 
     it 'returns a String reference to the object' do
       expect(commit.to_reference).to eq commit.id
@@ -47,7 +46,6 @@ describe Commit do
 
   describe '#reference_link_text' do
     let(:project) { create(:project, :repository, path: 'sample-project') }
-    let(:commit)  { project.commit }
 
     it 'returns a String reference to the object' do
       expect(commit.reference_link_text).to eq commit.short_id
@@ -191,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([]) }