Merge lp:~jelmer/brz/reference-info-file-id into lp:brz

[Jelmer Vernooij]

Store bzr reference information by file id rather than by path.

Store bzr reference information by file id rathe rthan by path.

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

Voting criteria not met
https://ci.breezy-vcs.org/job/brz/job/brz-land/619/

[Jelmer Vernooij (jelmer)]

review: Approve

=== modified file 'breezy/bzr/branch.py'
--- breezy/bzr/branch.py	2020-01-19 01:29:22 +0000
+++ breezy/bzr/branch.py	2020-01-19 03:23:48 +0000
@@ -450,28 +450,26 @@
             reconciler = BranchReconciler(self, thorough=thorough)
             return reconciler.reconcile()
 
-    def set_reference_info(self, tree_path, branch_location, file_id=None):
+    def set_reference_info(self, file_id, branch_location, path=None):
         """Set the branch location to use for a tree reference."""
         raise errors.UnsupportedOperation(self.set_reference_info, self)
 
-    def get_reference_info(self, path):
+    def get_reference_info(self, file_id, path=None):
         """Get the tree_path and branch_location for a tree reference."""
         raise errors.UnsupportedOperation(self.get_reference_info, self)
 
-    def reference_parent(self, path, possible_transports=None):
+    def reference_parent(self, file_id, path, possible_transports=None):
         """Return the parent branch for a tree-reference.
 
         :param path: The path of the nested tree in the tree
         :return: A branch associated with the nested tree
         """
-        branch_location = self.get_reference_info(path)[0]
-        if branch_location is None:
-            try:
-                return Branch.open_from_transport(
-                    self.controldir.root_transport.clone(path),
-                    possible_transports=possible_transports)
-            except errors.NotBranchError:
-                return None
+        try:
+            return Branch.open_from_transport(
+                self.controldir.root_transport.clone(path),
+                possible_transports=possible_transports)
+        except errors.NotBranchError:
+            return None
 
 
 class BzrBranch8(BzrBranch):
@@ -541,15 +539,15 @@
     def _set_all_reference_info(self, info_dict):
         """Replace all reference info stored in a branch.
 
-        :param info_dict: A dict of {file_id: (tree_path, branch_location)}
+        :param info_dict: A dict of {file_id: (branch_location, tree_path)}
         """
         s = BytesIO()
         writer = rio.RioWriter(s)
-        for tree_path, (branch_location, file_id) in viewitems(info_dict):
-            stanza = rio.Stanza(tree_path=tree_path,
+        for file_id, (branch_location, tree_path) in viewitems(info_dict):
+            stanza = rio.Stanza(file_id=file_id,
                                 branch_location=branch_location)
-            if file_id is not None:
-                stanza.add('file_id', file_id)
+            if tree_path is not None:
+                stanza.add('tree_path', tree_path)
             writer.write_stanza(stanza)
         with self.lock_write():
             self._transport.put_bytes('references', s.getvalue())
@@ -567,44 +565,43 @@
                 with self._transport.get('references') as rio_file:
                     stanzas = rio.read_stanzas(rio_file)
                     info_dict = {
-                        s['tree_path']: (
+                        s['file_id'].encode('ascii'): (
                             s['branch_location'],
-                            s['file_id'].encode('ascii')
-                            if 'file_id' in s else None)
+                            s['tree_path'] if 'tree_path' in s else None)
                         for s in stanzas}
             except errors.NoSuchFile:
                 info_dict = {}
             self._reference_info = info_dict
             return info_dict
 
-    def set_reference_info(self, tree_path, branch_location, file_id=None):
+    def set_reference_info(self, file_id, branch_location, tree_path=None):
         """Set the branch location to use for a tree reference.
 
-        :param tree_path: The path of the tree reference in the tree.
         :param branch_location: The location of the branch to retrieve tree
             references from.
         :param file_id: The file-id of the tree reference.
+        :param tree_path: The path of the tree reference in the tree.
         """
         info_dict = self._get_all_reference_info()
-        info_dict[tree_path] = (branch_location, file_id)
+        info_dict[file_id] = (branch_location, tree_path)
         if branch_location is None:
-            del info_dict[tree_path]
+            del info_dict[file_id]
         self._set_all_reference_info(info_dict)
 
-    def get_reference_info(self, path):
+    def get_reference_info(self, file_id):
         """Get the tree_path and branch_location for a tree reference.
 
-        :return: a tuple of (branch_location, file_id)
+        :return: a tuple of (branch_location, tree_path)
         """
-        return self._get_all_reference_info().get(path, (None, None))
+        return self._get_all_reference_info().get(file_id, (None, None))
 
-    def reference_parent(self, path, possible_transports=None):
+    def reference_parent(self, file_id, path, possible_transports=None):
         """Return the parent branch for a tree-reference.
 
         :param path: The path of the nested tree in the tree
         :return: A branch associated with the nested tree
         """
-        branch_location = self.get_reference_info(path)[0]
+        branch_location = self.get_reference_info(file_id)[0]
         if branch_location is None:
             try:
                 return Branch.open_from_transport(
@@ -716,9 +713,9 @@
 class BzrBranch7(BzrBranch8):
     """A branch with support for a fallback repository."""
 
-    def set_reference_info(self, tree_path, branch_location, file_id=None):
+    def set_reference_info(self, file_id, branch_location, tree_path=None):
         super(BzrBranch7, self).set_reference_info(
-            tree_path, branch_location, file_id=file_id)
+            file_id, branch_location, tree_path)
         format_string = BzrBranchFormat8.get_format_string()
         mutter('Upgrading branch to format %r', format_string)
         self._transport.put_bytes('format', format_string)

=== modified file 'breezy/bzr/inventorytree.py'
--- breezy/bzr/inventorytree.py	2020-01-19 01:29:22 +0000
+++ breezy/bzr/inventorytree.py	2020-01-19 03:23:48 +0000
@@ -790,7 +790,8 @@
 
     def reference_parent(self, path, branch=None, possible_transports=None):
         if branch is not None:
-            parent_url = branch.get_reference_info(path)[0]
+            file_id = self.path2id(path)
+            parent_url = branch.get_reference_info(file_id)[0]
         else:
             subdir = ControlDir.open_from_transport(
                 self._repository.user_transport.clone(path))
@@ -802,7 +803,7 @@
             possible_transports=possible_transports)
 
     def get_reference_info(self, path, branch=None):
