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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Review: Approve
> === 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.
>

Will do. I think generally it will only be compatible with newer bzr
versions. I had hoped generating a --2a could be compatible with
1.9-rich-root, but it turns out the old XML-serializer version was wrong
since it was using xml5 which was a non-rich-root format.

>
> === 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.

Sure. I'll at least put comments in.

>
> Is there a reason the orders should be different?
>

For whatever reason the *last* entry in the revisions is supposed to be
the target. However, the inventory stream *must* be in topological
order, or you don't have the parent text in order to apply the mpdiff.

We had a test that was doing (handwaving):

  create_bundle(include_revs=['rev1', 'rev2'], target_rev='rev1')

It seemed to be doing it by accident, but for whatever reason it was
including a child revision in the bundle, but making the parent revision
the "target" revision.

The remove() + append() would then order them in *reverse* topological
order, which was causing the inventory building to fail.

> +
> + def _add_inventory_mpdiffs_from_serializer(self, revision_order):
>
> Maybe a comment or docstring about the purpose of this?
>

done

...

> -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.

I think that would be ok, though I think you would end up with:

class Serializer_v6(V6TraitClass):
  pass

serializer_v6 = Serializer_v6()

Maybe that fits what you want, though. As then v6 is not an instance of
v5, even if the traits base classes are the same.

>
> === 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.
>

Well, it is on an exceptional circumstance, but nice catch. :)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkp4PIMACgkQJdeBCYSNAAOuawCgxjua0DiIig8vr6xQS0Q1OspE
gHoAoLDwV+X3IqPizoVhk84LVxBzpM7E
=rH5b
-----END PGP SIGNATURE-----