From 3f7d9f82fc61974a8bf628f9c328b767cd3086a0 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 22 Apr 2022 12:05:14 -0400 Subject: [PATCH] #curl_download: default try_partial to false 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). --- Library/Homebrew/download_strategy.rb | 7 ++++--- Library/Homebrew/utils/curl.rb | 2 +- Library/Homebrew/utils/spdx.rb | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 30bb8d9a71..db09e10f0a 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -375,6 +375,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy def initialize(url, name, version, **meta) super + @try_partial = true @mirrors = meta.fetch(:mirrors, []) end @@ -523,7 +524,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy 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, @@ -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 @@ -656,7 +657,7 @@ class CurlPostDownloadStrategy < CurlDownloadStrategy 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 diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index 20792f25a5..f1dd65e450 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -161,7 +161,7 @@ module Utils 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 diff --git a/Library/Homebrew/utils/spdx.rb b/Library/Homebrew/utils/spdx.rb index 93e2d7d1d9..38e0183e92 100644 --- a/Library/Homebrew/utils/spdx.rb +++ b/Library/Homebrew/utils/spdx.rb @@ -34,8 +34,8 @@ module SPDX 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)