Merge lp:~jelmer/brz/brz-git-renames into lp:brz

[Jelmer Vernooij]

Some refactoring around get_canonical_inventory_paths.

* Drop the 'inventory' bit since it doesn't apply to non-inventory trees
* Move to working trees; canonicalization currently only matters there, where
  files can be stored on case-insensitive (VFAT) or normalizing filesystems
  (i.e. Mac OS X)
* Run tests against all working tree formats, including Git, not just InventoryTree ones

This fixes 'bzr mv' and 'bzr rename' for Git.

[Martin Packman (gz)]

Okay, moves seem reasonable to me. The logic is not all correct, but we'll need to do another pass anyway to deal with filesystem mapping issues.

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/land-brz/477/

=== modified file 'breezy/builtins.py'
--- breezy/builtins.py	2018-08-12 23:18:19 +0000
+++ breezy/builtins.py	2018-09-14 14:37:48 +0000
@@ -1043,7 +1043,7 @@
                 into_existing = False
             else:
                 # 'fix' the case of a potential 'from'
-                from_path = tree.get_canonical_inventory_path(rel_names[0])
+                from_path = tree.get_canonical_path(rel_names[0])
                 if (not osutils.lexists(names_list[0]) and
                     tree.is_versioned(from_path) and
                     tree.stored_kind(from_path) == "directory"):
@@ -1053,7 +1053,7 @@
             # move into existing directory
             # All entries reference existing inventory items, so fix them up
             # for cicp file-systems.
-            rel_names = tree.get_canonical_inventory_paths(rel_names)
+            rel_names = list(tree.get_canonical_paths(rel_names))
             for src, dest in tree.move(rel_names[:-1], rel_names[-1], after=after):
                 if not is_quiet():
                     self.outf.write("%s => %s\n" % (src, dest))
@@ -1065,13 +1065,13 @@
 
             # for cicp file-systems: the src references an existing inventory
             # item:
-            src = tree.get_canonical_inventory_path(rel_names[0])
+            src = tree.get_canonical_path(rel_names[0])
             # Find the canonical version of the destination:  In all cases, the
             # parent of the target must be in the inventory, so we fetch the
             # canonical version from there (we do not always *use* the
             # canonicalized tail portion - we may be attempting to rename the
             # case of the tail)
-            canon_dest = tree.get_canonical_inventory_path(rel_names[1])
+            canon_dest = tree.get_canonical_path(rel_names[1])
             dest_parent = osutils.dirname(canon_dest)
             spec_tail = osutils.basename(rel_names[1])
             # For a CICP file-system, we need to avoid creating 2 inventory
 
=== modified file 'breezy/bzr/inventorytree.py'
--- breezy/bzr/inventorytree.py	2018-07-13 00:25:33 +0000
+++ breezy/bzr/inventorytree.py	2018-09-14 14:37:48 +0000
@@ -69,89 +69,6 @@
     private to external API users.
     """
 
-    def get_canonical_inventory_paths(self, paths):
-        """Like get_canonical_inventory_path() but works on multiple items.
-
-        :param paths: A sequence of paths relative to the root of the tree.
-        :return: A list of paths, with each item the corresponding input path
-        adjusted to account for existing elements that match case
-        insensitively.
-        """
-        with self.lock_read():
-            return list(self._yield_canonical_inventory_paths(paths))
-
-    def get_canonical_inventory_path(self, path):
-        """Returns the first inventory item that case-insensitively matches path.
-
-        If a path matches exactly, it is returned. If no path matches exactly
-        but more than one path matches case-insensitively, it is implementation
-        defined which is returned.
-
-        If no path matches case-insensitively, the input path is returned, but
-        with as many path entries that do exist changed to their canonical
-        form.
-
-        If you need to resolve many names from the same tree, you should
-        use get_canonical_inventory_paths() to avoid O(N) behaviour.
-
-        :param path: A paths relative to the root of the tree.
-        :return: The input path adjusted to account for existing elements
-        that match case insensitively.
-        """
-        with self.lock_read():
-            return next(self._yield_canonical_inventory_paths([path]))
-
-    def _yield_canonical_inventory_paths(self, paths):
-        for path in paths:
-            # First, if the path as specified exists exactly, just use it.
-            if self.path2id(path) is not None:
-                yield path
-                continue
-            # go walkin...
-            cur_id = self.get_root_id()
-            cur_path = ''
-            bit_iter = iter(path.split("/"))
-            for elt in bit_iter:
-                lelt = elt.lower()
-                new_path = None
-                try:
-                    for child in self.iter_child_entries(self.id2path(cur_id), 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.file_id))
-                            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.file_id
-                                new_path = osutils.pathjoin(cur_path, child_base)
-                                break
-                            elif child_base.lower() == lelt:
-                                cur_id = child.file_id
-                                new_path = osutils.pathjoin(cur_path, child_base)
-                        except errors.NoSuchId:
-                            # before a change is committed we can see this error...
-                            continue
-                except errors.NotADirectory:
-                    pass
-                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.
-                    cur_path = osutils.pathjoin(cur_path, elt, *list(bit_iter))
-                    break
-            yield cur_path
-        # all done.
-
     def _get_root_inventory(self):
         return self._inventory
 
@@ -451,7 +368,7 @@
     def _fix_case_of_inventory_path(self, path):
         """If our tree isn't case sensitive, return the canonical path"""
         if not self.case_sensitive:
