Merge pull request #3018 from Homebrew/revert-2848-refactoring

Revert "Refactor SVN and cURL download strategies."

Library/Homebrew/cask/lib/hbc/audit.rb

@@ -143,15 +143,7 @@ module Hbc
 
     def check_appcast_http_code
       odebug "Verifying appcast returns 200 HTTP response code"
-
+      result = @command.run("/usr/bin/curl", args: ["--compressed", "--location", "--user-agent", URL::FAKE_USER_AGENT, "--output", "/dev/null", "--write-out", "%{http_code}", cask.appcast], print_stderr: false)
-      curl_executable, *args = curl_args(
-        "--compressed", "--location", "--fail",
-        "--write-out", "%{http_code}",
-        "--output", "/dev/null",
-        cask.appcast,
-        user_agent: :fake
-      )
-      result = @command.run(curl_executable, args: args, print_stderr: false)
       if result.success?
         http_code = result.stdout.chomp
         add_warning "unexpected HTTP response code retrieving appcast: #{http_code}" unless http_code == "200"

Library/Homebrew/cask/lib/hbc/cask_loader.rb

@@ -71,7 +71,7 @@ module Hbc
 
         begin
           ohai "Downloading #{url}."
-          curl_download url, to: path
+          curl url, "-o", path
         rescue ErrorDuringExecution
           raise CaskUnavailableError.new(token, "Failed to download #{Formatter.url(url)}.")
         end

Library/Homebrew/cask/lib/hbc/container.rb

@@ -4,7 +4,6 @@ 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"
@@ -15,7 +14,6 @@ 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"
@@ -45,7 +43,6 @@ module Hbc
         Xz,    # pure xz
         Gpg,   # GnuPG signed data
         Executable,
-        SvnRepository,
       ]
       # for explicit use only (never autodetected):
       # Hbc::Container::Naked

Library/Homebrew/cask/lib/hbc/container/criteria.rb

@@ -13,11 +13,9 @@ 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 ||= File.open(@path, "rb") { |f| f.read(262) }
-        @magic_number.match?(regex)
+        @magic_number =~ regex
       end
     end
   end

Library/Homebrew/cask/lib/hbc/container/directory.rb

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

Library/Homebrew/cask/lib/hbc/container/executable.rb

@@ -8,7 +8,7 @@ module Hbc
         return true if criteria.magic_number(/^#!\s*\S+/)
 
         begin
-          criteria.path.file? && MachO.open(criteria.path).header.executable?
+          MachO.open(criteria.path).header.executable?
         rescue MachO::MagicError
           false
         end

Library/Homebrew/cask/lib/hbc/container/svn_repository.rb

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

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(*args, **options)
+    def initialize(cask, command = SystemCommand)
-      super(*args, **options)
+      super
       @ref_type, @ref = extract_ref
       @clone = Hbc.cache.join(cache_filename)
     end
@@ -64,6 +64,11 @@ 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
@@ -90,8 +95,13 @@ module Hbc
       end
     end
 
+    def downloaded_size
+      temporary_path.size? || 0
+    end
+
     def _fetch
-      curl_download url, *cask_curl_args, to: temporary_path, user_agent: uri_object.user_agent
+      odebug "Calling curl with args #{cask_curl_args}"
+      curl(*cask_curl_args)
     end
 
     def fetch
@@ -121,12 +131,33 @@ 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
 
     def cask_curl_args
-      cookies_args + referer_args
+      default_curl_args.tap do |args|
+        args.concat(user_agent_args)
+        args.concat(cookies_args)
+        args.concat(referer_args)
+      end
+    end
+
+    def default_curl_args
+      [url, "-C", downloaded_size, "-o", temporary_path]
+    end
+
+    def user_agent_args
+      if uri_object.user_agent
+        ["-A", uri_object.user_agent]
+      else
+        []
+      end
     end
 
     def cookies_args
@@ -160,7 +191,8 @@ module Hbc
 
   class CurlPostDownloadStrategy < CurlDownloadStrategy
     def cask_curl_args
-      super.concat(post_args)
+      super
+      default_curl_args.concat(post_args)
     end
 
     def post_args
@@ -193,8 +225,8 @@ module Hbc
 
     # super does not provide checks for already-existing downloads
     def fetch
-      if cached_location.directory?
+      if tarball_path.exist?
-        puts "Already downloaded: #{cached_location}"
+        puts "Already downloaded: #{tarball_path}"
       else
         @url = @url.sub(/^svn\+/, "") if @url =~ %r{^svn\+http://}
         ohai "Checking out #{@url}"
@@ -220,8 +252,9 @@ module Hbc
         else
           fetch_repo @clone, @url
         end
+        compress
       end
-      cached_location
+      tarball_path
     end
 
     # This primary reason for redefining this method is the trust_cert
@@ -255,6 +288,10 @@ 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/
@@ -267,5 +304,35 @@ 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

Library/Homebrew/cask/lib/hbc/dsl/appcast.rb

@@ -12,11 +12,7 @@ module Hbc
       end
 
       def calculate_checkpoint
-        curl_executable, *args = curl_args(
+        result = SystemCommand.run("/usr/bin/curl", args: ["--compressed", "--location", "--user-agent", URL::FAKE_USER_AGENT, "--fail", @uri], print_stderr: false)
-          "--compressed", "--location", "--fail", @uri,
-          user_agent: :fake
-        )
-        result = SystemCommand.run(curl_executable, args: args, print_stderr: false)
 
         checkpoint = if result.success?
           processed_appcast_text = result.stdout.gsub(%r{<pubDate>[^<]*</pubDate>}m, "")

Library/Homebrew/cask/lib/hbc/system_command.rb

@@ -112,7 +112,11 @@ module Hbc
                  processed_output[:stderr],
                  processed_status.exitstatus)
     end
+  end
+end
 
+module Hbc
+  class SystemCommand
     class Result
       attr_accessor :command, :stdout, :stderr, :exit_status
 

Library/Homebrew/cask/lib/hbc/url.rb

@@ -1,6 +1,8 @@
 module Hbc
   class URL
-    attr_reader :using, :revision, :trust_cert, :uri, :cookies, :referer, :data, :user_agent
+    FAKE_USER_AGENT = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10) https://caskroom.github.io".freeze
+
+    attr_reader :using, :revision, :trust_cert, :uri, :cookies, :referer, :data
 
     extend Forwardable
     def_delegators :uri, :path, :scheme, :to_s
