Fix audit version_scheme and revision checks.

Another attempt at fixing `brew audit` issues around detecting
`revision` and `version_scheme` changes correctly. First done in #1754
and #2086 (reverted in #2099 and #2100).

To ease future debugging a `ph` helper has been added to print a hash
and a series of RSpec tests to verify that the `revision`,
`version_scheme` and `version` formula version audits behave as
expected.

Fixes #1731.

Library/Homebrew/dev-cmd/audit.rb

@@ -827,51 +827,71 @@ class FormulaAuditor
     return unless formula.tap.git? # git log is required
     return if @new_formula
 
-    fv = FormulaVersions.new(formula, max_depth: 1)
+    fv = FormulaVersions.new(formula)
     attributes = [:revision, :version_scheme]
-
     attributes_map = fv.version_attributes_map(attributes, "origin/master")
 
-    attributes.each do |attribute|
-      stable_attribute_map = attributes_map[attribute][:stable]
-      next if stable_attribute_map.nil? || stable_attribute_map.empty?
-
-      attributes_for_version = stable_attribute_map[formula.version]
-      next if attributes_for_version.nil? || attributes_for_version.empty?
-
-      old_attribute = formula.send(attribute)
-      max_attribute = attributes_for_version.max
-      if max_attribute && old_attribute < max_attribute
-        problem "#{attribute} should not decrease (from #{max_attribute} to #{old_attribute})"
-      end
-    end
-
+    current_version_scheme = formula.version_scheme
     [:stable, :devel].each do |spec|
       spec_version_scheme_map = attributes_map[:version_scheme][spec]
       next if spec_version_scheme_map.nil? || spec_version_scheme_map.empty?
 
-      max_version_scheme = spec_version_scheme_map.values.flatten.max
+      version_schemes = spec_version_scheme_map.values.flatten
+      max_version_scheme = version_schemes.max
       max_version = spec_version_scheme_map.select do |_, version_scheme|
         version_scheme.first == max_version_scheme
       end.keys.max
 
-      formula_spec = formula.send(spec)
-      next if formula_spec.nil?
-
-      if max_version && formula_spec.version < max_version
-        problem "#{spec} version should not decrease (from #{max_version} to #{formula_spec.version})"
+      if max_version_scheme && current_version_scheme < max_version_scheme
+        problem "version_scheme should not decrease (from #{max_version_scheme} to #{current_version_scheme})"
       end
+
+      if max_version_scheme && current_version_scheme >= max_version_scheme &&
+         current_version_scheme > 1 &&
+         !version_schemes.include?(current_version_scheme - 1)
+        problem "version_schemes should only increment by 1"
+      end
+
+      formula_spec = formula.send(spec)
+      next unless formula_spec
+
+      spec_version = formula_spec.version
+      next unless max_version
+      next if spec_version >= max_version
+
+      above_max_version_scheme = current_version_scheme > max_version_scheme
+      map_includes_version = spec_version_scheme_map.keys.include?(spec_version)
+      next if !current_version_scheme.zero? &&
+              (above_max_version_scheme || map_includes_version)
+      problem "#{spec} version should not decrease (from #{max_version} to #{spec_version})"
     end
 
-    return if formula.revision.zero?
+    current_revision = formula.revision
     if formula.stable
-      revision_map = attributes_map[:revision][:stable]
-      stable_revisions = revision_map[formula.stable.version] if revision_map
-      if !stable_revisions || stable_revisions.empty?
-        problem "'revision #{formula.revision}' should be removed"
+      if revision_map = attributes_map[:revision][:stable]
+        if !revision_map.nil? && !revision_map.empty?
+          stable_revisions = revision_map[formula.stable.version]
+          stable_revisions ||= []
+          current_revision = formula.revision
+          max_revision = stable_revisions.max || 0
+
+          if current_revision < max_revision
+            problem "revision should not decrease (from #{max_revision} to #{current_revision})"
+          end
+
+          stable_revisions -= [formula.revision]
+          if !current_revision.zero? && stable_revisions.empty? &&
+             revision_map.keys.length > 1
+            problem "'revision #{formula.revision}' should be removed"
+          elsif current_revision > 1 &&
+                current_revision != max_revision &&
+                !stable_revisions.include?(current_revision - 1)
+            problem "revisions should only increment by 1"
+          end
+        end
       end
