Improve duplicate pull request handling

- change the messaging depending on how confident we are that we're
  actually looking at duplicates i.e. we're not confident without a
  version number supplied
- similarly, just warn instead of failing with an error (and no
  override) if we're not confident that we're looking at duplicates
  because a version wasn't supplied
- change `bump-cask-pr` and `bump-formula-pr` to always check for all
  pull requests with the new version number (to allow failing on this)
  rather than only checking closed pull requests with a version number
- change `bump` to check for definite/maybe duplicate PRs and only
  exit if they are definitely duplicates
- cleanup some variable usage to DRY things up a bit

Library/Homebrew/dev-cmd/bump-cask-pr.rb

@@ -254,25 +254,18 @@ def replace_version_and_checksum(cask, new_hash, new_version, replacement_pairs)
       def check_pull_requests(cask, new_version:)
         tap_remote_repo = cask.tap.full_name || cask.tap.remote_repo
 
+        file = cask.sourcefile_path.relative_path_from(cask.tap.path).to_s
+        quiet = args.quiet?
         GitHub.check_for_duplicate_pull_requests(cask.token, tap_remote_repo,
-                                                 state:   "open",
-                                                 version: nil,
-                                                 file:    cask.sourcefile_path.relative_path_from(cask.tap.path).to_s,
-                                                 quiet:   args.quiet?)
+                                                 state: "open", file:, quiet:)
 
-        # if we haven't already found open requests, try for an exact match across closed requests
+        # if we haven't already found open requests, try for an exact match across all pull requests
         new_version.instance_variables.each do |version_type|
-          version = new_version.instance_variable_get(version_type)
-          next if version.blank?
-
-          GitHub.check_for_duplicate_pull_requests(
-            cask.token,
-            tap_remote_repo,
-            state:   "closed",
-            version: shortened_version(version, cask:),
-            file:    cask.sourcefile_path.relative_path_from(cask.tap.path).to_s,
-            quiet:   args.quiet?,
-          )
+          version_type_version = new_version.instance_variable_get(version_type)
+          next if version_type_version.blank?
+
+          version = shortened_version(version_type_version, cask:)
+          GitHub.check_for_duplicate_pull_requests(cask.token, tap_remote_repo, version:, file:, quiet:)
         end
       end
 

Library/Homebrew/dev-cmd/bump-formula-pr.rb

@@ -134,7 +134,7 @@ def run
         remote_branch = formula.tap.git_repository.origin_branch_name
         previous_branch = "-"
 
-        check_open_pull_requests(formula, tap_remote_repo)
+        check_pull_requests(formula, tap_remote_repo, state: "open")
 
         new_version = args.version
         check_new_version(formula, tap_remote_repo, version: new_version) if new_version.present?
@@ -465,16 +465,21 @@ def formula_version(formula, contents = nil)
         end
       end
 
-      sig { params(formula: Formula, tap_remote_repo: String).returns(T.nilable(T::Array[String])) }
-      def check_open_pull_requests(formula, tap_remote_repo)
+      sig {
+        params(formula: Formula, tap_remote_repo: String, state: T.nilable(String),
+               version: T.nilable(String)).returns(T.nilable(T::Array[String]))
+      }
+      def check_pull_requests(formula, tap_remote_repo, state: nil, version: nil)
         tap = formula.tap
         return if tap.nil?
 
+        # if we haven't already found open requests, try for an exact match across all pull requests
         GitHub.check_for_duplicate_pull_requests(
           formula.name, tap_remote_repo,
-          state: "open",
-          file:  formula.path.relative_path_from(tap.path).to_s,
-          quiet: args.quiet?
+          version:,
+          state:,
+          file:    formula.path.relative_path_from(tap.path).to_s,
+          quiet:   args.quiet?
         )
       end
 
@@ -493,7 +498,7 @@ def check_new_version(formula, tap_remote_repo, version: nil, url: nil, tag: nil
         end
 
         check_throttle(formula, version)
-        check_closed_pull_requests(formula, tap_remote_repo, version:)
+        check_pull_requests(formula, tap_remote_repo, version:)
       end
 
       sig { params(formula: Formula, new_version: String).void }
@@ -514,24 +519,6 @@ def check_throttle(formula, new_version)
         odie "#{formula} should only be updated every #{throttled_rate} releases on multiples of #{throttled_rate}"
       end
 
-      sig {
-        params(formula: Formula, tap_remote_repo: String,
-               version: T.nilable(String)).returns(T.nilable(T::Array[String]))
-      }
-      def check_closed_pull_requests(formula, tap_remote_repo, version:)
-        tap = formula.tap
-        return if tap.nil?
-
-        # if we haven't already found open requests, try for an exact match across closed requests
-        GitHub.check_for_duplicate_pull_requests(
-          formula.name, tap_remote_repo,
-          version:,
-          state:   "closed",
-          file:    formula.path.relative_path_from(tap.path).to_s,
-          quiet:   args.quiet?
-        )
-      end
-
       sig { params(formula: Formula, new_formula_version: Version).returns(T.nilable(T::Array[String])) }
       def alias_update_pair(formula, new_formula_version)
         versioned_alias = formula.aliases.grep(/^.*@\d+(\.\d+)?$/).first

Library/Homebrew/dev-cmd/bump.rb

@@ -16,8 +16,8 @@ class VersionBumpInfo < T::Struct
         const :current_version, BumpVersionParser
         const :repology_latest, T.any(String, Version)
         const :new_version, BumpVersionParser
-        const :open_pull_requests, T.nilable(T.any(T::Array[String], String))
-        const :closed_pull_requests, T.nilable(T.any(T::Array[String], String))
+        const :duplicate_pull_requests, T.nilable(T.any(T::Array[String], String))
+        const :maybe_duplicate_pull_requests, T.nilable(T.any(T::Array[String], String))
       end
 
       cmd_args do
@@ -245,14 +245,13 @@ def livecheck_result(formula_or_cask)
         params(
           formula_or_cask: T.any(Formula, Cask::Cask),
           name:            String,
-          state:           String,
           version:         T.nilable(String),
         ).returns T.nilable(T.any(T::Array[String], String))
       }
