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

Thanks for the feedback. I'll need some more information before continuing, see below...

> 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 do you mean by "dereference them properly"? ie. What should the system do in the following cases
1) The symlink points to a location that is under the source controlled tree
2) The symlink points to a location that is not under the source controlled tree

My understanding is that in case (1), from what you are now saying, we need to dereference the link and then add via the dereferenced path. In case (2) the add should fail.

Functionally, my patch as it stands fulfills case (2). It also fulfills case (1) except that the scope of the fix should be expanded to dereference symlinks when they are added to the tree. Is it then fair to say that dereferencing of symlinks is currently not carried out by Bazaar when the add command adds a symlink? If so is it then ok to open a separate bug to carry out this work first? Then I can complete the work for the original bug afterwards?

Architecturally, the fix should be moved to MutableTree.smart_add to be correct.

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

This statement may not be correct, depending on interpretation. If you try to add my_bzr_repo/symlink_to_dir_outside_tree/thing I believe that it will be denied - as the thing truly lies outside the source controlled tree.

If you try to add my_bzr_repo/symlink_to_dir_inside_tree/thing I believe that it will be carried out. But (just repeating what I said earlier) this invalid addition may be better handled first as a separate bug.

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

Will do.

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

Will look into this.

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

Will do.