[MERGE] Fix bug 146715: bzr+ssh:// and sftp:// transports should not assume port-not-specified means port 22

Andrew Bennetts andrew at canonical.com
Mon Oct 8 03:30:33 BST 2007


This bundle fixes bug 146715.  The problem is that we can't assume that
bzr+ssh:// and sftp:// URLs with no port means we should connect to port 22,
because some users expect to be able to override the default port in their
OpenSSH ~/.ssh/config file.  So this bundle unsets the default_port for
bzr+ssh:// and sftp://, and adds some comments and tests to make sure this
behaviour isn't actually “corrected”.

While I was there I also tidied up some imports in test_transport.py that were
unused, or duplicated (or both!), so pyflakes now gives no warnings for that
file.

-Andrew.

-------------- next part --------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: andrew.bennetts at canonical.com-20071008022217-\
#   8gxkimbur9vzgbav
# target_branch: http://bazaar-vcs.org/bzr/bzr.dev
# testament_sha1: be29e95a333222543ec75df86d2ea883ae9e9556
# timestamp: 2007-10-08 12:24:25 +1000
# source_branch: http://people.ubuntu.com/~andrew/bzr/ssh-port-bug-\
#   146715
# base_revision_id: pqm at pqm.ubuntu.com-20071006144547-0e1mpht72yd6wyfz
# 
# Begin patch
=== modified file 'NEWS'
--- NEWS	2007-10-05 14:55:04 +0000
+++ NEWS	2007-10-08 02:22:17 +0000
@@ -77,6 +77,11 @@
 
   BUG FIXES:
 
+   * ``bzr+ssh://`` and ``sftp://`` URLs that do not specify ports explicitly
+     no longer assume that means port 22.  This allows people using OpenSSH to
+     override the default port in their ``~/.ssh/config`` if they wish.  This
+     fixes a bug introduced in bzr 0.91.  (Andrew Bennetts, #146715)
+
    * Commands reporting exceptions can now be profiled and still have their
      data correctly dumped to a file. For example, a ``bzr commit`` with
      no changes still reports the operation as pointless but doing so no

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py	2007-10-04 07:12:14 +0000
+++ bzrlib/tests/test_transport.py	2007-10-08 02:22:17 +0000
@@ -15,9 +15,6 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
 
-import os
-import sys
-import stat
 from cStringIO import StringIO
 
 import bzrlib
@@ -26,15 +23,11 @@
     osutils,
     urlutils,
     )
-from bzrlib.errors import (ConnectionError,
-                           DependencyNotPresent,
+from bzrlib.errors import (DependencyNotPresent,
                            FileExists,
                            InvalidURLJoin,
                            NoSuchFile,
                            PathNotChild,
-                           TransportNotPossible,
-                           ConnectionError,
-                           DependencyNotPresent,
                            ReadError,
                            UnsupportedProtocol,
                            )
@@ -49,7 +42,6 @@
                               LateReadError,
                               register_lazy_transport,
                               register_transport_proto,
-                              _clear_protocol_handlers,
                               Transport,
                               )
 from bzrlib.transport.chroot import ChrootServer
@@ -625,14 +617,14 @@
     """Tests for connected to remote server transports"""
 
     def test_parse_url(self):
-        t = ConnectedTransport('sftp://simple.example.com/home/source')
+        t = ConnectedTransport('http://simple.example.com/home/source')
         self.assertEquals(t._host, 'simple.example.com')
-        self.assertEquals(t._port, 22)
+        self.assertEquals(t._port, 80)
         self.assertEquals(t._path, '/home/source/')
         self.failUnless(t._user is None)
         self.failUnless(t._password is None)
 
-        self.assertEquals(t.base, 'sftp://simple.example.com/home/source/')
+        self.assertEquals(t.base, 'http://simple.example.com/home/source/')
 
     def test_parse_quoted_url(self):
         t = ConnectedTransport('http://ro%62ey:h%40t@ex%41mple.com:2222/path')
@@ -765,6 +757,80 @@
         self.assertEquals('bzr://host.com:666/', t.base)
 
 
