Merge lp:~jameinel/bzr/2.0.1-322807-branch-at-root into lp:bzr/2.0

[John A Meinel]

[John A Meinel (jameinel)]

This fixes bug #322807

Basically, cicp_canonical_relpath always assumed that 'abspath()' returned a path that did not have a trailing '/'. However, when working at the root of the filesytem, this was not true. (And, I believe, not true on Linux or Mac, either.)

So I changed:
  return current[len(abspath+1):]
to
  return current[len(abspath):].lstrip('/')

which seems to work just fine.

In the process of doing this, I also made the function handle when base doesn't actually exist, since it made testing easier. And I changed it to always run tests that I'm sure will pass. (We don't require the filesystem to have CICP to run _cicp_canonical_relpath, only if we wanted to call it by 'osutils.canonical_relpath()'. This should help increase PQM test coverage.)

[Andrew Bennetts (spiv)]

This seems good. I don't have a convenient Windows install to test it on, but I assume you've already taken care of that :)

I have some minor suggestions, so I'm voting Needs Fixing and marking the overall review as Approved (as that seems to be the closest approximation of bb:tweak).

81 + try:
82 + next_entries = _listdir(current)
83 + except OSError: # enoent

That's a bit odd. Why not actually check that the exc.errno == ENOENT?

87 for look in _listdir(current):

This repeats the listdir we just did. Why not reuse 'next_entries'?

129 + # This path probably does not exist
130 + # We can't test this on Windows, because it has a 'MIN_ABS_PATHLENGTH'
131 + # check...
132 + # self.assertRelpath('foo', '/', '/foo')

Ideally this would be in a separate test method that could do a requireFeature check, I think. Not a big deal.

review: Needs Fixing

[John A Meinel (jameinel)]

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

Andrew Bennetts wrote:
> Review: Needs Fixing
> This seems good. I don't have a convenient Windows install to test it on, but I assume you've already taken care of that :)
>
> I have some minor suggestions, so I'm voting Needs Fixing and marking the overall review as Approved (as that seems to be the closest approximation of bb:tweak).
>
> 81 + try:
> 82 + next_entries = _listdir(current)
> 83 + except OSError: # enoent
>
> That's a bit odd. Why not actually check that the exc.errno == ENOENT?

Because I figured EPERM and EANYTHINGELSE would be just as reasonable.
All we are doing here is trying to translate "foo" => "Foo" based on
what is in the filesytem. If we can't access the filesystem, we have
nothing to translate.

>
> 87 for look in _listdir(current):
>
> This repeats the listdir we just did. Why not reuse 'next_entries'?

I'm sure I meant to, and just forgot to.

>
> 129 + # This path probably does not exist
> 130 + # We can't test this on Windows, because it has a 'MIN_ABS_PATHLENGTH'
> 131 + # check...
> 132 + # self.assertRelpath('foo', '/', '/foo')
>
> Ideally this would be in a separate test method that could do a requireFeature check, I think. Not a big deal.

Sure, I could do that.

John
=:->

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

iEYEARECAAYFAkrNX2IACgkQJdeBCYSNAAMQsQCfbgmfEneaDM3KffULtewF0w8w
VUsAnROBTldhwAIp+cEEhJRBfjYg6xS0
=e7ss
-----END PGP SIGNATURE-----

[Andrew Bennetts (spiv)]

John A Meinel wrote:
[...]
> > 83 + except OSError: # enoent
> >
> > That's a bit odd. Why not actually check that the exc.errno == ENOENT?
>
> Because I figured EPERM and EANYTHINGELSE would be just as reasonable.
> All we are doing here is trying to translate "foo" => "Foo" based on
> what is in the filesytem. If we can't access the filesystem, we have
> nothing to translate.

Ah ok. Then I'd just remove the “# enoent” comment, because it seems to
imply that you only care about that one errno.

-Andrew.

=== modified file 'NEWS'
--- NEWS	2009-10-07 12:01:20 +0000
+++ NEWS	2009-10-08 17:16:09 +0000
@@ -2,8 +2,8 @@
 Bazaar Release Notes
 ####################
 
-2.0 series (not released)
-#########################
+2.0.1 (not released)
+####################
 
 Compatibility Breaks
 ********************