@@ -15,7 +17,7 @@ module Hbc
 
     def initialize(uri, options = {})
       @uri        = Hbc::UnderscoreSupportingURI.parse(uri)
-      @user_agent = options.fetch(:user_agent, :default)
+      @user_agent = options[:user_agent]
       @cookies    = options[:cookies]
       @referer    = options[:referer]
       @using      = options[:using]
@@ -23,5 +25,10 @@ module Hbc
       @trust_cert = options[:trust_cert]
       @data       = options[:data]
     end
+
+    def user_agent
+      return FAKE_USER_AGENT if @user_agent == :fake
+      @user_agent
+    end
   end
 end

Library/Homebrew/cask/lib/hbc/verify/gpg.rb

@@ -33,7 +33,7 @@ module Hbc
         meta_dir = cached || cask.metadata_subdir("gpg", :now, true)
         sig_path = meta_dir.join("signature.asc")
 
-        curl_download cask.gpg.signature, to: sig_path unless cached || force
+        curl(cask.gpg.signature, "-o", sig_path.to_s) unless cached || force
 
         sig_path
       end

Library/Homebrew/dev-cmd/audit.rb

@@ -242,10 +242,12 @@ class FormulaAuditor
 
   def self.http_content_headers_and_checksum(url, hash_needed: false, user_agent: :default)
     max_time = hash_needed ? "600" : "25"
-    output, = curl_output(
+    args = curl_args(
-      "--connect-timeout", "15", "--include", "--max-time", max_time, "--location", url,
+      extra_args: ["--connect-timeout", "15", "--include", "--max-time", max_time, url],
-      user_agent: user_agent
+      show_output: true,
+      user_agent: user_agent,
     )
+    output = Open3.popen3(*args) { |_, stdout, _, _| stdout.read }
 
     status_code = :unknown
     while status_code == :unknown || status_code.to_s.start_with?("3")

Library/Homebrew/dev-cmd/mirror.rb

@@ -25,9 +25,9 @@ module Homebrew
            "public_download_numbers": true,
            "public_stats": true}
         EOS
-        curl "--silent", "--fail", "--user", "#{bintray_user}:#{bintray_key}",
+        curl "--silent", "--fail", "-u#{bintray_user}:#{bintray_key}",
-             "--header", "Content-Type: application/json",
+             "-H", "Content-Type: application/json",
-             "--data", package_blob, bintray_repo_url
+             "-d", package_blob, bintray_repo_url
         puts
       end
 
