Merge lp:~jelmer/bzr/path2id-list into lp:bzr

[Jelmer Vernooij]

Reintroduce support for passing lists to Tree.path2id().

Allow passing in a list of path elements to Tree.path2id.

This was supported in the past, because Inventory.path2id allowed it,
and is still quite useful.

Also, add some more tests for Tree.path2id and Tree.id2path. Previously
these weren't directly tested (though they usually just called out to
Inventory.{path2id,id2path}, which are).

[Jelmer Vernooij (jelmer)]

There is some repetition here. Perhaps I should factor out:

12 + if relpath == []:
13 + relpath = [""]
14 + relpath = osutils.pathjoin(*relpath)

... into another helper method?

[Martin Packman (gz)]

Appears mostly harmless.

+ if isinstance(relpath, basestring):
+ names = osutils.splitpath(relpath)
+ else:

Think I'd prefer if this check was round the other way like the other branches, use `isinstance(relpath, list)` as the special case and assume string otherwise.

+ if path == []:
+ path = [""]
+ path = osutils.pathjoin(*path)

This makes me want to indulge and write `path and osutils.pathjoin(*path) or ""` but it's better not to.

The factoring out of a lot of this path stuff into a Path object is still something I'd like, then this methods could just use a from_string_or_list constructor or similar.

review: Approve

=== modified file 'bzrlib/inventory.py'
--- bzrlib/inventory.py	2012-02-17 16:37:15 +0000
+++ bzrlib/inventory.py	2012-03-05 17:33:21 +0000
@@ -2111,13 +2111,16 @@
     def path2id(self, relpath):
         """See CommonInventory.path2id()."""
         # TODO: perhaps support negative hits?
+        if isinstance(relpath, basestring):
+            names = osutils.splitpath(relpath)
+        else:
+            names = relpath
+            if relpath == []:
+                relpath = [""]
+            relpath = osutils.pathjoin(*relpath)
         result = self._path_to_fileid_cache.get(relpath, None)
         if result is not None:
             return result
-        if isinstance(relpath, basestring):
-            names = osutils.splitpath(relpath)
-        else:
-            names = relpath
         current_id = self.root_id
         if current_id is None:
             return None

=== modified file 'bzrlib/tests/per_tree/__init__.py'
--- bzrlib/tests/per_tree/__init__.py	2011-06-14 01:26:41 +0000
+++ bzrlib/tests/per_tree/__init__.py	2012-03-05 17:33:21 +0000
@@ -382,6 +382,7 @@
         'get_file_with_stat',
         'get_root_id',
         'get_symlink_target',
+        'ids',
         'inv',
         'iter_search_rules',
         'is_executable',

=== added file 'bzrlib/tests/per_tree/test_ids.py'
--- bzrlib/tests/per_tree/test_ids.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/per_tree/test_ids.py	2012-03-05 17:33:21 +0000
@@ -0,0 +1,51 @@
+# Copyright (C) 2012 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
+
+from bzrlib import (
+    errors,
+    )
+from bzrlib.tests.per_tree import TestCaseWithTree
+
+class IdTests(TestCaseWithTree):
+
+    def setUp(self):
+        super(IdTests, self).setUp()
+        work_a = self.make_branch_and_tree('wta')
+        self.build_tree(['wta/bla', 'wta/dir/', 'wta/dir/file'])
+        work_a.add(['bla', 'dir', 'dir/file'], ['bla-id', 'dir-id', 'file-id'])
+        work_a.commit('add files')
+        self.tree_a = self.workingtree_to_test_tree(work_a)
+
+    def test_path2id(self):
+        self.assertEquals('bla-id', self.tree_a.path2id('bla'))
+        self.assertEquals('dir-id', self.tree_a.path2id('dir'))
+        self.assertIs(None, self.tree_a.path2id('idontexist'))
+
+    def test_path2id_list(self):
+        self.assertEquals('bla-id', self.tree_a.path2id(['bla']))
+        self.assertEquals('dir-id', self.tree_a.path2id(['dir']))
+        self.assertEquals('file-id', self.tree_a.path2id(['dir', 'file']))
+        self.assertEquals(self.tree_a.get_root_id(),
+            self.tree_a.path2id([]))
+        self.assertIs(None, self.tree_a.path2id(['idontexist']))
+        self.assertIs(None, self.tree_a.path2id(['dir', 'idontexist']))
+
+    def test_id2path(self):
+        self.addCleanup(self.tree_a.lock_read().unlock)
+        self.assertEquals('bla', self.tree_a.id2path('bla-id'))
+        self.assertEquals('dir', self.tree_a.id2path('dir-id'))
+        self.assertEquals('dir/file', self.tree_a.id2path('file-id'))
+        self.assertRaises(errors.NoSuchId, self.tree_a.id2path, 'nonexistant')

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2012-02-06 23:38:33 +0000
+++ bzrlib/transform.py	2012-03-05 17:33:21 +0000
@@ -2117,6 +2117,10 @@
         return cur_parent
 
     def path2id(self, path):
+        if isinstance(path, list):
+            if path == []:
+                path = [""]
+            path = osutils.pathjoin(*path)
         return self._transform.final_file_id(self._path2trans_id(path))
 
     def id2path(self, file_id):

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2012-02-06 23:38:33 +0000
+++ bzrlib/workingtree_4.py	2012-03-05 17:33:21 +0000
@@ -893,6 +893,10 @@
     @needs_read_lock
     def path2id(self, path):
         """Return the id for path in this tree."""
+        if isinstance(path, list):
+            if path == []:
+                path = [""]
+            path = osutils.pathjoin(*path)
         path = path.strip('/')
         entry = self._get_entry(path=path)
         if entry == (None, None):
@@ -1776,7 +1780,8 @@
         if path is not None:
             path = path.encode('utf8')
         parent_index = self._get_parent_index()
-        return self._dirstate._get_entry(parent_index, fileid_utf8=file_id, path_utf8=path)
+        return self._dirstate._get_entry(parent_index, fileid_utf8=file_id,
+            path_utf8=path)
 
     def _generate_inventory(self):
         """Create and set self.inventory from the dirstate object.
@@ -2028,6 +2033,10 @@
     def path2id(self, path):
         """Return the id for path in this tree."""
         # lookup by path: faster than splitting and walking the ivnentory.
+        if isinstance(path, list):
+            if path == []:
+                path = [""]
+            path = osutils.pathjoin(*path)
         entry = self._get_entry(path=path)
         if entry == (None, None):
             return None

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-02-25 14:13:19 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-03-05 17:33:21 +0000
@@ -77,6 +77,9 @@
 .. Major internal changes, unlikely to be visible to users or plugin 
    developers, but interesting for bzr developers.
 
+* ``Tree.path2id`` now once again accepts a list of path elements
+  in addition to a path. (Jelmer Vernooij)
+
 Testing
 *******
 
@@ -84,6 +87,5 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
-
 ..
    vim: tw=74 ft=rst ff=unix