admin/brew
1
0
Fork 0
Code Issues Packages Projects Releases 6 Wiki Activity

Improve duplicate pull request handling

Browse Source 
- 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
This commit is contained in:
Mike McQuaid 2024-08-30 08:55:13 +01:00
parent 85c9c17ddf
commit fe909c41b8
No known key found for this signature in database
4 changed files with 48 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
Library/Homebrew/dev-cmd/bump-cask-pr.rb
View File

@ -254,25 +254,18 @@ module Homebrew
def check_pull_requests(cask, new_version:) def check_pull_requests(cask, new_version:)
tap_remote_repo = cask.tap.full_name || cask.tap.remote_repo 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, GitHub.check_for_duplicate_pull_requests(cask.token, tap_remote_repo,
state: "open", state: "open", file:, quiet:)
version: nil,
file: cask.sourcefile_path.relative_path_from(cask.tap.path).to_s,
quiet: args.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| new_version.instance_variables.each do |version_type|
version = new_version.instance_variable_get(version_type) version_type_version = new_version.instance_variable_get(version_type)
next if version.blank? next if version_type_version.blank?
GitHub.check_for_duplicate_pull_requests( version = shortened_version(version_type_version, cask:)
cask.token, GitHub.check_for_duplicate_pull_requests(cask.token, tap_remote_repo, version:, file:, quiet:)
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?,
)
end end
end end

33
Library/Homebrew/dev-cmd/bump-formula-pr.rb
View File

@ -134,7 +134,7 @@ module Homebrew
remote_branch = formula.tap.git_repository.origin_branch_name remote_branch = formula.tap.git_repository.origin_branch_name
previous_branch = "-" previous_branch = "-"
check_open_pull_requests(formula, tap_remote_repo) check_pull_requests(formula, tap_remote_repo, state: "open")
new_version = args.version new_version = args.version
check_new_version(formula, tap_remote_repo, version: new_version) if new_version.present? check_new_version(formula, tap_remote_repo, version: new_version) if new_version.present?
@ -465,14 +465,19 @@ module Homebrew
end end
end end
sig { params(formula: Formula, tap_remote_repo: String).returns(T.nilable(T::Array[String])) } sig {
def check_open_pull_requests(formula, tap_remote_repo) 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 tap = formula.tap
return if tap.nil? 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( GitHub.check_for_duplicate_pull_requests(
formula.name, tap_remote_repo, formula.name, tap_remote_repo,
state: "open", version:,
state:,
file: formula.path.relative_path_from(tap.path).to_s, file: formula.path.relative_path_from(tap.path).to_s,
quiet: args.quiet? quiet: args.quiet?
) )
@ -493,7 +498,7 @@ module Homebrew
end end
check_throttle(formula, version) check_throttle(formula, version)
check_closed_pull_requests(formula, tap_remote_repo, version:) check_pull_requests(formula, tap_remote_repo, version:)
end end
sig { params(formula: Formula, new_version: String).void } 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}" odie "#{formula} should only be updated every #{throttled_rate} releases on multiples of #{throttled_rate}"
end 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])) } sig { params(formula: Formula, new_formula_version: Version).returns(T.nilable(T::Array[String])) }
def alias_update_pair(formula, new_formula_version) def alias_update_pair(formula, new_formula_version)
versioned_alias = formula.aliases.grep(/^.*@\d+(\.\d+)?$/).first versioned_alias = formula.aliases.grep(/^.*@\d+(\.\d+)?$/).first

31
Library/Homebrew/dev-cmd/bump.rb
View File

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

20
Library/Homebrew/utils/github.rb
View File

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