-            path = self.get_canonical_inventory_path(path)
+            path = self.get_canonical_path(path)
         return path
 
     def smart_add(self, file_list, recurse=True, action=None, save=True):
 
=== modified file 'breezy/bzr/workingtree.py'
--- breezy/bzr/workingtree.py	2018-08-26 01:29:46 +0000
+++ breezy/bzr/workingtree.py	2018-09-14 14:37:48 +0000
@@ -78,6 +78,7 @@
     )
 from ..trace import mutter, note
 from ..tree import (
+    get_canonical_path,
     FileTimestampUnavailable,
     TreeDirectory,
     TreeFile,
@@ -1710,6 +1711,28 @@
         else:
             return self._check_for_tree_references(iterator)
 
+    def get_canonical_paths(self, paths):
+        """Look up canonical paths for multiple items.
+
+        :param paths: A sequence of paths relative to the root of the tree.
+        :return: A iterator over paths, with each item the corresponding input path
+            adjusted to account for existing elements that match case
+            insensitively.
+        """
+        with self.lock_read():
+            if not self.case_sensitive:
+                normalize = lambda x: x.lower()
+            elif sys.platform == 'darwin':
+                import unicodedata
+                normalize = lambda x: unicodedata.normalize('NFC', x)
+            else:
+                normalize = None
+            for path in paths:
+                if normalize is None or self.is_versioned(path):
+                    yield path
+                else:
+                    yield get_canonical_path(self, path, normalize)
+
 
 class WorkingTreeFormatMetaDir(bzrdir.BzrFormat, WorkingTreeFormat):
     """Base class for working trees that live in bzr meta directories."""
 
=== modified file 'breezy/tests/per_tree/test_inv.py'
--- breezy/tests/per_tree/test_inv.py	2018-07-03 02:40:57 +0000
+++ breezy/tests/per_tree/test_inv.py	2018-09-14 14:37:48 +0000
@@ -27,7 +27,6 @@
 from breezy.mutabletree import MutableTree
 from breezy.tests import TestSkipped
 from breezy.transform import _PreviewTree
-from breezy.uncommit import uncommit
 from breezy.tests import (
     features,
     )
@@ -98,84 +97,3 @@
         self.addCleanup(tree.unlock)
         self.assertEqual(set([]), tree.paths2ids(['file'],
                          require_versioned=False))
-
-    def _make_canonical_test_tree(self, commit=True):
-        # make a tree used by all the 'canonical' tests below.
-        work_tree = self.make_branch_and_tree('tree')
-        self.build_tree(['tree/dir/', 'tree/dir/file'])
-        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):
-        work_tree = self._make_canonical_test_tree()
-        if not isinstance(work_tree, InventoryTree):
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        self.assertEqual('dir/file',
-                         work_tree.get_canonical_inventory_path('Dir/File'))
-
-    def test_canonical_path_before_commit(self):
-        work_tree = self._make_canonical_test_tree(False)
-        if not isinstance(work_tree, InventoryTree):
-            # note: not committed.
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        self.assertEqual('dir/file',
-                         work_tree.get_canonical_inventory_path('Dir/File'))
-
-    def test_canonical_path_dir(self):
-        # check it works when asked for just the directory portion.
-        work_tree = self._make_canonical_test_tree()
-        if not isinstance(work_tree, InventoryTree):
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        self.assertEqual('dir', work_tree.get_canonical_inventory_path('Dir'))
-
-    def test_canonical_path_root(self):
-        work_tree = self._make_canonical_test_tree()
-        if not isinstance(work_tree, InventoryTree):
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        self.assertEqual('', work_tree.get_canonical_inventory_path(''))
-        self.assertEqual('/', work_tree.get_canonical_inventory_path('/'))
-
-    def test_canonical_path_invalid_all(self):
-        work_tree = self._make_canonical_test_tree()
-        if not isinstance(work_tree, InventoryTree):
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        self.assertEqual('foo/bar',
-                         work_tree.get_canonical_inventory_path('foo/bar'))
-
-    def test_canonical_invalid_child(self):
-        work_tree = self._make_canonical_test_tree()
-        if not isinstance(work_tree, InventoryTree):
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        self.assertEqual('dir/None',
-                         work_tree.get_canonical_inventory_path('Dir/None'))
-
-    def test_canonical_tree_name_mismatch(self):
-        # see <https://bugs.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(features.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)
-        if not isinstance(test_tree, InventoryTree):
-            raise tests.TestNotApplicable(
-                "test not applicable on non-inventory tests")
-        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 'breezy/tests/per_workingtree/__init__.py'
--- breezy/tests/per_workingtree/__init__.py	2017-09-10 20:57:03 +0000
+++ breezy/tests/per_workingtree/__init__.py	2018-09-14 14:37:48 +0000
@@ -123,6 +123,7 @@
         'basis_inventory',
         'basis_tree',
         'break_lock',
