[MERGE] HTTP redirection

[Vincent Ladeuil]

Argh, there is a couple of misunderstandings here I'm afraid, see below.

>>>>> "aaron" == Aaron Bentley <aaron.bentley at utoronto.ca> writes:

    aaron> Vincent Ladeuil wrote:
    >>>>>>> "aaron" == Aaron Bentley <aaron.bentley at utoronto.ca> writes:
    aaron> The way it looks to me, control formats are formats of
    aaron> control directories.  Examples would be 'CVS', '.svn',
    aaron> '{arch}', etc.  We currently have only one type of
    aaron> control directory-- a '.bzr' control directory.

    aaron> The inheritance makes it a bit confusing, but when
    aaron> viewed as a control dir, BzrDir is a concrete type,
    aaron> and the fact that it has subtypes is best ignored.
    >> 
    >> The original purpose was to allow *foreign* BzrDirFormat (see hg,
    >> svn and git plugins) to have distinct behaviors than native ones,
    >> and I felt that registering an abstract class was... surprising
    >> at best (hence the comment).

    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.

    aaron> From the context of BzrDir, BzrDirMetaFormat1 is not
    aaron> special.  It is one of several subtypes.

Sure.

    aaron> I think it would be a really good idea to split
    aaron> BzrDirFormat into BzrControlFormat and
    aaron> BzrDirFormatInterface.  BzrControlFormat would then be
    aaron> a concrete type with very few methods.  All the BzrDir
    aaron> formats would then derive from BzrDirFormatInterface.

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

Obviously this code is hard to read for everybody because a lot
of class methods and static methods are used to implement the
needed polymorphism.

    >> 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. Only BzrDirMetaFormat1 was
handling them explicitly in its own probe_transport. But let's
forget about it, that's not the point 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).

Have a look at bzr-svn, bzr-git, hg, they all redefine
probe_transport because that is the way find_format works, trying
probe_transport for each registered concrete class. And
find_format is called for BzrDirFormat so there is no way you can
redefine it with the current implementation.

    >> This is not *needed* anymore, so I can revert that if you
    >> wish. But it will have to be re-introduced if we need to
    >> have a specific behavior for native BzrDirFormats that we
    >> do not want to impose on foreign BzrDirFormats.

    aaron> I disagree.  Foreign control directory formats can
    aaron> implement anything they like in find_format.  They
    aaron> need not even derive from BzrDirFormat.  If they want
    aaron> to fail on redirect, they can simply raise a different
    aaron> error.

    >> >> Have I answered your concerns ?
    >> 
    aaron> Mostly, but I think you should revert your control_dir
    aaron> change.
    >> 
    >> Can you confirm that ?

    aaron> I confirm that.

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.

I don't feel knowledgeable enough of all the implications to do
that.

Anyway, the following patch takes bundles into account and
introduced a transport.do_catching_redirections function factored
out from BzrDir and reused for bundle.read_bundle_from_url.

That should ease the transition for other transport users needing
a silent (or not) redirection handling.

As the bundle is now even bigger, I just attached the diff, the
merge can be done from the
http://bazaar.launchpad.net/~bzr/bzr/bzr.http.redirection branch.

  Vincent

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 185 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070225/d5233b67/attachment-0001.pgp 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr.http.redirection.patch
Type: text/x-patch
Size: 32883 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070225/d5233b67/attachment-0001.bin