Merge lp:~mbp/bzr/368931-rename-case-2.0 into lp:bzr/2.0

[Martin Pool]

[Martin Pool (mbp)]

This finishes off Chris Jones's patch in https://bugs.edge.launchpad.net/bzr/+bug/368931 by adding a test.

This function could be faster but I'm not sure if it's actually a hot spot.

This finishes our last high-priority bug with a patch! There are still 34 more with lower importance.

[John A Meinel (jameinel)]

32 + self.build_tree(['test/', 'test/file', 'Test'])

This is not valid on Windows.

review: Needs Fixing

[John A Meinel (jameinel)]

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/368931-rename-case-2.0 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #368931 Rename may fail when file and directory have the same name differing by case
> https://bugs.launchpad.net/bugs/368931
>
>
> This finishes off Chris Jones's patch in https://bugs.edge.launchpad.net/bzr/+bug/368931 by adding a test.
>
> This function could be faster but I'm not sure if it's actually a hot spot.
>
> This finishes our last high-priority bug with a patch! There are still 34 more with lower importance.
>

I agree with your TODO and NOTE comments.

Also using "iter_children()" throws out all sorts of information. It
returns only file_ids, when it already looked up everything as a
InventoryEntry. Also, falling back to 'id2path' is wasteful given that
we already have the containing directory, and all we need is the child
name. (Which is on the IE we just through away from iter_children.)

I would also guess that there is still a bug if "Tree" is versioned but
"tree" is not (or vice versa).

It sort of depends if this is only supposed to work on versioned paths,
or work on both versioned and unversioned files.

(If tree is versioned but Tree is not, and you do "bzr status Tree",
what should it match?)

John
=:->

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

iEYEARECAAYFAkt0iSoACgkQJdeBCYSNAANrWgCgxtR1cfkQPV0VEEKqD6P186ad
uxgAn1rpE4/PtDIKhNPxC4joAiVBjTW2
=PvyE
-----END PGP SIGNATURE-----

[lp:~mbp/bzr/368931-rename-case-2.0]

4732. By Martin Pool on 2010-02-12

doc

4733. By Martin Pool on 2010-02-12

Add case_sensitive_filesystem_feature

4734. By Martin Pool on 2010-02-12

test_canonical_tree_name_mismatch needs a case-sensitive filesystem

[Martin Pool (mbp)]

This makes it conditional on having a case-insensitive filesystem.

We can't get away from that because some of the tree classes we want to test must exist on disk.

I tested under winepython and it works.

[lp:~mbp/bzr/368931-rename-case-2.0]

4735. By Martin Pool on 2010-02-12

feature moved back to bzrlib.tests with its friends

4736. By Martin Pool on 2010-02-12

Actually test the right tree class in per_tree

4737. By Martin Pool on 2010-02-12

cleanup unnecessary import

=== modified file 'NEWS'
--- NEWS	2010-02-05 06:30:42 +0000
+++ NEWS	2010-02-12 03:41:12 +0000
@@ -14,6 +14,9 @@
 Bug Fixes
 *********
 
