style: don't allow patches to be PR/MR urls

[dtrodrigues]

• 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 tests with your changes locally?

---

Having a patch be a PR url is risky because the contents of the PR may change. This prevents linking directly to PRs. The following formulae in the core taps currently have direct links to PRs/MRs:

• adios2.rb
• assimp.rb
• caffe.rb
• conjure-up.rb
• ctl.rb
• etcd.rb
• fontforge.rb
• gitg.rb
• gitless.rb
• h3.rb
• hashpump.rb
• include-what-you-use.rb
• john-jumbo.rb
• komposition.rb
• libcapn.rb
• mtr.rb
• nyancat.rb
• ooniprobe.rb
• pacvim.rb
• prodigal.rb
• pinentry-mac.rb
• scalapack.rb
• shellinabox.rb
• squashfs.rb
• telegram-cli.rb
• terraformer.rb
• tfenv.rb

[claui]

Shall we make merged PRs exempt from that audit?

[MikeMcQuaid]

Shall we make merged PRs exempt from that audit?

Either that or fix them up before this is merged. Note this could also be a RuboCop, I think, which would be preferable.

[dtrodrigues]

I'd rather get them fixed first since I don't think you can tell if a PR is closed without making an API call and it's better for people to be in the habit of not using PR diffs in my opinion.

Doing it as a RuboCop does make more sense, I'll make that switch.

[dtrodrigues]

I've moved the check into a RuboCop and included the sample output below.

Quickly looking at some of them, h3.rb is an example of why even if a PR has been merged, it's not appropriate to have it as a patch because the PR diff hash has changed, so building from source currently does not work. I welcome help getting these migrated over to using their commit hash, getting the relevant changes migrated into the formula-patches repo, and validating if they're even needed anymore.

Style output

$ brew style homebrew/core
== adios2.rb ==
C: 32: 10: https://github.com/ornladios/ADIOS2/pull/2305.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== assimp.rb ==
C: 24: 12: https://github.com/assimp/assimp/pull/1634.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== caffe.rb ==
C: 35: 10: https://github.com/BVLC/caffe/pull/6638.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== conjure-up.rb ==
C:110: 12: https://github.com/go-macaroon-bakery/py-macaroon-bakery/pull/75.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== ctl.rb ==
C: 22: 10: https://github.com/ampas/CTL/pull/73.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
C: 27: 10: https://github.com/ampas/CTL/pull/74.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== etcd.rb ==
C: 21: 10: https://github.com/etcd-io/etcd/pull/11937.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== fontforge.rb ==
C: 39: 10: https://github.com/fontforge/fontforge/pull/4258.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== gitless.rb ==
C: 70: 10: https://github.com/gitless-vcs/gitless/pull/230.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== h3.rb ==
C: 19: 10: https://github.com/uber/h3/pull/362.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== hashpump.rb ==
C: 21: 10: https://github.com/bwall/HashPump/pull/14.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== include-what-you-use.rb ==
C: 21: 10: https://github.com/include-what-you-use/include-what-you-use/pull/807.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== john-jumbo.rb ==
C: 37: 10: https://github.com/magnumripper/JohnTheRipper/pull/4101.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== komposition.rb ==
C: 36: 10: https://github.com/owickstrom/komposition/pull/102.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== libcapn.rb ==
C: 30: 10: https://github.com/adobkin/libcapn/pull/46.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== mtr.rb ==
C: 25: 10: https://github.com/traviscross/mtr/pull/315.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== nyancat.rb ==
C: 18: 10: https://github.com/klange/nyancat/pull/34.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== ooniprobe.rb ==
C:145: 12: https://github.com/pynetwork/pypcap/pull/79.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== pinentry-mac.rb ==
C: 23: 10: https://github.com/GPGTools/pinentry-mac/pull/7.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== scalapack.rb ==
C: 22: 10: https://github.com/Reference-ScaLAPACK/scalapack/pull/23.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== shellinabox.rb ==
C: 22: 10: https://github.com/shellinabox/shellinabox/pull/467.diff?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== squashfs.rb ==
C: 29: 10: https://github.com/plougher/squashfs-tools/pull/69.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== telegram-cli.rb ==
C: 29: 10: https://github.com/vysheng/tg/pull/1306.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== terraformer.rb ==
C: 20: 10: https://github.com/GoogleCloudPlatform/terraformer/pull/535.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
== tfenv.rb ==
C: 19: 12: https://github.com/tfutils/tfenv/pull/181.patch?full_index=1 is s a PR diff whose contents may change. Please link directly to a commit hash.
5147 files inspected, 25 offenses detected

[Rylan12]

When is submitting a patch to formula-patches desired over an inline patch?

If it's something concrete, can rubocop suggest submitting a patch there when applicable?

[jonchang]

I don't think we have any formal guidance on it. If it's a bunch of patches in a series like e.g. Homebrew/homebrew-core#58583 then that might be a good idea. But I don't know if that's a real audit that we could apply consistently.

[Rylan12]

Yeah, probably not. It sounds 1. relatively rare and 2. very obvious to maintainers who can suggest when it's needed

[dtrodrigues]

Thanks for all the PRs @gromit and @Rylan12! I've slightly updated the wording based on review, am now also searching for GitLab MRs, and added a couple tests.

[jonchang]

So it's checking for GitLab URLs now too? Looks good! 👍 from me once brew style is green on homebrew/core.

[dtrodrigues]

Hmm, not sure why CI is failing. Everything being flagged as an error has already been merged into homebrew/core.

[Rylan12]

It's failing on the Linux side. The changes have all been merged into homebrew-core but haven't been merged into linuxbrew-core yet (a manual process that's done by one of the linux maintainers at least once per day). There's some more info here.

I usually just wait for them to be merged (usually no longer than a day) but if you wanted to fix it more quickly you could look into doing the merge yourself. I've never done it but there are instructions at the link.

[dtrodrigues]

There were two linuxbrew-core-only issues I just submitted PRs for. I'll let the rest get merged over via the normal process.