Merge lp:~jelmer/brz/objects-2c into lp:brz

[Jelmer Vernooij]

Drop file_id attribute from MergeHookParams and Tree.plan_file_merge.

Drop file_id attribute from MergeHookParams and Tree.plan_file_merge.

(This depends on objects-2a and objects-2b, but I can't set two prerequisite branches)

[Martin Packman (gz)]

New changes here all seem fine. Some interface changes, which may be worth calling out in NEWS?

review: Approve

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

Merging failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/413/

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

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

=== modified file 'breezy/bzr/inventorytree.py'
--- breezy/bzr/inventorytree.py	2019-06-21 16:18:13 +0000
+++ breezy/bzr/inventorytree.py	2019-06-22 13:51:41 +0000
@@ -215,11 +215,12 @@
                 raise errors.NotADirectory(path)
             return iter(viewvalues(ie.children))
 
-    def _get_plan_merge_data(self, file_id, other, base):
+    def _get_plan_merge_data(self, path, other, base):
         from . import versionedfile
+        file_id = self.path2id(path)
         vf = versionedfile._PlanMergeVersionedFile(file_id)
         last_revision_a = self._get_file_revision(
-            self.id2path(file_id), file_id, vf, b'this:')
+            path, file_id, vf, b'this:')
         last_revision_b = other._get_file_revision(
             other.id2path(file_id), file_id, vf, b'other:')
         if base is None:
@@ -229,7 +230,7 @@
                 base.id2path(file_id), file_id, vf, b'base:')
         return vf, last_revision_a, last_revision_b, last_revision_base
 
-    def plan_file_merge(self, file_id, other, base=None):
+    def plan_file_merge(self, path, other, base=None):
         """Generate a merge plan based on annotations.
 
         If the file contains uncommitted changes in this tree, they will be
@@ -237,12 +238,12 @@
         uncommitted changes in the other tree, they will be assigned to the
         'other:' pseudo-revision.
         """
-        data = self._get_plan_merge_data(file_id, other, base)
+        data = self._get_plan_merge_data(path, other, base)
         vf, last_revision_a, last_revision_b, last_revision_base = data
         return vf.plan_merge(last_revision_a, last_revision_b,
                              last_revision_base)
 
-    def plan_file_lca_merge(self, file_id, other, base=None):
+    def plan_file_lca_merge(self, path, other, base=None):
         """Generate a merge plan based lca-newness.
 
         If the file contains uncommitted changes in this tree, they will be
@@ -250,7 +251,7 @@
         uncommitted changes in the other tree, they will be assigned to the
         'other:' pseudo-revision.
         """
-        data = self._get_plan_merge_data(file_id, other, base)
+        data = self._get_plan_merge_data(path, other, base)
         vf, last_revision_a, last_revision_b, last_revision_base = data
         return vf.plan_lca_merge(last_revision_a, last_revision_b,
                                  last_revision_base)

=== modified file 'breezy/merge.py'
--- breezy/merge.py	2019-06-22 11:16:17 +0000
+++ breezy/merge.py	2019-06-22 13:51:41 +0000
@@ -223,20 +223,18 @@
 
     There are some fields hooks can access:
 
-    :ivar file_id: the file ID of the file being merged
     :ivar base_path: Path in base tree
     :ivar other_path: Path in other tree
     :ivar this_path: Path in this tree
     :ivar trans_id: the transform ID for the merge of this file
