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

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

Your error class seems a bit... verbose
AddingContentsUnderSymLinkedDirectoryOutsideTreeNotSupported

Also, you have some lines that are longer than 80 characters. You can usually split them with:

foo = ("this is some text"
       " and a bit more text\n")

6 from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level
7
8 +def is_realpath_outside_tree(tree_path, filepath):
9 +
10 + if not osutils.is_inside(

^- You should have two blank spaces before the def, and is_realpath... should have a docstring.

..

66 + os.mkdir('tree/inside_working_copy')
67 + self.build_tree(['tree/inside_working_copy/a_file.txt'])

^- you can use:

self.build_tree(['tree/inside_working_copy/', 'tree/inside_working_copy/a_file.txt'])
instead.

I think it would be good to have a couple of tests directly for "is_realpath_outside_tree", rather than only indirectly via 'run_bzr(['add', ...])'. Especially since that doesn't actually require symlinks to do the testing, which means it can be done on all platforms.

Otherwise I think I'm happy with the conceptual change.

review: Needs Fixing

« Back to merge proposal