-    else # head/devel-only formula
-      problem "'revision #{formula.revision}' should be removed"
+    elsif !current_revision.zero? # head/devel-only formula
+      problem "'revision #{current_revision}' should be removed"
     end
   end
 
@@ -1211,6 +1231,10 @@ class FormulaAuditor
       end
     end
 
+    if line =~ /((revision|version_scheme)\s+0)/
+      problem "'#{$1}' should be removed"
+    end
+
     return unless @strict
 
     problem "`#{$1}` in formulae is deprecated" if line =~ /(env :(std|userpaths))/

Library/Homebrew/formula_versions.rb

@@ -7,21 +7,22 @@ class FormulaVersions
     ErrorDuringExecution, LoadError, MethodDeprecatedError
   ].freeze
 
+  MAX_VERSIONS_DEPTH = 2
+
   attr_reader :name, :path, :repository, :entry_name
 
-  def initialize(formula, options = {})
+  def initialize(formula)
     @name = formula.name
     @path = formula.path
     @repository = formula.tap.path
     @entry_name = @path.relative_path_from(repository).to_s
-    @max_depth = options[:max_depth]
+    @current_formula = formula
   end
 
   def rev_list(branch)
     repository.cd do
-      depth = 0
       Utils.popen_read("git", "rev-list", "--abbrev-commit", "--remove-empty", branch, "--", entry_name) do |io|
-        yield io.readline.chomp until io.eof? || (@max_depth && (depth += 1) > @max_depth)
+        yield io.readline.chomp until io.eof?
       end
     end
   end
@@ -49,11 +50,15 @@ class FormulaVersions
 
   def bottle_version_map(branch)
     map = Hash.new { |h, k| h[k] = [] }
+
+    versions_seen = 0
     rev_list(branch) do |rev|
       formula_at_revision(rev) do |f|
         bottle = f.bottle_specification
         map[f.pkg_version] << bottle.rebuild unless bottle.checksums.empty?
+        versions_seen = (map.keys + [f.pkg_version]).uniq.length
       end
+      return map if versions_seen > MAX_VERSIONS_DEPTH
     end
     map
   end
@@ -62,27 +67,35 @@ class FormulaVersions
     attributes_map = {}
     return attributes_map if attributes.empty?
 
-    attributes.each do |attribute|
-      attributes_map[attribute] ||= {}
-    end
-
+    stable_versions_seen = 0
     rev_list(branch) do |rev|
       formula_at_revision(rev) do |f|
         attributes.each do |attribute|
+          attributes_map[attribute] ||= {}
           map = attributes_map[attribute]
-          if f.stable
-            map[:stable] ||= {}
-            map[:stable][f.stable.version] ||= []
-            map[:stable][f.stable.version] << f.send(attribute)
-          end
-          next unless f.devel
-          map[:devel] ||= {}
-          map[:devel][f.devel.version] ||= []
-          map[:devel][f.devel.version] << f.send(attribute)
+          set_attribute_map(map, f, attribute)
+
+          stable_keys_length = (map[:stable].keys + [f.version]).uniq.length
+          stable_versions_seen = [stable_versions_seen, stable_keys_length].max
         end
       end
+      break if stable_versions_seen > MAX_VERSIONS_DEPTH
     end
 
     attributes_map
   end
+
+  private
+
+  def set_attribute_map(map, f, attribute)
+    if f.stable
+      map[:stable] ||= {}
+      map[:stable][f.stable.version] ||= []
+      map[:stable][f.stable.version] << f.send(attribute)
+    end
+    return unless f.devel
+    map[:devel] ||= {}
+    map[:devel][f.devel.version] ||= []
+    map[:devel][f.devel.version] << f.send(attribute)
+  end
 end