+class SSHPortTestMixin(object):
+    """Mixin class for testing SSH-based transports' use of ports in URLs.
+    
+    Unlike other connected transports, SSH-based transports (sftp, bzr+ssh)
+    don't have a default port, because the user may have OpenSSH configured to
+    use a non-standard port.
+    """
+
+    def make_url(self, netloc):
+        """Make a url for the given netloc, using the scheme defined on the
+        TestCase.
+        """
+        return '%s://%s/' % (self.scheme, netloc)
+
+    def test_relpath_with_implicit_port(self):
+        """SSH-based transports with the same URL are the same.
+        
+        Note than an unspecified port number is different to port 22 (because
+        OpenSSH may be configured to use a non-standard port).
+
+        So t.relpath(url) should always be '' if t.base is the same as url, but
+        raise PathNotChild if the ports in t and url are not both specified (or
+        both unspecified).
+        """
+        url_implicit_port = self.make_url('host.com')
+        url_explicit_port = self.make_url('host.com:22')
+
+        t_implicit_port = get_transport(url_implicit_port)
+        self.assertEquals('', t_implicit_port.relpath(url_implicit_port))
+        self.assertRaises(
+            PathNotChild, t_implicit_port.relpath, url_explicit_port)
+        
+        t_explicit_port = get_transport(url_explicit_port)
+        self.assertRaises(
+            PathNotChild, t_explicit_port.relpath, url_implicit_port)
+        self.assertEquals('', t_explicit_port.relpath(url_explicit_port))
+
+    def test_construct_with_no_port(self):
+        """If no port is specified, then the SSH-based transport's _port will
+        be None.
+        """
+        t = get_transport(self.make_url('host.com'))
+        self.assertEquals(None, t._port)
+
+    def test_url_with_no_port(self):
+        """If no port was specified, none is shown in the base URL."""
+        t = get_transport(self.make_url('host.com'))
+        self.assertEquals(self.make_url('host.com'), t.base)
+
+    def test_url_includes_port(self):
+        """An SSH-based transport's base will show the port if one was
+        specified, even if that port is 22, because we do not assume 22 is the
+        default port.
+        """
+        # 22 is the "standard" port for SFTP.
+        t = get_transport(self.make_url('host.com:22'))
+        self.assertEquals(self.make_url('host.com:22'), t.base)
+        # 666 is not a standard port.
+        t = get_transport(self.make_url('host.com:666'))
+        self.assertEquals(self.make_url('host.com:666'), t.base)
+
+
+class SFTPTransportPortTest(TestCase, SSHPortTestMixin):
+    """Tests for sftp:// transport (SFTPTransport)."""
+
+    scheme = 'sftp'
+
+
+class BzrSSHTransportPortTest(TestCase, SSHPortTestMixin):
+    """Tests for bzr+ssh:// transport (RemoteSSHTransport)."""
+
+    scheme = 'bzr+ssh'
+
+
 class TestTransportTrace(TestCase):
 
     def test_get(self):

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2007-10-06 07:25:06 +0000
+++ bzrlib/transport/__init__.py	2007-10-08 02:22:17 +0000
@@ -1680,9 +1680,10 @@
 register_lazy_transport('file://', 'bzrlib.transport.local', 'LocalTransport')
 transport_list_registry.set_default_transport("file://")
 
+# Note that sftp:// has no default_port, because the user's ~/.ssh/config
+# can set it to arbitrary values based on hostname.
 register_transport_proto('sftp://',
-            help="Access using SFTP (most SSH servers provide SFTP).",
-            default_port=22)
+            help="Access using SFTP (most SSH servers provide SFTP).")
 register_lazy_transport('sftp://', 'bzrlib.transport.sftp', 'SFTPTransport')
 # Decorated http transport
 register_transport_proto('http+urllib://',
@@ -1775,9 +1776,10 @@
 register_lazy_transport('bzr+https://',
                         'bzrlib.transport.remote',
                         'RemoteHTTPTransport')
+# Note that bzr+ssh:// has no default_port, because the user's ~/.ssh/config
+# can set it to arbitrary values based on hostname.
 register_transport_proto('bzr+ssh://',
-            help="Fast access using the Bazaar smart server over SSH.",
-            default_port=22)
+            help="Fast access using the Bazaar smart server over SSH.")
 register_lazy_transport('bzr+ssh://',
                         'bzrlib.transport.remote',
                         'RemoteSSHTransport')

# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWRqANckABapfgGRQW////3//
/7D////xYAxdfLgN1K5NyhQWOKvurR60o7Y9MSoCSG2hJJNTak9TTyTTA0JqeKA9TxNTI0eoNGR6
m1DxRtMpoEqYgBAmU9KbTVPQjE0HpDQNAGgAGj1ADmE0ZGhoZDCNDIaaNABiMmQDCAYBIiQENJk0
xTDSZT2pMah+pMyNTyRo0Bp6gAZBJKSf6oepNNBoaMmjRoBo0ZGIaaPU0yNAANASRCaAmTKp+TQ0
NTak9pkpshDTQ2oyNGhkGgNKhVNBvCaAULFkbuYMzRo43wt81VTfBJCuhmr6965dElGQwKpIh7En
d/XQoMEEccVeXgbXtd3wgKqMXPAsK66I85QY7T2mVPi4lMGCDfJOO61pdB8rUoODcGZgGZLh2YWE
ncvk1FFx4GhOMzY20ABSQAWHMT4MNvufYoNC7TYmpfLVipUbwalXTMwzAmYmwbBD+lP4yXHqX00N
E6kc9DWos9+zgw+k1Jt38V9KHXdZJHKLYo7CU2daK8mfXIxcCf2Z9Xj80MRNm84zJxnLrdaHTnOc
2OZE/WUPd702i6qIr4y9+GHkPJ56wLRawe3kEf89DYpOFPBxIn03glMhcEpZmd33s7Nf58enMdPY
aOQnrrx43VERocrLw7t29UVTX4N5PvQyE6DDEobj2tO61VnSWxUlJBXyHyRFZVS03yk4Zg1JYYC0
svl23vDtUjpTTFHaIChtTQdoVSCTYio0fUjeUJI6VI0M9MXLmMKdj321l/FcESPl0EbWZnkswG7d
8n/d/Roausz9nto6LOpW5N04ZQO3qO1NohskCT4hFdujxUrYwY0yR4mCo8wQNmvhDZ/kpWSPrOHx
UVKGkd6ScxDdB1kbcQeRBVRSyBqjgKMo23IzCW3w4eX1RVj4B6nuuqrZaDdXhyUkG0+Znne8cywe
b274q9/CIghaLuO6Vbvgr4MxnM3eIPWta2h3dRhuBucZgDccCwFuRz9tmwmeR8WetuhwaTipDZOy
cLOOo63hl5rMIuLghkc6FecHhiYIgVMeje4ggqeKhvZA/EraGSjh/lnBRecNuaBC5fcqVxUiPtEx
QKJlv0hXsDsLBLJctDaT2cdKEVzssAYmkDBXlkOgKu4LDZ9BNPQ0GMJulFkkJAyxk9NAU0BhV8cS
CqGOwPSen158UFjIiMsYkMskgyj4IFGB8sIGOaw1Uo68PKLbgEgfTj88tsCOlmKYknGyGAO3MgZ2
idyZZCp2eIvLhYNYoHZSZubuKFzLq0UTBEqqNCQoTI3wBSgx4UxVEFoBQgY4QncmqOTIZnOrMXMq
KKeIof12yC+dMkGLwuamHKZoPUbQU6E6dy2DNDC0BBDAr+KjcMS5UpSS4mNIS5aKwSJCKGxuLQEq
PJ2JRIgpEpALbkjWOncFLGgdtDo6ZF/B4fRlpnjn0Ya4zRTUwdyWaOOwNoEzhCnNSaU5eGigdUjK
oK8Cum2VJYfCg7sL8TmVRgOKd7LZts9Oer0tpzQ01hbEYiji5Afd6O2Q5k4FCrocDKoV3Yk44jED
okzhi2pJ4fWutYthpRwwOehbAc4iCOwOQSzJGepg5C7x41cH7F2GDQqTNB1XmZKp1lxZDyE0TBI2
dAZLg5rISEKEMyTJcWokhBjIuU+/MsZvMyhAxMZFix1gmhMa7Sasbs+NxAryVL4DEzIoLiJiYjmc
ka3Phk6AXpA3c+S6ufW2353EQeZaa3A873jOjwCkG84VJVchHaYYQB3te/iSMviW9eMieBxe9Cgg
qghq7n3B1aZriXa5oQhU0o5ent7vh3ly9vn9O8YNoaesyddoNEs67GT6Eue/NN+Gmj5cpT3vXEBh
s2UWMPHa6uqI3LketFiEiRuZFX7uWOyTHqm075+br31eU7fjliK0pdUCmTs+DOnh0rJ+okzeEaY9
eh6iG3EpjTdNnF0e2wlGeOppWDiUuBDaGtXqIbWpo6vD6dfjR0GBjKQri47NPS392bHdulENj8lZ
Ua8We3J6NdSoxpBie2nOZmi8zJw0b0nlAzevKT97E2K50rheRiLRDJd/9LpPXjUpi7imS0HdGXyI
6RyKSP8LxhZxNAzZ5WS0GtqOwIroX3W7AOkesrGZHLY3oNVZOTKQe/WodKHUaELLEpQLTUsogw4j
OcjyhnoJbKZKvrhVEOrMyCGLEdybT7UVYotBUlJm+gjr7z2F/g+P8z7zcTicZSVurtLYM70dIL0R
eU9TnsgLELl0eYyfVzETZftgxRt8qKwUs7e9g8jYoDzsWq7oOUu8ek1kS4gb+U5tZzcPVsQsj/UK
pH4Ez7NkS8wMDLoNA+sYzb28gPWBAAsZuRZ4Sy2iSjsTLz1WsTZuA6ZgvUHAZ3TCXv4TkcTqIER5
3dyzjzBUduMUNqIAlDcgGocjqcY/WJM6UaTcyqz6bjtCSnQksi7X4l09ad+umelU0Z3TStxu5FOK
zY7aWNHOQ8VdaqVtsmNMVssneIRodSOc6CS+kzaw8hv3H1Z8fDVydw5i2CPIbbitmZslFKwRpmX5
0UnMB7Ljp0DcZB+Mgi+AZH0zn14+K5iM14hMkTDu2EbyrfWZ0LRbcybVAFHuhxV0TixXx4Zoy7W+
EBFJaJzLqHNJbhqMT1F7CYTs4xM4zITCCe5hCznclCYGjos0cv5Gfrg7AyjpmgXbu6jqOvadZQOU
2I5CwOo5XmZgWpH8rxqgpjejxIMifTFcIcHIERsQTMmoyNwFxoHmpdbyj3wV6wXMsGmzTCMjRIpc
IHeNDGgMr7yQOrVuPoKe9BUOE8OwJH682oSt4sIcOOtC3QgAQ8s+AHdR0pWGKNqMQIrJi3sToPnP
nBVFBaBWHjZMnklPGgmxKv7avf10w8VILffFmS2rD4kiBxciSeDektc/ZWNASqrDY3ApdEjsXQ4R
cBTpbOG8MwKSFcJxFgbu2xmR1oYWzlEKcQDVZZM8z1FS5KKwJASXcxdIaKnbUAvShkmRzedBbnoT
mLAE8DEOBwrxoqHm57Cdlfx7bqvMHYEFqR9eEfY6MvVu10KUmCZhNBKYcOkzI5bN+22VCTCLyEK5
HEVaS5SkqIoGdkVtCmf2kuSVh2M5RlSJMilym0dAqaUERgIo9c4huwC5do7n6PLhvkcRy7VzQLdr
sJLsGrirnqGEND3H1UIrkhR2O9LRkzozJWQFWxIytIF7kMCtZqJFgsEodphqoNxDmtiY2AVOeacv
Y5v37xTFSFoHwWmm8VQNhuY22MbYzsiwqW0Gk4TBiaUQRDI2yEIkGoTWro3kYe5rNmTLdHjFo9u7
GI9P6gcHQ9W1ww6YbIKCmDfBPV21Vpfm+bMHsYmMTGK8vGJrh30PHQRtv1dxqWtqelJW3dGMBZsw
RxcmY8dvCK53lzGgM7BIgBjGhgwGFjIkZrihHKvZXMQc81Kq6Cwp1eUqmj+lqLCy864xHkAgtv+F
by2VjaE3ca13j2asuWBtDSW5HOIodMyN9lC5LBFBUMZUuvdJSZWMEw2cgTQ+wsClamAoWIp0DdnE
YwToFcGcGEf2ffoThK2OpQgaFcW6ZoZ0r5DfU586vawQwqtd8KooaWFxMccMiMVegiiLhgj0TAS7
WVtX62DSQQRBgl1doc91xSq1SA6YdkroX2Fw7kUikYJdwrhOr2FKorXgmPQcm2w0sHK7irJ3NgA6
aKBQZrRy7C9TgREyUVYK1RDWK1rniRCkmUxrHGB2gOhO4o5KRYkooufvmFKM0e3mjAvIxFGqsiKx
WJUcMydUI0RMCjJpygVxHHHE4IiCSq6m1wDSuxSCnR88EmnXWsM7bokasPR5QUSTIXnsNLIxUjH1
Bm8PorICuBOAV58GuPBJV8gooTzz6g2h4Mt17aayBrBX1pfo9B83/i7kinChIDUAa5I=


More information about the bazaar mailing list