@@ -40,8 +40,8 @@ module Homebrew
       content_url = "https://api.bintray.com/content/homebrew/mirror"
       content_url += "/#{bintray_package}/#{f.pkg_version}/#{filename}"
       content_url += "?publish=1"
-      curl "--silent", "--fail", "--user", "#{bintray_user}:#{bintray_key}",
+      curl "--silent", "--fail", "-u#{bintray_user}:#{bintray_key}",
-           "--upload-file", download, content_url
+           "-T", download, content_url
       puts
       ohai "Mirrored #{filename}!"
     end

Library/Homebrew/dev-cmd/pull.rb

@@ -228,7 +228,7 @@ module Homebrew
           "https://github.com/BrewTestBot/homebrew-#{tap.repo}/compare/homebrew:master...pr-#{issue}"
         end
 
-        curl "--silent", "--fail", "--output", "/dev/null", "--head", bottle_commit_url
+        curl "--silent", "--fail", "-o", "/dev/null", "-I", bottle_commit_url
 
         safe_system "git", "checkout", "--quiet", "-B", bottle_branch, orig_revision
         pull_patch bottle_commit_url, "bottle commit"
@@ -303,7 +303,7 @@ module Homebrew
       extra_msg = @description ? "(#{@description})" : nil
       ohai "Fetching patch #{extra_msg}"
       puts "Patch: #{patch_url}"
-      curl_download patch_url, to: patchpath
+      curl patch_url, "-s", "-o", patchpath
     end
 
     def apply_patch
@@ -433,10 +433,10 @@ module Homebrew
     end
     version = info.pkg_version
     ohai "Publishing on Bintray: #{package} #{version}"
-    curl "--write-out", '\n', "--silent", "--fail",
+    curl "-w", '\n', "--silent", "--fail",
-         "--user", "#{creds[:user]}:#{creds[:key]}", "--request", "POST",
+         "-u#{creds[:user]}:#{creds[:key]}", "-X", "POST",
-         "--header", "Content-Type: application/json",
+         "-H", "Content-Type: application/json",
-         "--data", '{"publish_wait_for_secs": 0}',
+         "-d", '{"publish_wait_for_secs": 0}',
          "https://api.bintray.com/content/homebrew/#{repo}/#{package}/#{version}/publish"
     true
   rescue => e
@@ -587,7 +587,7 @@ module Homebrew
         # We're in the cache; make sure to force re-download
         loop do
           begin
-            curl_download url, to: filename
+            curl url, "-o", filename
             break
           rescue
             if retry_count >= max_curl_retries
@@ -606,7 +606,7 @@ module Homebrew
   end
 
   def check_bintray_mirror(name, url)
-    headers, = curl_output("--connect-timeout", "15", "--location", "--head", url)
+    headers = curl_output("--connect-timeout", "15", "--head", url)[0]
     status_code = headers.scan(%r{^HTTP\/.* (\d+)}).last.first
     return if status_code.start_with?("2")
     opoo "The Bintray mirror #{url} is not reachable (HTTP status code #{status_code})."

Library/Homebrew/download_strategy.rb

@@ -375,59 +375,75 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy
       ohai "Downloading from #{url}"
     end
 
-    curl_download resolved_url(url), to: temporary_path
+    urls = actual_urls(url)
+    unless urls.empty?
+      ohai "Downloading from #{urls.last}"
+      if !ENV["HOMEBREW_NO_INSECURE_REDIRECT"].nil? && url.start_with?("https://") &&
+         urls.any? { |u| !u.start_with? "https://" }
+        puts "HTTPS to HTTP redirect detected & HOMEBREW_NO_INSECURE_REDIRECT is set."
+        raise CurlDownloadStrategyError, url
+      end
+      url = urls.last
+    end
+
+    curl url, "-C", downloaded_size, "-o", temporary_path
   end
 
   # Curl options to be always passed to curl,
-  # with raw head calls (`curl --head`) or with actual `fetch`.
+  # with raw head calls (`curl -I`) or with actual `fetch`.
   def _curl_opts
-    return ["--user" << meta.fetch(:user)] if meta.key?(:user)
+    copts = []
-    []
+    copts << "--user" << meta.fetch(:user) if meta.key?(:user)
+    copts
   end
 
