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

2009/8/4 John A Meinel <email address hidden>:
>> 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.

OK, so again maybe a comment would be good.

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

I was thinking that the behaviour like having rich roots would be in
the trait class, and then the specific version number would be in a
subclass, so that at the class graph level you could see what they had
in common but not get the incorrect conclusion that a CHK serializer
_was_ an XML serializer. Maybe I should send a hacking guide patch or
rfc.

--
Martin <http://launchpad.net/~mbp/>