[MERGE][bug #111702] bzr init <remote_branch> connects multiple times

[John Arbash Meinel]

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

Vincent Ladeuil wrote:
....

> Fire, fire ! I'm refactoring :-)

By the way, before you get too far down the track, I would like to get your
changes merged. So if you can come up with a semi-clean result, we can merge
it, and then we will continue to merge your work as you finish up.

That way you don't feel like your work misses the boat. As long as you feel
sufficiently motivated to finish :)

> 
>     john> 
>     john> - t = get_transport(safe_unicode(base))
>     john> +            t = get_transport(safe_unicode(base), possible_transports)
> 
>     john> get_transport() isn't defined anymore as strictly requiring a
>     john> Unicode path. It is actually specifically defined as taking a URL
>     john> or trying to be smart if it gets a unicode path. (If it thinks it
>     john> is a URL it tries to encode it, if it is a local path it treats
>     john> it appropriately).
> 
> Yes, I saw that. 
> 
> It perfectly fit my purpose in fact ; as "local" and "connected"
> transports already have a distinct treatment, handling connection
> sharing will be a natural consequence.
> 
>     john> So I would just remove the "safe_unicode()" calls. There are
>     john> other places as well.
> 
> Will do.
> 
>     john> This loop could be clearer:
>     john> +    transport = None
>     john> +    if possible_transports:
>     john> +        for t in possible_transports:
>     john> +            if t.base == base:
>     john> +                transport = t
>     john> +                break
>     john> +    if transport is None:
>     john> ...
>     john> +    if transport is None:
>     john> ...
>     john> +    return transport
> 
>     john> Instead, you could just do:
>     john> +    if possible_transports:
>     john> +        for t in possible_transports:
>     john> +            if t.base == base:
>     john> +                return t
>     john> +                break
> 
>     john> Then you don't need the 'if transport is None' checks.
> 
> Ok. This will be refactored anyway.
> 
>     john> Also, the way you have written this, the transport will
>     john> only be re-used if it is identically the same. I
>     john> thought we were trying to also handle cases when it is
>     john> a parent/child. Or is that something you are still
>     john> working on?
> 
> Still under work. Specifically I needed the scheme, host, user
> and password to be accessible attributes of the transport, so i
> began refactoring all "connected" transports to make them
> accessible. Every implementation did that in different ways and
> many comments indicated that some sharing was more than expected
> :-)
> 
> This work is done now, I will now begin to address the multiple
> connections bugs. So far, using ftp for the tests and disabling
> the cache reveals ('b' is the actual number of connections):
> 
> FAIL: commands.test_branch.TestBranch.test_branch_remote_local
>     not equal:
> a = 1
> b = 2
> 
> 
> FAIL: commands.test_cat.TestCat.test_cat
>     not equal:
> a = 1
> b = 2
> 
> 
> FAIL: commands.test_checkout.TestCheckout.test_checkout
>     not equal:
> a = 1
> b = 2
> 
> 
> FAIL: commands.test_merge.TestMerge.test_merge
>     not equal:
> a = 1
> b = 5
> 
> 
> FAIL: commands.test_missing.TestMissing.test_missing
>     not equal:
> a = 1
> b = 3
> 
> 
> FAIL: commands.test_pull.TestPull.test_pull
>     not equal:
> a = 1
> b = 4
> 
> 
> FAIL: commands.test_push.TestPush.test_push
>     not equal:
> a = 1
> b = 2
> 
> What that means is that writable transports don't share
> connections without a global cache.
> 
>     john> And I don't think this works for local relative paths. Because
>     john> T.base is expanded to the absolute path, rather than the relative
>     john> path that was passed in. (unless I misread it and the path has
>     john> already been converted to a full url).
> 
> We don't care about reusing connections for local transports nor
> memory transports ;-)
> 
>        Vincent
> 
> 

Well, you may also want to check what happens when you open:

http://bzr.arbash-meinel.com/branches/bzr/ünicóde-test

However, support for that seems broken at the moment (and maybe never worked).
Shame. I wanted that to work rather than requiring people to type:
http://bzr.arbash-meinel.com/branches/bzr/%c3%bcnic%c3%b3de-test/

So I guess don't worry about it (though it might be nice to have that work
eventually)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGZCCZJdeBCYSNAAMRAiwIAJ0RW9ewrkT/LyLMYP72oEiWL9xSkgCffgB4
mqLatmbjMawVNT7gClhF1OU=
=mLPy
-----END PGP SIGNATURE-----