-  def resolved_url(url)
+  def actual_urls(url)
-    redirect_url, _, status = curl_output(
+    urls = []
-      *_curl_opts, "--silent", "--head",
+    curl_args = _curl_opts << "-I" << "-L" << url
-      "--write-out", "%{redirect_url}",
+    Utils.popen_read("curl", *curl_args).scan(/^Location: (.+)$/).map do |m|
-      "--output", "/dev/null",
+      urls << URI.join(urls.last || url, m.first.chomp).to_s
-      url.to_s
-    )
-
-    return url unless status.success?
-    return url if redirect_url.empty?
-
-    ohai "Downloading from #{redirect_url}"
-    if ENV["HOMEBREW_NO_INSECURE_REDIRECT"] &&
-       url.start_with?("https://") && !redirect_url.start_with?("https://")
-      puts "HTTPS to HTTP redirect detected & HOMEBREW_NO_INSECURE_REDIRECT is set."
-      raise CurlDownloadStrategyError, url
     end
-
+    urls
-    redirect_url
   end
 
-  def curl(*args, **options)
+  def downloaded_size
+    temporary_path.size? || 0
+  end
+
+  def curl(*args)
     args.concat _curl_opts
     args << "--connect-timeout" << "5" unless mirrors.empty?
-    super(*args, **options)
+    super
   end
 end
 
 # Detect and download from Apache Mirror
 class CurlApacheMirrorDownloadStrategy < CurlDownloadStrategy
   def apache_mirrors
-    mirrors, = Open3.capture3(
+    rd, wr = IO.pipe
-      *curl_args(*_curl_opts, "--silent", "--location", "#{@url}&asjson=1"),
+    buf = ""
-    )
 
-    JSON.parse(mirrors)
+    pid = fork do
+      ENV.delete "HOMEBREW_CURL_VERBOSE"
+      rd.close
+      $stdout.reopen(wr)
+      $stderr.reopen(wr)
+      curl "#{@url}&asjson=1"
+    end
+    wr.close
+
+    rd.readline if ARGV.verbose? # Remove Homebrew output
+    buf << rd.read until rd.eof?
+    rd.close
+    Process.wait(pid)
+    buf
   end
 
   def _fetch
     return super if @tried_apache_mirror
     @tried_apache_mirror = true
 
