From 57560c03e64cb11a33287b3194b0c35cbea6e159 Mon Sep 17 00:00:00 2001 From: Jack Nagel Date: Mon, 30 Sep 2013 21:36:38 -0500 Subject: [PATCH] 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. --- Library/Homebrew/download_strategy.rb | 54 +++++++------------ .../Homebrew/test/test_download_strategies.rb | 32 +++++++++-- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 417aee51ac..21eadabd30 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 3a330e7976..cdcb92dc94 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