diff options
| -rw-r--r-- | Library/Homebrew/rubocops.rb | 2 | ||||
| -rw-r--r-- | Library/Homebrew/rubocops/extend/formula_cop.rb | 34 | ||||
| -rw-r--r-- | Library/Homebrew/rubocops/legacy_patches_cop.rb | 30 | ||||
| -rw-r--r-- | Library/Homebrew/rubocops/patches_cop.rb | 62 | ||||
| -rw-r--r-- | Library/Homebrew/test/rubocops/patches_cop_spec.rb (renamed from Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb) | 77 |
5 files changed, 138 insertions, 67 deletions
diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb index 4323e044c..b1144e075 100644 --- a/Library/Homebrew/rubocops.rb +++ b/Library/Homebrew/rubocops.rb @@ -6,7 +6,7 @@ require_relative "./rubocops/homepage_cop" require_relative "./rubocops/text_cop" require_relative "./rubocops/caveats_cop" require_relative "./rubocops/checksum_cop" -require_relative "./rubocops/legacy_patches_cop" +require_relative "./rubocops/patches_cop" require_relative "./rubocops/conflicts_cop" require_relative "./rubocops/options_cop" require_relative "./rubocops/urls_cop" diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index e4a9a5b45..4be0c0fe3 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -380,40 +380,6 @@ module RuboCop add_offense(@offensive_node, @offense_source_range, msg) end - def patch_problems(patch) - patch_url = string_content(patch) - gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/}, - %r{gist\.github\.com/raw}, - %r{gist\.github\.com/.+/raw}, - %r{gist\.githubusercontent\.com/.+/raw}]) - if regex_match_group(patch, gh_patch_patterns) - unless patch_url =~ /[a-fA-F0-9]{40}/ - problem "GitHub/Gist patches should specify a revision:\n#{patch_url}" - end - end - - gh_patch_diff_pattern = %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)} - if match_obj = regex_match_group(patch, gh_patch_diff_pattern) - problem <<-EOS.undent - use GitHub pull request URLs: - https://github.com/#{match_obj[1]}/#{match_obj[2]}/pull/#{match_ojb[3]}.patch - Rather than patch-diff: - #{patch_url} - EOS - end - - if regex_match_group(patch, %r{macports/trunk}) - problem "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}" - end - - if regex_match_group(patch, %r{^http://trac\.macports\.org}) - problem "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}" - end - - return unless regex_match_group(patch, %r{^http://bugs\.debian\.org}) - problem "Patches from Debian should be https://, not http:\n#{patch_url}" - end - private def formula_class?(node) diff --git a/Library/Homebrew/rubocops/legacy_patches_cop.rb b/Library/Homebrew/rubocops/legacy_patches_cop.rb deleted file mode 100644 index 25a580b12..000000000 --- a/Library/Homebrew/rubocops/legacy_patches_cop.rb +++ /dev/null @@ -1,30 +0,0 @@ -require_relative "./extend/formula_cop" -require_relative "../extend/string" - -module RuboCop - module Cop - module FormulaAudit - # This cop checks for and audits legacy patches in Formulae - class LegacyPatches < FormulaCop - def audit_formula(_node, _class_node, _parent_class_node, body) - patches_node = find_method_def(body, :patches) - return if patches_node.nil? - legacy_patches = find_strings(patches_node) - problem "Use the patch DSL instead of defining a 'patches' method" - legacy_patches.each { |p| patch_problems(p) } - end - end - - class ExternalPatches < FormulaCop - def audit_formula(_node, _class_node, _parent_class_node, body) - patches = find_all_blocks(body, :patch) - patches.each do |patch_block| - url_node = find_node_method_by_name(patch_block, :url) - url_string = parameters(url_node).first - patch_problems(url_string) - end - end - end - end - end -end diff --git a/Library/Homebrew/rubocops/patches_cop.rb b/Library/Homebrew/rubocops/patches_cop.rb new file mode 100644 index 000000000..fb14d8acc --- /dev/null +++ b/Library/Homebrew/rubocops/patches_cop.rb @@ -0,0 +1,62 @@ +require_relative "./extend/formula_cop" +require_relative "../extend/string" + +module RuboCop + module Cop + module FormulaAudit + # This cop audits patches in Formulae + class Patches < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body) + external_patches = find_all_blocks(body, :patch) + 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) + end + + patches_node = find_method_def(body, :patches) + return if patches_node.nil? + legacy_patches = find_strings(patches_node) + problem "Use the patch DSL instead of defining a 'patches' method" + legacy_patches.each { |p| patch_problems(p) } + end + + private + + def patch_problems(patch) + patch_url = string_content(patch) + gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/}, + %r{gist\.github\.com/raw}, + %r{gist\.github\.com/.+/raw}, + %r{gist\.githubusercontent\.com/.+/raw}]) + if regex_match_group(patch, gh_patch_patterns) + unless patch_url =~ /[a-fA-F0-9]{40}/ + problem "GitHub/Gist patches should specify a revision:\n#{patch_url}" + end + end + + gh_patch_diff_pattern = %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)} + if match_obj = regex_match_group(patch, gh_patch_diff_pattern) + problem <<-EOS.undent + use GitHub pull request URLs: + https://github.com/#{match_obj[1]}/#{match_obj[2]}/pull/#{match_obj[3]}.patch + Rather than patch-diff: + #{patch_url} + EOS + end + + if regex_match_group(patch, %r{macports/trunk}) + problem "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}" + end + + if regex_match_group(patch, %r{^http://trac\.macports\.org}) + problem "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}" + end + + return unless regex_match_group(patch, %r{^http://bugs\.debian\.org}) + problem "Patches from Debian should be https://, not http:\n#{patch_url}" + end + end + end + end +end diff --git a/Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb b/Library/Homebrew/test/rubocops/patches_cop_spec.rb index a08fa614d..4bd79bf35 100644 --- a/Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/patches_cop_spec.rb @@ -1,9 +1,9 @@ require "rubocop" require "rubocop/rspec/support" require_relative "../../extend/string" -require_relative "../../rubocops/legacy_patches_cop" +require_relative "../../rubocops/patches_cop" -describe RuboCop::Cop::FormulaAudit::LegacyPatches do +describe RuboCop::Cop::FormulaAudit::Patches do subject(:cop) { described_class.new } context "When auditing legacy patches" do @@ -47,6 +47,7 @@ describe RuboCop::Cop::FormulaAudit::LegacyPatches do "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", ] patch_urls.each do |patch_url| source = <<-EOS.undent @@ -84,6 +85,15 @@ describe RuboCop::Cop::FormulaAudit::LegacyPatches do line: 5, column: 5, source: source }] + elsif patch_url =~ %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)} + expected_offenses = [{ message: "use GitHub pull request URLs:\n"\ + " https://github.com/foo/foo-bar/pull/100.patch\n"\ + "Rather than patch-diff:\n"\ + " https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch\n", + severity: :convention, + line: 5, + column: 5, + source: source }] end expected_offenses.zip([cop.offenses.last]).each do |expected, actual| expect_offense(expected, actual) @@ -125,4 +135,67 @@ describe RuboCop::Cop::FormulaAudit::LegacyPatches do end end end + + context "When auditing external patches" do + it "Patch URLs" do + 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", + ] + patch_urls.each do |patch_url| + source = <<-EOS.undent + class Foo < Formula + homepage "ftp://example.com/foo" + url "http://example.com/foo-1.0.tgz" + patch do + url "#{patch_url}" + sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621" + end + end + EOS + + inspect_source(cop, source) + if patch_url =~ %r{/raw\.github\.com/} + expected_offenses = [{ message: "GitHub/Gist patches should specify a revision:\n#{patch_url}", + severity: :convention, + line: 5, + column: 16, + source: source }] + elsif patch_url =~ %r{macports/trunk} + expected_offenses = [{ message: "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}", + severity: :convention, + line: 5, + column: 37, + source: source }] + elsif patch_url =~ %r{^http://trac\.macports\.org} + expected_offenses = [{ message: "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}", + severity: :convention, + line: 5, + column: 9, + source: source }] + elsif patch_url =~ %r{^http://bugs\.debian\.org} + expected_offenses = [{ message: "Patches from Debian should be https://, not http:\n#{patch_url}", + severity: :convention, + line: 5, + column: 9, + source: source }] + elsif patch_url =~ %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)} + expected_offenses = [{ message: "use GitHub pull request URLs:\n"\ + " https://github.com/foo/foo-bar/pull/100.patch\n"\ + "Rather than patch-diff:\n"\ + " https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch\n", + severity: :convention, + line: 5, + column: 9, + source: source }] + end + expected_offenses.zip([cop.offenses.last]).each do |expected, actual| + expect_offense(expected, actual) + end + end + end + end end |