Move `deltas` and `diff_from_parents` logic to Gitlab::Git::Commit

This helps keep the abstraction layers simpler, and also keep the
interface of those methods consistent, in case of implementation
changes.

7 files changed, 46 insertions, 53 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 96605c9168b..638fddc5d3d 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -321,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
+    raw.diffs(*args)
   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)
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index b08d7e8fec3..c3eb3fc44f5 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -187,25 +187,6 @@ module Gitlab
           Gitlab::Git::Commit.new(repository, 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
-        end
-
         # Returns the `Rugged` sorting type constant for one or more given
         # sort types. Valid keys are `:none`, `:topo`, and `:date`, or an array
         # containing more than one of them. `:date` uses a combination of date and
@@ -270,19 +251,50 @@ 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.
       # If there is no parent, then the diff is between this commit and an
-      # empty repo.  See Repository#diff for keys allowed in the +options+
+      # 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 raw_commit.parents.empty?
+                raw_commit.diff(actual_options.merge(reverse: true))
+              else
+                raw_commit.parents[0].diff(raw_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?
diff --git a/lib/gitlab/git/commit_stats.rb b/lib/gitlab/git/commit_stats.rb
index 57c29ad112c..00acb4763e9 100644
--- a/lib/gitlab/git/commit_stats.rb
+++ b/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
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 2a97a025e58..793de65595f 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -29,15 +29,14 @@ 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)
diff --git a/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb b/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb
index 18843cbe992..f4dfa53f050 100644
--- a/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb
+++ b/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'
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 23a77865aff..858616117d5 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -759,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
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index a6c48c178b3..f4a715e43d6 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -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