Merge lp:~florian-vichot/bzr/861008-normpath-2-leading-slashes into lp:bzr/2.4

[Florian Vichot]

Add posixpath.normpath wrapper in osutils to remove double leading slash weirdness

Fix for bug 861008, according to previous merge proposal https://code.launchpad.net/~florian-vichot/bzr/giveback/+merge/77262

[Martin Pool (mbp)]

  review approve

I think that's reasonable though I'd like a second opinion.

review: Approve

[Martin Packman (gz)]

This is fantastic Florian! I'll go ahead and send this to be merged.

review: Approve

[Martin Packman (gz)]

sent to pqm by email

[Florian Vichot (florian-vichot)]

Actually thank you, you were all very helpful, it was greatly appreciated :)

[Martin Packman (gz)]

There was a single test failure on PQM:

error: bzrlib.tests.test_transport.TestWin32LocalTransport.test_unc_clone_to_root

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/testtools/runtest.py", line 144, in _run_user
    return fn(*args)
  File "/usr/lib/python2.6/dist-packages/testtools/testcase.py", line 465, in _run_test_method
    testMethod()
  File "/home/pqm/pqm-workdir/srv/2.4/bzrlib/tests/test_transport.py", line 753, in test_unc_clone_to_root
    t = t.clone('..')
  File "/home/pqm/pqm-workdir/srv/2.4/bzrlib/transport/local.py", line 583, in clone
    return EmulatedWin32LocalTransport(abspath)
  File "/home/pqm/pqm-workdir/srv/2.4/bzrlib/transport/local.py", line 562, in __init__
    self._local_base = urlutils._win32_local_path_from_url(base)
  File "/home/pqm/pqm-workdir/srv/2.4/bzrlib/urlutils.py", line 231, in _win32_local_path_from_url
    raise errors.InvalidURL(url, 'Win32 file urls start with'
InvalidURL: Invalid url supplied to transport: "file:////:OST/path/to/some/": Win32 file urls start with file:///x:/, where x is a valid drive letter

You should find this one pretty easy to resolve. It's a test suite only issue, the EmulatedWin32LocalTransport class is trying to act like win32, but uses osutils path functions. It actually passes on windows, where those functions resolve to the right things.

[Florian Vichot (florian-vichot)]

I've doubled checked every call to osutils.normpath(), it should be good now.

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2011-07-25 07:11:56 +0000
+++ bzrlib/osutils.py	2011-10-05 00:23:23 +0000
@@ -277,13 +277,28 @@
     # copy posixpath.abspath, but use os.getcwdu instead
     if not posixpath.isabs(path):
         path = posixpath.join(getcwd(), path)
-    return posixpath.normpath(path)
+    return _posix_normpath(path)
 
 
 def _posix_realpath(path):
     return posixpath.realpath(path.encode(_fs_enc)).decode(_fs_enc)
 
 
+def _posix_normpath(path):
+    path = posixpath.normpath(path)
+    # Bug 861008: posixpath.normpath() returns a path normalized according to
+    # the POSIX standard, which stipulates (for compatibility reasons) that two
+    # leading slashes must not be simplified to one, and only if there are 3 or
+    # more should they be simplified as one. So we treat the leading 2 slashes
+    # as a special case here by simply removing the first slash, as we consider
+    # that breaking POSIX compatibility for this obscure feature is acceptable.
+    # This is not a paranoid precaution, as we notably get paths like this when
+    # the repo is hosted at the root of the filesystem, i.e. in "/".    
+    if path.startswith('//'):
+        path = path[1:]
+    return path
+
+
 def _win32_fixdrive(path):
     """Force drive letters to be consistent.
 
@@ -377,7 +392,7 @@
 abspath = _posix_abspath
 realpath = _posix_realpath
 pathjoin = os.path.join
-normpath = os.path.normpath
+normpath = _posix_normpath
 getcwd = os.getcwdu
 rename = os.rename
 dirname = os.path.dirname

=== modified file 'bzrlib/tests/stub_sftp.py'
--- bzrlib/tests/stub_sftp.py	2011-01-12 01:01:53 +0000
+++ bzrlib/tests/stub_sftp.py	2011-10-05 00:23:23 +0000
@@ -118,9 +118,9 @@
     else:
         def canonicalize(self, path):
             if os.path.isabs(path):
-                return os.path.normpath(path)
+                return osutils.normpath(path)
             else:
-                return os.path.normpath('/' + os.path.join(self.home, path))
+                return osutils.normpath('/' + os.path.join(self.home, path))
 
     def chattr(self, path, attr):
         try:

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2011-06-16 18:34:26 +0000
+++ bzrlib/tests/test_osutils.py	2011-10-05 00:23:23 +0000
@@ -818,6 +818,16 @@
         self.assertEqual(None, osutils.safe_file_id(None))
 
 
+class TestPosixFuncs(tests.TestCase):
+    """Test that the posix version of normpath returns an appropriate path
+       when used with 2 leading slashes."""
+
+    def test_normpath(self):
+        self.assertEqual('/etc/shadow', osutils._posix_normpath('/etc/shadow'))
+        self.assertEqual('/etc/shadow', osutils._posix_normpath('//etc/shadow'))
+        self.assertEqual('/etc/shadow', osutils._posix_normpath('///etc/shadow'))
+
+
 class TestWin32Funcs(tests.TestCase):
     """Test that _win32 versions of os utilities return appropriate paths."""
 

=== modified file 'bzrlib/transport/local.py'
--- bzrlib/transport/local.py	2011-07-05 06:41:37 +0000
+++ bzrlib/transport/local.py	2011-10-05 00:23:23 +0000
@@ -562,7 +562,7 @@
         self._local_base = urlutils._win32_local_path_from_url(base)
 
     def abspath(self, relpath):
-        path = osutils.normpath(osutils.pathjoin(
+        path = osutils._win32_normpath(osutils.pathjoin(
                     self._local_base, urlutils.unescape(relpath)))
         return urlutils._win32_local_path_to_url(path)
 

=== modified file 'bzrlib/urlutils.py'
--- bzrlib/urlutils.py	2010-10-21 22:27:43 +0000
+++ bzrlib/urlutils.py	2011-10-05 00:23:23 +0000
@@ -22,7 +22,7 @@
 
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
-from posixpath import split as _posix_split, normpath as _posix_normpath
+from posixpath import split as _posix_split
 import urllib
 import urlparse
 
@@ -200,8 +200,7 @@
     """
     # importing directly from posixpath allows us to test this
     # on non-posix platforms
-    return 'file://' + escape(_posix_normpath(
-        osutils._posix_abspath(path)))
+    return 'file://' + escape(osutils._posix_abspath(path))
 
 
 def _win32_local_path_from_url(url):

=== modified file 'bzrlib/version.py'
--- bzrlib/version.py	2010-04-30 11:03:59 +0000
+++ bzrlib/version.py	2011-10-05 00:23:23 +0000
@@ -73,7 +73,7 @@
     else:
         to_file.write(bzrlib.__path__[0] + '\n')
     if show_config:
-        config_dir = os.path.normpath(config.config_dir())  # use native slashes
+        config_dir = osutils.normpath(config.config_dir())  # use native slashes
         if not isinstance(config_dir, unicode):
             config_dir = config_dir.decode(osutils.get_user_encoding())
         to_file.write("  Bazaar configuration: %s\n" % (config_dir,))

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-09-30 16:36:13 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-10-05 00:23:23 +0000
@@ -42,6 +42,11 @@
 * Return early from create_delta_index_from_delta given tiny inputs. This
   avoids raising a spurious MemoryError on certain platforms such as AIX.
   (John Arbash Meinel, #856731)
+  
+* Fixed an infinite loop when creating a repo at the root of the filesystem, 
+  i.e. "/", due to posixpath.normpath() not collapsing 2 leading slashes into 
+  one, thus respecting the POSIX standard, but making relpath() loop infinitely.
+  (Florian Vichot, #861008)
 
 Documentation
 *************