Code review comment for lp:~spiv/bzr/inventory-delta

[not a complete reply, just for some points that have been dealt with]

Robert Collins wrote:
[...]
> > Ok, I've deleted the repository_class part of the if. Should I delete the
> > “to_format.supports_chk” part too?
>
> yes. self is chk_supporting, so self._serializer ==
> to_format.serializer, or whatever, is fine.

Done.

> > > > + if not unstacked_repo._format.supports_chks:
> > > > + # these assertions aren't valid for groupcompress repos, which may
> > > > + # transfer data than strictly necessary to avoid breaking up an
> > > > + # already-compressed block of data.
> > > > + self.assertFalse(unstacked_repo.has_revision('left'))
> > > > + self.assertFalse(unstacked_repo.has_revision('right'))
> > >
> > > ^ please check the comment, its not quite clear.
> >
> > s/transfer data/transfer more data/
> >
> > I'm not 100% certain that's actually true? What do you think with the adjusted
> > comment?
>
> Big alarm bell. gc repos aren't permitted to do that for the revision
> objects, because of our invariants about revisions. Something else is
> wrong - that if _must not_ be needed.

That if guards turns out to be unnecessary; that test passes for all scenarios
with those assertions run unconditionally. Whatever bug I had that prompted me
to add it is clearly gone.

-Andrew.