Merge branch 'gitaly-9-5-2-patch' into '9-5-stable-patch-3'

Refactor Gitlab::Git::Commit to include a repository and add missing specs

See merge request !13815

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)

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

@@ -64,6 +64,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(

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

@@ -51,7 +51,7 @@ module Gitlab
         # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/321
         def find(repo, commit_id = "HEAD")
           return commit_id if commit_id.is_a?(Gitlab::Git::Commit)
-          return decorate(commit_id) if commit_id.is_a?(Rugged::Commit)
+          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)
@@ -61,7 +61,7 @@ module Gitlab
 
           return nil unless obj.is_a?(Rugged::Commit)
 
-          decorate(obj)
+          decorate(repo, obj)
         rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError, Gitlab::Git::Repository::NoRepository
           nil
         end
@@ -102,7 +102,7 @@ module Gitlab
             if is_enabled
               repo.gitaly_commit_client.between(base, head)
             else
-              repo.rugged_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 +169,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,8 +183,8 @@ module Gitlab
           Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options)
         end
 
-        def decorate(commit, ref = nil)
-          Gitlab::Git::Commit.new(commit, ref)
+        def decorate(repository, commit, ref = nil)
+          Gitlab::Git::Commit.new(repository, commit, ref)
         end
 
         # Returns a diff object for the changes introduced by +rugged_commit+.
@@ -231,7 +231,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
@@ -239,12 +239,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
 
@@ -319,14 +320,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
 
       def stats

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

@@ -321,7 +321,7 @@ module Gitlab
         options[:limit] ||= 0
         options[:offset] ||= 0
 
-        raw_log(options).map { |c| Commit.decorate(c) }
+        raw_log(options).map { |c| Commit.decorate(self, c) }
       end
 
       def count_commits(options)

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

@@ -107,8 +107,7 @@ module Gitlab
         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)
@@ -170,7 +169,7 @@ module Gitlab
       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,
@@ -183,8 +182,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/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]) }
@@ -91,7 +91,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
         committer: committer
       )
     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) }
@@ -113,45 +113,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(
+        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 +160,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 +169,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 +181,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 +199,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 +217,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 +237,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 +257,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 +278,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 +294,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 +322,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 +374,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 +384,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 +451,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
@@ -461,6 +461,17 @@ describe Gitlab::Git::Commit, seed_helper: true do
     it { is_expected.not_to include("feature") }
   end
 
+  describe '#rugged_commit' do
+    let(:raw_commit) { { id: SeedRepo::Commit::ID } }
+    let(:commit) { described_class.decorate(repository, raw_commit) }
+    subject { commit.send(:rugged_commit) }
+
+    it 'returns a rugged commit based on the id of a non-rugged raw commit' do
+      expect(subject.class).to be(Rugged::Commit)
+      expect(subject).to eq(rugged_commit)
+    end
+  end
+
   def sample_commit_hash
     {
       author_email: "dmitriy.zaporozhets@gmail.com",

spec/lib/gitlab/git/repository_spec.rb

@@ -511,17 +511,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

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',

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