Bazaar

Merge lp:~parthm/bzr/138600 into lp:bzr

  1. 138600
  2. Merge into bzr.dev
Proposed by Parth Malwankar
Status: Merged
Merged at revision: not available
Proposed branch: lp:~parthm/bzr/138600
Merge into: lp:bzr
Diff against target: 137 lines (+67/-8) (has conflicts)
3 files modified
NEWS (+6/-0)
bzrlib/builtins.py (+8/-3)
bzrlib/tests/blackbox/test_versioning.py (+53/-5)
Text conflict in NEWS
To merge this branch: bzr merge lp:~parthm/bzr/138600
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Andrew Bennetts Approve
Review via email: mp+19471@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Parth Malwankar (parthm) wrote :

This patch fixes bug #138600
Basically bzr was creating the directory before adding it to the tree. If adding failed
the directory still remains. This change takes care of the following scenarios i.e. ensures no directory is created in case of the following failures:

1. mkdir fails in a repository because there is no working tree.
2. 'bzr mkdir foo' fails because '.' is not versioned.
3. 'bzr mkdir bar/foo' fails because 'bar' is not versioned (but '../bar' is versioned).
4. 'cd bar; bzr mkdir foo' fails because 'bar' is not versioned (but '../bar' is versioned).

Revision history for this message
Andrew Bennetts (spiv) wrote :

The NEWS entry is a bit unclear. Perhaps:

* ``bzr mkdir DIR`` will not create DIR unless DIR's parent is a versioned directory. (Parth Malwankar, #138600)

(with appropriate word-wrapping, of course).

The tests look like they cover all the interesting cases, thanks! They are a bit oddly inconsistent cosmetically (e.g. calling run_bzr with 'mkdir abc' vs. ['mkdir', 'abc']), but that's not a big deal.

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

Oh, and this would be reasonable to backport to 2.1 for the 2.1.1 release, I think.

Revision history for this message
Parth Malwankar (parthm) wrote :

