GitHub::check_for_duplicate_pull_requests should have a strict failure mode

[woodruffw]

Verification

• This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

Provide a detailed description of the proposed feature

Filing this for myself to implement once I have a moment 🙂

TL;DR: I'd like to add a strict kwarg to check_for_duplicate_pull_requests that restores the "fail hard" behavior. This kwarg will be false by default to make it compatible with the current behavior, while consumers like brew-pip-audit can set it to true to restore their previous API expectations.

What is the motivation for the feature?

check_for_duplicate_pull_requests provides the duplicate PR checking functionality for a bunch of dev commands, as well as for brew-pip-audit.

Starting with #18206, the behavior of check_for_duplicate_pull_requests changed slightly: before it would fail hard (with an exit) if a duplicate existed, and after it only fails hard if being run on an official tap and the version kwarg is present. In all other cases, the duplicate check now fails "softly" by emitting a warning.

This works well for the common use case (deduplicating version bump PRs), but it unfortunately means that brew-pip-audit now creates duplicate PRs some of the time, e.g. Homebrew/homebrew-core#221601 and Homebrew/homebrew-core#221607.

How will the feature be relevant to at least 90% of Homebrew users?

This will make brew-pip-audit less noisy (like it was before), which in turn will reduce maintainer load/manual work when triaging brew-pip-audit PRs.

What alternatives to the feature have been considered?

Alternatives:

1. Do nothing and keep submitting duplicate PRs. This seems non-ideal in terms of noise/maintainer overhead, but we could do it.
2. Refactor check_for_duplicate_pull_requests more broadly to be a "pure" API, i.e. use exceptions and return types to signal its status instead of emitting warnings and failing directly. This is probably conceptually cleaner, but it requires a more intensive set of changes (e.g. within all of the consuming dev-cmds) and would have larger external API compat implications. However, if people prefer this route, I can go this way instead 🙂

CC @alex

[woodruffw]

For context, here is the current warning/failure logic:

brew/Library/Homebrew/utils/github.rb

Lines 662 to 677 in 0e7f19f

	if !official_tap
	opoo duplicates_message
	elsif 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

[MikeMcQuaid]

@woodruffw Definitely game to have a stricter opt-in mode here 👍🏻

[Bo98]

would have larger external API compat implications

Worth noting none of utils/github is public API so this shouldn't be a concern unless you see evidence of widespread usage.

[woodruffw]

Worth noting none of utils/github is public API so this shouldn't be a concern unless you see evidence of widespread usage.

Nope, I have no evidence -- I imagine some people are using it, but I didn't realize it was a private API. That changes my opinion on the break budget towards doing things more generally here 🙂

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.