-    :ivar this_kind: kind of file_id in 'this' tree
-    :ivar other_kind: kind of file_id in 'other' tree
+    :ivar this_kind: kind of file in 'this' tree
+    :ivar other_kind: kind of file in 'other' tree
     :ivar winner: one of 'this', 'other', 'conflict'
     """
 
-    def __init__(self, merger, file_id, paths, trans_id, this_kind, other_kind,
+    def __init__(self, merger, paths, trans_id, this_kind, other_kind,
                  winner):
         self._merger = merger
-        self.file_id = file_id
         self.paths = paths
         self.base_path, self.other_path, self.this_path = paths
         self.trans_id = trans_id
@@ -804,16 +802,18 @@
         with ui.ui_factory.nested_progress_bar() as child_pb:
             for num, (file_id, changed, paths3, parents3, names3,
                       executable3) in enumerate(entries):
+                trans_id = self.tt.trans_id_file_id(file_id)
+
                 # Try merging each entry
                 child_pb.update(gettext('Preparing file merge'),
                                 num, len(entries))
-                self._merge_names(file_id, paths3, parents3,
+                self._merge_names(trans_id, file_id, paths3, parents3,
                                   names3, resolver=resolver)
                 if changed:
-                    file_status = self._do_merge_contents(paths3, file_id)
+                    file_status = self._do_merge_contents(paths3, trans_id, file_id)
                 else:
                     file_status = 'unmodified'
-                self._merge_executable(paths3, file_id, executable3,
+                self._merge_executable(paths3, trans_id, executable3,
                                        file_status, resolver=resolver)
         self.tt.fixup_new_roots()
         self._finish_computing_transform()
@@ -1182,8 +1182,8 @@
         # At this point, the lcas disagree, and the tip disagree
         return 'conflict'
 
-    def _merge_names(self, file_id, paths, parents, names, resolver):
-        """Perform a merge on file_id names and parents"""
+    def _merge_names(self, trans_id, file_id, paths, parents, names, resolver):
+        """Perform a merge on file names and parents"""
         base_name, other_name, this_name = names
         base_parent, other_parent, this_parent = parents
         unused_base_path, other_path, this_path = paths
@@ -1202,7 +1202,6 @@
             # Creating helpers (.OTHER or .THIS) here cause problems down the
             # road if a ContentConflict needs to be created so we should not do
             # that
-            trans_id = self.tt.trans_id_file_id(file_id)
             self._raw_conflicts.append(('path conflict', trans_id, file_id,
                                         this_parent, this_name,
                                         other_parent, other_name))
@@ -1225,10 +1224,9 @@
                 parent_trans_id = transform.ROOT_PARENT
             else:
                 parent_trans_id = self.tt.trans_id_file_id(parent_id)
-            self.tt.adjust_path(name, parent_trans_id,
-                                self.tt.trans_id_file_id(file_id))
+            self.tt.adjust_path(name, parent_trans_id, trans_id)
 
-    def _do_merge_contents(self, paths, file_id):
+    def _do_merge_contents(self, paths, trans_id, file_id):
         """Performs a merge on file_id contents."""
         def contents_pair(tree, path):
             if path is None:
@@ -1272,10 +1270,8 @@
             return "unmodified"
         # We have a hypothetical conflict, but if we have files, then we
         # can try to merge the content
-        trans_id = self.tt.trans_id_file_id(file_id)
         params = MergeFileHookParams(
-            self, file_id, (base_path, other_path,
-                            this_path), trans_id, this_pair[0],
+            self, (base_path, other_path, this_path), trans_id, this_pair[0],
             other_pair[0], winner)
         hooks = self.active_hooks
         hook_status = 'not_applicable'
@@ -1372,8 +1368,8 @@
             return 'delete', None
         else:
             raise AssertionError(
-                'winner is OTHER, but file_id %r not in THIS or OTHER tree'
-                % (file_id,))
+                'winner is OTHER, but file %r not in THIS or OTHER tree'
+                % (merge_hook_params.base_path,))
 
     def merge_contents(self, merge_hook_params):
         """Fallback merge logic after user installed hooks."""
@@ -1389,7 +1385,7 @@
             # have agreement that output should be a file.
             try:
                 self.text_merge(merge_hook_params.trans_id,
-                                merge_hook_params.paths, merge_hook_params.file_id)
+                                merge_hook_params.paths)
             except errors.BinaryFile:
                 return 'not_applicable', None
             return 'done', None
@@ -1409,8 +1405,8 @@
                 return []
             return tree.get_file_lines(path)
 
-    def text_merge(self, trans_id, paths, file_id):
-        """Perform a three-way text merge on a file_id"""
+    def text_merge(self, trans_id, paths):
+        """Perform a three-way text merge on a file"""
         # it's possible that we got here with base as a different type.
         # if so, we just want two-way text conflicts.
         base_path, other_path, this_path = paths
@@ -1445,6 +1441,7 @@
             self._raw_conflicts.append(('text conflict', trans_id))
             name = self.tt.final_name(trans_id)
             parent_id = self.tt.final_parent(trans_id)
+            file_id = self.tt.final_file_id(trans_id)
             file_group = self._dump_conflicts(name, paths, parent_id, file_id,
                                               this_lines, base_lines,
                                               other_lines)
@@ -1515,14 +1512,7 @@
             filter_tree_path=filter_tree_path)
         return trans_id
 
-    def merge_executable(self, paths, file_id, file_status):
-        """Perform a merge on the execute bit."""
-        executable = [self.executable(t, p, file_id)
-                      for t, p in zip([self.base_tree, self.other_tree, self.this_tree], paths)]
-        self._merge_executable(paths, file_id, executable, file_status,
-                               resolver=self._three_way)
-
-    def _merge_executable(self, paths, file_id, executable, file_status,
+    def _merge_executable(self, paths, trans_id, executable, file_status,
                           resolver):
         """Perform a merge on the execute bit."""
         base_executable, other_executable, this_executable = executable
@@ -1539,7 +1529,6 @@
                 winner = "other"
         if winner == 'this' and file_status != "modified":
             return
-        trans_id = self.tt.trans_id_file_id(file_id)
         if self.tt.final_kind(trans_id) != "file":
             return
         if winner == "this":
@@ -1552,7 +1541,6 @@
             elif base_path is not None:
                 executability = base_executable
         if executability is not None:
-            trans_id = self.tt.trans_id_file_id(file_id)
             self.tt.set_executability(executability, trans_id)
 
     def cook_conflicts(self, fs_conflicts):
@@ -1634,11 +1622,11 @@
     history_based = True
     requires_file_merge_plan = True
 
-    def _generate_merge_plan(self, file_id, base):
-        return self.this_tree.plan_file_merge(file_id, self.other_tree,
+    def _generate_merge_plan(self, this_path, base):
+        return self.this_tree.plan_file_merge(this_path, self.other_tree,
                                               base=base)
 
-    def _merged_lines(self, file_id):
+    def _merged_lines(self, this_path):
         """Generate the merged lines.
         There is no distinction between lines that are meant to contain <<<<<<<
         and conflicts.
