Bazaar

Merge lp:~declanmg/bzr/264275-fix into lp:bzr

  1. 264275-fix
  2. Merge into bzr.dev
Proposed by Declan McGrath
Status: Superseded
Proposed branch: lp:~declanmg/bzr/264275-fix
Merge into: lp:bzr
Diff against target: 145 lines (+91/-0)
4 files modified
bzrlib/builtins.py (+25/-0)
bzrlib/errors.py (+5/-0)
bzrlib/tests/blackbox/test_add.py (+55/-0)
bzrlib/tests/test_errors.py (+6/-0)
To merge this branch: bzr merge lp:~declanmg/bzr/264275-fix
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Review via email: mp+14688@code.launchpad.net

This proposal has been superseded by a proposal from 2009-11-23.

To post a comment you must log in.
Revision history for this message
Declan McGrath (declanmg) wrote :

Please read this comment https://bugs.launchpad.net/ubuntu/+source/bzr/+bug/264275/comments/7 for a high level overview of this changeset (you can skip the 'Update' section).

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
lp:~declanmg/bzr/264275-fix updated
4742. By jack <jack@pixie>

Made some post-review changes as requested (more to do) as follows...

* Renamed overly verbose error class
* Made all text lines less than 80 chars long
* Added docstring to is_realpath_outside_tree()
* Removed use of os.mkdir where appropriate in tests

Revision history for this message
Declan McGrath (declanmg) wrote :

Thanks for the review. I have made the following changes
* Renamed overly verbose error class
* Made all text lines less than 80 chars long
* Added docstring to is_realpath_outside_tree()
* Removed use of os.mkdir where appropriate in tests

I have the following work left to do
* Add a couple of tests directly for is_realpath_outside_tree()

I have the following questions
* When you talk of adding tests directly for is_realpath_outside_tree() I take it that you mean adding more blackbox tests to test_add.py. Is this correct?
* "You should have two blank spaces before the def" Why? Surely there should be no spaces before the def - in the same way as the function that follows it (def tree_files) has no preceding spaces
* Would is_realpath_outside_tree() be better off in osutils.py?

Revision history for this message
Declan McGrath (declanmg) wrote :

More work committed and pushed to branch. Here are some questions

* I have added 2 tests for is_realpath_outside_tree(). To exercise the method I had to 'import bzrlib.builtins' at the top of the tests/blackbox/test_add.py
  - Please advise if this is ok. It may violate the spirit of a blackbox test.

* Alternatively, I can move the method to osutils.py. I was thinking I could reverse the method's logic and rename it to is_realfilepath_in_realfolderpath() to make it more generic (as it doesn't need to be tree specific).

lp:~declanmg/bzr/264275-fix updated
4743. By jack <jack@pixie>

Added tests for is_realpath_outside_tree().

Unmerged revisions

4743. By jack <jack@pixie>

Added tests for is_realpath_outside_tree().

4742. By jack <jack@pixie>

Made some post-review changes as requested (more to do) as follows...

* Renamed overly verbose error class
* Made all text lines less than 80 chars long
* Added docstring to is_realpath_outside_tree()
* Removed use of os.mkdir where appropriate in tests

4741. By jack <jack@pixie>

Tiny refactor. Send for review now.

4740. By jack <jack@pixie>

Moved tests from wip test to test_add.py. Bug 264275 ready for comment.

4739. By jack <jack@pixie>

Fixed typos in tests.

4738. By jack <jack@pixie>

Finished writing test logic.

4737. By jack <jack@pixie>

Added an error test for bug 264275 fix.

4736. By jack <jack@pixie>

Refactored code to reveal intent and tidied tests.

4735. By jack <jack@pixie>

Added a test concerning adding a file under a symlinked dir outside working copy

4734. By Declan McGrath <jack@jack>

