feat(diff): refactor Git::Diff to separate concerns and improve AP

jcouball · jcouball · commit e22eb10bf2e4 · 2025-07-02T16:27:27.000-07:00
This pull request refactors the Git::Diff decomposing it into new, more focused
classes, while backward compatibility is maintained via a deprecated facade.
diff --git a/lib/git/base.rb b/lib/git/base.rb
@@ -782,6 +782,27 @@ def merge_base(*args)
       shas.map { |sha| gcommit(sha) }
     end
 
+# Returns a Git::Diff::Stats object for accessing diff statistics.
+    #
+    # @param objectish [String] The first commit or object to compare. Defaults to 'HEAD'.
+    # @param obj2 [String, nil] The second commit or object to compare.
+    # @return [Git::Diff::Stats]
+    def diff_stats(objectish = 'HEAD', obj2 = nil)
+      Git::DiffStats.new(self, objectish, obj2)
+    end
+
+    # Returns a Git::Diff::PathStatus object for accessing the name-status report.
+    #
+    # @param objectish [String] The first commit or object to compare. Defaults to 'HEAD'.
+    # @param obj2 [String, nil] The second commit or object to compare.
+    # @return [Git::Diff::PathStatus]
+    def diff_path_status(objectish = 'HEAD', obj2 = nil)
+      Git::DiffPathStatus.new(self, objectish, obj2)
+    end
+
+    # Provided for backwards compatibility
+    alias diff_name_status diff_path_status
+
     private
 
     # Normalize options before they are sent to Git::Base.new
diff --git a/lib/git/diff.rb b/lib/git/diff.rb
@@ -1,8 +1,10 @@
 # frozen_string_literal: true
 
-module Git
+require_relative 'diff_path_status'
+require_relative 'diff_stats'
 
-  # object that holds the last X commits on given branch
+module Git
+  # object that holds the diff between two commits
   class Diff
     include Enumerable
 
@@ -12,63 +14,68 @@ def initialize(base, from = nil, to = nil)
       @to = to && to.to_s
 
       @path = nil
-      @full_diff = nil
       @full_diff_files = nil
-      @stats = nil
     end
     attr_reader :from, :to
 
-    def name_status
-      cache_name_status
-    end
-
     def path(path)
       @path = path
       self
     end
 
-    def size
-      cache_stats
-      @stats[:total][:files]
+    def patch
+      @base.lib.diff_full(@from, @to, { path_limiter: @path })
     end
+    alias_method :to_s, :patch
 
-    def lines
-      cache_stats
-      @stats[:total][:lines]
+    def [](key)
+      process_full
+      @full_diff_files.assoc(key)[1]
     end
 
-    def deletions
-      cache_stats
-      @stats[:total][:deletions]
+    def each(&block)
+      process_full
+      @full_diff_files.map { |file| file[1] }.each(&block)
     end
 
-    def insertions
-      cache_stats
-      @stats[:total][:insertions]
+    #
+    # DEPRECATED METHODS
+    #
+
+    def name_status
+      Git::Deprecation.warn("Git::Diff#name_status is deprecated. Use Git::Base#diff_path_status instead.")
+      path_status_provider.to_h
     end
 
-    def stats
-      cache_stats
-      @stats
+    def size
+      Git::Deprecation.warn("Git::Diff#size is deprecated. Use Git::Base#diff_stats(...).total[:files] instead.")
+      stats_provider.total[:files]
     end
 
-    # if file is provided and is writable, it will write the patch into the file
-    def patch(file = nil)
-      cache_full
-      @full_diff
+
+
+    def lines
+      Git::Deprecation.warn("Git::Diff#lines is deprecated. Use Git::Base#diff_stats(...).lines instead.")
+      stats_provider.lines
     end
-    alias_method :to_s, :patch
 
-    # enumerable methods
+    def deletions
+      Git::Deprecation.warn("Git::Diff#deletions is deprecated. Use Git::Base#diff_stats(...).deletions instead.")
+      stats_provider.deletions
+    end
 
