Merge lp:~jameinel/bzr/2.3-avoid-xmlrpc-ssh-397739 into lp:bzr/2.3

[John A Meinel]

Fix bug #397739, resolve 'lp:foo' locally as long as we have a launchpad-login to use bzr+ssh.

This is a quick fix for handling resolving of "lp:foo" style URLs.

I made sure to do the fix on a 2.3 branch, because it seems safe enough. We can land it on dev first, if we want to give it a little bit more time to simmer before landing it in the 2.3 series for release in Natty.

In real-world use cases, I've seen the XMLRPC request take more than a second. (Where it puts "Using saved push location lp..." and then waits a noticeable amount of time before I see the SSH connection starting.)

[Jelmer Vernooij (jelmer)]

On Wed, 2011-03-23 at 13:40 +0000, John A Meinel wrote:
> For more details, see:
> https://code.launchpad.net/~jameinel/bzr/2.3-avoid-xmlrpc-ssh-397739/+merge/54523
>
> This is a quick fix for handling resolving of "lp:foo" style URLs.
>
> I made sure to do the fix on a 2.3 branch, because it seems safe enough. We can land it on dev first, if we want to give it a little bit more time to simmer before landing it in the 2.3 series for release in Natty.
>
> In real-world use cases, I've seen the XMLRPC request take more than a second. (Where it puts "Using saved push location lp..." and then waits a noticeable amount of time before I see the SSH connection starting.)
As mentioned on IRC, it seems like HTTP does support short URLs too. For
example, I'm able to branch from https://code.launchpad.net/bzr-svn . Is
there any reason this couldn't be used?

  review needsinformation

Cheers,

Jelmer

review: Needs Information

[John A Meinel (jameinel)]

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

On 3/23/2011 3:06 PM, Jelmer Vernooij wrote:
> Review: Needs Information
> On Wed, 2011-03-23 at 13:40 +0000, John A Meinel wrote:
>> For more details, see:
>> https://code.launchpad.net/~jameinel/bzr/2.3-avoid-xmlrpc-ssh-397739/+merge/54523
>>
>> This is a quick fix for handling resolving of "lp:foo" style URLs.
>>
>> I made sure to do the fix on a 2.3 branch, because it seems safe enough. We can land it on dev first, if we want to give it a little bit more time to simmer before landing it in the 2.3 series for release in Natty.
>>
>> In real-world use cases, I've seen the XMLRPC request take more than a second. (Where it puts "Using saved push location lp..." and then waits a noticeable amount of time before I see the SSH connection starting.)
> As mentioned on IRC, it seems like HTTP does support short URLs too. For
> example, I'm able to branch from https://code.launchpad.net/bzr-svn . Is
> there any reason this couldn't be used?
>
> review needsinformation
>
> Cheers,
>
> Jelmer
>

https://code.launchpad.net/FOO is a trick we implemented a long time
ago. Before we had XMLRPC. It actually fakes a .bzr/branch/location file
to make it a 'lightweight checkout' to the real http location.

You can verify it by using

 https://code.launchpad.net/bzr-svn/.bzr/branch/location

I suppose we could revert to that old hack. I'm certainly impressed it
is still working correctly.

I think one loss is that it requires https. With a self-signed cert,
IIRC. While 'bazaar.launchpad.net' urls are all plain http.

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

iEYEARECAAYFAk2J/xoACgkQJdeBCYSNAANuuwCgyGcWBofhxDwWCi+e/e8wwcC9
DHAAoJhdRQZyjZWRKABx2LPLiUqGAbQq
=UYkr
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

Nice one.

+1 on landing on trunk first.

review: Approve

[Martin Pool (mbp)]

That's great, thanks for fixing it!

I would not put this into 2.3 until we were sure it was ok in trunk;
there is a medium-high risk of interaction bugs.

Martin

[John A Meinel (jameinel)]

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

On 3/24/2011 5:32 AM, Martin Pool wrote:
> That's great, thanks for fixing it!
>
> I would not put this into 2.3 until we were sure it was ok in trunk;
> there is a medium-high risk of interaction bugs.
>
> Martin

Looks like Andrew has already found one. Namely, stacking doesn't seem
to be triggering correctly on +branch/~user/project/branch listings.

We could change the local resolution to be absolute if it is fully
specified.

So if you supply 2 parts (or 3 parts with one being ubuntu/debian) then
we would use +branch, otherwise we just do the full expansion.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2K4c8ACgkQJdeBCYSNAAPD0wCeNa9xRqetVlGGrwQeWnzg79Uz
JIsAn0HdHD2HR2aGq8hSdbGT/F9jAx2u
=qSZ1
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

This should not be merged directly into 2.3. Instead lp:~jameinel/bzr/2.3-three-part-resolution-stacking-741375 should be the branch that gets merged. Marking rejected.

=== modified file 'bzrlib/plugins/launchpad/lp_directory.py'
--- bzrlib/plugins/launchpad/lp_directory.py	2011-01-26 19:34:58 +0000
+++ bzrlib/plugins/launchpad/lp_directory.py	2011-03-23 13:40:44 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007-2010 Canonical Ltd
+# Copyright (C) 2007-2011 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -65,10 +65,23 @@
         """See DirectoryService.look_up"""
         return self._resolve(url)
 
