Simplify SubversionDownloadStrategy.

8 files changed, 55 insertions, 99 deletions

diff --git a/Library/Homebrew/cask/lib/hbc/container.rb b/Library/Homebrew/cask/lib/hbc/container.rb
index 93e825e03..fab3a3c1c 100644
--- a/Library/Homebrew/cask/lib/hbc/container.rb
+++ b/Library/Homebrew/cask/lib/hbc/container.rb
@@ -4,6 +4,7 @@ require "hbc/container/bzip2"
 require "hbc/container/cab"
 require "hbc/container/criteria"
 require "hbc/container/dmg"
+require "hbc/container/directory"
 require "hbc/container/executable"
 require "hbc/container/generic_unar"
 require "hbc/container/gpg"
@@ -14,6 +15,7 @@ require "hbc/container/otf"
 require "hbc/container/pkg"
 require "hbc/container/seven_zip"
 require "hbc/container/sit"
+require "hbc/container/svn_repository"
 require "hbc/container/tar"
 require "hbc/container/ttf"
 require "hbc/container/rar"
@@ -43,6 +45,7 @@ module Hbc
         Xz,    # pure xz
         Gpg,   # GnuPG signed data
         Executable,
+        SvnRepository,
       ]
       # for explicit use only (never autodetected):
       # Hbc::Container::Naked
diff --git a/Library/Homebrew/cask/lib/hbc/container/criteria.rb b/Library/Homebrew/cask/lib/hbc/container/criteria.rb
index 66ecb8c87..52f171d6a 100644
--- a/Library/Homebrew/cask/lib/hbc/container/criteria.rb
+++ b/Library/Homebrew/cask/lib/hbc/container/criteria.rb
@@ -13,9 +13,11 @@ module Hbc
       end
 
       def magic_number(regex)
+        return false if path.directory?
+
         # 262: length of the longest regex (currently: Hbc::Container::Tar)
-        @magic_number ||= File.open(@path, "rb") { |f| f.read(262) }
-        @magic_number =~ regex
+        @magic_number ||= File.open(path, "rb") { |f| f.read(262) }
+        @magic_number.match?(regex)
       end
     end
   end
