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.

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

Allow incomplete downloads to be resumed even when server rejects HEAD requests #5421

Merged
merged 1 commit into from Dec 23, 2018

Conversation

Projects
None yet
3 participants
@smammy
Copy link

smammy commented Dec 18, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example. In order to write a test for this I'd have to refactor curl_download or (I guess) mock curl_output and curl and I don't know how to do that. Old behavior wasn't unit-tested either so I don't think it's a huge deal.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Use GET rather than HEAD when checking for range support in curl_download.

  • Some HTTP servers apparently support ranges but don't support HEAD.
  • This is a more realistic check anyway since the actual download request
    will use GET (not HEAD).
  • This fixes #5420.

@MikeMcQuaid MikeMcQuaid requested a review from reitermarkus Dec 18, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Dec 18, 2018

CC @reitermarkus for thoughts.

@smammy

This comment has been minimized.

Copy link

smammy commented Dec 18, 2018

Although, honestly I don't understand what was wrong with the behavior before 0721271 where curl_download always tried --continue-at - and recovered from the error if the server didn't support partial content. (Most servers seem to these days, so the old pre-0721271 behavior would save us an HTTP request in the common case.) #4847 doesn't explain why the upfront check was added, but I assume there was some reason for it.

Sam Hathaway
Use GET rather than HEAD when checking for range support in curl_down…
…load.

   * Some HTTP servers apparently support ranges but don't support HEAD.
   * This is a more realistic check anyway since the actual download request
     will use GET (not HEAD).
   * This fixes #5420.

@smammy smammy force-pushed the smammy:5420-curl-use-GET-on-range-check branch from 3a96335 to 6e603c2 Dec 21, 2018

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Dec 22, 2018

I think the reason for the upfront check was that some servers which don't support ranges wouldn't actually fail with the appropriate error code.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Dec 23, 2018

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @smammy!

@MikeMcQuaid MikeMcQuaid merged commit 537fe2d into Homebrew:master Dec 23, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 1a78cc7...6e603c2
Details
codecov/project 71.17% (-0.01%) compared to 1a78cc7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smammy smammy deleted the smammy:5420-curl-use-GET-on-range-check branch Dec 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment