Code review comment for lp:~amanica/bzr/mv_after

Revision history for this message
Marius Kruger (amanica) wrote :

> Robert Collins wrote:
> > Review: Needs Fixing
> > Its not safe to mutate the inventory in this way. You're aliasing the entry
> in the basis with the working tree, and later mutations will alter both
> inventories. It will also be copying all the children in which will fail badly
> if the children weren't all removed.
> >
> > So the right way to do this is to create a new inventory entry with the
> right file id, parent id, kind and basename.
>
> Isn't InventoryEntry.copy() the right way to do that? I know it
> preserves file_id, revision, and *doesn't* preserve .children for
> directories....

I followed John's advice and just .copy() the InventoryEntry now before re-inserting it, which seems correct.
(I tried to add a test to check if it was really cloned, but could not figure out how to compare them in the end. It may be a little overkill in any case.)

I pushed my changes to this branch now, but I can't figure out how to re-submit for review and to update the diff.

(Sorry for being quiet on this. I've been sick, on holiday and busy (in that order))

« Back to merge proposal