-    mirrors = apache_mirrors
+    mirrors = JSON.parse(apache_mirrors)
     path_info = mirrors.fetch("path_info")
     @url = mirrors.fetch("preferred") + path_info
     @mirrors |= %W[https://archive.apache.org/dist/#{path_info}]
@@ -444,7 +460,7 @@ end
 class CurlPostDownloadStrategy < CurlDownloadStrategy
   def _fetch
     base_url, data = @url.split("?")
-    curl_download base_url, "--data", data, to: temporary_path
+    curl base_url, "-d", data, "-C", downloaded_size, "-o", temporary_path
   end
 end
 
@@ -514,7 +530,7 @@ class S3DownloadStrategy < CurlDownloadStrategy
       s3url = obj.public_url
     end
 
-    curl_download s3url, to: temporary_path
+    curl s3url, "-C", downloaded_size, "-o", temporary_path
   end
 end
 
@@ -550,7 +566,7 @@ class GitHubPrivateRepositoryDownloadStrategy < CurlDownloadStrategy
   end
 
   def _fetch
-    curl_download download_url, to: temporary_path
+    curl download_url, "-C", downloaded_size, "-o", temporary_path
   end
 
   private
@@ -599,7 +615,7 @@ class GitHubPrivateRepositoryReleaseDownloadStrategy < GitHubPrivateRepositoryDo
   def _fetch
     # HTTP request header `Accept: application/octet-stream` is required.
     # Without this, the GitHub API will respond with metadata, not binary.
-    curl_download download_url, "--header", "Accept: application/octet-stream", to: temporary_path
+    curl download_url, "-C", downloaded_size, "-o", temporary_path, "-H", "Accept: application/octet-stream"
   end
 
   private
@@ -899,27 +915,18 @@ class GitHubGitDownloadStrategy < GitDownloadStrategy
   def github_last_commit
     return if ENV["HOMEBREW_NO_GITHUB_API"]
 
-    output, _, status = curl_output(
+    output, _, status = curl_output "-H", "Accept: application/vnd.github.v3.sha", \
-      "--silent", "--head", "--location",
+      "-I", "https://api.github.com/repos/#{@user}/#{@repo}/commits/#{@ref}"
-      "-H", "Accept: application/vnd.github.v3.sha",
-      "https://api.github.com/repos/#{@user}/#{@repo}/commits/#{@ref}"
-    )
 
-    return unless status.success?
+    commit = output[/^ETag: \"(\h+)\"/, 1] if status.success?
-
-    commit = output[/^ETag: \"(\h+)\"/, 1]
     version.update_commit(commit) if commit
     commit
   end
 
   def multiple_short_commits_exist?(commit)
     return if ENV["HOMEBREW_NO_GITHUB_API"]
-
+    output, _, status = curl_output "-H", "Accept: application/vnd.github.v3.sha", \
-    output, _, status = curl_output(
+      "-I", "https://api.github.com/repos/#{@user}/#{@repo}/commits/#{commit}"
-      "--silent", "--head", "--location",
-      "-H", "Accept: application/vnd.github.v3.sha",
-      "https://api.github.com/repos/#{@user}/#{@repo}/commits/#{commit}"
-    )
 
     !(status.success? && output && output[/^Status: (200)/, 1] == "200")
   end
@@ -1152,13 +1159,15 @@ class DownloadStrategyDetector
       SubversionDownloadStrategy
     when %r{^cvs://}
       CVSDownloadStrategy
-    when %r{^hg://}, %r{^https?://(.+?\.)?googlecode\.com/hg}
+    when %r{^https?://(.+?\.)?googlecode\.com/hg}
+      MercurialDownloadStrategy
+    when %r{^hg://}
       MercurialDownloadStrategy
     when %r{^bzr://}
       BazaarDownloadStrategy
     when %r{^fossil://}
       FossilDownloadStrategy
-    when %r{^svn\+http://}, %r{^http://svn\.apache\.org/repos/}
+    when %r{^http://svn\.apache\.org/repos/}, %r{^svn\+http://}
       SubversionDownloadStrategy
     when %r{^https?://(.+?\.)?sourceforge\.net/hgweb/}
       MercurialDownloadStrategy

Library/Homebrew/formulary.rb

@@ -165,7 +165,7 @@ module Formulary
     def load_file
       HOMEBREW_CACHE_FORMULA.mkpath
       FileUtils.rm_f(path)
-      curl_download url, to: path
+      curl url, "-o", path
       super
     rescue MethodDeprecatedError => e
       if url =~ %r{github.com/([\w-]+)/homebrew-([\w-]+)/}

Library/Homebrew/test/cask/download_strategy_spec.rb

@@ -27,11 +27,8 @@ describe "download strategies", :cask do
 
       expect(downloader).to have_received(:curl).with(
         cask.url.to_s,
-        "--location",
+        "-C", 0,
-        "--remote-time",
+        "-o", kind_of(Pathname)
-        "--continue-at", "-",
-        "--output", kind_of(Pathname),
-        user_agent: :default
       )
     end
 
@@ -39,25 +36,25 @@ describe "download strategies", :cask do
       let(:url_options) { { user_agent: "Mozilla/25.0.1" } }
 
       it "adds the appropriate curl args" do
-        expect(downloader).to receive(:safe_system) { |*args|
+        curl_args = []
-          expect(args.each_cons(2)).to include(["--user-agent", "Mozilla/25.0.1"])
+        allow(downloader).to receive(:curl) { |*args| curl_args = args }
-        }
 
         downloader.fetch
+
+        expect(curl_args.each_cons(2)).to include(["-A", "Mozilla/25.0.1"])
       end
     end
 
     context "with a generalized fake user agent" do
-      alias_matcher :a_string_matching, :match
-
       let(:url_options) { { user_agent: :fake } }
 
       it "adds the appropriate curl args" do
-        expect(downloader).to receive(:safe_system) { |*args|
+        curl_args = []
-          expect(args.each_cons(2).to_a).to include(["--user-agent", a_string_matching(/Mozilla.*Mac OS X 10.*AppleWebKit/)])
+        allow(downloader).to receive(:curl) { |*args| curl_args = args }
-        }
 
         downloader.fetch
+
+        expect(curl_args.each_cons(2)).to include(["-A", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10) https://caskroom.github.io"])
       end
     end
 
@@ -141,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, command: fake_system_command) }
+    let(:downloader) { Hbc::SubversionDownloadStrategy.new(cask, fake_system_command) }
     before do
       allow(fake_system_command).to receive(:run!)
     end
@@ -150,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.cached_location)
+      expect(downloader.fetch).to equal(downloader.tarball_path)
     end
 
     it "calls fetch_repo with default arguments for a simple Cask" do
@@ -240,5 +237,44 @@ 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

Library/Homebrew/test/cask/dsl/appcast_spec.rb

@@ -33,18 +33,13 @@ describe Hbc::DSL::Appcast do
 
   describe "#calculate_checkpoint" do
     before do
-      expect(Hbc::SystemCommand).to receive(:run) do |executable, **options|
+      expect(Hbc::SystemCommand).to receive(:run).with(*cmd_args).and_return(cmd_result)
-        expect(executable).to eq "/usr/bin/curl"
-        expect(options[:args]).to include(*cmd_args)
-        expect(options[:print_stderr]).to be false
-        cmd_result
-      end
       allow(cmd_result).to receive(:success?).and_return(cmd_success)
       allow(cmd_result).to receive(:stdout).and_return(cmd_stdout)
     end
 
     context "when server returns a successful HTTP status" do
-      let(:cmd_args) { [HOMEBREW_USER_AGENT_FAKE_SAFARI, "--compressed", "--location", "--fail", uri] }
+      let(:cmd_args) { ["/usr/bin/curl", args: ["--compressed", "--location", "--user-agent", Hbc::URL::FAKE_USER_AGENT, "--fail", uri], print_stderr: false] }
       let(:cmd_result) { double("Hbc::SystemCommand::Result") }
       let(:cmd_success) { true }
       let(:cmd_stdout) { "hello world" }
@@ -61,7 +56,7 @@ describe Hbc::DSL::Appcast do
     end
 
     context "when server returns a non-successful HTTP status" do
-      let(:cmd_args) { [HOMEBREW_USER_AGENT_FAKE_SAFARI, "--compressed", "--location", "--fail", uri] }
+      let(:cmd_args) { ["/usr/bin/curl", args: ["--compressed", "--location", "--user-agent", Hbc::URL::FAKE_USER_AGENT, "--fail", uri], print_stderr: false] }
       let(:cmd_result) { double("Hbc::SystemCommand::Result") }
       let(:cmd_success) { false }
       let(:cmd_stdout) { "some error message from the server" }

Library/Homebrew/utils/curl.rb

@@ -1,46 +1,42 @@
 require "pathname"
 require "open3"
 
-def curl_executable
+def curl_args(options = {})
   curl = Pathname.new ENV["HOMEBREW_CURL"]
   curl = Pathname.new "/usr/bin/curl" unless curl.exist?
-  return curl if curl.executable?
+  raise "#{curl} is not executable" unless curl.exist? && curl.executable?
-  raise "#{curl} is not executable"
-end
 
-def curl_args(*extra_args, show_output: false, user_agent: :default)
   args = [
-    curl_executable.to_s,
+    curl.to_s,
-    "--fail",
+    "--remote-time",
-    "--show-error",
+    "--location",
   ]
 
-  args << "--user-agent" << case user_agent
+  case options[:user_agent]
-  when :browser, :fake
+  when :browser
-    HOMEBREW_USER_AGENT_FAKE_SAFARI
+    args << "--user-agent" << HOMEBREW_USER_AGENT_FAKE_SAFARI
-  when :default
-    HOMEBREW_USER_AGENT_CURL
   else
-    user_agent
+    args << "--user-agent" << HOMEBREW_USER_AGENT_CURL
   end
 
-  unless show_output
+  unless options[:show_output]
     args << "--progress-bar" unless ARGV.verbose?
     args << "--verbose" if ENV["HOMEBREW_CURL_VERBOSE"]
+    args << "--fail"
     args << "--silent" if !$stdout.tty? || ENV["TRAVIS"]
   end
 
-  args + extra_args
+  args += options[:extra_args] if options[:extra_args]
+  args
 end
 
 def curl(*args)
-  safe_system(*curl_args(*args))
+  safe_system(*curl_args(extra_args: args))
 end
 
-def curl_download(*args, to: nil, **options)
+def curl_output(*args)
-  curl(*args, "--location", "--remote-time", "--continue-at", "-", "--output", to, **options)
+  curl_args = curl_args(extra_args: args, show_output: true)
-end
+  Open3.popen3(*curl_args) do |_, stdout, stderr, wait_thread|
-
+    [stdout.read, stderr.read, wait_thread.value]
-def curl_output(*args, **options)
+  end
-  Open3.capture3(*curl_args(*args, show_output: true, **options))
 end

Library/Homebrew/utils/github.rb

@@ -166,7 +166,7 @@ module GitHub
 
       args += ["--dump-header", headers_tmpfile.path]
 
-      output, errors, status = curl_output(url.to_s, "--location", *args)
+      output, errors, status = curl_output(url.to_s, *args)
       output, _, http_code = output.rpartition("\n")
       output, _, http_code = output.rpartition("\n") if http_code == "000"
       headers = headers_tmpfile.read