Skip to content

rubocops/patches: GitHub/GitLab patches should end with .patch #8701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

SeekingMeaning
Copy link
Contributor

@SeekingMeaning SeekingMeaning commented Sep 11, 2020

  • 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?

See discussion below

Old description

GitLab doesn't support ?full_index=1 for patch URLs, but instead we can use .diff to get full indices. This PR adds a check for GitLab patches ending with .patch that will be reported by brew audit

brew audit gstreamer:

gstreamer:
  * C: 33: col 10: GitLab patches should end with .diff, not .patch:
      https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/40c994cddee41954053f80dde40f56107ddfcfd4.patch
Error: 1 problem in 1 formula detected

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/40c994c.patch (short indices):

index eefe1c9d646..4370acd9562 100755

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/40c994c.diff (full indices):

index eefe1c9d646d81fc0f09a81e11482449f4f4e144..4370acd956210fe23abea91da162f0aa6c02e26f 100755

Sorry, something went wrong.

@MikeMcQuaid
Copy link
Member

Have this broken recently/before? The patch is slightly preferable, in my book, due to the increased metadata.

@SeekingMeaning
Copy link
Contributor Author

Have this broken recently/before?

No

The patch is slightly preferable, in my book, due to the increased metadata.

I think it's okay to not have the patch metadata since we already usually add a comment to the merge request, for example: https://github.com/Homebrew/homebrew-core/blob/b599f82/Formula/gstreamer.rb#L31

Although, I'm fine with enforcing .patch instead — currently we're using a mix of both .patch and .diff in homebrew/core

@MikeMcQuaid
Copy link
Member

Although, I'm fine with enforcing .patch instead — currently we're using a mix of both .patch and .diff in homebrew/core

👍🏻 I'm OK with that, I don't feel strongly either way but I would also like to have them be consistent.

@SeekingMeaning SeekingMeaning changed the title rubocops/patches: GitLab patches should end with .diff rubocops/patches: GitHub/GitLab patches should end with .patch Sep 11, 2020
@SeekingMeaning SeekingMeaning force-pushed the rubocops/gitlab-patches branch 2 times, most recently from ed103b0 to 9baabfe Compare September 11, 2020 17:03
@claui
Copy link
Contributor

claui commented Sep 13, 2020

I think it's okay to not have the patch metadata since we already usually add a comment to the merge request

Metadata can be important when backporting multi-commit patches (example).

However, those are rare, plus they typically go on formula-patches. LGTM 👍

@SeekingMeaning
Copy link
Contributor Author

Currently failing because of existing patches ending with .diff — would it be acceptable to change these to .patch as CI-syntax-only changes?

@MikeMcQuaid
Copy link
Member

Yes please @SeekingMeaning!

@stale
Copy link

stale bot commented Oct 12, 2020

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

@stale stale bot added the stale No recent activity label Oct 12, 2020
@jonchang jonchang mentioned this pull request Oct 18, 2020
5 tasks
@jonchang jonchang force-pushed the rubocops/gitlab-patches branch from 9baabfe to 2f46cc2 Compare October 20, 2020 11:42
@stale stale bot removed the stale No recent activity label Oct 20, 2020

Verified

This commit was signed with the committer’s verified signature.
dawidd6 Dawid Dziurla
@jonchang jonchang force-pushed the rubocops/gitlab-patches branch from 2f46cc2 to ea10192 Compare October 20, 2020 13:04
@jonchang
Copy link
Contributor

Gonna merge this since the last stragglers have been fixed on both macOS and Linux. Thanks @SeekingMeaning!

@jonchang jonchang merged commit f8f7970 into Homebrew:master Oct 20, 2020
@fxcoudert
Copy link
Member

This means that if we're re-running old workflows, we get tons of errors unrelated to the pull request being tested: https://github.com/Homebrew/homebrew-core/pull/62208/checks?check_run_id=1297673335

Could this be avoided?

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 1, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants