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

[Parth Malwankar]

[Parth Malwankar (parthm)]

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

[Andrew Bennetts (spiv)]

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

[Andrew Bennetts (spiv)]

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

[lp:~parthm/bzr/138600]

5044. By Parth Malwankar on 2010-02-26

updated based on review comments.
NEWS entry to be clearer. run_bzr now uses list for args.

[Parth Malwankar (parthm)]

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

[Parth Malwankar (parthm)]

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.

[Vincent Ladeuil (vila)]

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

[Vincent Ladeuil (vila)]

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

=== modified file 'NEWS'
--- NEWS	2010-02-25 06:17:27 +0000
+++ NEWS	2010-02-26 05:12:20 +0000
@@ -86,9 +86,15 @@
   prevents ``bzr status --short`` from crashing when those files are
   present.  (John Arbash Meinel, #303275)
 
+<<<<<<< TREE
 * Tolerate patches with leading noise in ``bzr-handle-patch``.
   (Toshio Kuratomi, Martin Pool, #502076)
 
+=======
+* ``bzr mkdir DIR`` will not create DIR unless DIR's parent is a versioned
+   directory. (Parth Malwankar, #138600)
+
+>>>>>>> MERGE-SOURCE
 API Changes
 ***********
 

=== 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 @@
 
     def run(self, dir_list):
         for d in dir_list:
-            os.mkdir(d)
             wt, dd = WorkingTree.open_containing(d)
-            wt.add([dd])
-            self.outf.write('added %s\n' % d)
+            base = os.path.dirname(dd)
+            id = wt.path2id(base)
+            if id != None:
+                os.mkdir(d)
+                wt.add([dd])
+                self.outf.write('added %s\n' % d)
+            else:
+                raise errors.NotVersionedError(path=base)
 
 
 class cmd_relpath(Command):

=== 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 @@
 from bzrlib.workingtree import WorkingTree
 
 
+class TestMkdir(TestCaseWithTransport):
+
+    def test_mkdir_in_repo(self):
+        """'mkdir' should fail cleanly withing a repo. Bug #138600"""
+        shared_repo = self.make_repository('./foobar')
+        self.run_bzr(['mkdir', 'foobar/abc'], retcode=3)
+        self.failIfExists('foobar/abc')
+
+    def test_mkdir_outside_unversioned_dir(self):
+        """'mkdir' should fail with unversioned directory in path. Bug #138600"""
+        dir = pathjoin('.', 'unversioned_dir0')
+        newdir = pathjoin(dir, 'abc')
+        os.mkdir(dir)
+        self.run_bzr(['mkdir', newdir], retcode=3)
+        self.failIfExists(newdir)
+
+    def test_mkdir_inside_unversioned_dir(self):
+        """'mkdir' should fail cleanly from within an unversioned directory. Bug #138600"""
+        dir = pathjoin('.', 'unversioned_dir1')
+        os.mkdir(dir)
+        self.run_bzr(['mkdir', 'abc'], working_dir=dir, retcode=3)
+        newdir = pathjoin(dir, 'abc')
+        self.failIfExists(newdir)
+
+    def test_mkdir_outside_unversioned_dir_within_branch(self):
+        """'mkdir' should fail with unversioned directory inside branch. Bug #138600"""
+        b = 'foobar0'
+        self.make_branch_and_tree(b)
+        os.chdir(b)
+        dir = pathjoin('.', 'unversioned_dir0')
+        newdir = pathjoin(dir, 'abc')
+        os.mkdir(dir)
+        self.run_bzr(['mkdir', newdir], retcode=3)
+        self.failIfExists(newdir)
+
+    def test_mkdir_inside_unversioned_dir_within_branch(self):
+        """'mkdir' should fail cleanly from within an unversioned directory
+        inside branch. Bug #138600"""
+        b = 'foobar1'
+        self.make_branch_and_tree(b)
+        os.chdir(b)
+        dir = pathjoin('.', 'unversioned_dir1')
+        os.mkdir(dir)
+        self.run_bzr(['mkdir', 'abc'], working_dir=dir, retcode=3)
+        newdir = pathjoin(dir, 'abc')
+        self.failIfExists(newdir)
+
+
 class TestVersioning(TestCaseInTempDir):
 
     def test_mkdir(self):
         """Basic 'bzr mkdir' operation"""
 
         self.run_bzr('init')
-        self.run_bzr('mkdir foo')
+        self.run_bzr(['mkdir', 'foo'])
         self.assert_(os.path.isdir('foo'))
 
-        self.run_bzr('mkdir foo', retcode=3)
+        self.run_bzr(['mkdir', 'foo'], retcode=3)
 
         wt = WorkingTree.open('.')
 
@@ -54,12 +102,12 @@
         """'bzr mkdir' operation in subdirectory"""
 
         self.run_bzr('init')
-        self.run_bzr('mkdir dir')
+        self.run_bzr(['mkdir', 'dir'])
         self.assert_(os.path.isdir('dir'))
 
         os.chdir('dir')
         self.log('Run mkdir in subdir')
-        self.run_bzr('mkdir subdir')
+        self.run_bzr(['mkdir', 'subdir'])
         self.assert_(os.path.isdir('subdir'))
         os.chdir('..')
 
@@ -86,7 +134,7 @@
         self.run_bzr('init')
         os.chdir('../..')
 
-        self.run_bzr('mkdir dir a/dir a/b/dir')
+        self.run_bzr(['mkdir', 'dir', 'a/dir', 'a/b/dir'])
         self.failUnless(os.path.isdir('dir'))
         self.failUnless(os.path.isdir('a/dir'))
         self.failUnless(os.path.isdir('a/b/dir'))