[MERGE] HTTP redirection
aaron.bentley at utoronto.ca
Tue Feb 27 18:25:17 GMT 2007
-----BEGIN PGP SIGNED MESSAGE-----
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.
> 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
> 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
> + 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the bazaar