Skip to content

Commit

Permalink
#curl_download: default try_partial to false
Browse files Browse the repository at this point in the history
When its `try_partial` argument is `true`, `#curl_download` makes a
`HEAD` request before downloading the file using `#curl`. Currently
`try_partial` defaults to `true`, so any `#curl_download` call that
doesn't explicitly specify `try_partial: false` will make a `HEAD`
request first. This can potentially involve several requests if the
URL redirects, so it can be a bit of unnecessary overhead when a
partial download isn't needed.

Partial downloads are generally only useful when we're working with
larger files, however there's currently only one place in brew where
`#curl_download` is used and this is the case:
`CurlDownloadStrategy`. The other `#curl_download` calls are fetching
smaller [text] files and don't need to support partial downloads.

This commit changes the default `try_partial` value to `false`,
making partial downloads opt-in rather than opt-out.

We want `try_partial` to continue to default to `true` in
`CurlDownloadStrategy` and there are various ways to accomplish this.
In this commit, I've chosen to update its `#initialize` method to
accept a `try_partial` argument that defaults to `true`, as this
value can also be used in classes that inherit from
`CurlDownloadStrategy` (e.g., `HomebrewCurlDownloadStrategy`). This
instance variable is passed to `#curl_download` in related methods,
effectively maintaining the previous `try_partial: true` value, while
also allowing this value to be overridden when necessary.

Other uses of `#curl_download` in brew are
`Formulary::FromUrlLoader#load_file` and
`Cask::CaskLoader::FromURILoader#load`, which did not provide a
`try_partial` argument but should have been using
`try_partial: false`. With the `try_partial: false` default in this
commit, these calls are now fine without a `try_partial` argument.

The only other use of `#curl_download` in brew is
`SPDX#download_latest_license_data!`. These calls were previously
using `try_partial: false` but we can now omit this argument with
the new `false` default (aligning with the above).
  • Loading branch information
samford committed Apr 22, 2022
1 parent 33398d7 commit 6190b1d
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 7 deletions.
9 changes: 5 additions & 4 deletions Library/Homebrew/download_strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,9 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy

attr_reader :mirrors

def initialize(url, name, version, **meta)
def initialize(url, name, version, try_partial: true, **meta)
super
@try_partial = try_partial
@mirrors = meta.fetch(:mirrors, [])
end

Expand Down Expand Up @@ -523,7 +524,7 @@ def _fetch(url:, resolved_url:, timeout:)
end

def _curl_download(resolved_url, to, timeout)
curl_download resolved_url, to: to, timeout: timeout
curl_download resolved_url, to: to, try_partial: @try_partial, timeout: timeout
end

# Curl options to be always passed to curl,
Expand Down Expand Up @@ -577,7 +578,7 @@ class HomebrewCurlDownloadStrategy < CurlDownloadStrategy
def _curl_download(resolved_url, to, timeout)
raise HomebrewCurlDownloadStrategyError, url unless Formula["curl"].any_version_installed?

curl_download resolved_url, to: to, timeout: timeout, use_homebrew_curl: true
curl_download resolved_url, to: to, try_partial: @try_partial, timeout: timeout, use_homebrew_curl: true
end
end

Expand Down Expand Up @@ -656,7 +657,7 @@ def _fetch(url:, resolved_url:, timeout:)
query.nil? ? [url, "-X", "POST"] : [url, "-d", query]
end

curl_download(*args, to: temporary_path, timeout: timeout)
curl_download(*args, to: temporary_path, try_partial: @try_partial, timeout: timeout)
end
end

Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/utils/curl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def curl(*args, print_stdout: true, **options)
result
end

def curl_download(*args, to: nil, try_partial: true, **options)
def curl_download(*args, to: nil, try_partial: false, **options)
destination = Pathname(to)
destination.dirname.mkpath

Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/utils/spdx.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ def latest_tag

def download_latest_license_data!(to: DATA_PATH)
data_url = "https://raw.githubusercontent.com/spdx/license-list-data/#{latest_tag}/json/"
curl_download("#{data_url}licenses.json", to: to/"spdx_licenses.json", try_partial: false)
curl_download("#{data_url}exceptions.json", to: to/"spdx_exceptions.json", try_partial: false)
curl_download("#{data_url}licenses.json", to: to/"spdx_licenses.json")
curl_download("#{data_url}exceptions.json", to: to/"spdx_exceptions.json")
end

def parse_license_expression(license_expression)
Expand Down

0 comments on commit 6190b1d

Please sign in to comment.