@@ -1647,7 +1635,7 @@
             base = self.base_tree
         else:
             base = None
-        plan = self._generate_merge_plan(file_id, base)
+        plan = self._generate_merge_plan(this_path, base)
         if 'merge' in debug.debug_flags:
             plan = list(plan)
             trans_id = self.tt.trans_id_file_id(file_id)
@@ -1663,13 +1651,13 @@
             base_lines = None
         return lines, base_lines
 
-    def text_merge(self, trans_id, paths, file_id):
+    def text_merge(self, trans_id, paths):
         """Perform a (weave) text merge for a given file and file-id.
         If conflicts are encountered, .THIS and .OTHER files will be emitted,
         and a conflict will be noted.
         """
         base_path, other_path, this_path = paths
-        lines, base_lines = self._merged_lines(file_id)
+        lines, base_lines = self._merged_lines(this_path)
         lines = list(lines)
         # Note we're checking whether the OUTPUT is binary in this case,
         # because we don't want to get into weave merge guts.
@@ -1680,6 +1668,7 @@
             self._raw_conflicts.append(('text conflict', trans_id))
             name = self.tt.final_name(trans_id)
             parent_id = self.tt.final_parent(trans_id)
+            file_id = self.tt.final_file_id(trans_id)
             file_group = self._dump_conflicts(name, paths, parent_id, file_id,
                                               no_base=False,
                                               base_lines=base_lines)
@@ -1690,8 +1679,8 @@
 
     requires_file_merge_plan = True
 