+* Handle renames correctly when there are files or directories that 
+  differ only in case.  (Chris Jones, Martin Pool, #368931)
+
 * If ``bzr push --create-prefix`` triggers an unexpected ``NoSuchFile``
   error, report that error rather than failing with an unhelpful
   ``UnboundLocalError``.

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-10-29 04:19:13 +0000
+++ bzrlib/tests/__init__.py	2010-02-12 03:41:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 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
@@ -4060,6 +4060,23 @@
 CaseInsensitiveFilesystemFeature = _CaseInsensitiveFilesystemFeature()
 
 
+class _CaseSensitiveFilesystemFeature(Feature):
+
+    def _probe(self):
+        if CaseInsCasePresFilenameFeature.available():
+            return False
+        elif CaseInsensitiveFilesystemFeature.available():
+            return False
+        else:
+            return True
+
+    def feature_name(self):
+        return 'case-sensitive filesystem'
+
+# new coding style is for feature instances to be lowercase
+case_sensitive_filesystem_feature = _CaseSensitiveFilesystemFeature()
+
+
 class _SubUnitFeature(Feature):
     """Check if subunit is available."""
 

=== modified file 'bzrlib/tests/per_tree/test_inv.py'
--- bzrlib/tests/per_tree/test_inv.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_tree/test_inv.py	2010-02-12 03:41:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007, 2008 Canonical Ltd
+# Copyright (C) 2007, 2008, 2010 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
@@ -23,7 +23,10 @@
 from bzrlib import (
     tests,
     )
-from bzrlib.tests import per_tree
+from bzrlib.tests import (
+    features,
+    per_tree,
+    )
 from bzrlib.mutabletree import MutableTree
 from bzrlib.tests import SymlinkFeature, TestSkipped
 from bzrlib.transform import _PreviewTree
@@ -131,6 +134,8 @@
         work_tree.add(['dir', 'dir/file'])
         if commit:
             work_tree.commit('commit 1')
+        # XXX: this isn't actually guaranteed to return the class we want to
+        # test -- mbp 2010-02-12
         return work_tree
 
     def test_canonical_path(self):
@@ -163,3 +168,21 @@
         work_tree = self._make_canonical_test_tree()
         self.assertEqual('dir/None',
                          work_tree.get_canonical_inventory_path('Dir/None'))
+
+    def test_canonical_tree_name_mismatch(self):
+        # see <https://bugs.edge.launchpad.net/bzr/+bug/368931>
+        # some of the trees we want to use can only exist on a disk, not in
+        # memory - therefore we can only test this if the filesystem is
+        # case-sensitive.
+        self.requireFeature(tests.case_sensitive_filesystem_feature)
+        work_tree = self.make_branch_and_tree('.')
+        self.build_tree(['test/', 'test/file', 'Test'])
+        work_tree.add(['test/', 'test/file', 'Test'])
+
+        test_tree = self._convert_tree(work_tree)
+        test_tree.lock_read()
+        self.addCleanup(test_tree.unlock)
+
+        self.assertEqual(['test', 'test/file', 'Test', 'test/foo', 'Test/foo'],
+            test_tree.get_canonical_inventory_paths(
+                ['test', 'test/file', 'Test', 'test/foo', 'Test/foo']))

=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py	2009-08-26 05:38:16 +0000
+++ bzrlib/tree.py	2010-02-12 03:41:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2009 Canonical Ltd
+# Copyright (C) 2005, 2009, 2010 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
@@ -404,16 +404,34 @@
             bit_iter = iter(path.split("/"))
             for elt in bit_iter:
                 lelt = elt.lower()
+                new_path = None
                 for child in self.iter_children(cur_id):
                     try:
+                        # XXX: it seem like if the child is known to be in the
+                        # tree, we shouldn't need to go from its id back to
+                        # its path -- mbp 2010-02-11
+                        #
+                        # XXX: it seems like we could be more efficient
+                        # by just directly looking up the original name and
+                        # only then searching all children; also by not
+                        # chopping paths so much. -- mbp 2010-02-11
                         child_base = os.path.basename(self.id2path(child))
-                        if child_base.lower() == lelt:
+                        if (child_base == elt):
+                            # if we found an exact match, we can stop now; if
+                            # we found an approximate match we need to keep
+                            # searching because there might be an exact match
+                            # later.  
                             cur_id = child
-                            cur_path = osutils.pathjoin(cur_path, child_base)
+                            new_path = osutils.pathjoin(cur_path, child_base)
                             break
+                        elif child_base.lower() == lelt:
+                            cur_id = child
+                            new_path = osutils.pathjoin(cur_path, child_base)
                     except NoSuchId:
                         # before a change is committed we can see this error...
                         continue
+                if new_path:
+                    cur_path = new_path
                 else:
                     # got to the end of this directory and no entries matched.
                     # Return what matched so far, plus the rest as specified.