Library/Homebrew/test/dev-cmd/audit_spec.rb

@@ -5,6 +5,13 @@ RSpec::Matchers.alias_matcher :have_data, :be_data
 RSpec::Matchers.alias_matcher :have_end, :be_end
 RSpec::Matchers.alias_matcher :have_trailing_newline, :be_trailing_newline
 
+module Count
+  def self.increment
+    @count ||= 0
+    @count  += 1
+  end
+end
+
 describe FormulaText do
   let(:dir) { mktmpdir }
 
@@ -518,4 +525,187 @@ describe FormulaAuditor do
         .to match('xcodebuild should be passed an explicit "SYMROOT"')
     end
   end
+
+  describe "#audit_revision_and_version_scheme" do
+    let(:origin_tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" }
+    let(:formula_subpath) { "Formula/foo#{@foo_version}.rb" }
+    let(:origin_formula_path) { origin_tap_path/formula_subpath }
+    let(:tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-bar" }
+    let(:formula_path) { tap_path/formula_subpath }
+
+    before(:each) do
+      @foo_version = Count.increment
+
+      origin_formula_path.write <<-EOS.undent
+        class Foo#{@foo_version} < Formula
+          url "https://example.com/foo-1.0.tar.gz"
+          revision 2
+          version_scheme 1
+        end
+      EOS
+
+      origin_tap_path.mkpath
+      origin_tap_path.cd do
+        shutup do
+          system "git", "init"
+          system "git", "add", "--all"
+          system "git", "commit", "-m", "init"
+        end
+      end
+
+      tap_path.mkpath
+      tap_path.cd do
+        shutup do
+          system "git", "clone", origin_tap_path, "."
+        end
+      end
+    end
+
+    subject do
+      fa = described_class.new(Formulary.factory(formula_path))
+      fa.audit_revision_and_version_scheme
+      fa.problems.first
+    end
+
+    def formula_gsub(before, after = "")
+      text = formula_path.read
+      text.gsub! before, after
+      formula_path.unlink
+      formula_path.write text
+    end
+
+    def formula_gsub_commit(before, after = "")
+      text = origin_formula_path.read
+      text.gsub!(before, after)
+      origin_formula_path.unlink
+      origin_formula_path.write text
+
+      origin_tap_path.cd do
+        shutup do
+          system "git", "commit", "-am", "commit"
+        end
+      end
+
+      tap_path.cd do
+        shutup do
+          system "git", "fetch"
+          system "git", "reset", "--hard", "origin/master"
+        end
+      end
+    end
+
+    context "revisions" do
+      context "should not be removed when first committed above 0" do
+        it { is_expected.to be_nil }
+      end
+
+      context "should not decrease with the same version" do
+        before { formula_gsub_commit "revision 2", "revision 1" }
+
+        it { is_expected.to match("revision should not decrease (from 2 to 1)") }
+      end
+
+      context "should not be removed with the same version" do
+        before { formula_gsub_commit "revision 2" }
+
+        it { is_expected.to match("revision should not decrease (from 2 to 0)") }
+      end
+
+      context "should not decrease with the same, uncommitted version" do
+        before { formula_gsub "revision 2", "revision 1" }
+
+        it { is_expected.to match("revision should not decrease (from 2 to 1)") }
+      end
+
+      context "should be removed with a newer version" do
+        before { formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz" }
+
+        it { is_expected.to match("'revision 2' should be removed") }
+      end
+
+      context "should not warn on an newer version revision removal" do
+        before do
+          formula_gsub_commit "revision 2", ""
+          formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz"
+        end
+
+        it { is_expected.to be_nil }
+      end
+
+      context "should only increment by 1 with an uncommitted version" do
+        before do
+          formula_gsub "foo-1.0.tar.gz", "foo-1.1.tar.gz"
+          formula_gsub "revision 2", "revision 4"
+        end
+
+        it { is_expected.to match("revisions should only increment by 1") }
+      end
+
+      context "should not warn on past increment by more than 1" do
+        before do
+          formula_gsub_commit "revision 2", "# no revision"
+          formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz"
+          formula_gsub_commit "# no revision", "revision 3"
+        end
+
+        it { is_expected.to be_nil }
+      end
+    end
+
+    context "version_schemes" do
+      context "should not decrease with the same version" do
+        before { formula_gsub_commit "version_scheme 1" }
+
+        it { is_expected.to match("version_scheme should not decrease (from 1 to 0)") }
+      end
+
+      context "should not decrease with a new version" do
+        before do
+          formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz"
+          formula_gsub_commit "version_scheme 1", ""
+          formula_gsub_commit "revision 2", ""
+        end
+
+        it { is_expected.to match("version_scheme should not decrease (from 1 to 0)") }
+      end
+
+      context "should only increment by 1" do
+        before do
+          formula_gsub_commit "version_scheme 1", "# no version_scheme"
+          formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz"
+          formula_gsub_commit "revision 2", ""
+          formula_gsub_commit "# no version_scheme", "version_scheme 3"
+        end
+
+        it { is_expected.to match("version_schemes should only increment by 1") }
+      end
+    end
+
+    context "versions" do
+      context "uncommitted should not decrease" do
+        before { formula_gsub "foo-1.0.tar.gz", "foo-0.9.tar.gz" }
+
+        it { is_expected.to match("stable version should not decrease (from 1.0 to 0.9)") }
+      end
+
+      context "committed can decrease" do
+        before do
+          formula_gsub_commit "revision 2"
+          formula_gsub_commit "foo-1.0.tar.gz", "foo-0.9.tar.gz"
+        end
+
+        it { is_expected.to be_nil }
+      end
+
+      context "can decrease with version_scheme increased" do
+        before do
+          formula_gsub "revision 2"
+          formula_gsub "foo-1.0.tar.gz", "foo-0.9.tar.gz"
+          formula_gsub "version_scheme 1", "version_scheme 2"
+        end
+
+        it { is_expected.to be_nil }
+      end
+    end
+  end
 end

Library/Homebrew/test/spec_helper.rb

@@ -93,6 +93,7 @@ RSpec.configure do |config|
         HOMEBREW_PREFIX/"opt",
         HOMEBREW_PREFIX/"Caskroom",
         HOMEBREW_LIBRARY/"Taps/caskroom",
+        HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bar",
         HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bundle",
         HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-foo",
         HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-services",

Library/Homebrew/test/utils_spec.rb

@@ -270,4 +270,12 @@ describe "globally-scoped helper methods" do
       }.to raise_error(MethodDeprecatedError, %r{method.*replacement.*homebrew/homebrew-core.*homebrew/core}m)
     end
   end
+
+  describe "#puts_hash" do
+    it "outputs a hash" do
+      expect {
+        puts_hash(a: 1, b: 2, c: [3, { "d"=>4 }])
+      }.to output("a: 1\nb: 2\nc: [3, {\"d\"=>4}]\n").to_stdout
+    end
+  end
 end

Library/Homebrew/utils.rb

@@ -517,3 +517,19 @@ def migrate_legacy_keg_symlinks_if_necessary
   end
   FileUtils.rm_rf legacy_pinned_kegs
 end
+
+def puts_hash(hash, indent: 0)
+  return hash unless hash.is_a? Hash
+  hash.each do |key, value|
+    indent_spaces = " " * (indent * 2)
+    printf "#{indent_spaces}#{key}:"
+    if value.is_a? Hash
+      puts
+      puts_hash(value, indent: indent+1)
+    else
+      puts " #{value}"
+    end
+  end
+  hash
+end
+alias ph puts_hash