diff --git a/Library/Homebrew/cask/lib/hbc/container/directory.rb b/Library/Homebrew/cask/lib/hbc/container/directory.rb
new file mode 100644
index 000000000..e4bb1095b
--- /dev/null
+++ b/Library/Homebrew/cask/lib/hbc/container/directory.rb
@@ -0,0 +1,24 @@
+require "hbc/container/base"
+
+module Hbc
+  class Container
+    class Directory < Base
+      def self.me?(*)
+        false
+      end
+
+      def extract
+        @path.children.each do |child|
+          next if skip_path?(child)
+          FileUtils.cp child, @cask.staged_path
+        end
+      end
+
+      private
+
+      def skip_path?(*)
+        false
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/cask/lib/hbc/container/executable.rb b/Library/Homebrew/cask/lib/hbc/container/executable.rb
index 848f6d4be..af3b36fd1 100644
--- a/Library/Homebrew/cask/lib/hbc/container/executable.rb
+++ b/Library/Homebrew/cask/lib/hbc/container/executable.rb
@@ -8,7 +8,7 @@ module Hbc
         return true if criteria.magic_number(/^#!\s*\S+/)
 
         begin
-          MachO.open(criteria.path).header.executable?
+          criteria.path.file? && MachO.open(criteria.path).header.executable?
         rescue MachO::MagicError
           false
         end
diff --git a/Library/Homebrew/cask/lib/hbc/container/svn_repository.rb b/Library/Homebrew/cask/lib/hbc/container/svn_repository.rb
new file mode 100644
index 000000000..cae613b2d
--- /dev/null
+++ b/Library/Homebrew/cask/lib/hbc/container/svn_repository.rb
@@ -0,0 +1,15 @@
+require "hbc/container/directory"
+
+module Hbc
+  class Container
+    class SvnRepository < Directory
+      def self.me?(criteria)
+        criteria.path.join(".svn").directory?
+      end
+
+      def skip_path?(path)
+        path.basename.to_s == ".svn"
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/cask/lib/hbc/download_strategy.rb b/Library/Homebrew/cask/lib/hbc/download_strategy.rb
index 016bb66e6..fb4319f5e 100644
--- a/Library/Homebrew/cask/lib/hbc/download_strategy.rb
+++ b/Library/Homebrew/cask/lib/hbc/download_strategy.rb
@@ -10,7 +10,7 @@ module Hbc
   class AbstractDownloadStrategy
     attr_reader :cask, :name, :url, :uri_object, :version
 
-    def initialize(cask, command = SystemCommand)
+    def initialize(cask, command: SystemCommand)
       @cask       = cask
       @command    = command
       # TODO: this excess of attributes is a function of integrating
@@ -33,8 +33,8 @@ module Hbc
   class HbVCSDownloadStrategy < AbstractDownloadStrategy
     REF_TYPES = [:branch, :revision, :revisions, :tag].freeze
 
-    def initialize(cask, command = SystemCommand)
-      super
+    def initialize(*args, **options)
+      super(*args, **options)
       @ref_type, @ref = extract_ref
       @clone = Hbc.cache.join(cache_filename)
     end
@@ -64,11 +64,6 @@ module Hbc
   end
 
   class CurlDownloadStrategy < AbstractDownloadStrategy
-    # TODO: should be part of url object
-    def mirrors
-      @mirrors ||= []
-    end
-
     def tarball_path
       @tarball_path ||= Hbc.cache.join("#{name}--#{version}#{ext}")
     end
@@ -131,11 +126,6 @@ module Hbc
         ignore_interrupts { temporary_path.rename(tarball_path) }
       end
       tarball_path
-    rescue CurlDownloadStrategyError
-      raise if mirrors.empty?
-      puts "Trying a mirror..."
-      @url = mirrors.shift
-      retry
     end
 
     private
@@ -225,8 +215,8 @@ module Hbc
 
     # super does not provide checks for already-existing downloads
     def fetch
-      if tarball_path.exist?
-        puts "Already downloaded: #{tarball_path}"
+      if cached_location.directory?
+        puts "Already downloaded: #{cached_location}"
       else
         @url = @url.sub(/^svn\+/, "") if @url =~ %r{^svn\+http://}
         ohai "Checking out #{@url}"
@@ -252,9 +242,8 @@ module Hbc
         else
           fetch_repo @clone, @url
         end
-        compress
       end
-      tarball_path
+      cached_location
     end
 
     # This primary reason for redefining this method is the trust_cert
@@ -288,10 +277,6 @@ module Hbc
                     print_stderr: false)
     end
 
-    def tarball_path
-      @tarball_path ||= cached_location.dirname.join(cached_location.basename.to_s + "-#{@cask.version}.tar")
-    end
-
     def shell_quote(str)
       # Oh god escaping shell args.
       # See http://notetoself.vrensk.com/2008/08/escaping-single-quotes-in-ruby-harder-than-expected/
@@ -304,35 +289,5 @@ module Hbc
         yield name, url
       end
     end
-
-    private
-
-    # TODO/UPDATE: the tar approach explained below is fragile
-    # against challenges such as case-sensitive filesystems,
-    # and must be re-implemented.
-    #
-    # Seems nutty: we "download" the contents into a tape archive.
-    # Why?
-    # * A single file is tractable to the rest of the Cask toolchain,
-    # * An alternative would be to create a Directory container type.
-    #   However, some type of file-serialization trick would still be
-    #   needed in order to enable calculating a single checksum over
-    #   a directory.  So, in that alternative implementation, the
-    #   special cases would propagate outside this class, including
-    #   the use of tar or equivalent.
-    # * SubversionDownloadStrategy.cached_location is not versioned
-    # * tarball_path provides a needed return value for our overridden
-    #   fetch method.
-    # * We can also take this private opportunity to strip files from
-    #   the download which are protocol-specific.
-
-    def compress
-      Dir.chdir(cached_location) do
-        @command.run!("/usr/bin/tar",
-                      args:         ['-s/^\.//', "--exclude", ".svn", "-cf", Pathname.new(tarball_path), "--", "."],
-                      print_stderr: false)
-      end
-      clear_cache
-    end
   end
 end
diff --git a/Library/Homebrew/cask/lib/hbc/system_command.rb b/Library/Homebrew/cask/lib/hbc/system_command.rb
index 901617b71..b735ae4f9 100644
--- a/Library/Homebrew/cask/lib/hbc/system_command.rb
+++ b/Library/Homebrew/cask/lib/hbc/system_command.rb
@@ -112,11 +112,7 @@ module Hbc
                  processed_output[:stderr],
                  processed_status.exitstatus)
     end
