Bazaar

Merge lp:~jelmer/bzr/mkdir-p into lp:bzr

  1. mkdir-p
  2. Merge into bzr.dev
Proposed by Jelmer Vernooij
Status: Merged
Approved by: Martin Packman
Approved revision: no longer in the source branch.
Merged at revision: 6357
Proposed branch: lp:~jelmer/bzr/mkdir-p
Merge into: lp:bzr
Diff against target: 134 lines (+84/-11)
4 files modified
bzrlib/builtins.py (+31/-11)
bzrlib/tests/blackbox/__init__.py (+1/-0)
bzrlib/tests/blackbox/test_mkdir.py (+48/-0)
doc/en/release-notes/bzr-2.5.txt (+4/-0)
To merge this branch: bzr merge lp:~jelmer/bzr/mkdir-p
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
Ian Clatworthy Pending
Martin Pool Pending
Review via email: mp+85237@code.launchpad.net

This proposal supersedes a proposal from 2009-10-14.

Commit message

Add -p option to 'bzr mkdir'.

Description of the change

This fixes bug #253529 (https://bugs.edge.launchpad.net/bzr/+bug/253529) by providing a -p (--parents) option for "bzr mkdir"

To post a comment you must log in.
Revision history for this message
Jared Hance (mathepic) wrote : Posted in a previous version of this proposal

This fixes bug #253529 (https://bugs.edge.launchpad.net/bzr/+bug/253529) by providing a -p (--parents) option for "bzr mkdir"

Revision history for this message
Jared Hance (mathepic) wrote : Posted in a previous version of this proposal

Don't merge this yet, I need to make an alternative for smart_add since it adds the files in the directory, something I forgot about in the original implementation.

Revision history for this message
Jared Hance (mathepic) wrote : Posted in a previous version of this proposal

> Don't merge this yet, I need to make an alternative for smart_add since it
> adds the files in the directory, something I forgot about in the original
> implementation.

Actually, I just checked and "bzr add" does not add files in the parent directories of the target directories. Since the target directory is always empty, it should not be a problem.

Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote : Posted in a previous version of this proposal

Thanks for the patch. Some minor things to fix ...

1. This is a boolean option so the parameter ought to be declared as "parent=False" instead of "parent=None".

2. It should also be tested as simply "if parents" instead of testing against None.

3. You'll need to tweak the help argument in the Option declaration so that it starts with a capital letter and ends with a full stop. Otherwise, it won't merge because our style checker will fall over!