Trying to target the specific test case more accurately. Still WIP.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/builtins.py'
2--- bzrlib/builtins.py 2009-11-20 05:54:54 +0000
3+++ bzrlib/builtins.py 2009-11-22 23:34:15 +0000
4@@ -67,6 +67,19 @@
5 )
6 from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level
7
8+def is_realpath_outside_tree(tree_path, filepath):
9+ """True if filepath is outside tree once symlinks are considered.
10+
11+ :param tree_path Path to the tree
12+ :param filepath Path to test if outside tree
13+ """
14+
15+ if not osutils.is_inside(
16+ osutils.realpath(tree_path),
17+ osutils.realpath(filepath)):
18+ return True
19+
20+ return False
21
22 def tree_files(file_list, default_branch=u'.', canonicalize=True,
23 apply_view=True):
24@@ -107,6 +120,18 @@
25 file_list = view_files
26 view_str = views.view_display_str(view_files)
27 note("Ignoring files outside view. View is %s" % view_str)
28+
29+ # Check if any parent dirs are sym-links whose dereferenced
30+ # path is outside the working copy tree
31+ if file_list:
32+ tree_path = tree.basedir
33+ for file in file_list:
34+ parent_directory = os.path.split(file)[0]
35+ if osutils.islink(parent_directory) and is_realpath_outside_tree(
36+ tree_path,
37+ parent_directory):
38+ raise errors.SymLinkFolderContentsOutsideTree(file)
39+
40 return tree, file_list
41
42
43
44=== modified file 'bzrlib/errors.py'
45--- bzrlib/errors.py 2009-08-30 21:34:42 +0000
46+++ bzrlib/errors.py 2009-11-22 23:34:15 +0000
47@@ -548,6 +548,11 @@
48
49 _fmt = 'Hard-linking "%(path)s" is not supported'
50
51+class SymLinkFolderContentsOutsideTree(PathError):
52+
53+ _fmt = ('Adding contents "%(path)s" under a sym-link which points to a '
54+ 'directory outside the working copy is not supported')
55+
56
57 class ReadingCompleted(InternalBzrError):
58
59
60=== modified file 'bzrlib/tests/blackbox/test_add.py'
61--- bzrlib/tests/blackbox/test_add.py 2009-11-08 00:05:26 +0000
62+++ bzrlib/tests/blackbox/test_add.py 2009-11-22 23:34:15 +0000
63@@ -19,6 +19,7 @@
64
65 import os
66
67+import bzrlib.builtins
68 from bzrlib import osutils
69 from bzrlib.tests import (
70 condition_isinstance,
71@@ -224,3 +225,57 @@
72 os.symlink(osutils.abspath('target'), 'tree/link')
73 out = self.run_bzr(['add', 'tree/link'])[0]
74 self.assertEquals(out, 'adding link\n')
75+
76+ def test_add_file_under_symlinked_directory_inside_working_copy(self):
77+ self.requireFeature(SymlinkFeature)
78+ self.make_branch_and_tree('tree')
79+ self.build_tree(['tree/inside_working_copy/',
80+ 'tree/inside_working_copy/a_file.txt'])
81+ os.symlink(osutils.abspath('tree/inside_working_copy'), 'tree/link')
82+ out = self.run_bzr(['add', 'tree/link/a_file.txt'])[0]
83+ self.assertEquals(out, 'adding link\nadding link/a_file.txt\n')
84+
85+ def test_add_file_under_symlinked_directory_outside_working_copy(self):
86+ self.requireFeature(SymlinkFeature)
87+ self.make_branch_and_tree('tree')
88+ self.build_tree(['outside_working_copy/',
89+ 'outside_working_copy/a_file.txt'])
90+ os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
91+ err = self.run_bzr(['add', 'tree/link/a_file.txt'], retcode = 3)[1]
92+ expected = ('bzr: ERROR: Adding contents '
93+ '"' + osutils.getcwd() + '/tree/link/a_file.txt" under a sym-link '
94+ 'which points to a directory outside the working copy is not '
95+ 'supported\n')
96+ self.assertEqual(err, expected)
97+
98+ def test_add_a_symlink_under_symlinked_directory_outside_working_copy(self):
99+ self.requireFeature(SymlinkFeature)
100+ self.make_branch_and_tree('tree')
101+ self.build_tree(['outside_working_copy/'])
102+ os.symlink(osutils.abspath('outside_working_copy/does_not_exit'),
103+ osutils.abspath('outside_working_copy/broken_link'))
104+ os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
105+ err = self.run_bzr(['add', 'tree/link/broken_link'], retcode = 3)[1]
106+ expected = ('bzr: ERROR: Adding contents '
107+ '"' + osutils.getcwd() + '/tree/link/broken_link" under a sym-link '
108+ 'which points to a directory outside the working copy is not '
109+ 'supported\n')
110+ self.assertEqual(err, expected)
111+
112+ def test_realpath_is_outside_tree(self):
113+ self.make_branch_and_tree('tree')
114+ self.build_tree(['containing_folder/',
115+ 'a_file.txt'])
116+ out = bzrlib.builtins.is_realpath_outside_tree(
117+ osutils.abspath('containing_folder/'),
118+ osutils.abspath('a_file.txt'))
119+ self.assertEqual(out, True)
120+
121+ def test_realpath_is_under_tree(self):
122+ self.make_branch_and_tree('tree')
123+ self.build_tree(['tree/containing_folder/',
124+ 'tree/containing_folder/a_file.txt'])
125+ out = bzrlib.builtins.is_realpath_outside_tree(
126+ osutils.abspath('containing_folder/'),
127+ osutils.abspath('containing_folder/a_file.txt'))
128+ self.assertEqual(out, False)
129
130=== modified file 'bzrlib/tests/test_errors.py'
131--- bzrlib/tests/test_errors.py 2009-08-21 02:37:18 +0000
132+++ bzrlib/tests/test_errors.py 2009-11-22 23:34:15 +0000
133@@ -528,6 +528,12 @@
134 "user encoding " + osutils.get_user_encoding(),
135 str(err))
136
137+ def test_unable_to_add_contents_under_symlinked_folder_outside_tree(self):
138+ err = errors.SymLinkFolderContentsOutsideTree('foo')
139+ self.assertEquals(("Adding contents \"foo\" under a sym-link which "
140+ "points to a directory outside the working copy is not supported"),
141+ str(err))
142+
143 def test_unknown_format(self):
144 err = errors.UnknownFormatError('bar', kind='foo')
145 self.assertEquals("Unknown foo format: 'bar'", str(err))