-    def [](key)
-      process_full
-      @full_diff_files.assoc(key)[1]
+    def insertions
+      Git::Deprecation.warn("Git::Diff#insertions is deprecated. Use Git::Base#diff_stats(...).insertions instead.")
+      stats_provider.insertions
     end
 
-    def each(&block) # :yields: each Git::DiffFile in turn
-      process_full
-      @full_diff_files.map { |file| file[1] }.each(&block)
+    def stats
+      Git::Deprecation.warn("Git::Diff#stats is deprecated. Use Git::Base#diff_stats instead.")
+      # CORRECTED: Re-create the original hash structure for backward compatibility
+      {
+        files: stats_provider.files,
+        total: stats_provider.total
+      }
     end
 
     class DiffFile
@@ -102,56 +109,48 @@ def blob(type = :dst)
 
     private
 
-      def cache_full
-        @full_diff ||= @base.lib.diff_full(@from, @to, {:path_limiter => @path})
-      end
-
-      def process_full
-        return if @full_diff_files
-        cache_full
-        @full_diff_files = process_full_diff
-      end
+    def process_full
+      return if @full_diff_files
+      @full_diff_files = process_full_diff
+    end
 
-      def cache_stats
-        @stats ||=  @base.lib.diff_stats(@from, @to, {:path_limiter => @path})
-      end
+    # CORRECTED: Pass the @path variable to the new objects
+    def path_status_provider
+      @path_status_provider ||= Git::DiffPathStatus.new(@base, @from, @to, @path)
+    end
 
-      def cache_name_status
-        @name_status ||= @base.lib.diff_name_status(@from, @to, {:path => @path})
-      end
+    # CORRECTED: Pass the @path variable to the new objects
+    def stats_provider
+      @stats_provider ||= Git::DiffStats.new(@base, @from, @to, @path)
+    end
 
-      # break up @diff_full
-      def process_full_diff
-        defaults = {
-          :mode => '',
-          :src => '',
-          :dst => '',
-          :type => 'modified'
-        }
-        final = {}
-        current_file = nil
-        @full_diff.split("\n").each do |line|
-          if m = %r{\Adiff --git ("?)a/(.+?)\1 ("?)b/(.+?)\3\z}.match(line)
-            current_file = Git::EscapedPath.new(m[2]).unescape
-            final[current_file] = defaults.merge({:patch => line, :path => current_file})
-          else
-            if m = /^index ([0-9a-f]{4,40})\.\.([0-9a-f]{4,40})( ......)*/.match(line)
-              final[current_file][:src] = m[1]
-              final[current_file][:dst] = m[2]
-              final[current_file][:mode] = m[3].strip if m[3]
-            end
-            if m = /^([[:alpha:]]*?) file mode (......)/.match(line)
-              final[current_file][:type] = m[1]
-              final[current_file][:mode] = m[2]
-            end
-            if m = /^Binary files /.match(line)
-              final[current_file][:binary] = true
-            end
-            final[current_file][:patch] << "\n" + line
+    def process_full_diff
+      defaults = {
+        mode: '', src: '', dst: '', type: 'modified'
+      }
+      final = {}
+      current_file = nil
+      patch.split("\n").each do |line|
+        if m = %r{\Adiff --git ("?)a/(.+?)\1 ("?)b/(.+?)\3\z}.match(line)
+          current_file = Git::EscapedPath.new(m[2]).unescape
+          final[current_file] = defaults.merge({ patch: line, path: current_file })
+        else
+          if m = /^index ([0-9a-f]{4,40})\.\.([0-9a-f]{4,40})( ......)*/.match(line)
+            final[current_file][:src] = m[1]
+            final[current_file][:dst] = m[2]
+            final[current_file][:mode] = m[3].strip if m[3]
+          end
+          if m = /^([[:alpha:]]*?) file mode (......)/.match(line)
+            final[current_file][:type] = m[1]
+            final[current_file][:mode] = m[2]
+          end
+          if m = /^Binary files /.match(line)
+            final[current_file][:binary] = true
           end
+          final[current_file][:patch] << "\n" + line
         end