-    def _generate_merge_plan(self, file_id, base):
-        return self.this_tree.plan_file_lca_merge(file_id, self.other_tree,
+    def _generate_merge_plan(self, this_path, base):
+        return self.this_tree.plan_file_lca_merge(this_path, self.other_tree,
                                                   base=base)
 
 
@@ -1708,7 +1697,7 @@
                 out_file.write(line)
         return out_path
 
-    def text_merge(self, trans_id, paths, file_id):
+    def text_merge(self, trans_id, paths):
         """Perform a diff3 merge using a specified file-id and trans-id.
         If conflicts are encountered, .BASE, .THIS. and .OTHER conflict files
         will be dumped, and a will be conflict noted.
@@ -1732,6 +1721,7 @@
             if status == 1:
                 name = self.tt.final_name(trans_id)
                 parent_id = self.tt.final_parent(trans_id)
+                file_id = self.tt.final_file_id(trans_id)
                 self._dump_conflicts(name, paths, parent_id, file_id)
                 self._raw_conflicts.append(('text conflict', trans_id))
         finally:

=== modified file 'breezy/plugins/changelog_merge/tests/test_changelog_merge.py'
--- breezy/plugins/changelog_merge/tests/test_changelog_merge.py	2018-11-11 04:08:32 +0000
+++ breezy/plugins/changelog_merge/tests/test_changelog_merge.py	2019-06-22 13:51:41 +0000
@@ -193,10 +193,9 @@
         # won't write the new value to disk where get_user_option can get it).
         merger.this_branch.get_config().set_user_option(
             'changelog_merge_files', 'ChangeLog')
-        merge_hook_params = merge.MergeFileHookParams(merger, b'clog-id',
-                                                      ['ChangeLog', 'ChangeLog',
-                                                          'ChangeLog'], None,
-                                                      'file', 'file', 'conflict')
+        merge_hook_params = merge.MergeFileHookParams(
+            merger, ['ChangeLog', 'ChangeLog', 'ChangeLog'], None,
+            'file', 'file', 'conflict')
         changelog_merger = changelog_merge.ChangeLogMerger(merger)
         return changelog_merger, merge_hook_params
 

=== modified file 'breezy/tests/per_merger.py'
--- breezy/tests/per_merger.py	2019-06-17 23:01:58 +0000
+++ breezy/tests/per_merger.py	2019-06-22 13:51:41 +0000
@@ -242,7 +242,7 @@
         class HookSuccess(_mod_merge.AbstractPerFileMerger):
             def merge_contents(self, merge_params):
                 test.hook_log.append(('success',))
-                if merge_params.file_id == b'1':
+                if merge_params.this_path == 'name1':
                     return 'success', [b'text-merged-by-hook']
                 return 'not_applicable', None
 
@@ -257,7 +257,7 @@
         class HookConflict(_mod_merge.AbstractPerFileMerger):
             def merge_contents(self, merge_params):
                 test.hook_log.append(('conflict',))
-                if merge_params.file_id == b'1':
+                if merge_params.this_path == 'name1':
                     return ('conflicted',
                             [b'text-with-conflict-markers-from-hook'])
                 return 'not_applicable', None
@@ -273,7 +273,7 @@
         class HookDelete(_mod_merge.AbstractPerFileMerger):
             def merge_contents(self, merge_params):
                 test.hook_log.append(('delete',))
-                if merge_params.file_id == b'1':
+                if merge_params.this_path == 'name1':
                     return 'delete', None
                 return 'not_applicable', None
 
@@ -309,8 +309,9 @@
         self.addCleanup(builder.cleanup)
         return builder
 
-    def create_file_needing_contents_merge(self, builder, file_id):
-        builder.add_file(file_id, builder.tree_root, "name1", b"text1", True)
+    def create_file_needing_contents_merge(self, builder, name):
+        file_id = name.encode('ascii') + b'-id'
+        builder.add_file(file_id, builder.tree_root, name, b"text1", True)
         builder.change_contents(file_id, other=b"text4", this=b"text3")
 
     def test_change_vs_change(self):
@@ -340,19 +341,19 @@
         """A hook's result can be the deletion of a file."""
         self.install_hook_delete()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
         self.assertEqual(conflicts, [])
-        self.assertRaises(errors.NoSuchId, builder.this.id2path, b'1')
+        self.assertFalse(builder.this.is_versioned('name1'))
         self.assertEqual([], list(builder.this.list_files()))
 
     def test_result_can_be_conflict(self):
         """A hook's result can be a conflict."""
         self.install_hook_conflict()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
-        self.assertEqual(conflicts, [TextConflict('name1', file_id=b'1')])
+        self.assertEqual(conflicts, [TextConflict('name1', file_id=b'name1-id')])
         # The hook still gets to set the file contents in this case, so that it
         # can insert custom conflict markers.
         with builder.this.get_file('name1') as f:
@@ -375,7 +376,7 @@
         self.install_hook_inactive()
         self.install_hook_success()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
         self.assertEqual(conflicts, [])
         with builder.this.get_file('name1') as f:
@@ -389,7 +390,7 @@
         self.install_hook_noop()
         self.install_hook_success()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
         self.assertEqual(conflicts, [])
         with builder.this.get_file('name1') as f:
@@ -402,7 +403,7 @@
         self.install_hook_success()
         self.install_hook_noop()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
         self.assertEqual([('success',)], self.hook_log)
 
@@ -412,7 +413,7 @@
         self.install_hook_conflict()
         self.install_hook_noop()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
         self.assertEqual([('conflict',)], self.hook_log)
 
@@ -422,6 +423,6 @@
         self.install_hook_delete()
         self.install_hook_noop()
         builder = self.make_merge_builder()
-        self.create_file_needing_contents_merge(builder, b"1")
+        self.create_file_needing_contents_merge(builder, "name1")
         conflicts = builder.merge(self.merge_type)
         self.assertEqual([('delete',)], self.hook_log)

=== modified file 'breezy/tests/per_tree/test_tree.py'
--- breezy/tests/per_tree/test_tree.py	2019-06-18 13:48:45 +0000
+++ breezy/tests/per_tree/test_tree.py	2019-06-22 13:51:41 +0000
@@ -51,7 +51,6 @@
         work_a = self.make_branch_and_tree('wta')
         self.build_tree_contents([('wta/file', b'a\nb\nc\nd\n')])
         work_a.add('file')
-        file_id = work_a.path2id('file')
         work_a.commit('base version')
         work_b = work_a.controldir.sprout('wtb').open_workingtree()
         self.build_tree_contents([('wta/file', b'b\nc\nd\ne\n')])
@@ -72,7 +71,7 @@
             ('unchanged', b'd\n'),
             ('new-a', b'e\n'),
             ('new-b', b'f\n'),
-        ], list(tree_a.plan_file_merge(file_id, tree_b)))
+        ], list(tree_a.plan_file_merge('file', tree_b)))
 
 
 class TestReference(TestCaseWithTree):

