Code review comment for lp:~jameinel/bzr/1.18-bundle-and-stack-393349

=== modified file 'NEWS'
--- NEWS 2009-07-16 08:00:03 +0000
+++ NEWS 2009-07-27 18:03:19 +0000
@@ -33,6 +33,9 @@
 * ``bzr mv`` no longer takes out branch locks, which allows it to work
   when the branch is readonly. (Robert Collins, #216541)

+* ``bzr send`` now generates valid bundles with ``--2a`` formats.
+ (John Arbash Meinel, #393349)
+
 * Fixed spurious "Source branch does not support stacking" warning when
   pushing. (Andrew Bennetts, #388908)

It'd be useful to mention in the news file the constraints on interoperation
with other formats and clients.

=== modified file 'bzrlib/bundle/serializer/v4.py'
--- bzrlib/bundle/serializer/v4.py 2009-06-10 03:56:49 +0000
+++ bzrlib/bundle/serializer/v4.py 2009-07-27 17:51:10 +0000
@@ -315,12 +315,69 @@
     def write_revisions(self):
         """Write bundle records for all revisions and signatures"""
         inv_vf = self.repository.inventories
- revision_order = [key[-1] for key in multiparent.topo_iter_keys(inv_vf,
- self.revision_keys)]
+ topological_order = [key[-1] for key in multiparent.topo_iter_keys(
+ inv_vf, self.revision_keys)]
+ revision_order = topological_order
         if self.target is not None and self.target in self.revision_ids:
+ # Make sure the target is always the last entry
+ revision_order = list(topological_order)
             revision_order.remove(self.target)
             revision_order.append(self.target)
- self._add_mp_records_keys('inventory', inv_vf, [(revid,) for revid in revision_order])
+ if self.repository._serializer.support_altered_by_hack:
+ self._add_mp_records_keys('inventory', inv_vf,
+ [(revid,) for revid in topological_order])
+ else:
+ self._add_inventory_mpdiffs_from_serializer(topological_order)
+ self._add_revision_texts(revision_order)

I think you should have, here, at least some description of why you're
going to use support_altered_by_hack here, because it's (at least to me)
not very clear that it's the same thing as being able to get the
inventory mp records from the versionedfile.

Is there a reason the orders should be different?

+
+ def _add_inventory_mpdiffs_from_serializer(self, revision_order):

Maybe a comment or docstring about the purpose of this?

+ inventory_key_order = [(r,) for r in revision_order]
+ parent_map = self.repository.inventories.get_parent_map(
+ inventory_key_order)
+ missing_keys = set(inventory_key_order).difference(parent_map)
+ if missing_keys:
+ raise errors.RevisionNotPresent(list(missing_keys)[0],
+ self.repository.inventories)
+ inv_to_str = self.repository._serializer.write_inventory_to_string
+ # Make sure that we grab the parent texts first
+ just_parents = set()
+ map(just_parents.update, parent_map.itervalues())
+ just_parents.difference_update(parent_map)
+ # Ignore ghost parents
+ present_parents = self.repository.inventories.get_parent_map(
+ just_parents)
+ ghost_keys = just_parents.difference(present_parents)
+ needed_inventories = list(present_parents) + inventory_key_order
+ needed_inventories = [k[-1] for k in needed_inventories]
+ all_lines = {}
+ for inv in self.repository.iter_inventories(needed_inventories):
+ revision_id = inv.revision_id
+ key = (revision_id,)
+ as_bytes = inv_to_str(inv)
+ # The sha1 is validated as the xml/textual form, not as the
+ # form-in-the-repository
+ sha1 = osutils.sha_string(as_bytes)
+ as_lines = osutils.split_lines(as_bytes)
+ del as_bytes
+ all_lines[key] = as_lines
+ if key in just_parents:
+ # We don't transmit those entries
+ continue
+ # Create an mpdiff for this text, and add it to the output
+ parent_keys = parent_map[key]
+ # See the comment in VF.make_mpdiffs about how this effects
+ # ordering when there are ghosts present. I think we have a latent
+ # bug
+ parent_lines = [all_lines[p_key] for p_key in parent_keys
+ if p_key not in ghost_keys]
+ diff = multiparent.MultiParent.from_lines(
+ as_lines, parent_lines)
+ text = ''.join(diff.to_patch())
+ parent_ids = [k[-1] for k in parent_keys]
+ self.bundle.add_multiparent_record(text, sha1, parent_ids,
+ 'inventory', revision_id, None)
+
+ def _add_revision_texts(self, revision_order):
         parent_map = self.repository.get_parent_map(revision_order)
         revision_to_str = self.repository._serializer.write_revision_to_string
         revisions = self.repository.get_revisions(revision_order)

=== modified file 'bzrlib/chk_serializer.py'
--- bzrlib/chk_serializer.py 2009-07-01 10:46:27 +0000
+++ bzrlib/chk_serializer.py 2009-07-22 20:22:21 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008 Canonical Ltd
+# Copyright (C) 2008, 2009 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
@@ -21,8 +21,8 @@
     cache_utf8,
     inventory,
     revision as _mod_revision,
- xml5,
     xml6,
+ xml7,
     )

@@ -131,7 +131,7 @@
         return self.read_revision_from_string(f.read())

-class CHKSerializerSubtree(BEncodeRevisionSerializer1, xml6.Serializer_v6):
+class CHKSerializerSubtree(BEncodeRevisionSerializer1, xml7.Serializer_v7):
     """A CHKInventory based serializer that supports tree references"""

     supported_kinds = set(['file', 'directory', 'symlink', 'tree-reference'])
@@ -152,14 +152,14 @@
             return inventory.TreeReference(file_id, name, parent_id, revision,
                                            reference_revision)
         else:
- return xml6.Serializer_v6._unpack_entry(self, elt)
+ return xml7.Serializer_v7._unpack_entry(self, elt)

     def __init__(self, node_size, search_key_name):
         self.maximum_size = node_size
         self.search_key_name = search_key_name

-class CHKSerializer(xml5.Serializer_v5):
+class CHKSerializer(xml6.Serializer_v6):
     """A CHKInventory based serializer with 'plain' behaviour."""

     format_num = '9'

It would be nice (not necessarily for this patch) if we didn't have
concrete final classes inheriting from each other, but rather just made
them inherit from some kind of trait class.

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py 2009-07-02 23:10:53 +0000
+++ bzrlib/repository.py 2009-07-22 19:00:01 +0000
@@ -2190,6 +2190,7 @@
         """
         if ((None in revision_ids)
             or (_mod_revision.NULL_REVISION in revision_ids)):
+ import pdb; pdb.set_trace()
             raise ValueError('cannot get null revision inventory')
         return self._iter_inventories(revision_ids)

Uh...

Otherwise looks ok.