-  end
-end
 
-module Hbc
-  class SystemCommand
     class Result
       attr_accessor :command, :stdout, :stderr, :exit_status
 
diff --git a/Library/Homebrew/test/cask/download_strategy_spec.rb b/Library/Homebrew/test/cask/download_strategy_spec.rb
index 222352c07..0c18f56b5 100644
--- a/Library/Homebrew/test/cask/download_strategy_spec.rb
+++ b/Library/Homebrew/test/cask/download_strategy_spec.rb
@@ -138,7 +138,7 @@ describe "download strategies", :cask do
   describe Hbc::SubversionDownloadStrategy do
     let(:url_options) { { using: :svn } }
     let(:fake_system_command) { class_double(Hbc::SystemCommand) }
-    let(:downloader) { Hbc::SubversionDownloadStrategy.new(cask, fake_system_command) }
+    let(:downloader) { Hbc::SubversionDownloadStrategy.new(cask, command: fake_system_command) }
     before do
       allow(fake_system_command).to receive(:run!)
     end
@@ -147,7 +147,7 @@ describe "download strategies", :cask do
       allow(downloader).to receive(:compress)
       allow(downloader).to receive(:fetch_repo)
 
-      expect(downloader.fetch).to equal(downloader.tarball_path)
+      expect(downloader.fetch).to equal(downloader.cached_location)
     end
 
     it "calls fetch_repo with default arguments for a simple Cask" do
@@ -237,44 +237,5 @@ describe "download strategies", :cask do
         )
       end
     end
-
-    it "runs tar to serialize svn downloads" do
-      # sneaky stub to remake the directory, since homebrew code removes it
-      # before tar is called
-      allow(downloader).to receive(:fetch_repo) {
-        downloader.cached_location.mkdir
-      }
-
-      downloader.fetch
-
-      expect(fake_system_command).to have_received(:run!).with(
-        "/usr/bin/tar",
-        hash_including(args: [
-                         '-s/^\\.//',
-                         "--exclude",
-                         ".svn",
-                         "-cf",
-                         downloader.tarball_path,
-                         "--",
-                         ".",
-                       ]),
-      )
-    end
   end
-
-  # does not work yet, because (for unknown reasons), the tar command
-  # returns an error code when running under the test suite
-  # it 'creates a tarball matching the expected checksum' do
-  #   cask = Hbc::CaskLoader.load('svn-download-check-cask')
-  #   downloader = Hbc::SubversionDownloadStrategy.new(cask)
-  #   # special mocking required for tar to have something to work with
-  #   def downloader.fetch_repo(target, url, revision = nil, ignore_externals=false)
-  #     target.mkpath
-  #     FileUtils.touch(target.join('empty_file.txt'))
-  #     File.utime(1000,1000,target.join('empty_file.txt'))
-  #   end
-  #   expect(downloader.fetch).to equal(downloader.tarball_path)
-  #   d = Hbc::Download.new(cask)
-  #   d.send(:_check_sums, downloader.tarball_path, cask.sums)
-  # end
 end