-        return branch.get_reference_info(path)[0]
+        return branch.get_reference_info(self.path2id(path))[0]
 
     def list_files(self, include_root=False, from_dir=None, recursive=True,
                    recurse_nested=False):

=== modified file 'breezy/bzr/workingtree.py'
--- breezy/bzr/workingtree.py	2020-01-19 01:29:22 +0000
+++ breezy/bzr/workingtree.py	2020-01-19 03:23:48 +0000
@@ -1797,13 +1797,20 @@
                     yield get_canonical_path(self, path, normalize)
 
     def get_reference_info(self, path, branch=None):
-        return self.branch.get_reference_info(path)[0]
+        file_id = self.path2id(path)
+        if file_id is None:
+            return None
+        return self.branch.get_reference_info(file_id)[0]
 
     def set_reference_info(self, tree_path, branch_location):
-        self.branch.set_reference_info(tree_path, branch_location)
+        file_id = self.path2id(tree_path)
+        if file_id is None:
+            raise errors.NoSuchFile(tree_path)
+        self.branch.set_reference_info(file_id, branch_location, tree_path)
 
     def reference_parent(self, path, branch=None, possible_transports=None):
         return self.branch.reference_parent(
+            self.path2id(path),
             path, possible_transports=possible_transports)
 
 

=== modified file 'breezy/tests/blackbox/test_branch.py'
--- breezy/tests/blackbox/test_branch.py	2020-01-18 16:14:28 +0000
+++ breezy/tests/blackbox/test_branch.py	2020-01-19 03:23:48 +0000
@@ -367,7 +367,7 @@
         subtree.add(['a'])
         subtree.commit('add subtree contents')
         orig.add_reference(subtree)
-        orig.branch.set_reference_info('subtree', subtree.branch.user_url)
+        orig.set_reference_info('subtree', subtree.branch.user_url)
         orig.commit('add subtree')
 
         self.run_bzr('branch source target')

=== modified file 'breezy/tests/blackbox/test_reference.py'
--- breezy/tests/blackbox/test_reference.py	2020-01-18 22:02:05 +0000
+++ breezy/tests/blackbox/test_reference.py	2020-01-19 03:23:48 +0000
@@ -32,10 +32,10 @@
     def test_no_args_lists(self):
         tree = self.make_branch_and_tree('branch')
         branch = tree.branch
-        branch.set_reference_info('path', 'http://example.org')
         tree.add_reference(self.make_branch_and_tree('branch/path'))
         tree.add_reference(self.make_branch_and_tree('branch/lath'))
-        branch.set_reference_info('lath', 'http://example.org/2')
+        tree.set_reference_info('path', 'http://example.org')
+        tree.set_reference_info('lath', 'http://example.org/2')
         out, err = self.run_bzr('reference', working_dir='branch')
         lines = out.splitlines()
         self.assertEqual('lath http://example.org/2', lines[0])
