From fe909c41b80fc4b03bc8f777e96ca64361c81205 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Fri, 30 Aug 2024 08:55:13 +0100 Subject: [PATCH] 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 | 23 +++++-------- Library/Homebrew/dev-cmd/bump-formula-pr.rb | 37 +++++++-------------- Library/Homebrew/dev-cmd/bump.rb | 31 +++++++++-------- Library/Homebrew/utils/github.rb | 20 +++++++---- 4 files changed, 48 insertions(+), 63 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bump-cask-pr.rb b/Library/Homebrew/dev-cmd/bump-cask-pr.rb index a68e733f3d..1f42fda187 100644 --- a/Library/Homebrew/dev-cmd/bump-cask-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-cask-pr.rb @@ -254,25 +254,18 @@ module Homebrew 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? + version_type_version = new_version.instance_variable_get(version_type) + next if version_type_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 = shortened_version(version_type_version, cask:) + GitHub.check_for_duplicate_pull_requests(cask.token, tap_remote_repo, version:, file:, quiet:) end end diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index 6e6c70abee..a1a51c738e 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -134,7 +134,7 @@ module Homebrew 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 @@ module Homebrew 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 @@ module Homebrew 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 @@ module Homebrew 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 diff --git a/Library/Homebrew/dev-cmd/bump.rb b/Library/Homebrew/dev-cmd/bump.rb index 86ca3a1433..2c9986da04 100644 --- a/Library/Homebrew/dev-cmd/bump.rb +++ b/Library/Homebrew/dev-cmd/bump.rb @@ -16,8 +16,8 @@ module Homebrew 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 @@ module Homebrew 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 @@ module Homebrew 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 @@ module Homebrew current_version:, repology_latest:, new_version:, - open_pull_requests:, - closed_pull_requests:, + duplicate_pull_requests:, + maybe_duplicate_pull_requests:, ) end @@ -417,8 +416,8 @@ module Homebrew 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 @@ module Homebrew 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 @@ module Homebrew 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}] diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 6328b489e7..74fbe2afea 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -629,7 +629,7 @@ module GitHub 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 @@ module GitHub 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