Code review comment for lp:~declanmg/bzr/264275-fix

Revision history for this message
Robert Collins (lifeless) wrote :

Sadly I think this is basically not avoiding the problem of dereferencing symlinks. We actually have to dereference them properly, and not add via the link.

What it is doing according to the tests is adding the thing the link points at as a child of the link, but that is an invalid directory structure and will almost certainly blow up on commmit.

Secondly, doing the check in the command isn't the best place - better to check in WorkingTree.smart_add.

So, to merge this I'd like to see:
 - the entry point code checks moved into MutableTree.smart_add
 - tests of smart_add that check when link/thing is added that the inventory
   *does not* contain children under the link entry.
 - the tests for is_realpath_outside_tree move to test_osutils.py (as that is
   where the function is)

A minor tweak to the tests would be to test with both a relative symlink (tree/link pointing to 'target' rather than abspath('target')).

And I think (but don't insist) that the helper function would be better named as 'realpath_is_outside_path', as the function works in paths, not trees.

« Back to merge proposal