Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

download_strategy: compare cached file size to Content-Length #19460

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+11 −8
Open

download_strategy: compare cached file size to Content-Length #19460

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions Library/Homebrew/download_strategy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,24 +407,27 @@ def fetch(timeout: nil)

ohai "Downloading #{url}"

use_cached_location = cached_location.exist?
use_cached_location = false if version.respond_to?(:latest?) && version.latest?
cached_location_valid = cached_location.exist? && !(version.respond_to?(:latest?) && version.latest?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was pre-existing, but the use of respond_to? means this isn't typesafe, do you what the potential types are? (e.g. if latest? is only for the Cask::DSL::Version case, than I'd like us to use a static helper method in that class, use to_s directly, or some other typesafe approach.)

🤔 Maybe this will nerd snipe me into typing this file…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'twas my plan all along, bwa ha ha


resolved_url, _, last_modified, _, is_redirection = begin
resolved_url, _, last_modified, file_size, is_redirection = begin
resolve_url_basename_time_file_size(url, timeout: Utils::Timer.remaining!(end_time))
rescue ErrorDuringExecution
raise unless use_cached_location
raise unless cached_location_valid
end

# Authorization is no longer valid after redirects
meta[:headers]&.delete_if { |header| header.start_with?("Authorization") } if is_redirection

# The cached location is no longer fresh if Last-Modified is after the file's timestamp
if cached_location.exist? && last_modified && last_modified > cached_location.mtime
use_cached_location = false
# The cached location is no longer fresh if either:
# - Last-Modified value is newer than the file's timestamp
# - Content-Length value is different than the file's size
cached_location_valid = if cached_location_valid
newer_last_modified = last_modified && last_modified > cached_location.mtime
different_file_size = file_size && file_size != cached_location.size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tripping over the logic with the naming here. It seems if there is a cached_location.size, but no file_size, different_file_size is false, but those are different values (is that a plausible scenario?). (Naming is hard, I'm not necessarily asking to rename anything, mostly just checking that the logic is correct.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last_modified and file_size come from line 412 where the header values of a HEAD request dispatched to the download URL are used to populate them — if they exist. If either header value is not provided, newer_last_modified and different_file_size will be false, and so cached_location_valid won't be made false on their account.

!(newer_last_modified || different_file_size)
end

if use_cached_location
if cached_location_valid
puts "Already downloaded: #{cached_location}"
else
begin
Expand Down
Loading