Merge branch 'feature/migrate-repository-diff' into 'master'

Migrate Gitlab::Git::Repository#diff to Gitaly

Closes gitaly#524

See merge request gitlab-org/gitlab-ce!14251

4 files changed, 60 insertions, 15 deletions

diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 1dabd4ebdd0..fcac85ff892 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -5,7 +5,7 @@ module Gitlab
 
       delegate :new_file?, :deleted_file?, :renamed_file?,
         :old_path, :new_path, :a_mode, :b_mode, :mode_changed?,
-        :submodule?, :expanded?, :too_large?, :collapsed?, :line_count, to: :diff, prefix: false
+        :submodule?, :expanded?, :too_large?, :collapsed?, :line_count, :has_binary_notice?, to: :diff, prefix: false
 
       # Finding a viewer for a diff file happens based only on extension and whether the
       # diff file blobs are binary or text, which means 1 diff file should only be matched by 1 viewer,
@@ -166,7 +166,7 @@ module Gitlab
       end
 
       def binary?
-        old_blob&.binary? || new_blob&.binary?
+        has_binary_notice? || old_blob&.binary? || new_blob&.binary?
       end
 
       def text?
diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb
index a23c8cf0dd1..096301d300f 100644
--- a/lib/gitlab/git/diff.rb
+++ b/lib/gitlab/git/diff.rb
@@ -206,6 +206,10 @@ module Gitlab
         Diff.binary_message(@old_path, @new_path)
       end
 
+      def has_binary_notice?
+        @diff.start_with?('Binary')
+      end
+
       private
 
       def init_from_rugged(rugged)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 0be35034d24..10ba29acbd1 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -475,7 +475,15 @@ module Gitlab
       # diff options.  The +options+ hash can also include :break_rewrites to
       # split larger rewrites into delete/add pairs.
       def diff(from, to, options = {}, *paths)
-        Gitlab::Git::DiffCollection.new(diff_patches(from, to, options, *paths), options)
+        iterator = gitaly_migrate(:diff_between) do |is_enabled|
+          if is_enabled
+            gitaly_commit_client.diff(from, to, options.merge(paths: paths))
+          else
+            diff_patches(from, to, options, *paths)
+          end
+        end
+
+        Gitlab::Git::DiffCollection.new(iterator, options)
       end
 
       # Returns a RefName for a given SHA
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index b536eb1868c..cf3a3554552 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -32,20 +32,38 @@ module Gitlab
         GitalyClient.call(@repository.storage, :commit_service, :commit_is_ancestor, request).value
       end
 
+      def diff(from, to, options = {})
+        from_id = case from
+                  when NilClass
+                    EMPTY_TREE_ID
+                  when Rugged::Commit
+                    from.oid
+                  else
+                    from
+                  end
+
+        to_id = case to
+                when NilClass
+                  EMPTY_TREE_ID
+                when Rugged::Commit
+                  to.oid
+                else
+                  to
+                end
+
+        request_params = diff_between_commits_request_params(from_id, to_id, options)
+
+        call_commit_diff(request_params, options)
+      end
+
       def diff_from_parent(commit, options = {})
-        request_params = commit_diff_request_params(commit, options)
-        request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false)
-        request_params[:enforce_limits] = options.fetch(:limits, true)
-        request_params[:collapse_diffs] = request_params[:enforce_limits] || !options.fetch(:expanded, true)
-        request_params.merge!(Gitlab::Git::DiffCollection.collection_limits(options).to_h)
+        request_params = diff_from_parent_request_params(commit, options)
 
-        request = Gitaly::CommitDiffRequest.new(request_params)
-        response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request)
-        GitalyClient::DiffStitcher.new(response)
+        call_commit_diff(request_params, options)
       end
 
       def commit_deltas(commit)
-        request = Gitaly::CommitDeltaRequest.new(commit_diff_request_params(commit))
+        request = Gitaly::CommitDeltaRequest.new(diff_from_parent_request_params(commit))
         response = GitalyClient.call(@repository.storage, :diff_service, :commit_delta, request)
 
         response.flat_map { |msg| msg.deltas }
@@ -214,13 +232,28 @@ module Gitlab
 
       private
 
-      def commit_diff_request_params(commit, options = {})
+      def call_commit_diff(request_params, options = {})
+        request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false)
+        request_params[:enforce_limits] = options.fetch(:limits, true)
+        request_params[:collapse_diffs] = request_params[:enforce_limits] || !options.fetch(:expanded, true)
+        request_params.merge!(Gitlab::Git::DiffCollection.collection_limits(options).to_h)
+
+        request = Gitaly::CommitDiffRequest.new(request_params)
+        response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request)
+        GitalyClient::DiffStitcher.new(response)
+      end
+
+      def diff_from_parent_request_params(commit, options = {})
         parent_id = commit.parent_ids.first || EMPTY_TREE_ID
 
+        diff_between_commits_request_params(parent_id, commit.id, options)
+      end
+
+      def diff_between_commits_request_params(from_id, to_id, options)
         {
           repository: @gitaly_repo,
-          left_commit_id: parent_id,
-          right_commit_id: commit.id,
+          left_commit_id: from_id,
+          right_commit_id: to_id,
           paths: options.fetch(:paths, []).map { |path| GitalyClient.encode(path) }
         }
       end