Skip to content

Conversation

@Moisan
Copy link
Member

@Moisan Moisan commented Oct 25, 2025

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Add correctors for all problems in the patches audit that can be automatically fixed. It also remove a few redundant tests.

Copilot AI review requested due to automatic review settings October 25, 2025 18:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds automatic correctors for patches audit violations that can be automatically fixed, while removing redundant test cases that are now covered by the corrector tests.

  • Adds correctors for GitHub .diff to .patch conversion, GitLab .patch to .diff conversion, and GitHub full_index parameter addition
  • Removes redundant test cases for MacPorts Trac HTTP patches and Debian HTTP patches
  • Removes the TODO comment indicating correctors were needed

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Library/Homebrew/rubocops/patches.rb Adds corrector blocks to three patch URL validation rules and removes completed TODO comment
Library/Homebrew/test/rubocops/patches_spec.rb Adds three new corrector tests and removes redundant test cases for HTTP patch URLs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if regex_match_group(patch_url_node, %r{https://github.com/[^/]*/[^/]*/commit/[a-fA-F0-9]*\.diff})
problem "GitHub patches should end with .patch, not .diff: #{patch_url}"
problem "GitHub patches should end with .patch, not .diff: #{patch_url}" do |corrector|
correct = patch_url_node.source.gsub(".diff", ".patch")
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using gsub could incorrectly replace .diff occurring elsewhere in the URL (e.g., in a repository name like my-diff-tool). Use sub(/.diff\"$/, '.patch\"') to only replace the extension at the end of the URL string.

Suggested change
correct = patch_url_node.source.gsub(".diff", ".patch")
correct = patch_url_node.source.sub(/\.diff"$/, '.patch"')

Copilot uses AI. Check for mistakes.
if regex_match_group(patch_url_node, %r{.*gitlab.*/commit/[a-fA-F0-9]*\.patch})
problem "GitLab patches should end with .diff, not .patch: #{patch_url}"
problem "GitLab patches should end with .diff, not .patch: #{patch_url}" do |corrector|
correct = patch_url_node.source.gsub(".patch", ".diff")
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using gsub could incorrectly replace .patch occurring elsewhere in the URL (e.g., in a repository name). Use sub(/.patch\"$/, '.diff\"') to only replace the extension at the end of the URL string.

Suggested change
correct = patch_url_node.source.gsub(".patch", ".diff")
correct = patch_url_node.source.sub(/\.patch"$/, '.diff"')

Copilot uses AI. Check for mistakes.
Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-correcting the patch URL will invalidate its checksum because the contents are different (as in, .patch shows the commit in email format while .diff shows only the diff). Can that be handled any better?

Just thinking out loud, LGTM besides that. Thanks!

@MikeMcQuaid
Copy link
Member

Auto-correcting the patch URL will invalidate its checksum because the contents are different (as in, .patch shows the commit in email format while .diff shows only the diff). Can that be handled any better?

should probably remove the sha256 if changing extension.

@Moisan
Copy link
Member Author

Moisan commented Oct 28, 2025

should probably remove the sha256 if changing extension

Good idea, I also removed it when adding full_index=1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants