FormulaAuditor: Separate stable version audit

[samford]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

The "stable version should not decrease" formula audit currently prevents us from being able to create bottles when downgrading a formula version (e.g., Homebrew/homebrew-core#156900). We previously worked around this by bumping version_scheme but this wasn't an intended use case and we now avoid using it for this purpose.

We can handle simple formula downgrades by reverting changes in a syntax-only PR but that isn't sufficient when we need new bottles (i.e., if additional changes have been made to the formula in the interim time). In the latter case, the only available solution may be to revert all changes made after the previous version using a syntax-only PR and then create another PR to reintroduce the other changes and create new bottles.

To avoid the aforementioned approach, this splits the stable version audit into a separate method, so we can use brew audit --except=stable_version to selectively skip it. A new CLI flag will be added to test-bot (--skip-stable-version) to pass this --except argument to brew audit and homebrew/core CI will be updated to control this with a CI-version-downgrade label. I already have those changes worked out and will create follow-up PRs if/when this is merged.

---

I've had to do a bit of refactoring in related areas of FormulaAuditor and test/dev-cmd/audit_spec.rb and I'm not entirely sure I've approached it in the best way or not, so let me know if there's room for improvement. In both areas, the challenge was how to handle shared logic/variables/etc. between the new #audit_stable_version method and the existing #audit_revision_and_version_scheme method (which the former was extracted from).

One area that could be improved is that the before block in audit_spec.rb is duplicated in the tests for both of the aforementioned methods. It would be ideal if we could avoid that duplication but I'm not yet aware of a way of elegantly accomplishing that in RSpec.

---

That said, one alternative to all this would be to simply modify the related audit to not apply if the revision has increased (like how a version_scheme bump currently works). I didn't go in that direction because it can lead to unexpected behavior. Namely, this audit wouldn't be enforced when a formula is revision bumped for an entirely different reason, so it seemed better to manually disable the audit with a label only when appropriate.

[MikeMcQuaid]

Looks good @samford! A few suggested naming and style tweaks but open to disagreement there. Looks good overall!

[MikeMcQuaid]

Suggested change

	def collect_previous_info
	def get_previous_versions_commits

or something

[samford]

Fwiw, we have to avoid the get_ prefix because of the Naming/AccessorMethodName RuboCop: Do not prefix reader method names with get_.

get_previous_versions_commits doesn't convey the same purpose to me, as the goal of the method is to extract information from the commits, not just fetch the commits themselves.

If we rework the method to return the instance variables in an array (i.e., [@previous_committed, @newest_committed]) as mentioned in another comment, how about committed_version_info? previous_committed_version_info would be less ambiguous but also a bit lengthy.

[samford]

The latest push reworks the previous commit to:

• Rename @previous to @previous_committed
• Rename #collect_version_info to #committed_version_info
• Return a [@previous_committed, @newest_committed] array from #committed_version_info, to restrict usage of the @previous_committed and @newest_committed instance variables to that method

I can understand if we still need to workshop the name of the #committed_version_info method (naming's hard), so I left that discussion unresolved above.

I can incorporate additional changes if we need to further minimize usage of the instance variables in #committed_version_info but it makes the method more verbose, so I left it out for now:

diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb
index ecbe1f8520ee3c48d79e1719ccc09d88759ab5ee..f10049cd2e31a25908efaa3b3445726583680633 100644
--- a/Library/Homebrew/formula_auditor.rb
+++ b/Library/Homebrew/formula_auditor.rb
@@ -957,6 +957,16 @@ module Homebrew
       current_version = formula.stable.version
       current_revision = formula.revision
 
+      previous_committed_version = T.let(nil, T.nilable(Version))
+      previous_committed_checksum = T.let(nil, T.nilable(String))
+      previous_committed_version_scheme = T.let(nil, T.nilable(Integer))
+      previous_committed_revision = T.let(nil, T.nilable(Integer))
+
+      newest_committed_version = T.let(nil, T.nilable(Version))
+      newest_committed_checksum = T.let(nil, T.nilable(String))
+      newest_committed_revision = T.let(nil, T.nilable(Integer))
+      newest_committed_url = T.let(nil, T.nilable(String))
+
       fv = FormulaVersions.new(formula)
       fv.rev_list("origin/HEAD") do |revision, path|
         begin
@@ -964,26 +974,36 @@ module Homebrew
             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
+            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
+            newest_committed_version ||= previous_committed_version
+            newest_committed_checksum ||= previous_committed_checksum
+            newest_committed_revision ||= previous_committed_revision
+            newest_committed_url ||= stable.url
           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 = {
+        version:        previous_committed_version,
+        checksum:       previous_committed_checksum,
+        version_scheme: previous_committed_version_scheme,
+        revision:       previous_committed_revision,
+      }.compact
+      @newest_committed = {
+        version:  newest_committed_version,
+        checksum: newest_committed_checksum,
+        revision: newest_committed_revision,
+        url:      newest_committed_url,
+      }.compact
 
       [@previous_committed, @newest_committed]
     end

[MikeMcQuaid]

Looks great, thanks again @samford!