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
=== modified file 'NEWS'
--- NEWS 2010-02-25 06:17:27 +0000
+++ NEWS 2010-02-26 05:12:20 +0000
@@ -86,9 +86,15 @@
86 prevents ``bzr status --short`` from crashing when those files are86 prevents ``bzr status --short`` from crashing when those files are
87 present. (John Arbash Meinel, #303275)87 present. (John Arbash Meinel, #303275)
8888
89<<<<<<< TREE
89* Tolerate patches with leading noise in ``bzr-handle-patch``.90* Tolerate patches with leading noise in ``bzr-handle-patch``.
90 (Toshio Kuratomi, Martin Pool, #502076)91 (Toshio Kuratomi, Martin Pool, #502076)
9192
93=======
94* ``bzr mkdir DIR`` will not create DIR unless DIR's parent is a versioned
95 directory. (Parth Malwankar, #138600)
96
97>>>>>>> MERGE-SOURCE
92API Changes98API Changes
93***********99***********
94100
95101
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2010-02-23 07:43:11 +0000
+++ bzrlib/builtins.py 2010-02-26 05:12:20 +0000
@@ -699,10 +699,15 @@
699699
700 def run(self, dir_list):700 def run(self, dir_list):
701 for d in dir_list:701 for d in dir_list:
702 os.mkdir(d)
703 wt, dd = WorkingTree.open_containing(d)702 wt, dd = WorkingTree.open_containing(d)
704 wt.add([dd])703 base = os.path.dirname(dd)
705 self.outf.write('added %s\n' % d)704 id = wt.path2id(base)
705 if id != None:
706 os.mkdir(d)
707 wt.add([dd])
708 self.outf.write('added %s\n' % d)
709 else:
710 raise errors.NotVersionedError(path=base)
706711
707712
708class cmd_relpath(Command):713class cmd_relpath(Command):
709714
=== modified file 'bzrlib/tests/blackbox/test_versioning.py'
--- bzrlib/tests/blackbox/test_versioning.py 2009-08-28 05:00:33 +0000
+++ bzrlib/tests/blackbox/test_versioning.py 2010-02-26 05:12:19 +0000
@@ -29,16 +29,64 @@
29from bzrlib.workingtree import WorkingTree29from bzrlib.workingtree import WorkingTree
3030
3131
32class TestMkdir(TestCaseWithTransport):
33
34 def test_mkdir_in_repo(self):
35 """'mkdir' should fail cleanly withing a repo. Bug #138600"""
36 shared_repo = self.make_repository('./foobar')
37 self.run_bzr(['mkdir', 'foobar/abc'], retcode=3)
38 self.failIfExists('foobar/abc')
39
40 def test_mkdir_outside_unversioned_dir(self):
41 """'mkdir' should fail with unversioned directory in path. Bug #138600"""
42 dir = pathjoin('.', 'unversioned_dir0')
43 newdir = pathjoin(dir, 'abc')
44 os.mkdir(dir)
45 self.run_bzr(['mkdir', newdir], retcode=3)
46 self.failIfExists(newdir)
47
48 def test_mkdir_inside_unversioned_dir(self):
49 """'mkdir' should fail cleanly from within an unversioned directory. Bug #138600"""
50 dir = pathjoin('.', 'unversioned_dir1')
51 os.mkdir(dir)
52 self.run_bzr(['mkdir', 'abc'], working_dir=dir, retcode=3)
53 newdir = pathjoin(dir, 'abc')
54 self.failIfExists(newdir)
55
56 def test_mkdir_outside_unversioned_dir_within_branch(self):
57 """'mkdir' should fail with unversioned directory inside branch. Bug #138600"""
58 b = 'foobar0'
59 self.make_branch_and_tree(b)
60 os.chdir(b)
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_within_branch(self):
68 """'mkdir' should fail cleanly from within an unversioned directory
69 inside branch. Bug #138600"""
70 b = 'foobar1'
71 self.make_branch_and_tree(b)
72 os.chdir(b)
73 dir = pathjoin('.', 'unversioned_dir1')
74 os.mkdir(dir)
75 self.run_bzr(['mkdir', 'abc'], working_dir=dir, retcode=3)
76 newdir = pathjoin(dir, 'abc')
77 self.failIfExists(newdir)
78
79
32class TestVersioning(TestCaseInTempDir):80class TestVersioning(TestCaseInTempDir):
3381
34 def test_mkdir(self):82 def test_mkdir(self):
35 """Basic 'bzr mkdir' operation"""83 """Basic 'bzr mkdir' operation"""
3684
37 self.run_bzr('init')85 self.run_bzr('init')
38 self.run_bzr('mkdir foo')86 self.run_bzr(['mkdir', 'foo'])
39 self.assert_(os.path.isdir('foo'))87 self.assert_(os.path.isdir('foo'))
4088
41 self.run_bzr('mkdir foo', retcode=3)89 self.run_bzr(['mkdir', 'foo'], retcode=3)
4290
43 wt = WorkingTree.open('.')91 wt = WorkingTree.open('.')
4492
@@ -54,12 +102,12 @@
54 """'bzr mkdir' operation in subdirectory"""102 """'bzr mkdir' operation in subdirectory"""
55103
56 self.run_bzr('init')104 self.run_bzr('init')
57 self.run_bzr('mkdir dir')105 self.run_bzr(['mkdir', 'dir'])
58 self.assert_(os.path.isdir('dir'))106 self.assert_(os.path.isdir('dir'))
59107
60 os.chdir('dir')108 os.chdir('dir')
61 self.log('Run mkdir in subdir')109 self.log('Run mkdir in subdir')
62 self.run_bzr('mkdir subdir')110 self.run_bzr(['mkdir', 'subdir'])
63 self.assert_(os.path.isdir('subdir'))111 self.assert_(os.path.isdir('subdir'))
64 os.chdir('..')112 os.chdir('..')
65113
@@ -86,7 +134,7 @@
86 self.run_bzr('init')134 self.run_bzr('init')
87 os.chdir('../..')135 os.chdir('../..')
88136
89 self.run_bzr('mkdir dir a/dir a/b/dir')137 self.run_bzr(['mkdir', 'dir', 'a/dir', 'a/b/dir'])
90 self.failUnless(os.path.isdir('dir'))138 self.failUnless(os.path.isdir('dir'))
91 self.failUnless(os.path.isdir('a/dir'))139 self.failUnless(os.path.isdir('a/dir'))
92 self.failUnless(os.path.isdir('a/b/dir'))140 self.failUnless(os.path.isdir('a/b/dir'))