Handle invalid names in download strategies

When subformulae are initialized without a name parameter, Homebrew
assigns the name "__UNKNOWN__". This may cause collisions in the cache.

Currently CurlDownloadStrategy and its descendants handles this by
extracting the basename form the URL and using that as the cached
filename. However, other strategies simply raise an exception.

We can improve the other strategies by URL-encoding the URL string and
using that as the cached directory name.

Note that this happens very rarely, especially now that resources (which
always have a name) are preferred to subformulae. The most common case
is a subformula that specifies a head download.

Closes Homebrew/homebrew#22949.

2 files changed, 47 insertions, 39 deletions

diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb
index 417aee51a..21eadabd3 100644
--- a/Library/Homebrew/download_strategy.rb
+++ b/Library/Homebrew/download_strategy.rb
@@ -1,5 +1,6 @@
 require 'open-uri'
 require 'utils/json'
+require 'erb'
 
 class AbstractDownloadStrategy
   attr_reader :name, :resource
@@ -33,6 +34,14 @@ class AbstractDownloadStrategy
     safe_system(*expand_safe_system_args(args))
   end
 
+  def checkout_name(tag)
+    if name.empty? || name == '__UNKNOWN__'
+      "#{ERB::Util.url_encode(@url)}--#{tag}"
+    else
+      "#{name}--#{tag}"
+    end
+  end
+
   # All download strategies are expected to implement these methods
   def fetch; end
   def stage; end
@@ -298,13 +307,11 @@ class SubversionDownloadStrategy < AbstractDownloadStrategy
     super
     @@svn ||= 'svn'
 
-    if name.to_s.empty? || name == '__UNKNOWN__'
-      raise NotImplementedError, "strategy requires a name parameter"
+    if ARGV.build_head?
+      @co = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("svn-HEAD")}")
     else
-      @co = Pathname.new("#{HOMEBREW_CACHE}/#{name}--svn")
+      @co = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("svn")}")
     end
-
-    @co = Pathname.new(@co.to_s + '-HEAD') if ARGV.build_head?
   end
 
   def cached_location
@@ -410,12 +417,7 @@ class GitDownloadStrategy < AbstractDownloadStrategy
   def initialize name, resource
     super
     @@git ||= 'git'
-
-    if name.to_s.empty? || name == '__UNKNOWN__'
-      raise NotImplementedError, "strategy requires a name parameter"
-    else
-      @clone = Pathname.new("#{HOMEBREW_CACHE}/#{name}--git")
-    end
+    @clone = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("git")}")
   end
 
   def cached_location
@@ -562,13 +564,7 @@ end
 class CVSDownloadStrategy < AbstractDownloadStrategy
   def initialize name, resource
     super
-
-    if name.to_s.empty? || name == '__UNKNOWN__'
-      raise NotImplementedError, "strategy requires a name parameter"
-    else
-      @unique_token = "#{name}--cvs"
-      @co = Pathname.new("#{HOMEBREW_CACHE}/#{@unique_token}")
-    end
+    @co = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("cvs")}")
   end
 
   def cached_location; @co; end
@@ -585,7 +581,7 @@ class CVSDownloadStrategy < AbstractDownloadStrategy
     unless @co.exist?
       Dir.chdir HOMEBREW_CACHE do
         safe_system '/usr/bin/cvs', '-d', url, 'login'
-        safe_system '/usr/bin/cvs', '-d', url, 'checkout', '-d', @unique_token, mod
+        safe_system '/usr/bin/cvs', '-d', url, 'checkout', '-d', checkout_name("cvs"), mod
       end
     else
       puts "Updating #{@co}"
@@ -618,12 +614,7 @@ end
 class MercurialDownloadStrategy < AbstractDownloadStrategy
   def initialize name, resource
     super
-
-    if name.to_s.empty? || name == '__UNKNOWN__'
-      raise NotImplementedError, "strategy requires a name parameter"
-    else
-      @clone = Pathname.new("#{HOMEBREW_CACHE}/#{name}--hg")
-    end
+    @clone = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("hg")}")
   end
 
   def cached_location; @clone; end
@@ -678,12 +669,7 @@ end
 class BazaarDownloadStrategy < AbstractDownloadStrategy
   def initialize name, resource
     super
-
-    if name.to_s.empty? || name == '__UNKNOWN__'
-      raise NotImplementedError, "strategy requires a name parameter"
-    else
-      @clone = Pathname.new("#{HOMEBREW_CACHE}/#{name}--bzr")
-    end
+    @clone = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("bzr")}")
   end
 
   def cached_location; @clone; end
@@ -731,11 +717,7 @@ end
 class FossilDownloadStrategy < AbstractDownloadStrategy
   def initialize name, resource
     super
-    if name.to_s.empty? || name == '__UNKNOWN__'
-      raise NotImplementedError, "strategy requires a name parameter"
-    else
-      @clone = Pathname.new("#{HOMEBREW_CACHE}/#{name}--fossil")
-    end
+    @clone = Pathname.new("#{HOMEBREW_CACHE}/#{checkout_name("fossil")}")
   end
 
   def cached_location; @clone; end
diff --git a/Library/Homebrew/test/test_download_strategies.rb b/Library/Homebrew/test/test_download_strategies.rb
index 3a330e797..cdcb92dc9 100644
--- a/Library/Homebrew/test/test_download_strategies.rb
+++ b/Library/Homebrew/test/test_download_strategies.rb
@@ -2,7 +2,7 @@ require 'testing_env'
 require 'download_strategy'
 require 'bottles' # XXX: hoist these regexps into constants in Pathname?
 
-class SoftwareSpecDouble
+class ResourceDouble
   attr_reader :url, :specs
 
   def initialize(url="http://foo.com/bar.tar.gz", specs={})
@@ -14,8 +14,8 @@ end
 class AbstractDownloadStrategyTests < Test::Unit::TestCase
   def setup
     @name = "foo"
-    @package = SoftwareSpecDouble.new
-    @strategy = AbstractDownloadStrategy.new(@name, @package)
+    @resource = ResourceDouble.new
+    @strategy = AbstractDownloadStrategy.new(@name, @resource)
     @args = %w{foo bar baz}
   end
 
@@ -37,6 +37,32 @@ class AbstractDownloadStrategyTests < Test::Unit::TestCase
   end
 end
 
+class DownloadStrategyCheckoutNameTests < Test::Unit::TestCase
+  def setup
+    @resource = ResourceDouble.new("http://foo.com/bar")
+    @strategy = AbstractDownloadStrategy
+  end
+
+  def escaped(tag)
+    "#{ERB::Util.url_encode(@resource.url)}--#{tag}"
+  end
+
+  def test_explicit_name
+    downloader = @strategy.new("baz", @resource)
+    assert_equal "baz--foo", downloader.checkout_name("foo")
+  end
+
+  def test_empty_name
+    downloader = @strategy.new("", @resource)
+    assert_equal escaped("foo"), downloader.checkout_name("foo")
+  end
+
+  def test_unknown_name
+    downloader = @strategy.new("__UNKNOWN__", @resource)
+    assert_equal escaped("foo"), downloader.checkout_name("foo")
+  end
+end
+
 class DownloadStrategyDetectorTests < Test::Unit::TestCase
   def setup
     @d = DownloadStrategyDetector.new