[MERGE] HTTP redirection

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Argh, there is a couple of misunderstandings here I'm afraid, see below.
> 
>>>>>> "aaron" == Aaron Bentley <aaron.bentley at utoronto.ca> writes:

>     aaron> I agree that it's surprising, but I think it's preferable.
> 
>     aaron> It is not reasonable to use BzrDirMetaFormat1 as a
>     aaron> stand-in for BzrDir.  One is not supposed to invoke
>     aaron> probe_transport on BzrDirMeta1, but on BzrDir, because
>     aaron> it can return any of the subtypes of BzrDir.
> 
> Nobody invokes probe_transport except BzrDirFormat.find_format.

I don't see what that has to do with it.

>     aaron> I think it would be a really good idea to split
>     aaron> BzrDirFormat into BzrControlFormat and
>     aaron> BzrDirFormatInterface.

> Gee, I feel doing that will be a mine-field for *me*, but I'll
> *welcome* such a refactoring !

Yeah, that was more of an observation than a suggestion.

>     >> And my previous implementation was changing the way
>     >> BzrDirMetaFormat1 behave (to avoid imposing the redirection
>     >> handling to foreign branches).
> 
>     aaron> It would be wrong to make the change for
>     aaron> BzrDirMetaFormat1 only.  That would have introduced a
>     aaron> regresion for BzrDir4,5&6.
> 
> No.
> 
> The previous implementation leaved the BzrDir4,5&6 handle
> redirections silently as before.

As I understood it, no redirections are ever being handled silently anymore.

>     aaron> Further, I think that if any control directory's
>     aaron> find_format method raises a RedirectRested,
>     aaron> redirecting is the only reasonable choice.  A control
>     aaron> dir that does not want this processing should handle
>     aaron> the redirect in find_format.
> 
> You cannot redefine find_format in a daughter class. Well, you
> can, it will just be never called (that's precisely the point a
> registering the concrete class).

Okay, so it's probe_transport where they would need to handle a redirect
differently.

> I reverted it then but the point remains: if we are not able to
> define specific probe_transport for bzr, one day or another some
> refactoring will be in order.

The specific probe_transport for bzr is BzrDirFormat.probe_transport.

I don't see any test to ensure that redirecting a bundle works.  We
should have one of those.  It might also be nice to test
do_catching_redirections.

> +        def redirected(transport, e, redirection_notice):
> +            qualified_source = e.get_source_url()
> +            relpath = transport.relpath(qualified_source)
> +            if not e.target.endswith(relpath):
> +                # Not redirected to a branch-format, not a branch
> +                raise errors.NotBranchError(path=e.target)
> +            target = e.target[:-len(relpath)]
> +            note('%s is%s redirected to %s',
> +                 transport.base, e.permanently, target)
> +            # Let's try with a new transport
> +            qualified_target = e.get_target_url()[:-len(relpath)]
> +            # FIXME: If 'transport' have a qualifier, this should

^^ should be "If 'transport' has..."


> +def do_catching_redirections(action, transport, redirected, exception):
> +    """Execute an action with given transport catching redirections.
> +
> +    This is a facility provided for callers needing to follow redirections
> +    silently. The silent is relative: it is the caller responsability to
> +    inform the user about each redirection or only inform the user of a user
> +    via the exception parameter.
> +
> +    :param action: A callable, what the caller want to do while catching
> +                  redirections.
> +    :param transport: The initial transport used.
> +    :param redirected: A callable receiving the redirected transport and the 
> +                  RedirectRequested exception.
> +    :param exception: The exception raised if too much redirections occur.
> +
> +    :return: Whatever 'action' returns
> +    """
> +    MAX_REDIRECTIONS = 8
> +
> +    t = transport
> +    # If a loop occurs, there is little we can do. So we don't try to detect
> +    # them, just getting out if too much redirections occurs. The solution
> +    # is outside: where the loop is defined.
> +    for redirections in range(MAX_REDIRECTIONS):
> +        try:
> +            return action(t)
> +        except errors.RedirectRequested, e:
> +            redirection_notice = '%s is%s redirected to %s' % (
> +                e.get_source_url(), e.permanently, e.get_target_url())
> +            t = redirected(t, e, redirection_notice)
> +    else:
> +        # Loop exited without resolving redirect ? Either the user has kept
> +        # a very very very old reference or a loop occured in the
> +        # redirections.  Nothing we can cure here: tell the user. Note that
> +        # as the user has been informed about each redirection (it is the
> +        # caller responsibility to do that in redirected via the provided
> +        # redirection_notice), there is no need to issue an additional error
> +        # message.
> +        raise exception

I think it would make sense to raise TooManyRedirections here, and let
callers re-raise as something else, if appropriate.

> @@ -226,12 +226,13 @@
>          code, response_file = self._get(relpath, None)
>          return response_file
>  
> -    def _get(self, relpath, ranges):
> +    def _get(self, relpath, ranges, tail_amount=0):
>          """Get a file, or part of a file.
>  
>          :param relpath: Path relative to transport base URL
> -        :param byte_range: None to get the whole file;
> -            or [(start,end)] to fetch parts of a file.
> +        :param ranges: None to get the whole file;
> +            or [(start,end)+], a list of tuples to fetch parts of a file.
> +        :param tail_amount: to fetch that amount from file tail.

This documentation doesn't clarify what tail_amount is, to me.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF5HeN0F+nu1YWqI0RAnv5AJ9vkCswjCFvQXHER37H1LWNWegDFACeJxWm
SRtAe8f+HOzXYFdoNlxqQqw=
=3l4/
-----END PGP SIGNATURE-----