BTW, "bzr init" has a "create-prefix" option that does a very similar thing. I think consistency with posix mkdir (as you've done) makes more sense. Maybe we ought to change init down the track to be consistent with this? If so, it's outside the scope of this patch. I was just mentioning it for completeness.

review: Needs Fixing
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote : Posted in a previous version of this proposal

Ian Clatworthy wrote:
> Review: Needs Fixing
> Thanks for the patch. Some minor things to fix ...
>
> 1. This is a boolean option so the parameter ought to be declared as "parent=False" instead of "parent=None".

Sorry. I meant "parents=False".

Ian C.

Revision history for this message
Jared Hance (mathepic) wrote : Posted in a previous version of this proposal

Okay, fixed.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

What do you mean by "No error if existing, "? If the target directory exists, os.makedirs will error. And because of the way you rely on smart_add, this is probably a good thing?

I will tweak that text and land it.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal
Download full text (3.6 KiB)

This seems to cause a failure like so

blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir FAIL 54ms
   Unexpected return code
not equal:
a = 0
b = 3

======================================================================
FAIL: test_mkdir_in_subdir (bzrlib.tests.blackbox.test_versioning.TestVersioning)

vvvv[log from bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir]
329.240 run bzr: ['init']
329.240 bzr arguments: ['init']
329.243 encoding stdout as sys.stdout encoding 'UTF-8'
329.250 creating repository in file:///tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work/.bzr/.
329.253 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x69da250> in file:///tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work/.bzr/
329.263 trying to create missing lock '/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work/.bzr/checkout/dirstate'
329.263 opening working tree '/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work'
329.270 opening working tree '/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work'
329.272 output:
'Created a standalone tree (format: 2a)\n'
329.272 run bzr: ['mkdir', 'dir']
329.272 bzr arguments: ['mkdir', 'dir']
329.274 encoding stdout as sys.stdout encoding 'UTF-8'
329.278 opening working tree '/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work'
329.283 output:
'added dir\n'
329.283 Run mkdir in subdir
329.283 run bzr: ['mkdir', 'subdir']
329.283 bzr arguments: ['mkdir', 'subdir']
329.285 encoding stdout as sys.stdout encoding 'UTF-8'
329.289 opening working tree '/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work'
329.292 Traceback (most recent call last):
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/commands.py", line 1140, in run_bzr_catch_user_errors
   return run_bzr(argv)
make: *** [check-nodocs] Error 1
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/commands.py", line 1038, in run_bzr
   ret = run(*run_argv)
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/commands.py", line 655, in run_argv_aliases
   return self.run(**all_cmd_args)
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/builtins.py", line 706, in run
   wt.smart_add([dd])
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/mutabletree.py", line 49, in tree_write_locked
   return unbound(self, *args, **kwargs)
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/mutabletree.py", line 403, in smart_add
   kind = osutils.file_kind(abspath)
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/osutils.py", line 1860, in file_kind
   raise errors.NoSuchFile(f)
NoSuchFile: No such file: u'/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_subdir/work/dir/dir/subdir'

329.293 errors:
"bzr: ERROR: No such file: u'/tmp/testbzr-oemsyS.tmp/bzrlib.tests.blackbox.test_versioning.TestVersioning.test_mkdir_in_s...

Read more...

review: Needs Fixing
Revision history for this message
Martin Packman (gz) wrote :

Good job picking up this.

Amusing (though... not actually related), I've just been reading this in the make documentation: "For example, don't use ‘mkdir -p’, convenient as it may be, because a few systems don't support it at all and with others, it is not safe for parallel execution."

Style nit with the code, rather than complicating the simple run() method with nested functions that don't need to close over anything, I'd prefer if they were @staticmethod or something on the class.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

> Good job picking up this.
>
> Amusing (though... not actually related), I've just been reading this in the
> make documentation: "For example, don't use ‘mkdir -p’, convenient as it may
> be, because a few systems don't support it at all and with others, it is not
> safe for parallel execution."
:-) I guess the same will be true for older versions of bzr...

> Style nit with the code, rather than complicating the simple run() method with
> nested functions that don't need to close over anything, I'd prefer if they
> were @staticmethod or something on the class.
Fixed.

Revision history for this message
Martin Packman (gz) wrote :

All seems fine.

+ help='No error if existing, make parent directories as needed.',

This isn't actually correct? There aren't any tests for trying to create existing dirs, but I don't see the harm in throwing an error.

review: Approve
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