-      def retrieve_pull_requests(formula_or_cask, name, state:, version: nil)
+      def retrieve_pull_requests(formula_or_cask, name, version: nil)
         tap_remote_repo = formula_or_cask.tap&.remote_repo || formula_or_cask.tap&.full_name
         pull_requests = begin
-          GitHub.fetch_pull_requests(name, tap_remote_repo, state:, version:)
+          GitHub.fetch_pull_requests(name, tap_remote_repo, version:)
         rescue GitHub::API::ValidationFailedError => e
           odebug "Error fetching pull requests for #{formula_or_cask} #{name}: #{e}"
           nil
@@ -351,12 +350,12 @@ def retrieve_versions_by_arch(formula_or_cask:, repositories:, name:)
           new_version.general.to_s
         end
 
-        open_pull_requests = if !args.no_pull_requests? && (args.named.present? || new_version.present?)
-          retrieve_pull_requests(formula_or_cask, name, state: "open")
+        duplicate_pull_requests = unless args.no_pull_requests?
+          retrieve_pull_requests(formula_or_cask, name, version: pull_request_version)
         end.presence
 
-        closed_pull_requests = if !args.no_pull_requests? && open_pull_requests.blank? && new_version.present?
-          retrieve_pull_requests(formula_or_cask, name, state: "closed", version: pull_request_version)
+        maybe_duplicate_pull_requests = if !args.no_pull_requests? && duplicate_pull_requests.blank?
+          retrieve_pull_requests(formula_or_cask, name)
         end.presence
 
         VersionBumpInfo.new(
@@ -366,8 +365,8 @@ def retrieve_versions_by_arch(formula_or_cask:, repositories:, name:)
           current_version:,
           repology_latest:,
           new_version:,
-          open_pull_requests:,
-          closed_pull_requests:,
+          duplicate_pull_requests:,
+          maybe_duplicate_pull_requests:,
         )
       end
 
@@ -417,8 +416,8 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a
         end
 
         version_label = version_info.version_name
-        open_pull_requests = version_info.open_pull_requests.presence
-        closed_pull_requests = version_info.closed_pull_requests.presence
+        duplicate_pull_requests = version_info.duplicate_pull_requests.presence
+        maybe_duplicate_pull_requests = version_info.maybe_duplicate_pull_requests.presence
 
         ohai title
         puts <<~EOS
@@ -436,8 +435,8 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a
           EOS
         end
         puts <<~EOS unless args.no_pull_requests?
-          Open pull requests:       #{open_pull_requests || "none"}
-          Closed pull requests:     #{closed_pull_requests || "none"}
+          Duplicate pull requests:       #{duplicate_pull_requests       || "none"}
+          Maybe duplicate pull requests: #{maybe_duplicate_pull_requests || "none"}
         EOS
 
         return unless args.open_pr?
@@ -457,7 +456,7 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a
           return
         end
 
-        return if open_pull_requests.present? || closed_pull_requests.present?
+        return if duplicate_pull_requests.present?
 
         version_args = if version_info.multiple_versions
           %W[--version-arm=#{new_version.arm} --version-intel=#{new_version.intel}]

Library/Homebrew/utils/github.rb

@@ -629,7 +629,7 @@ def self.fetch_open_pull_requests(name, tap_remote_repo, version: nil)
     pull_requests || []
   end
 
-  def self.check_for_duplicate_pull_requests(name, tap_remote_repo, state:, file:, quiet:, version: nil)
+  def self.check_for_duplicate_pull_requests(name, tap_remote_repo, file:, quiet:, state: nil, version: nil)
     pull_requests = fetch_pull_requests(name, tap_remote_repo, state:, version:)
 
     pull_requests.select! do |pr|
@@ -639,22 +639,28 @@ def self.check_for_duplicate_pull_requests(name, tap_remote_repo, state:, file:,
     end
     return if pull_requests.blank?
 
+    confidence = version ? "are" : "might be"
     duplicates_message = <<~EOS
-      These #{state} pull requests may be duplicates:
+      These #{state} pull requests #{confidence} duplicates:
       #{pull_requests.map { |pr| "#{pr["title"]} #{pr["html_url"]}" }.join("\n")}
     EOS
     error_message = <<~EOS
-      Duplicate PRs should not be opened.
-      Manually open these PRs if you are sure that they are not duplicates.
+      Duplicate PRs must not be opened.
+      Manually open these PRs if you are sure that they are not duplicates (and tell us that in the PR).
     EOS
 
-    if quiet
-      odie error_message
-    else
+    if version
       odie <<~EOS
         #{duplicates_message.chomp}
         #{error_message}
       EOS
+    elsif quiet
+      opoo error_message
+    else
+      opoo <<~EOS
+        #{duplicates_message.chomp}
+        #{error_message}
+      EOS
     end
   end