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

Revision history for this message
John A Meinel (jameinel) wrote :

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

Marius Kruger 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))

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

iEYEARECAAYFAkpNc/YACgkQJdeBCYSNAANSaQCfVoH5L72yOiBPXwbAgffDRmWx
FQ8AoIfiRDqucFVlxfTlyWtrqF9UcbIF
=RGNC
-----END PGP SIGNATURE-----

review: Needs Resubmitting

« Back to merge proposal