[merge] http multirange support

[Martin Pool]

On 18 Jul 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:
> >> +    elif code == 404:
> >> +        raise errors.NoSuchFile(url)
> >> +
> >> +    # TODO: jam 20060713 Properly handle redirects (302 Found, etc)
> >> +    #       The '_get' code says to follow redirects, we probably 
> >> +    #       should actually handle the return values
> >> +
> >> +    raise errors.InvalidHttpResponse(url, "Unknown response code %s" % (code,))
> >> +
> >>
> > 
> > Just for obviousness I would suggest to put the 'raise' in an else block
> > immediately after the last elif...
> 
> Do you want it in an else, or just a comment:
> 
> # If we got this far, we've failed to handle the return, raise an error.

Well, it's a pretty small point.  But I think 

  if code == 200:
  elif code == 304: # or whatever
  elif code == 404:
    .. 
  else:
    raise

makes it somewhat more clear - the code directly reflects the decision
structure.

> > I see you now call _raise_curl_connection_error from other places - is
> > it really no longer required from here?  There are some other places
> > too.
> 
> I believe we were getting 'code==0' because we were suppressing the
> pycurl errors. Since I am now raising them, I removed these lines.
> Also, there were 2 errors that mean unable to connect, and I handle
> those explicitly, rather than just dealing with 'code==0'.

OK great.

> >> +    def _get_full(self, relpath):
> >> +        """Make a request for the entire file"""
> >> +        curl = self._base_curl
> >> +        abspath, data, header = self._setup_get_request(curl, relpath)
> >>          self._curl_perform(curl)
> >> +
> >>          code = curl.getinfo(pycurl.HTTP_CODE)
> >> +        data.seek(0)
> >> +
> >>          if code == 404:
> >>              raise NoSuchFile(abspath)
> >> -        elif code == 200:
> >> -            sio.seek(0)
> >> -            return code, sio
> >> -        elif code == 206 and (ranges is not None):
> >> -            sio.seek(0)
> >> -            return code, sio
> >> -        elif code == 0:
> >> -            self._raise_curl_connection_error(curl)
> >> -        else:
> >> -            self._raise_curl_http_error(curl)
> >> +        if code != 200:
> >> +            raise errors.InvalidHttpResponse(abspath,
> >> +                'Expected a 200 or 404 response, not: %s' % code)
> >> +
> >> +        return code, data
> > 
> > Raising directly from here seems like a regression from calling
> > _raise_curl_http_error, which centralizes it and includes the eventual
> > url, which is often useful.
> 
> Well, 'abspath' is the full requested url. 

Oh I see.  Perhaps we should consistently call them 'url' not 'path' if
they in fact are?

> I suppose if we hit a
> redirect, then _raise_curl_http_error would print out the final URL
> rather than the initial redirect. And this is kind of a special case.
> This code path is when we have a full request. Getting an HTTP 206
> (partial content) would be wrong, but it isn't really an invalid http
> response. So I'm clarifying that with *this* code path, we don't support
> anything other than 200 or 404.

OK, I see.  Also it may be useful to include the status string from the
response - ie '500 Server error', because sometimes that helps in
working it out.  At least it gives more of a clue to people who don't
know http by heart.

> If I get redirect working properly, do I have a +1 to merge this in?

Yes, please do.  If the changes for redirect are substantial you may
want them reviewed - up to you.

-- 
Martin