@@ -45,9 +45,8 @@
         tree = self.make_branch_and_tree('tree')
         subtree = self.make_branch_and_tree('tree/newpath')
         tree.add_reference(subtree)
+        tree.set_reference_info('newpath', 'http://example.org')
         tree.commit('add reference')
-        tree.set_reference_info('newpath', 'http://example.org')
-        tree.set_reference_info('lath', 'http://example.org/2')
         return tree
 
     def test_uses_working_tree_location(self):
@@ -57,7 +56,6 @@
 
     def test_uses_basis_tree_location(self):
         tree = self.make_tree_with_reference()
-        tree.commit('add newpath')
         tree.controldir.destroy_workingtree()
         out, err = self.run_bzr('reference', working_dir='tree')
         self.assertContainsRe(out, 'newpath http://example.org\n')
@@ -90,10 +88,11 @@
 
     def test_missing_file_forced(self):
         tree = self.make_branch_and_tree('tree')
+        tree.add_reference(self.make_branch_and_tree('tree/file'))
         out, err = self.run_bzr(
             'reference --force-unversioned file http://example.org',
             working_dir='tree')
-        location, file_id = tree.branch.get_reference_info('file')
+        location = tree.get_reference_info('file')
         self.assertEqual('http://example.org', location)
         self.assertEqual('', out)
         self.assertEqual('', err)

=== modified file 'breezy/tests/per_workingtree/test_workingtree.py'
--- breezy/tests/per_workingtree/test_workingtree.py	2020-01-19 01:29:22 +0000
+++ breezy/tests/per_workingtree/test_workingtree.py	2020-01-19 03:23:48 +0000
@@ -1377,18 +1377,10 @@
         self.assertIs(None, loc)
 
     def test_set_reference_info(self):
-        tree = self.make_branch_and_tree('branch')
-        try:
-            tree.set_reference_info('path/to/file', 'path/to/location')
-        except errors.UnsupportedOperation:
-            raise tests.TestNotApplicable('Branch cannot hold references.')
+        self.make_tree_with_reference('branch', 'path/to/location')
 
     def test_set_get_reference_info(self):
-        tree = self.make_branch_and_tree('branch')
-        try:
-            tree.set_reference_info('path/to/file', 'path/to/location')
-        except errors.UnsupportedOperation:
-            raise tests.TestNotApplicable('Branch cannot hold references.')
+        tree = self.make_tree_with_reference('branch', 'path/to/location')
         # Create a new instance to ensure storage is permanent
         tree = WorkingTree.open('branch')
         branch_location = tree.get_reference_info('path/to/file')
@@ -1398,12 +1390,14 @@
 
     def test_set_null_reference_info(self):
         tree = self.make_branch_and_tree('branch')
+        self.build_tree(['branch/file'])
+        tree.add(['file'])
         try:
-            tree.set_reference_info('path/to/file', 'path/to/location')
+            tree.set_reference_info('file', 'path/to/location')
         except errors.UnsupportedOperation:
             raise tests.TestNotApplicable('Branch cannot hold references.')
-        tree.set_reference_info('path/to/file', None)
-        branch_location = tree.get_reference_info('path/to/file')
+        tree.set_reference_info('file', None)
+        branch_location = tree.get_reference_info('file')
         self.assertIs(None, branch_location)
 
     def test_set_null_reference_info_when_null(self):
@@ -1413,10 +1407,19 @@
         except errors.UnsupportedOperation:
             raise tests.TestNotApplicable('Branch cannot hold references.')
         self.assertIs(None, branch_location)
-        tree.set_reference_info('path/to/file', None)
+        self.build_tree(['branch/file'])
+        tree.add(['file'])
+        try:
+            tree.set_reference_info('file', None)
+        except errors.UnsupportedOperation:
+            raise tests.TestNotApplicable('Branch cannot hold references.')
 
     def make_tree_with_reference(self, location, reference_location):
         tree = self.make_branch_and_tree(location)
+        self.build_tree(
+            [os.path.join(location, name)
+             for name in ['path/', 'path/to/', 'path/to/file']])
+        tree.add(['path', 'path/to', 'path/to/file'])
         try:
             tree.set_reference_info('path/to/file', reference_location)
         except errors.UnsupportedOperation:
@@ -1431,12 +1434,7 @@
         self.assertEqual(parent.base, referenced_branch.base)
 
     def test_branch_relative_reference_location(self):
-        tree = self.make_branch_and_tree('branch')
-        try:
-            tree.set_reference_info('path/to/file', '../reference_branch')
-        except errors.UnsupportedOperation:
-            raise tests.TestNotApplicable('Branch cannot hold references.')
-        tree.commit('add reference')
+        tree = self.make_tree_with_reference('branch', '../reference_branch')
         referenced_branch = self.make_branch('reference_branch')
         parent = tree.reference_parent('path/to/file')
         self.assertEqual(parent.base, referenced_branch.base)
@@ -1492,6 +1490,8 @@
         tree = self.make_tree_with_reference('branch', 'reference')
         new_tree = tree.controldir.sprout(
             'branch/new-branch').open_workingtree()
+        self.build_tree(['branch/new-branch/foo'])
+        new_tree.add('foo')
         new_tree.set_reference_info('foo', '../foo')
         new_tree.branch.update_references(tree.branch)
         self.assertEqual(
@@ -1504,6 +1504,8 @@
         tree = self.make_tree_with_reference('branch', 'reference')
         new_tree = tree.controldir.sprout(
             'branch/new-branch').open_workingtree()
+        self.build_tree(['branch/new-branch/foo'])
+        new_tree.add('foo')
         new_tree.set_reference_info('foo', '../foo')
         new_tree.commit('set reference')
         tree.pull(new_tree.branch)
@@ -1515,6 +1517,8 @@
         tree = self.make_tree_with_reference('branch', 'reference')
         new_tree = tree.controldir.sprout(
             'branch/new-branch').open_workingtree()
+        self.build_tree(['branch/new-branch/foo'])
+        new_tree.add(['foo'])
         new_tree.set_reference_info('foo', '../foo')
         new_tree.commit('add reference')
         tree.pull(new_tree.branch)

=== modified file 'breezy/tests/test_branch.py'
--- breezy/tests/test_branch.py	2020-01-19 01:29:22 +0000
+++ breezy/tests/test_branch.py	2020-01-19 03:23:48 +0000
@@ -491,11 +491,11 @@
     def test_reference_info_caches_cleared(self):
         branch = self.make_branch('branch')
         with branch.lock_write():
-            branch.set_reference_info('path2', 'location2', b'file-id')
+            branch.set_reference_info(b'file-id', 'location2', 'path2')
         doppelganger = _mod_branch.Branch.open('branch')
-        doppelganger.set_reference_info('path3', 'location3', b'file-id')
-        self.assertEqual(('location3', b'file-id'),
-                         branch.get_reference_info('path3'))
+        doppelganger.set_reference_info(b'file-id', 'location3', 'path3')
+        self.assertEqual(('location3', 'path3'),
+                         branch.get_reference_info(b'file-id'))
 
     def _recordParentMapCalls(self, repo):
         self._parent_map_calls = []

=== modified file 'breezy/tests/test_bzrdir.py'
--- breezy/tests/test_bzrdir.py	2020-01-19 01:29:22 +0000
+++ breezy/tests/test_bzrdir.py	2020-01-19 03:23:48 +0000
@@ -876,8 +876,7 @@
         sub_tree.add('file')
         tree.commit('Initial commit')
         # The following line force the orhaning to reveal bug #634470
-        tree.branch.get_config_stack().set(
-            'transform.orphan_policy', 'move')
+        tree.branch.get_config_stack().set('transform.orphan_policy', 'move')
         tree.controldir.destroy_workingtree()
         # FIXME: subtree/.bzr is left here which allows the test to pass (or
         # fail :-( ) -- vila 20100909

=== modified file 'breezy/tests/test_reconfigure.py'
--- breezy/tests/test_reconfigure.py	2018-11-12 01:41:38 +0000
+++ breezy/tests/test_reconfigure.py	2020-01-19 03:23:48 +0000
@@ -461,10 +461,10 @@
         format = controldir.format_registry.make_controldir('1.9')
         format.set_branch_format(_mod_bzrbranch.BzrBranchFormat8())
         tree = self.make_branch_and_tree('tree', format=format)
-        tree.branch.set_reference_info('path', '../location', 'file_id')
+        tree.branch.set_reference_info(b'file_id', '../location', 'path')
         checkout = tree.branch.create_checkout('checkout', lightweight=True)
         reconfiguration = reconfigure.Reconfigure.to_tree(checkout.controldir)
         reconfiguration.apply()
         checkout_branch = checkout.controldir.open_branch()
-        self.assertEqual(('../location', b'file_id'),
-                         checkout_branch.get_reference_info('path'))
+        self.assertEqual(('../location', 'path'),
+                         checkout_branch.get_reference_info(b'file_id'))