-    def _resolve(self, url,
-                 _request_factory=ResolveLaunchpadPathRequest,
-                 _lp_login=None):
-        """Resolve the base URL for this transport."""
+    def _resolve_locally(self, path):
+        # Launchpad is now capable of doing the short-name to long name
+        # resolution inside the bzr+ssh server. We aren't yet able to do so for
+        # HTTP urls. - jam 20110323
+        return {'urls': ['bzr+ssh://bazaar.launchpad.net/+branch/' + path]}
+
+    def _resolve_via_xmlrpc(self, path, url, _request_factory):
+        service = LaunchpadService.for_url(url)
+        resolve = _request_factory(path)
+        try:
+            result = resolve.submit(service)
+        except xmlrpclib.Fault, fault:
+            raise errors.InvalidURL(
+                path=url, extra=fault.faultString)
+        return result
+
+    def _update_url_scheme(self, url):
         # Do ubuntu: and debianlp: expansions.
         scheme, netloc, path, query, fragment = urlsplit(url)
         if scheme in ('ubuntu', 'debianlp'):
@@ -106,22 +119,30 @@
                 series=series,
                 project=project)
             scheme, netloc, path, query, fragment = urlsplit(url)
-        service = LaunchpadService.for_url(url)
-        if _lp_login is None:
-            _lp_login = get_lp_login()
-        path = path.strip('/')
+        return url, path
+
+    def _expand_user(self, path, url, lp_login):
         if path.startswith('~/'):
-            if _lp_login is None:
+            if lp_login is None:
                 raise errors.InvalidURL(path=url,
                     extra='Cannot resolve "~" to your username.'
                           ' See "bzr help launchpad-login"')
-            path = '~' + _lp_login + path[1:]
-        resolve = _request_factory(path)
-        try:
-            result = resolve.submit(service)
-        except xmlrpclib.Fault, fault:
-            raise errors.InvalidURL(
-                path=url, extra=fault.faultString)
+            path = '~' + lp_login + path[1:]
+        return path
+
+    def _resolve(self, url,
+                 _request_factory=ResolveLaunchpadPathRequest,
+                 _lp_login=None):
+        """Resolve the base URL for this transport."""
+        url, path = self._update_url_scheme(url)
+        if _lp_login is None:
+            _lp_login = get_lp_login()
+        path = path.strip('/')
+        path = self._expand_user(path, url, _lp_login)
+        if _lp_login is not None:
+            result = self._resolve_locally(path)
+        else:
+            result = self._resolve_via_xmlrpc(path, url, _request_factory)
 
         if 'launchpad' in debug.debug_flags:
             trace.mutter("resolve_lp_path(%r) == %r", url, result)

=== modified file 'bzrlib/plugins/launchpad/test_lp_directory.py'
--- bzrlib/plugins/launchpad/test_lp_directory.py	2011-01-10 22:20:12 +0000
+++ bzrlib/plugins/launchpad/test_lp_directory.py	2011-03-23 13:40:44 +0000
@@ -167,16 +167,16 @@
         self.assertEquals('http://bazaar.launchpad.net/~apt/apt/devel',
                           directory._resolve('lp:///apt', factory))
 
-    def test_rewrite_bzr_ssh_launchpad_net(self):
+    def test_with_login_avoid_resolve_factory(self):
         # Test that bzr+ssh URLs get rewritten to include the user's
         # Launchpad ID (assuming we know the Launchpad ID).
         factory = FakeResolveFactory(
             self, 'apt', dict(urls=[
-                    'bzr+ssh://bazaar.launchpad.net/~apt/apt/devel',
+                    'bzr+ssh://my-super-custom/special/devel',
                     'http://bazaar.launchpad.net/~apt/apt/devel']))
         directory = LaunchpadDirectory()
         self.assertEquals(
-            'bzr+ssh://bazaar.launchpad.net/~apt/apt/devel',
+            'bzr+ssh://bazaar.launchpad.net/+branch/apt',
             directory._resolve('lp:///apt', factory, _lp_login='username'))
 
     def test_no_rewrite_of_other_bzr_ssh(self):
@@ -199,15 +199,15 @@
     def test_resolve_tilde_to_user(self):
         factory = FakeResolveFactory(
             self, '~username/apt/test', dict(urls=[
-                    'bzr+ssh://bazaar.launchpad.net/~username/apt/test']))
+                'bzr+ssh://bazaar.launchpad.net/+branch/~username/apt/test']))
         directory = LaunchpadDirectory()
         self.assertEquals(
-            'bzr+ssh://bazaar.launchpad.net/~username/apt/test',
+            'bzr+ssh://bazaar.launchpad.net/+branch/~username/apt/test',
             directory._resolve('lp:~/apt/test', factory, _lp_login='username'))
         # Should also happen when the login is just set by config
         set_lp_login('username')
         self.assertEquals(
-            'bzr+ssh://bazaar.launchpad.net/~username/apt/test',
+            'bzr+ssh://bazaar.launchpad.net/+branch/~username/apt/test',
             directory._resolve('lp:~/apt/test', factory))
 
     def test_tilde_fails_no_login(self):

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-03-23 11:07:49 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-03-23 13:40:44 +0000
@@ -33,6 +33,15 @@
   of CHK data, down to just 150MB.) This has noticeable affects for things
   like building checkouts, etc.  (John Arbash Meinel, #737234)
 
+* Resolve ``lp:FOO`` urls locally rather than doing an XMLRPC request if
+  the user has done ``bzr launchpad-login``. The bzr+ssh URLs were already
+  being handed off to the remote server anyway (xmlrpc has been mapping
+  ``lp:bzr`` to ``bzr+ssh://bazaar.launchpad.net/+branch/bzr``, rather
+  than ``bzr+ssh://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev`` for a few
+  months now.) By doing it ourselves, we can cut out substantial startup
+  time. From Netherlands to London it was taking 368ms to do the XMLRPC
+  call as much as 2s from Sydney. (John Arbash Meinel, #397739)
+
 Bug Fixes
 *********