@@ -29,6 +29,21 @@
 * Fixed ``ObjectNotLocked`` errors when doing some log and diff operations
   on branches via a smart server.  (Andrew Bennetts, #389413)
 
+* Handle things like ``bzr add foo`` and ``bzr rm foo`` when the tree is
+  at the root of a drive. ``osutils._cicp_canonical_relpath`` always
+  assumed that ``abspath()`` returned a path that did not have a trailing
+  ``/``, but that is not true when working at the root of the filesystem.
+  (John Arbash Meinel, Jason Spashett, #322807)
+
+* Improve the time for ``bzr log DIR`` for 2a format repositories.
+  We had been using the same code path as for <2a formats, which required
+  iterating over all objects in all revisions.
+  (John Arbash Meinel, #374730)
+
+* Make sure that we unlock the tree if we fail to create a TreeTransform
+  object when doing a merge, and there is limbo, or pending-deletions
+  directory.  (Gary van der Merwe, #427773)
+
 * Occasional IndexError on renamed files have been fixed. Operations that
   set a full inventory in the working tree will now go via the
   apply_inventory_delta code path which is simpler and easier to
@@ -37,6 +52,9 @@
   but such operations are already doing full tree scans, so no radical
   performance change should be observed. (Robert Collins, #403322)
 
+* Retrieving file text or mtime from a _PreviewTree has good performance when
+  there are many changes.  (Aaron Bentley)
+
 * When a file kind becomes unversionable after being added, a sensible
   error will be shown instead of a traceback. (Robert Collins, #438569)
 
@@ -60,25 +78,6 @@
 Testing
 *******
 
-bzr 2.0.1
-##########
-
-Bug Fixes
-*********
-
-* Improve the time for ``bzr log DIR`` for 2a format repositories.
-  We had been using the same code path as for <2a formats, which required
-  iterating over all objects in all revisions.
-  (John Arbash Meinel, #374730)
-
-* Make sure that we unlock the tree if we fail to create a TreeTransform
-  object when doing a merge, and there is limbo, or pending-deletions
-  directory.  (Gary van der Merwe, #427773)
-
-* Retrieving file text or mtime from a _PreviewTree has good performance when
-  there are many changes.  (Aaron Bentley)
-
-
 bzr 2.0.0
 #########
 

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2009-07-23 16:01:17 +0000
+++ bzrlib/osutils.py	2009-10-08 17:16:09 +0000
@@ -1083,7 +1083,14 @@
     bit_iter = iter(rel.split('/'))
     for bit in bit_iter:
         lbit = bit.lower()
-        for look in _listdir(current):
+        try:
+            next_entries = _listdir(current)
+        except OSError: # enoent, eperm, etc
+            # We can't find this in the filesystem, so just append the
+            # remaining bits.
+            current = pathjoin(current, bit, *list(bit_iter))
+            break
+        for look in next_entries:
             if lbit == look.lower():
                 current = pathjoin(current, look)
                 break
@@ -1093,7 +1100,7 @@
             # the target of a move, for example).
             current = pathjoin(current, bit, *list(bit_iter))
             break
-    return current[len(abs_base)+1:]
+    return current[len(abs_base):].lstrip('/')
 
 # XXX - TODO - we need better detection/integration of case-insensitive
 # file-systems; Linux often sees FAT32 devices (or NFS-mounted OSX

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2009-07-23 16:01:17 +0000
+++ bzrlib/tests/test_osutils.py	2009-10-08 17:16:09 +0000
@@ -460,6 +460,49 @@
         self.failUnlessEqual('work/MixedCaseParent/nochild', actual)
 
 
+class Test_CICPCanonicalRelpath(tests.TestCaseWithTransport):
+
+    def assertRelpath(self, expected, base, path):
+        actual = osutils._cicp_canonical_relpath(base, path)
+        self.assertEqual(expected, actual)
+
+    def test_simple(self):
+        self.build_tree(['MixedCaseName'])
+        base = osutils.realpath(self.get_transport('.').local_abspath('.'))
+        self.assertRelpath('MixedCaseName', base, 'mixedcAsename')
+
+    def test_subdir_missing_tail(self):
+        self.build_tree(['MixedCaseParent/', 'MixedCaseParent/a_child'])
+        base = osutils.realpath(self.get_transport('.').local_abspath('.'))
+        self.assertRelpath('MixedCaseParent/a_child', base,
+                           'MixedCaseParent/a_child')
+        self.assertRelpath('MixedCaseParent/a_child', base,
+                           'MixedCaseParent/A_Child')
+        self.assertRelpath('MixedCaseParent/not_child', base,
+                           'MixedCaseParent/not_child')
+
+    def test_at_root_slash(self):
+        # We can't test this on Windows, because it has a 'MIN_ABS_PATHLENGTH'
+        # check...
+        if osutils.MIN_ABS_PATHLENGTH > 1:
+            raise tests.TestSkipped('relpath requires %d chars'
+                                    % osutils.MIN_ABS_PATHLENGTH)
+        self.assertRelpath('foo', '/', '/foo')
+
+    def test_at_root_drive(self):
+        if sys.platform != 'win32':
+            raise tests.TestNotApplicable('we can only test drive-letter relative'
+                                          ' paths on Windows where we have drive'
+                                          ' letters.')
+        # see bug #322807
+        # The specific issue is that when at the root of a drive, 'abspath'
+        # returns "C:/" or just "/". However, the code assumes that abspath
+        # always returns something like "C:/foo" or "/foo" (no trailing slash).
+        self.assertRelpath('foo', 'C:/', 'C:/foo')
+        self.assertRelpath('foo', 'X:/', 'X:/foo')
+        self.assertRelpath('foo', 'X:/', 'X://foo')
+
+
 class TestPumpFile(tests.TestCase):
     """Test pumpfile method."""