Merge pull request #4595 from reitermarkus/refactor-download-strategies

Refactor download strategies.

Library/Homebrew/download_strategy.rb

@@ -14,8 +14,9 @@ class AbstractDownloadStrategy
     end
   end
 
-  attr_reader :meta, :name, :version
-  attr_reader :shutup
+  attr_reader :cached_location
+  attr_reader :meta, :name, :version, :shutup
+  private :meta, :name, :version, :shutup
 
   def initialize(url, name, version, **meta)
     @url = url
@@ -52,11 +53,21 @@ class AbstractDownloadStrategy
                   .extract_nestedly(basename: basename_without_params,
                                     extension_only: true,
                                     verbose: ARGV.verbose? && !shutup)
+    chdir
   end
 
-  # @!attribute [r] cached_location
-  # The path to the cached file or directory associated with the resource.
-  def cached_location; end
+  def chdir
+    entries = Dir["*"]
+    case entries.length
+    when 0 then raise "Empty archive"
+    when 1 then begin
+        Dir.chdir entries.first
+      rescue
+        nil
+      end
+    end
+  end
+  private :chdir
 
   # @!attribute [r]
   # return most recent modified time for all files in the current working directory after stage.
@@ -94,7 +105,7 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
     super
     @ref_type, @ref = extract_ref(meta)
     @revision = meta[:revision]
-    @clone = HOMEBREW_CACHE/cache_filename
+    @cached_location = HOMEBREW_CACHE/"#{name}--#{cache_tag}"
   end
 
   def fetch
@@ -132,10 +143,6 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
     commit != @last_commit
   end
 
-  def cached_location
-    @clone
-  end
-
   def head?
     version.respond_to?(:head?) && version.head?
   end
@@ -152,10 +159,6 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
     raise NotImplementedError
   end
 
-  def cache_filename
-    "#{name}--#{cache_tag}"
-  end
-
   def repo_valid?
     raise NotImplementedError
   end
@@ -173,6 +176,11 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
 end
 
 class AbstractFileDownloadStrategy < AbstractDownloadStrategy
+  def initialize(url, name, version, **meta)
+    super
+    @cached_location = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
+  end
+
   def stage
     super
     chdir
@@ -180,18 +188,6 @@ class AbstractFileDownloadStrategy < AbstractDownloadStrategy
 
   private
 
-  def chdir
-    entries = Dir["*"]
-    case entries.length
-    when 0 then raise "Empty archive"
-    when 1 then begin
-        Dir.chdir entries.first
-      rescue
-        nil
-      end
-    end
-  end
-
   def ext
     # We need a Pathname because we've monkeypatched extname to support double
     # extensions (e.g. tar.gz).
@@ -207,12 +203,11 @@ class AbstractFileDownloadStrategy < AbstractDownloadStrategy
 end
 
 class CurlDownloadStrategy < AbstractFileDownloadStrategy
-  attr_reader :mirrors, :tarball_path, :temporary_path
+  attr_reader :mirrors, :temporary_path
 
   def initialize(url, name, version, **meta)
     super
     @mirrors = meta.fetch(:mirrors, [])
-    @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
     @temporary_path = Pathname.new("#{cached_location}.incomplete")
   end
 
@@ -236,10 +231,6 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy
     retry
   end
 
-  def cached_location
-    tarball_path
-  end
-
   def clear_cache
     super
     rm_rf(temporary_path)
@@ -364,8 +355,6 @@ end
 
 # This strategy extracts local binary packages.
 class LocalBottleDownloadStrategy < AbstractFileDownloadStrategy
-  attr_reader :cached_location
-
   def initialize(path)
     @cached_location = path
   end
@@ -518,11 +507,11 @@ end
 #     url "scp://example.com/src/abc.1.0.tar.gz"
 #     ...
 class ScpDownloadStrategy < AbstractFileDownloadStrategy
-  attr_reader :tarball_path, :temporary_path
+  attr_reader :temporary_path
 
   def initialize(url, name, version, **meta)
     super
-    @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
+    @cached_location = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
     @temporary_path = Pathname.new("#{cached_location}.incomplete")
     parse_url_pattern
   end
@@ -552,10 +541,6 @@ class ScpDownloadStrategy < AbstractFileDownloadStrategy
     end
   end
 
-  def cached_location
-    tarball_path
-  end
-
   def clear_cache
     super
     rm_rf(temporary_path)

Library/Homebrew/test/download_strategies_spec.rb

@@ -234,8 +234,8 @@ describe CurlDownloadStrategy do
     expect(subject.send(:_curl_args)).to eq(["--user", "download:123456"])
   end
 
-  describe "#tarball_path" do
-    subject { described_class.new(url, name, version, **specs).tarball_path }
+  describe "#cached_location" do
+    subject { described_class.new(url, name, version, **specs).cached_location }
 
     context "when URL ends with file" do
       it { is_expected.to eq(HOMEBREW_CACHE/"foo-.tar.gz") }