+        'canonical_path',
         'changes_from',
         'check',
         'check_state',
 
=== added file 'breezy/tests/per_workingtree/test_canonical_path.py'
--- breezy/tests/per_workingtree/test_canonical_path.py	1970-01-01 00:00:00 +0000
+++ breezy/tests/per_workingtree/test_canonical_path.py	2018-09-14 14:37:48 +0000
@@ -0,0 +1,109 @@
+# Copyright (C) 2007-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
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Tests for interface conformance of canonical paths of trees."""
+
+
+from breezy import (
+    tests,
+    )
+from breezy.tests.per_workingtree import (
+    TestCaseWithWorkingTree,
+    )
+from breezy.tests import (
+    features,
+    )
+
+
+class TestCanonicalPaths(TestCaseWithWorkingTree):
+
+    def _make_canonical_test_tree(self, commit=True):
+        # make a tree used by all the 'canonical' tests below.
+        work_tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/dir/', 'tree/dir/file'])
+        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):
+        work_tree = self._make_canonical_test_tree()
+        if features.CaseInsensitiveFilesystemFeature.available():
+            self.assertEqual('dir/file',
+                             work_tree.get_canonical_path('Dir/File'))
+        else:
+            self.assertEqual('Dir/File',
+                             work_tree.get_canonical_path('Dir/File'))
+
+    def test_canonical_path_before_commit(self):
+        work_tree = self._make_canonical_test_tree(False)
+        if features.CaseInsensitiveFilesystemFeature.available():
+            self.assertEqual('dir/file',
+                             work_tree.get_canonical_path('Dir/File'))
+        else:
+            self.assertEqual('Dir/File',
+                             work_tree.get_canonical_path('Dir/File'))
+
+    def test_canonical_path_dir(self):
+        # check it works when asked for just the directory portion.
+        work_tree = self._make_canonical_test_tree()
+        if features.CaseInsensitiveFilesystemFeature.available():
+            self.assertEqual('dir', work_tree.get_canonical_path('Dir'))
+        else:
+            self.assertEqual('Dir', work_tree.get_canonical_path('Dir'))
+
+    def test_canonical_path_root(self):
+        work_tree = self._make_canonical_test_tree()
+        self.assertEqual('', work_tree.get_canonical_path(''))
+        self.assertEqual('/', work_tree.get_canonical_path('/'))
+
+    def test_canonical_path_invalid_all(self):
+        work_tree = self._make_canonical_test_tree()
+        self.assertEqual('foo/bar',
+                         work_tree.get_canonical_path('foo/bar'))
+
+    def test_canonical_invalid_child(self):
+        work_tree = self._make_canonical_test_tree()
+        if features.CaseInsensitiveFilesystemFeature.available():
+            self.assertEqual('dir/None',
+                             work_tree.get_canonical_path('Dir/None'))
+        else:
+            self.assertEqual('Dir/None',
+                             work_tree.get_canonical_path('Dir/None'))
+
+    def test_canonical_tree_name_mismatch(self):
+        # see <https://bugs.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(features.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'])
+
+        self.assertEqual(['test', 'Test', 'test/file', 'Test/file'],
+                         list(work_tree.get_canonical_paths(
+                             ['test', 'Test', 'test/file', 'Test/file'])))
+
+        test_revid = work_tree.commit('commit')
+        test_tree = work_tree.branch.repository.revision_tree(test_revid)
+        test_tree.lock_read()
+        self.addCleanup(test_tree.unlock)
+
+        self.assertEqual(['', 'Test', 'test', 'test/file'],
+            [p for p, e in test_tree.iter_entries_by_dir()])
 
=== modified file 'breezy/tree.py'
--- breezy/tree.py	2018-07-26 22:12:29 +0000
+++ breezy/tree.py	2018-09-14 14:37:48 +0000
@@ -1364,3 +1364,47 @@
         return to_tree.id2path(file_id)
     except errors.NoSuchId:
         return None
+
+
+def get_canonical_path(tree, path, normalize):
+    """Find the canonical path of an item, ignoring case.
+
+    :param tree: Tree to traverse
+    :param path: Case-insensitive path to look up
+    :param normalize: Function to normalize a filename for comparison
+    :return: The canonical path
+    """
+    # go walkin...
+    cur_id = self.get_root_id()
+    cur_path = ''
+    bit_iter = iter(path.split("/"))
+    for elt in bit_iter:
+        lelt = normalize(elt)
+        new_path = None
+        try:
+            for child in self.iter_child_entries(cur_path, cur_id):
+                try:
+                    if child.name == 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.file_id
+                        new_path = osutils.pathjoin(cur_path, child.name)
+                        break
+                    elif normalize(child.name) == lelt:
+                        cur_id = child.file_id
+                        new_path = osutils.pathjoin(cur_path, child.name)
+                except errors.NoSuchId:
+                    # before a change is committed we can see this error...
+                    continue
+        except errors.NotADirectory:
+            pass
+        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.
+            cur_path = osutils.pathjoin(cur_path, elt, *list(bit_iter))
+            break
+    return cur_path
 
=== modified file 'breezy/workingtree.py'
--- breezy/workingtree.py	2018-08-07 19:23:46 +0000
+++ breezy/workingtree.py	2018-09-14 14:37:48 +0000
@@ -1337,6 +1337,42 @@
         """Return the ShelfManager for this WorkingTree."""
         raise NotImplementedError(self.get_shelf_manager)
 
+    def get_canonical_paths(self, paths):
+        """Like get_canonical_path() but works on multiple items.
+
+        :param paths: A sequence of paths relative to the root of the tree.
+        :return: A list of paths, with each item the corresponding input path
+            adjusted to account for existing elements that match case
+            insensitively.
+        """
+        with self.lock_read():
+            for path in paths:
+                yield path
+
+    def get_canonical_path(self, path):
+        """Returns the first item in the tree that matches a path.
+
+        This is meant to allow case-insensitive path lookups on e.g.
+        FAT filesystems.
+
+        If a path matches exactly, it is returned. If no path matches exactly
+        but more than one path matches according to the underlying file system,
+        it is implementation defined which is returned.
+
+        If no path matches according to the file system, the input path is
+        returned, but with as many path entries that do exist changed to their
+        canonical form.
+
+        If you need to resolve many names from the same tree, you should
+        use get_canonical_paths() to avoid O(N) behaviour.
+
+        :param path: A paths relative to the root of the tree.
+        :return: The input path adjusted to account for existing elements
+        that match case insensitively.
+        """
+        with self.lock_read():
+            return next(self.get_canonical_paths([path]))
+
 
 class WorkingTreeFormatRegistry(controldir.ControlComponentFormatRegistry):
     """Registry for working tree formats."""