-        final.map { |e| [e[0], DiffFile.new(@base, e[1])] }
       end
-
+      final.map { |e| [e[0], DiffFile.new(@base, e[1])] }
+    end
   end
 end
diff --git a/lib/git/diff_path_status.rb b/lib/git/diff_path_status.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module Git
+  class DiffPathStatus
+    include Enumerable
+
+    # @private
+    def initialize(base, from, to, path_limiter = nil)
+      # Eagerly check for invalid arguments
+      [from, to].compact.each do |arg|
+        raise ArgumentError, "Invalid argument: '#{arg}'" if arg.start_with?('-')
+      end
+
+      @base = base
+      @from = from
+      @to = to
+      @path_limiter = path_limiter
+      @path_status = nil
+    end
+
+    # Iterates over each file's status.
+    #
+    # @yield [path, status]
+    def each(&block)
+      fetch_path_status.each(&block)
+    end
+
+    # Returns the name-status report as a Hash.
+    #
+    # @return [Hash<String, String>] A hash where keys are file paths
+    #   and values are their status codes.
+    def to_h
+      fetch_path_status
+    end
+
+    private
+
+    # Lazily fetches and caches the path status from the git lib.
+    def fetch_path_status
+      @path_status ||= @base.lib.diff_path_status(
+        @from, @to, { path: @path_limiter }
+      )
+    end
+  end
+end
diff --git a/lib/git/diff_stats.rb b/lib/git/diff_stats.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+module Git
+  # Provides access to the statistics of a diff between two commits,
+  # including insertions, deletions, and file-level details.
+  class DiffStats
+    # @private
+    def initialize(base, from, to, path_limiter = nil)
+      # Eagerly check for invalid arguments
+      [from, to].compact.each do |arg|
+        raise ArgumentError, "Invalid argument: '#{arg}'" if arg.start_with?('-')
+      end
+
+      @base = base
+      @from = from
+      @to = to
+      @path_limiter = path_limiter
+      @stats = nil
+    end
+
+    # Returns the total number of lines deleted.
+    def deletions
+      fetch_stats[:total][:deletions]
+    end
+
+    # Returns the total number of lines inserted.
+    def insertions
+      fetch_stats[:total][:insertions]
+    end
+
+    # Returns the total number of lines changed (insertions + deletions).
+    def lines
+      fetch_stats[:total][:lines]
+    end
+
+    # Returns a hash of statistics for each file in the diff.
+    #
+    # @return [Hash<String, {insertions: Integer, deletions: Integer}>]
+    def files
+      fetch_stats[:files]
+    end
+
+    # Returns a hash of the total statistics for the diff.
+    #
+    # @return [{insertions: Integer, deletions: Integer, lines: Integer, files: Integer}]
+    def total
+      fetch_stats[:total]
+    end
+
+    private
+
+    # Lazily fetches and caches the stats from the git lib.
+    def fetch_stats
+      @stats ||= @base.lib.diff_stats(
+        @from, @to, { path_limiter: @path_limiter }
+      )
+    end
+  end
+end
diff --git a/lib/git/lib.rb b/lib/git/lib.rb
@@ -848,7 +848,7 @@ def diff_stats(obj1 = 'HEAD', obj2 = nil, opts = {})
       hsh
     end
 
-    def diff_name_status(reference1 = nil, reference2 = nil, opts = {})
+    def diff_path_status(reference1 = nil, reference2 = nil, opts = {})
       assert_args_are_not_options('commit or commit range', reference1, reference2)
 
       opts_arr = ['--name-status']
diff --git a/tests/units/test_diff.rb b/tests/units/test_diff.rb
@@ -128,7 +128,7 @@ def test_diff_patch_with_bad_commit
     end
   end
 
-  def test_diff_name_status_with_bad_commit
+  def test_diff_path_status_with_bad_commit
     assert_raise(ArgumentError) do
       @git.diff('-s').name_status
     end
diff --git a/tests/units/test_diff_path_status.rb b/tests/units/test_diff_path_status.rb
diff --git a/tests/units/test_diff_stats.rb b/tests/units/test_diff_stats.rb
