audit: Update Urls Cop with more rules of audit_urls and corresponding tests

3 files changed, 250 insertions, 117 deletions

diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb
index 327c3e811..b15d719d2 100644
--- a/Library/Homebrew/dev-cmd/audit.rb
+++ b/Library/Homebrew/dev-cmd/audit.rb
@@ -1283,120 +1283,6 @@ class ResourceAuditor
 
   def audit_urls
     urls = [url] + mirrors
-    # Prefer HTTP/S when possible over FTP protocol due to possible firewalls.
-    urls.each do |p|
-      case p
-      when %r{^ftp://ftp\.mirrorservice\.org}
-        problem "Please use https:// for #{p}"
-      when %r{^ftp://ftp\.cpan\.org/pub/CPAN(.*)}i
-        problem "#{p} should be `http://search.cpan.org/CPAN#{Regexp.last_match(1)}`"
-      end
-    end
-
-    # Check SourceForge urls
-    urls.each do |p|
-      # Skip if the URL looks like a SVN repo
-      next if p.include? "/svnroot/"
-      next if p.include? "svn.sourceforge"
-
-      # Is it a sourceforge http(s) URL?
-      next unless p =~ %r{^https?://.*\b(sourceforge|sf)\.(com|net)}
-
-      if p =~ /(\?|&)use_mirror=/
-        problem "Don't use #{Regexp.last_match(1)}use_mirror in SourceForge urls (url is #{p})."
-      end
-
-      if p.end_with?("/download")
-        problem "Don't use /download in SourceForge urls (url is #{p})."
-      end
-
-      if p =~ %r{^https?://sourceforge\.}
-        problem "Use https://downloads.sourceforge.net to get geolocation (url is #{p})."
-      end
-
-      if p =~ %r{^https?://prdownloads\.}
-        problem "Don't use prdownloads in SourceForge urls (url is #{p}).\n" \
-                "\tSee: http://librelist.com/browser/homebrew/2011/1/12/prdownloads-is-bad/"
-      end
-
-      if p =~ %r{^http://\w+\.dl\.}
-        problem "Don't use specific dl mirrors in SourceForge urls (url is #{p})."
-      end
-
-      problem "Please use https:// for #{p}" if p.start_with? "http://downloads"
-    end
-
-    # Debian has an abundance of secure mirrors. Let's not pluck the insecure
-    # one out of the grab bag.
-    urls.each do |u|
-      next unless u =~ %r{^http://http\.debian\.net/debian/(.*)}i
-      problem <<-EOS.undent
-        Please use a secure mirror for Debian URLs.
-        We recommend:
-          https://mirrors.ocf.berkeley.edu/debian/#{Regexp.last_match(1)}
-      EOS
-    end
-
-    # Check for Google Code download urls, https:// is preferred
-    # Intentionally not extending this to SVN repositories due to certificate
-    # issues.
-    urls.grep(%r{^http://.*\.googlecode\.com/files.*}) do |u|
-      problem "Please use https:// for #{u}"
-    end
-
-    # Check for new-url Google Code download urls, https:// is preferred
-    urls.grep(%r{^http://code\.google\.com/}) do |u|
-      problem "Please use https:// for #{u}"
-    end
-
-    # Check for git:// GitHub repo urls, https:// is preferred.
-    urls.grep(%r{^git://[^/]*github\.com/}) do |u|
-      problem "Please use https:// for #{u}"
-    end
-
-    # Check for git:// Gitorious repo urls, https:// is preferred.
-    urls.grep(%r{^git://[^/]*gitorious\.org/}) do |u|
-      problem "Please use https:// for #{u}"
-    end
-
-    # Check for http:// GitHub repo urls, https:// is preferred.
-    urls.grep(%r{^http://github\.com/.*\.git$}) do |u|
-      problem "Please use https:// for #{u}"
-    end
-
-    # Check for master branch GitHub archives.
-    urls.grep(%r{^https://github\.com/.*archive/master\.(tar\.gz|zip)$}) do
-      problem "Use versioned rather than branch tarballs for stable checksums."
-    end
-
-    # Use new-style archive downloads
-    urls.each do |u|
-      next unless u =~ %r{https://.*github.*/(?:tar|zip)ball/} && u !~ /\.git$/
-      problem "Use /archive/ URLs for GitHub tarballs (url is #{u})."
-    end
-
-    # Don't use GitHub .zip files
-    urls.each do |u|
-      next unless u =~ %r{https://.*github.*/(archive|releases)/.*\.zip$} && u !~ %r{releases/download}
-      problem "Use GitHub tarballs rather than zipballs (url is #{u})."
-    end
-
-    # Don't use GitHub codeload URLs
-    urls.each do |u|
-      next unless u =~ %r{https?://codeload\.github\.com/(.+)/(.+)/(?:tar\.gz|zip)/(.+)}
-      problem <<-EOS.undent
-        use GitHub archive URLs:
-          https://github.com/#{Regexp.last_match(1)}/#{Regexp.last_match(2)}/archive/#{Regexp.last_match(3)}.tar.gz
-        Rather than codeload:
-          #{u}
-      EOS
-    end
-
-    # Check for Maven Central urls, prefer HTTPS redirector over specific host
-    urls.each do |u|
-      next unless u =~ %r{https?://(?:central|repo\d+)\.maven\.org/maven2/(.+)$}
-      problem "#{u} should be `https://search.maven.org/remotecontent?filepath=#{Regexp.last_match(1)}`"
-    end
 
     if name == "curl" && !urls.find { |u| u.start_with?("http://") }
       problem "should always include at least one HTTP url"
diff --git a/Library/Homebrew/rubocops/urls_cop.rb b/Library/Homebrew/rubocops/urls_cop.rb
index 830b68ead..94f049aed 100644
--- a/Library/Homebrew/rubocops/urls_cop.rb
+++ b/Library/Homebrew/rubocops/urls_cop.rb
@@ -71,6 +71,122 @@ module RuboCop
           audit_urls(urls, debian_pattern) do |match, url|
             problem "#{url} should be `https://anonscm.debian.org/git/users/#{match[1]}`"
           end
+
+          # Prefer HTTP/S when possible over FTP protocol due to possible firewalls.
+          mirror_service_pattern = %r{^ftp://ftp\.mirrorservice\.org}
+          audit_urls(urls, mirror_service_pattern) do |_, url|
+            problem "Please use https:// for #{url}"
+          end
+
+          cpan_ftp_pattern = %r{^ftp://ftp\.cpan\.org/pub/CPAN(.*)}i
+          audit_urls(urls, cpan_ftp_pattern) do |match_obj, url|
+            problem "#{url} should be `http://search.cpan.org/CPAN#{match_obj[1]}`"
+          end
+
+          # SourceForge url patterns
+          sourceforge_patterns = %r{^https?://.*\b(sourceforge|sf)\.(com|net)}
+          audit_urls(urls, sourceforge_patterns) do |_, url|
+            # Skip if the URL looks like a SVN repo
+            next if url.include? "/svnroot/"
+            next if url.include? "svn.sourceforge"
+            next if url.include? "/p/"
+
+            if url =~ /(\?|&)use_mirror=/
+              problem "Don't use #{Regexp.last_match(1)}use_mirror in SourceForge urls (url is #{url})."
+            end
+
+            if url.end_with?("/download")
+              problem "Don't use /download in SourceForge urls (url is #{url})."
+            end
+
+            if url =~ %r{^https?://sourceforge\.}
+              problem "Use https://downloads.sourceforge.net to get geolocation (url is #{url})."
+            end
+
+            if url =~ %r{^https?://prdownloads\.}
+              problem "Don't use prdownloads in SourceForge urls (url is #{url}).\n" \
+                "\tSee: http://librelist.com/browser/homebrew/2011/1/12/prdownloads-is-bad/"
+            end
+
+            if url =~ %r{^http://\w+\.dl\.}
+              problem "Don't use specific dl mirrors in SourceForge urls (url is #{url})."
+            end
+
+            problem "Please use https:// for #{url}" if url.start_with? "http://downloads"
+          end
+
+          # Debian has an abundance of secure mirrors. Let's not pluck the insecure
+          # one out of the grab bag.
+          unsecure_deb_pattern = %r{^http://http\.debian\.net/debian/(.*)}i
+          audit_urls(urls, unsecure_deb_pattern) do |match, _|
+            problem <<-EOS.undent
+              Please use a secure mirror for Debian URLs.
+              We recommend:
+                https://mirrors.ocf.berkeley.edu/debian/#{match[1]}
+            EOS
+          end
+
+          # Check for new-url Google Code download urls, https:// is preferred
+          google_code_pattern = Regexp.union([%r{^http://.*\.googlecode\.com/files.*},
+                                              %r{^http://code\.google\.com/}])
+          audit_urls(urls, google_code_pattern) do |_, url|
+            problem "Please use https:// for #{url}"
+          end
+
+          # Check for git:// GitHub repo urls, https:// is preferred.
+          git_gh_pattern = %r{^git://[^/]*github\.com/}
+          audit_urls(urls, git_gh_pattern) do |_, url|
+            problem "Please use https:// for #{url}"
+          end
+
+          # Check for git:// Gitorious repo urls, https:// is preferred.
+          git_gitorious_pattern = %r{^git://[^/]*gitorious\.org/}
+          audit_urls(urls, git_gitorious_pattern) do |_, url|
+            problem "Please use https:// for #{url}"
+          end
+
+          # Check for http:// GitHub repo urls, https:// is preferred.
+          gh_pattern = %r{^http://github\.com/.*\.git$}
+          audit_urls(urls, gh_pattern) do |_, url|
+            problem "Please use https:// for #{url}"
+          end
+
+          # Check for master branch GitHub archives.
+          tarball_gh_pattern = %r{^https://github\.com/.*archive/master\.(tar\.gz|zip)$}
+          audit_urls(urls, tarball_gh_pattern) do
+            problem "Use versioned rather than branch tarballs for stable checksums."
+          end
+
+          # Use new-style archive downloads
+          archive_gh_pattern = %r{https://.*github.*/(?:tar|zip)ball/}
+          audit_urls(urls, archive_gh_pattern) do |_, url|
+            next unless url !~ /\.git$/
+            problem "Use /archive/ URLs for GitHub tarballs (url is #{url})."
+          end
+
+          # Don't use GitHub .zip files
+          zip_gh_pattern = %r{https://.*github.*/(archive|releases)/.*\.zip$}
+          audit_urls(urls, zip_gh_pattern) do |_, url|
+            next unless url !~ %r{releases/download}
+            problem "Use GitHub tarballs rather than zipballs (url is #{url})."
+          end
+
+          # Don't use GitHub codeload URLs
+          codeload_gh_pattern = %r{https?://codeload\.github\.com/(.+)/(.+)/(?:tar\.gz|zip)/(.+)}
+          audit_urls(urls, codeload_gh_pattern) do |match, url|
+            problem <<-EOS.undent
+              Use GitHub archive URLs:
+                https://github.com/#{match[1]}/#{match[2]}/archive/#{match[3]}.tar.gz
+              Rather than codeload:
+                #{url}
+            EOS
+          end
+
+          # Check for Maven Central urls, prefer HTTPS redirector over specific host
+          maven_pattern = %r{https?://(?:central|repo\d+)\.maven\.org/maven2/(.+)$}
+          audit_urls(urls, maven_pattern) do |match, url|
+            problem "#{url} should be `https://search.maven.org/remotecontent?filepath=#{match[1]}`"
+          end
         end
 
         private
@@ -80,8 +196,9 @@ module RuboCop
             url_string_node = parameters(url_node).first
             url_string = string_content(url_string_node)
             match_object = regex_match_group(url_string_node, regex)
-            offending_node(url_string_node.parent) if match_object
-            yield match_object, url_string if match_object
+            next unless match_object
+            offending_node(url_string_node.parent)
+            yield match_object, url_string
           end
         end
       end
diff --git a/Library/Homebrew/test/rubocops/urls_cop_spec.rb b/Library/Homebrew/test/rubocops/urls_cop_spec.rb
index 2e56dbf03..733732bd0 100644
--- a/Library/Homebrew/test/rubocops/urls_cop_spec.rb
+++ b/Library/Homebrew/test/rubocops/urls_cop_spec.rb
@@ -28,8 +28,82 @@ describe RuboCop::Cop::FormulaAudit::Urls do
         "url" => "http://ftp.gnome.org/pub/GNOME/binaries/mac/banshee/banshee-2.macosx.intel.dmg",
         "msg" => "http://ftp.gnome.org/pub/GNOME/binaries/mac/banshee/banshee-2.macosx.intel.dmg should be `https://download.gnome.org/binaries/mac/banshee/banshee-2.macosx.intel.dmg`",
         "col" => 2,
+      }, {
+        "url" => "git://anonscm.debian.org/users/foo/foostrap.git",
+        "msg" => "git://anonscm.debian.org/users/foo/foostrap.git should be `https://anonscm.debian.org/git/users/foo/foostrap.git`",
+        "col" => 2,
+      }, {
+        "url" => "ftp://ftp.mirrorservice.org/foo-1.tar.gz",
+        "msg" => "Please use https:// for ftp://ftp.mirrorservice.org/foo-1.tar.gz",
+        "col" => 2,
+      }, {
+        "url" => "ftp://ftp.cpan.org/pub/CPAN/foo-1.tar.gz",
+        "msg" => "ftp://ftp.cpan.org/pub/CPAN/foo-1.tar.gz should be `http://search.cpan.org/CPAN/foo-1.tar.gz`",
+        "col" => 2,
+      }, {
+        "url" => "http://sourceforge.net/projects/something/files/Something-1.2.3.dmg",
+        "msg" => "Use https://downloads.sourceforge.net to get geolocation (url is http://sourceforge.net/projects/something/files/Something-1.2.3.dmg).",
+        "col" => 2,
+      }, {
+        "url" => "https://downloads.sourceforge.net/project/foo/download",
+        "msg" => "Don't use /download in SourceForge urls (url is https://downloads.sourceforge.net/project/foo/download).",
+        "col" => 2,
+      }, {
+        "url" => "https://sourceforge.net/project/foo",
+        "msg" => "Use https://downloads.sourceforge.net to get geolocation (url is https://sourceforge.net/project/foo).",
+        "col" => 2,
+      }, {
+        "url" => "http://prdownloads.sourceforge.net/foo/foo-1.tar.gz",
+        "msg" => "Don't use prdownloads in SourceForge urls (url is http://prdownloads.sourceforge.net/foo/foo-1.tar.gz).\n" \
+                "\tSee: http://librelist.com/browser/homebrew/2011/1/12/prdownloads-is-bad/",
+        "col" => 2,
+      }, {
+        "url" => "http://foo.dl.sourceforge.net/sourceforge/foozip/foozip_1.0.tar.bz2",
+        "msg" => "Don't use specific dl mirrors in SourceForge urls (url is http://foo.dl.sourceforge.net/sourceforge/foozip/foozip_1.0.tar.bz2).",
+        "col" => 2,
+      }, {
+        "url" => "http://downloads.sourceforge.net/project/foo/foo/2/foo-2.zip",
+        "msg" => "Please use https:// for http://downloads.sourceforge.net/project/foo/foo/2/foo-2.zip",
+        "col" => 2,
+      }, {
+        "url" => "http://http.debian.net/debian/dists/foo/",
+        "msg" => "Please use a secure mirror for Debian URLs.\nWe recommend:\n"\
+                 "  https://mirrors.ocf.berkeley.edu/debian/dists/foo/\n",
+        "col" => 2,
+      }, {
+        "url" => "http://foo.googlecode.com/files/foo-1.0.zip",
+        "msg" => "Please use https:// for http://foo.googlecode.com/files/foo-1.0.zip",
+        "col" => 2,
+      }, {
+        "url" => "git://github.com/foo.git",
+        "msg" => "Please use https:// for git://github.com/foo.git",
+        "col" => 2,
+      }, {
+        "url" => "git://gitorious.org/foo/foo5",
+        "msg" => "Please use https:// for git://gitorious.org/foo/foo5",
+        "col" => 2,
+      }, {
+        "url" => "http://github.com/foo/foo5.git",
+        "msg" => "Please use https:// for http://github.com/foo/foo5.git",
+        "col" => 2,
+      }, {
+        "url" => "https://github.com/foo/foobar/archive/master.zip",
+        "msg" => "Use versioned rather than branch tarballs for stable checksums.",
+        "col" => 2,
+      }, {
+        "url" => "https://github.com/foo/bar/tarball/v1.2.3",
+        "msg" => "Use /archive/ URLs for GitHub tarballs (url is https://github.com/foo/bar/tarball/v1.2.3).",
+        "col" => 2,
+      }, {
+        "url" => "https://codeload.github.com/foo/bar/tar.gz/v0.1.1",
+        "msg" => "Use GitHub archive URLs:\n  https://github.com/foo/bar/archive/v0.1.1.tar.gz\n"\
+                 "Rather than codeload:\n  https://codeload.github.com/foo/bar/tar.gz/v0.1.1\n",
+        "col" => 2,
+      }, {
+        "url" => "https://central.maven.org/maven2/com/bar/foo/1.1/foo-1.1.jar",
+        "msg" => "https://central.maven.org/maven2/com/bar/foo/1.1/foo-1.1.jar should be `https://search.maven.org/remotecontent?filepath=com/bar/foo/1.1/foo-1.1.jar`",
+        "col" => 2,
       }]
-
       formulas.each do |formula|
         source = <<-EOS.undent
           class Foo < Formula
@@ -50,5 +124,61 @@ describe RuboCop::Cop::FormulaAudit::Urls do
         end
       end
     end
+
+    it "with offenses in stable/devel/head block" do
+      formulas = [{
+        "url" => "git://github.com/foo.git",
+        "msg" => "Please use https:// for git://github.com/foo.git",
+        "col" => 4,
+      }]
+      formulas.each do |formula|
+        source = <<-EOS.undent
+          class Foo < Formula
+            desc "foo"
+            url "https://foo.com"
+
+            devel do
+              url "#{formula["url"]}",
+                  :tag => "v1.0.0-alpha.1",
+                  :revision => "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+              version "1.0.0-alpha.1"
+            end
+          end
+        EOS
+        expected_offenses = [{ message: formula["msg"],
+                               severity: :convention,
+                               line: 6,
+                               column: formula["col"],
+                               source: source }]
+
+        inspect_source(cop, source)
+
+        expected_offenses.zip(cop.offenses.reverse).each do |expected, actual|
+          expect_offense(expected, actual)
+        end
+      end
+    end
+
+    it "with duplicate mirror" do
+      source = <<-EOS.undent
+          class Foo < Formula
+            desc "foo"
+            url "https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz"
+            mirror "https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz"
+          end
+      EOS
+
+      expected_offenses = [{ message: "URL should not be duplicated as a mirror: https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz",
+                             severity: :convention,
+                             line: 4,
+                             column: 2,
+                             source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses.reverse).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
   end
 end