=== modified file 'breezy/tests/per_workingtree/test_merge_from_branch.py'
--- breezy/tests/per_workingtree/test_merge_from_branch.py	2018-11-11 04:08:32 +0000
+++ breezy/tests/per_workingtree/test_merge_from_branch.py	2019-06-22 13:51:41 +0000
@@ -115,7 +115,7 @@
         this.commit('content -> baz')
 
         class QuxMerge(merge.Merge3Merger):
-            def text_merge(self, trans_id, paths, file_id):
+            def text_merge(self, trans_id, paths):
                 self.tt.create_file([b'qux'], trans_id)
         this.merge_from_branch(other.branch, merge_type=QuxMerge)
         self.assertEqual(b'qux', this.get_file_text('foo'))

=== modified file 'breezy/tests/test_transform.py'
--- breezy/tests/test_transform.py	2019-06-22 11:16:17 +0000
+++ breezy/tests/test_transform.py	2019-06-22 13:51:41 +0000
@@ -3264,7 +3264,7 @@
             ('unchanged', b'd\n'),
             ('new-a', b'e\n'),
             ('new-b', b'f\n'),
-        ], list(tree_a.plan_file_merge(b'file-id', tree_b)))
+        ], list(tree_a.plan_file_merge('file', tree_b)))
 
     def test_plan_file_merge_revision_tree(self):
         work_a = self.make_branch_and_tree('wta')
@@ -3287,7 +3287,7 @@
             ('unchanged', b'd\n'),
             ('new-a', b'e\n'),
             ('new-b', b'f\n'),
-        ], list(tree_a.plan_file_merge(b'file-id', tree_b)))
+        ], list(tree_a.plan_file_merge('file', tree_b)))
 
     def test_walkdirs(self):
         preview = self.get_empty_preview()

=== modified file 'breezy/transform.py'
--- breezy/transform.py	2019-06-22 11:16:17 +0000
+++ breezy/transform.py	2019-06-22 13:51:41 +0000
@@ -2675,8 +2675,7 @@
                             divert.add(file_id)
                     if (file_id not in divert
                         and _content_match(
-                            tree, entry, tree_path, file_id, kind,
-                            target_path)):
+                            tree, entry, tree_path, kind, target_path)):
                         tt.delete_contents(tt.trans_id_tree_path(tree_path))
                         if kind == 'directory':
                             reparent = True
@@ -2785,7 +2784,7 @@
     return by_parent[old_parent]
 
 
-def _content_match(tree, entry, tree_path, file_id, kind, target_path):
+def _content_match(tree, entry, tree_path, kind, target_path):
     if entry.kind != kind:
         return False
     if entry.kind == "directory":