sent to pqm by email

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 2011-11-30 20:06:59 +0000
3+++ bzrlib/builtins.py 2011-12-11 02:34:29 +0000
4@@ -753,20 +753,40 @@
5 """
6
7 takes_args = ['dir+']
8+ takes_options = [
9+ Option(
10+ 'parents',
11+ help='No error if existing, make parent directories as needed.',
12+ short_name='p'
13+ )
14+ ]
15 encoding_type = 'replace'
16
17- def run(self, dir_list):
18- for d in dir_list:
19- wt, dd = WorkingTree.open_containing(d)
20- base = os.path.dirname(dd)
21- id = wt.path2id(base)
22- if id != None:
23- os.mkdir(d)
24- wt.add([dd])
25- if not is_quiet():
26- self.outf.write(gettext('added %s\n') % d)
27+ @classmethod
28+ def add_file_with_parents(cls, wt, relpath):
29+ if wt.path2id(relpath) is not None:
30+ return
31+ cls.add_file_with_parents(wt, osutils.dirname(relpath))
32+ wt.add([relpath])
33+
34+ @classmethod
35+ def add_file_single(cls, wt, relpath):
36+ wt.add([relpath])
37+
38+ def run(self, dir_list, parents=False):
39+ if parents:
40+ add_file = self.add_file_with_parents
41+ else:
42+ add_file = self.add_file_single
43+ for dir in dir_list:
44+ wt, relpath = WorkingTree.open_containing(dir)
45+ if parents:
46+ os.makedirs(dir)
47 else:
48- raise errors.NotVersionedError(path=base)
49+ os.mkdir(dir)
50+ add_file(wt, relpath)
51+ if not is_quiet():
52+ self.outf.write(gettext('added %s\n') % dir)
53
54
55 class cmd_relpath(Command):
56
57=== modified file 'bzrlib/tests/blackbox/__init__.py'
58--- bzrlib/tests/blackbox/__init__.py 2011-09-27 12:13:53 +0000
59+++ bzrlib/tests/blackbox/__init__.py 2011-12-11 02:34:29 +0000
60@@ -85,6 +85,7 @@
61 'test_merge',
62 'test_merge_directive',
63 'test_missing',
64+ 'test_mkdir',
65 'test_modified',
66 'test_mv',
67 'test_nick',
68
69=== added file 'bzrlib/tests/blackbox/test_mkdir.py'
70--- bzrlib/tests/blackbox/test_mkdir.py 1970-01-01 00:00:00 +0000
71+++ bzrlib/tests/blackbox/test_mkdir.py 2011-12-11 02:34:29 +0000
72@@ -0,0 +1,48 @@
73+# Copyright (C) 2011 Canonical Ltd
74+#
75+# This program is free software; you can redistribute it and/or modify
76+# it under the terms of the GNU General Public License as published by
77+# the Free Software Foundation; either version 2 of the License, or
78+# (at your option) any later version.
79+#
80+# This program is distributed in the hope that it will be useful,
81+# but WITHOUT ANY WARRANTY; without even the implied warranty of
82+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
83+# GNU General Public License for more details.
84+#
85+# You should have received a copy of the GNU General Public License
86+# along with this program; if not, write to the Free Software
87+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
88+
89+
90+"""Black-box tests for bzr mkdir.
91+"""
92+
93+import os
94+from bzrlib import tests
95+
96+
97+class TestMkdir(tests.TestCaseWithTransport):
98+
99+ def test_mkdir(self):
100+ tree = self.make_branch_and_tree('.')
101+ self.run_bzr(['mkdir', 'somedir'])
102+
103+ self.assertEquals(tree.kind(tree.path2id('somedir')), "directory")
104+
105+ def test_mkdir_multi(self):
106+ tree = self.make_branch_and_tree('.')
107+ self.run_bzr(['mkdir', 'somedir', 'anotherdir'])
108+ self.assertEquals(tree.kind(tree.path2id('somedir')), "directory")
109+ self.assertEquals(tree.kind(tree.path2id('anotherdir')), "directory")
110+
111+ def test_mkdir_parents(self):
112+ tree = self.make_branch_and_tree('.')
113+ self.run_bzr(['mkdir', '-p', 'somedir/foo'])
114+ self.assertEquals(tree.kind(tree.path2id('somedir/foo')), "directory")
115+
116+ def test_mkdir_parents_with_unversioned_parent(self):
117+ tree = self.make_branch_and_tree('.')
118+ os.mkdir('somedir')
119+ self.run_bzr(['mkdir', '-p', 'somedir/foo'])
120+ self.assertEquals(tree.kind(tree.path2id('somedir/foo')), "directory")
121
122=== modified file 'doc/en/release-notes/bzr-2.5.txt'
123--- doc/en/release-notes/bzr-2.5.txt 2011-12-09 12:04:25 +0000
124+++ doc/en/release-notes/bzr-2.5.txt 2011-12-11 02:34:29 +0000
125@@ -20,6 +20,10 @@
126
127 .. New commands, options, etc that users may wish to try out.
128
129+* "bzr mkdir" now includes -p (--parents) option for recursively adding
130+ parent directories.
131+ (Jared Hance, Jelmer Vernooij, #253529)
132+
133 Improvements
134 ************
135