Merge pull request #2790 from GauthamGoli/audit_legacy_patches_rubocop

audit: Port audit_legacy_patches method to rubocop and add tests

Library/Homebrew/dev-cmd/audit.rb

@@ -817,16 +817,6 @@ class FormulaAuditor
     end
   end
 
-  def audit_legacy_patches
-    return unless formula.respond_to?(:patches)
-    legacy_patches = Patch.normalize_legacy_patches(formula.patches).grep(LegacyPatch)
-
-    return if legacy_patches.empty?
-
-    problem "Use the patch DSL instead of defining a 'patches' method"
-    legacy_patches.each { |p| patch_problems(p) }
-  end
-
   def patch_problems(patch)
     case patch.url
     when /raw\.github\.com/, %r{gist\.github\.com/raw}, %r{gist\.github\.com/.+/raw},

Library/Homebrew/rubocops/legacy_patches_cop.rb

@@ -0,0 +1,53 @@
+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
+
+        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
+      end
+    end
+  end
+end

Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb

@@ -0,0 +1,128 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/legacy_patches_cop"
+
+describe RuboCop::Cop::FormulaAudit::LegacyPatches do
+  subject(:cop) { described_class.new }
+
+  context "When auditing legacy patches" do
+    it "When there is no legacy patch" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+        end
+      EOS
+      inspect_source(cop, source)
+      expect(cop.offenses).to eq([])
+    end
+
+    it "Formula with `def patches`" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          homepage "ftp://example.com/foo"
+          url "http://example.com/foo-1.0.tgz"
+          def patches
+            DATA
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "Use the patch DSL instead of defining a 'patches' method",
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    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",
+      ]
+      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"
+            def patches
+              "#{patch_url}"
+            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: 12,
+                                  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: 33,
+                                  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: 5,
+                                  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: 5,
+                                  source: source }]
+        end
+        expected_offenses.zip([cop.offenses.last]).each do |expected, actual|
+          expect_offense(expected, actual)
+        end
+      end
+    end
+
+    it "Formula with nested `def patches`" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          homepage "ftp://example.com/foo"
+          url "http://example.com/foo-1.0.tgz"
+          def patches
+            files = %w[patch-domain_resolver.c patch-colormask.c patch-trafshow.c patch-trafshow.1 patch-configure]
+            {
+              :p0 =>
+              files.collect{|p| "http://trac.macports.org/export/68507/trunk/dports/net/trafshow/files/\#{p}"}
+            }
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "Use the patch DSL instead of defining a 'patches' method",
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source },
+                           {  message: "Patches from MacPorts Trac should be https://, not http:\n"\
+                                       "http://trac.macports.org/export/68507/trunk/dports/net/trafshow/files/",
+                              severity: :convention,
+                              line: 8,
+                              column: 26,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+  end
+end