diff --git a/Library/Homebrew/rubocops/patches.rb b/Library/Homebrew/rubocops/patches.rb index 9570b851dcc92..1a8859d4525f5 100644 --- a/Library/Homebrew/rubocops/patches.rb +++ b/Library/Homebrew/rubocops/patches.rb @@ -7,7 +7,6 @@ module RuboCop module Cop module FormulaAudit # This cop audits `patch`es in formulae. - # TODO: Many of these could be auto-corrected. class Patches < FormulaCop extend AutoCorrector @@ -22,7 +21,9 @@ def audit_formula(formula_nodes) external_patches.each do |patch_block| url_node = find_every_method_call_by_name(patch_block, :url).first url_string = parameters(url_node).first - patch_problems(url_string) + sha256_node = find_every_method_call_by_name(patch_block, :sha256).first + sha256_string = parameters(sha256_node).first if sha256_node + patch_problems(url_string, sha256_string) end inline_patches = find_every_method_call_by_name(body_node, :patch) @@ -38,13 +39,13 @@ def audit_formula(formula_nodes) legacy_patches = find_strings(patches_node) problem "Use the `patch` DSL instead of defining a `patches` method" - legacy_patches.each { |p| patch_problems(p) } + legacy_patches.each { |p| patch_problems(p, nil) } end private - sig { params(patch_url_node: RuboCop::AST::Node).void } - def patch_problems(patch_url_node) + sig { params(patch_url_node: RuboCop::AST::Node, sha256_node: T.nilable(RuboCop::AST::Node)).void } + def patch_problems(patch_url_node, sha256_node) patch_url = string_content(patch_url_node) if regex_match_group(patch_url_node, %r{https://github.com/[^/]*/[^/]*/pull}) @@ -56,7 +57,12 @@ def patch_problems(patch_url_node) end 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| + # Replace .diff with .patch, keeping either the closing quote or query parameter start + correct = patch_url_node.source.sub(/\.diff(["?])/, '.patch\1') + corrector.replace(patch_url_node.source_range, correct) + corrector.replace(sha256_node.source_range, '""') if sha256_node + end end bitbucket_regex = %r{bitbucket\.org/([^/]+)/([^/]+)/commits/([a-f0-9]+)/raw}i @@ -65,18 +71,28 @@ def patch_problems(patch_url_node) correct_url = "https://api.bitbucket.org/2.0/repositories/#{owner}/#{repo}/diff/#{commit}" problem "Bitbucket patches should use the API URL: #{correct_url}" do |corrector| corrector.replace(patch_url_node.source_range, %Q("#{correct_url}")) + corrector.replace(sha256_node.source_range, '""') if sha256_node end end # Only .diff passes `--full-index` to `git diff` and there is no documented way # to get .patch to behave the same for GitLab. 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| + # Replace .patch with .diff, keeping either the closing quote or query parameter start + correct = patch_url_node.source.sub(/\.patch(["?])/, '.diff\1') + corrector.replace(patch_url_node.source_range, correct) + corrector.replace(sha256_node.source_range, '""') if sha256_node + end end gh_patch_param_pattern = %r{https?://github\.com/.+/.+/(?:commit|pull)/[a-fA-F0-9]*.(?:patch|diff)} if regex_match_group(patch_url_node, gh_patch_param_pattern) && !patch_url.match?(/\?full_index=\w+$/) - problem "GitHub patches should use the full_index parameter: #{patch_url}?full_index=1" + problem "GitHub patches should use the full_index parameter: #{patch_url}?full_index=1" do |corrector| + correct = patch_url_node.source.sub(/"$/, '?full_index=1"') + corrector.replace(patch_url_node.source_range, correct) + corrector.replace(sha256_node.source_range, '""') if sha256_node + end end gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/}, diff --git a/Library/Homebrew/test/rubocops/patches_spec.rb b/Library/Homebrew/test/rubocops/patches_spec.rb index 9d9d568bda1c9..1fc7e23eadc04 100644 --- a/Library/Homebrew/test/rubocops/patches_spec.rb +++ b/Library/Homebrew/test/rubocops/patches_spec.rb @@ -35,8 +35,6 @@ def patches patch_urls = [ "https://raw.github.com/mogaal/sendemail", "https://mirrors.ustc.edu.cn/macports/trunk/", - "http://trac.macports.org/export/102865/trunk/dports/mail/uudeview/files/inews.c.patch", - "http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=patch-libunac1.txt;att=1;bug=623340", "https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch", "https://github.com/dlang/dub/commit/2c916b1a7999a050ac4970c3415ff8f91cd487aa.patch", "https://bitbucket.org/multicoreware/x265_git/commits/b354c009a60bcd6d7fc04014e200a1ee9c45c167/raw", @@ -60,14 +58,6 @@ def patches expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 4, source:) FormulaAudit/Patches: MacPorts patches should specify a revision instead of trunk: #{patch_url} EOS - elsif patch_url.start_with?("http://trac.macports.org/") - expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 4, source:) - FormulaAudit/Patches: Patches from MacPorts Trac should be https://, not http: #{patch_url} - EOS - elsif patch_url.start_with?("http://bugs.debian.org/") - expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 4, source:) - FormulaAudit/Patches: Patches from Debian should be https://, not http: #{patch_url} - EOS # GitHub patch diff regexps can't be any shorter. # rubocop:disable Layout/LineLength elsif patch_url.match?(%r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}) @@ -191,8 +181,6 @@ class Foo < Formula "https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch", "https://github.com/uber/h3/pull/362.patch?full_index=1", "https://gitlab.gnome.org/GNOME/gitg/-/merge_requests/142.diff", - "https://github.com/michaeldv/pit/commit/f64978d.diff?full_index=1", - "https://gitlab.gnome.org/GNOME/msitools/commit/248450a.patch", ] patch_urls.each do |patch_url| source = <<~RUBY @@ -222,14 +210,6 @@ class Foo < Formula expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 8, source:) FormulaAudit/Patches: Use a commit hash URL rather than an unstable merge request URL: #{patch_url} EOS - elsif patch_url.match?(%r{https://github.com/[^/]*/[^/]*/commit/}) - expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 8, source:) - FormulaAudit/Patches: GitHub patches should end with .patch, not .diff: #{patch_url} - EOS - elsif patch_url.match?(%r{.*gitlab.*/commit/}) - expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 8, source:) - FormulaAudit/Patches: GitLab patches should end with .diff, not .patch: #{patch_url} - EOS # GitHub patch diff regexps can't be any shorter. # rubocop:disable Layout/LineLength elsif patch_url.match?(%r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}) @@ -248,7 +228,7 @@ class Foo < Formula end end - context "when auditing auditing external patches with corrector" do + context "when auditing external patches with corrector" do it "corrects Bitbucket patch URLs to use API format" do expect_offense(<<~RUBY) class Foo < Formula @@ -266,7 +246,7 @@ class Foo < Formula url "https://brew.sh/foo-1.0.tgz" patch do url "https://api.bitbucket.org/2.0/repositories/multicoreware/x265_git/diff/b354c009a60bcd6d7fc04014e200a1ee9c45c167" - sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + sha256 "" end end RUBY @@ -317,5 +297,147 @@ class Foo < Formula end RUBY end + + it "corrects GitHub commit URLs from .diff to .patch" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://github.com/michaeldv/pit/commit/f64978d.diff" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/Patches: GitHub patches should end with .patch, not .diff: https://github.com/michaeldv/pit/commit/f64978d.diff + sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://github.com/michaeldv/pit/commit/f64978d.patch?full_index=1" + sha256 "" + end + end + RUBY + end + + it "corrects GitLab commit URLs from .patch to .diff" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://gitlab.com/inkscape/lib2geom/-/commit/0b8b4c26b4a.patch" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/Patches: GitLab patches should end with .diff, not .patch: https://gitlab.com/inkscape/lib2geom/-/commit/0b8b4c26b4a.patch + sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://gitlab.com/inkscape/lib2geom/-/commit/0b8b4c26b4a.diff" + sha256 "" + end + end + RUBY + end + + it "corrects GitHub patch URLs to add full_index parameter" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://github.com/foo/bar/commit/abc123.patch" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/Patches: GitHub patches should use the full_index parameter: https://github.com/foo/bar/commit/abc123.patch?full_index=1 + sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://github.com/foo/bar/commit/abc123.patch?full_index=1" + sha256 "" + end + end + RUBY + end + + it "corrects GitHub URLs with 'diff' in the path" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://github.com/diff-tool/diff-utils/commit/abc123.diff" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/Patches: GitHub patches should end with .patch, not .diff: https://github.com/diff-tool/diff-utils/commit/abc123.diff + sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://github.com/diff-tool/diff-utils/commit/abc123.patch?full_index=1" + sha256 "" + end + end + RUBY + end + + it "corrects GitLab URLs with 'patch' in the path" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://gitlab.com/patch-tool/patch-utils/-/commit/abc123.patch" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/Patches: GitLab patches should end with .diff, not .patch: https://gitlab.com/patch-tool/patch-utils/-/commit/abc123.patch + sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch do + url "https://gitlab.com/patch-tool/patch-utils/-/commit/abc123.diff" + sha256 "" + end + end + RUBY + end + + it "corrects GitHub URLs without sha256 field (e.g. with on_linux block)" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch :p2 do + on_linux do + url "https://github.com/foo/bar/commit/abc123.diff" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/Patches: GitHub patches should end with .patch, not .diff: https://github.com/foo/bar/commit/abc123.diff + directory "gl" + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + patch :p2 do + on_linux do + url "https://github.com/foo/bar/commit/abc123.patch?full_index=1" + directory "gl" + end + end + end + RUBY + end end end