formula_auditor: audit revision and compatibility_version

Add a new audit method to check if the `revision` and
`compatibility_version` are consistent with each other.

This ensures that when a formula's `compatibility_version` is increased,
the `revision` of dependent formulas was also increased.

Inversely, when a formula's `revision` is increased in the same PR as
one of its dependencies, the `compatibility_version` of dependent
formulae must be increased by 1.

Author: MikeMcQuaid

Library/Homebrew/formula_auditor.rb

@@ -7,6 +7,7 @@
 require "resource_auditor"
 require "utils/shared_audits"
 require "utils/output"
+require "utils/git"
 
 module Homebrew
   # Auditor for checking common violations in {Formula}e.
@@ -39,8 +40,7 @@ def initialize(formula, options = {})
       @spdx_license_data = options[:spdx_license_data]
       @spdx_exception_data = options[:spdx_exception_data]
       @tap_audit = options[:tap_audit]
-      @previous_committed = {}
-      @newest_committed = {}
+      @committed_version_info_cache = {}
     end
 
     def audit_style
@@ -877,8 +877,10 @@ def audit_revision
       new_formula_problem("New formulae should not define a revision.") if @new_formula && !formula.revision.zero?
 
       return unless @git
-      return unless formula.tap # skip formula not from core or any taps
-      return unless formula.tap.git? # git log is required
+
+      tap = formula.tap
+      return if tap.nil?
+      return unless tap.git?
       return if formula.stable.blank?
 
       current_version = formula.stable.version
@@ -900,6 +902,81 @@ def audit_revision
             current_revision > (newest_committed[:revision] + 1)
         problem "`revision` should only increment by 1"
       end
+
+      revision_increment = if (previous_revision = previous_committed[:revision])
+        current_revision - previous_revision
+      else
+        current_revision
+      end
+      return if revision_increment != 1
+
+      dependency_names = formula.recursive_dependencies.map(&:name)
+      return if dependency_names.empty?
+
+      changed_dependency_paths = changed_formulae_paths(tap, only_names: dependency_names)
+      return if changed_dependency_paths.empty?
+
+      missing_compatibility_bumps = changed_dependency_paths.filter_map do |path|
+        changed_formula = Formulary.factory(path)
+        # Each changed dependency must raise its compatibility_version by exactly one.
+        previous_committed_dependency, = committed_version_info(formula: changed_formula)
+        previous_compatibility_version = previous_committed_dependency[:compatibility_version] || 0
+        current_compatibility_version = changed_formula.compatibility_version || 0
+        next if current_compatibility_version == previous_compatibility_version + 1
+
+        "#{changed_formula.name} (#{previous_compatibility_version} to #{current_compatibility_version})"
+      end
+      return if missing_compatibility_bumps.empty?
+
+      problem "`revision` increased but recursive dependencies must increase `compatibility_version` by 1: " \
+              "#{missing_compatibility_bumps.join(", ")}."
+    end
+
+    def audit_compatibility_version
+      return unless @git
+
+      tap = formula.tap
+      return if tap.nil?
+      return unless tap.git?
+
+      previous_committed, = committed_version_info
+      return if previous_committed.empty?
+
+      previous_compatibility_version = previous_committed[:compatibility_version] || 0
+      current_compatibility_version = formula.compatibility_version || previous_compatibility_version
+
+      if current_compatibility_version < previous_compatibility_version
+        problem "`compatibility_version` should not decrease " \
+                "from #{previous_compatibility_version} to #{current_compatibility_version}"
+        return
+      elsif current_compatibility_version > (previous_compatibility_version + 1)
+        problem "`compatibility_version` should only increment by 1"
+        return
+      end
+
+      compatibility_increment = current_compatibility_version - previous_compatibility_version
+      return if compatibility_increment.zero?
+
+      dependent_revision_bumps = changed_formulae_paths(tap).filter_map do |path|
+        changed_formula = Formulary.factory(path)
+        next if changed_formula.name == formula.name
+
+        dependencies = changed_formula.recursive_dependencies.map(&:name)
+        # Only formulae that depend (recursively) on the audited formula can justify the bump.
+        next unless dependencies.include?(formula.name)
+
+        previous_committed_dependent, = committed_version_info(formula: changed_formula)
+        previous_revision = previous_committed_dependent[:revision] || 0
+        current_revision = changed_formula.revision
+        next if current_revision != previous_revision + 1
+
+        changed_formula.name
+      end
+      return if dependent_revision_bumps.any?
+
+      problem "`compatibility_version` increased from #{previous_compatibility_version} to " \
+              "#{current_compatibility_version} but no recursive dependent formulae increased " \
+              "`revision` by 1 in this PR."
     end
 
     def audit_version_scheme
@@ -1064,12 +1141,57 @@ def linux_only_gcc_dep?(formula)
       true
     end
 
-    def committed_version_info
-      return [] unless @git
-      return [] unless formula.tap # skip formula not from core or any taps
-      return [] unless formula.tap.git? # git log is required
-      return [] if formula.stable.blank?
-      return [@previous_committed, @newest_committed] if @previous_committed.present? || @newest_committed.present?
+    sig { params(tap: Tap, only_names: T.nilable(T::Array[String])).returns(T::Array[Pathname]) }
+    def changed_formulae_paths(tap, only_names: nil)
+      return [] unless tap.git?
+
+      base_ref = "origin/HEAD"
+      changed_paths = Utils.safe_popen_read(Utils::Git.git, "-C", tap.path, "diff", "--name-only", base_ref)
+                           .lines
+                           .filter_map do |line|
+        relative_path = line.chomp
+        next unless relative_path.end_with?(".rb")
+
+        absolute_path = tap.path/relative_path
+        next unless absolute_path.exist?
+        next unless absolute_path.to_s.start_with?(tap.formula_dir.to_s)
+
+        absolute_path
+      end
+
+      return changed_paths if only_names.blank?
+
+      expected_paths = only_names.filter_map do |name|
+        relative_name = name.to_s.delete_prefix("#{tap.name}/")
+        relative_path = Pathname(relative_name)
+        relative_path = relative_path.sub_ext(".rb") if relative_path.extname.empty?
+        absolute_path = tap.formula_dir/relative_path
+        absolute_path.expand_path if absolute_path.exist?
+      rescue
+        nil
+      end.map(&:to_s)
+
+      return [] if expected_paths.empty?
+
+      changed_paths.select { |path| expected_paths.include?(path.expand_path.to_s) }
+    end
+
+    def committed_version_info(formula: @formula)
+      empty_result = [{}, {}]
+      return empty_result unless @git
+      return empty_result unless formula.tap # skip formula not from core or any taps
+      return empty_result unless formula.tap.git? # git log is required
+      return empty_result if formula.stable.blank?
+
+      cache_key = if formula.respond_to?(:full_name)
+        formula.full_name
+      else
+        formula.name
+      end
+      return @committed_version_info_cache[cache_key] if @committed_version_info_cache.key?(cache_key)
+
+      previous_committed = {}
+      newest_committed = {}
 
       current_version = formula.stable.version
       current_revision = formula.revision
@@ -1081,28 +1203,30 @@ def committed_version_info
             stable = f.stable
             next if stable.blank?
 
-            @previous_committed[:version] = stable.version
-            @previous_committed[:checksum] = stable.checksum
-            @previous_committed[:version_scheme] = f.version_scheme
-            @previous_committed[:revision] = f.revision
-
-            @newest_committed[:version] ||= @previous_committed[:version]
-            @newest_committed[:checksum] ||= @previous_committed[:checksum]
-            @newest_committed[:revision] ||= @previous_committed[:revision]
-            @newest_committed[:url] ||= stable.url
+            previous_committed[:version] = stable.version
+            previous_committed[:checksum] = stable.checksum
+            previous_committed[:version_scheme] = f.version_scheme
+            previous_committed[:revision] = f.revision
+            previous_committed[:compatibility_version] = f.compatibility_version
+
+            newest_committed[:version] ||= previous_committed[:version]
+            newest_committed[:checksum] ||= previous_committed[:checksum]
+            newest_committed[:revision] ||= previous_committed[:revision]
+            newest_committed[:url] ||= stable.url
+            newest_committed[:compatibility_version] ||= previous_committed[:compatibility_version]
           end
         rescue MacOSVersion::Error
           break
         end
 
-        break if @previous_committed[:version] && current_version != @previous_committed[:version]
-        break if @previous_committed[:revision] && current_revision != @previous_committed[:revision]
+        break if previous_committed[:version] && current_version != previous_committed[:version]
+        break if previous_committed[:revision] && current_revision != previous_committed[:revision]
       end
 
-      @previous_committed.compact!
-      @newest_committed.compact!
+      previous_committed.compact!
+      newest_committed.compact!
 
-      [@previous_committed, @newest_committed]
+      @committed_version_info_cache[cache_key] = [previous_committed, newest_committed]
     end
   end
 end