> The NEWS entry is a bit unclear. Perhaps:
>
> * ``bzr mkdir DIR`` will not create DIR unless DIR's parent is a versioned
> directory. (Parth Malwankar, #138600)
>
> (with appropriate word-wrapping, of course).
>
> The tests look like they cover all the interesting cases, thanks! They are a
> bit oddly inconsistent cosmetically (e.g. calling run_bzr with 'mkdir abc' vs.
> ['mkdir', 'abc']), but that's not a big deal.

Thanks for reviewing this Andrew. I have done the updates suggested.
Will create a branch and send a merge request against 2.1.

Revision history for this message
Parth Malwankar (parthm) wrote :

It seems I (unintentionally, %s/// :-)) ended up mkdir run_bzr take a list argument across test_versioning.py (I have added only TestMkdir). I suppose thats a desired change. The tests are certainly passing.

Revision history for this message
Vincent Ladeuil (vila) wrote :

51 +class TestMkdir(TestCaseWithTransport):
52 +

use a safety bzr repo to guard against leaking tests (that may try
to write in a bzr containing repo, outside of their environment,
see bzrlib.tests.TestCaseWithMemoryTransport._check_safety_net).

So:

59 + def test_mkdir_outside_unversioned_dir(self):
60 + """'mkdir' should fail with unversioned directory in path. Bug #138600"""

67 + def test_mkdir_inside_unversioned_dir(self):
68 + """'mkdir' should fail cleanly from within an unversioned directory. Bug #138600"""

and

75 + def test_mkdir_outside_unversioned_dir_within_branch(self):
76 + """'mkdir' should fail with unversioned directory inside branch. Bug #138600"""

don't test what you think they test, since they are all inside a valid branch.

86 + def test_mkdir_inside_unversioned_dir_within_branch(self):
87 + """'mkdir' should fail cleanly from within an unversioned directory

is valid, but don't need to create a branch since you're working outside of this created branch
anyway.

Since we requires a working tree,

53 + def test_mkdir_in_repo(self):
54 + """'mkdir' should fail cleanly withing a repo. Bug #138600"""

is enough to test your change since we indeed raise NotVersionedError here.

I'll delete these tests and merge.

I'll delete them and merge.

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

Bah, it's neither a safety repo nor branch it's a full safety working tree, sorry for the imprecise comments.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-02-25 06:17:27 +0000
3+++ NEWS 2010-02-26 05:12:20 +0000
4@@ -86,9 +86,15 @@
5 prevents ``bzr status --short`` from crashing when those files are
6 present. (John Arbash Meinel, #303275)
7
8+<<<<<<< TREE
9 * Tolerate patches with leading noise in ``bzr-handle-patch``.
10 (Toshio Kuratomi, Martin Pool, #502076)
11
12+=======
13+* ``bzr mkdir DIR`` will not create DIR unless DIR's parent is a versioned
14+ directory. (Parth Malwankar, #138600)
15+
16+>>>>>>> MERGE-SOURCE
17 API Changes
18 ***********
19
20
21=== modified file 'bzrlib/builtins.py'
22--- bzrlib/builtins.py 2010-02-23 07:43:11 +0000
23+++ bzrlib/builtins.py 2010-02-26 05:12:20 +0000
24@@ -699,10 +699,15 @@
25
26 def run(self, dir_list):
27 for d in dir_list:
28- os.mkdir(d)
29 wt, dd = WorkingTree.open_containing(d)
30- wt.add([dd])
31- self.outf.write('added %s\n' % d)
32+ base = os.path.dirname(dd)
33+ id = wt.path2id(base)
34+ if id != None:
35+ os.mkdir(d)
36+ wt.add([dd])
37+ self.outf.write('added %s\n' % d)
38+ else:
39+ raise errors.NotVersionedError(path=base)
40
41
42 class cmd_relpath(Command):
43
44=== modified file 'bzrlib/tests/blackbox/test_versioning.py'
45--- bzrlib/tests/blackbox/test_versioning.py 2009-08-28 05:00:33 +0000
46+++ bzrlib/tests/blackbox/test_versioning.py 2010-02-26 05:12:19 +0000
47@@ -29,16 +29,64 @@
48 from bzrlib.workingtree import WorkingTree
49
50
51+class TestMkdir(TestCaseWithTransport):
52+
53+ def test_mkdir_in_repo(self):
54+ """'mkdir' should fail cleanly withing a repo. Bug #138600"""
55+ shared_repo = self.make_repository('./foobar')
56+ self.run_bzr(['mkdir', 'foobar/abc'], retcode=3)
57+ self.failIfExists('foobar/abc')
58+
59+ def test_mkdir_outside_unversioned_dir(self):
60+ """'mkdir' should fail with unversioned directory in path. Bug #138600"""
61+ dir = pathjoin('.', 'unversioned_dir0')
62+ newdir = pathjoin(dir, 'abc')
63+ os.mkdir(dir)
64+ self.run_bzr(['mkdir', newdir], retcode=3)
65+ self.failIfExists(newdir)
66+
67+ def test_mkdir_inside_unversioned_dir(self):
68+ """'mkdir' should fail cleanly from within an unversioned directory. Bug #138600"""
69+ dir = pathjoin('.', 'unversioned_dir1')
70+ os.mkdir(dir)
71+ self.run_bzr(['mkdir', 'abc'], working_dir=dir, retcode=3)
72+ newdir = pathjoin(dir, 'abc')
73+ self.failIfExists(newdir)
74+
75+ def test_mkdir_outside_unversioned_dir_within_branch(self):
76+ """'mkdir' should fail with unversioned directory inside branch. Bug #138600"""
77+ b = 'foobar0'
78+ self.make_branch_and_tree(b)
79+ os.chdir(b)
80+ dir = pathjoin('.', 'unversioned_dir0')
81+ newdir = pathjoin(dir, 'abc')
82+ os.mkdir(dir)
83+ self.run_bzr(['mkdir', newdir], retcode=3)
84+ self.failIfExists(newdir)
85+
86+ def test_mkdir_inside_unversioned_dir_within_branch(self):
87+ """'mkdir' should fail cleanly from within an unversioned directory
88+ inside branch. Bug #138600"""
89+ b = 'foobar1'
90+ self.make_branch_and_tree(b)
91+ os.chdir(b)
92+ dir = pathjoin('.', 'unversioned_dir1')
93+ os.mkdir(dir)
94+ self.run_bzr(['mkdir', 'abc'], working_dir=dir, retcode=3)
95+ newdir = pathjoin(dir, 'abc')
96+ self.failIfExists(newdir)
97+
98+
99 class TestVersioning(TestCaseInTempDir):
100
101 def test_mkdir(self):
102 """Basic 'bzr mkdir' operation"""
103
104 self.run_bzr('init')
105- self.run_bzr('mkdir foo')
106+ self.run_bzr(['mkdir', 'foo'])
107 self.assert_(os.path.isdir('foo'))
108
109- self.run_bzr('mkdir foo', retcode=3)
110+ self.run_bzr(['mkdir', 'foo'], retcode=3)
111
112 wt = WorkingTree.open('.')
113
114@@ -54,12 +102,12 @@
115 """'bzr mkdir' operation in subdirectory"""
116
117 self.run_bzr('init')
118- self.run_bzr('mkdir dir')
119+ self.run_bzr(['mkdir', 'dir'])
120 self.assert_(os.path.isdir('dir'))
121
122 os.chdir('dir')
123 self.log('Run mkdir in subdir')
124- self.run_bzr('mkdir subdir')
125+ self.run_bzr(['mkdir', 'subdir'])
126 self.assert_(os.path.isdir('subdir'))
127 os.chdir('..')
128
129@@ -86,7 +134,7 @@
130 self.run_bzr('init')
131 os.chdir('../..')
132
133- self.run_bzr('mkdir dir a/dir a/b/dir')
134+ self.run_bzr(['mkdir', 'dir', 'a/dir', 'a/b/dir'])
135 self.failUnless(os.path.isdir('dir'))
136 self.failUnless(os.path.isdir('a/dir'))
137 self.failUnless(os.path.isdir('a/b/dir'))