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
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2011-11-30 20:06:59 +0000
+++ bzrlib/builtins.py 2011-12-11 02:34:29 +0000
@@ -753,20 +753,40 @@
753 """753 """
754754
755 takes_args = ['dir+']755 takes_args = ['dir+']
756 takes_options = [
757 Option(
758 'parents',
759 help='No error if existing, make parent directories as needed.',
760 short_name='p'
761 )
762 ]
756 encoding_type = 'replace'763 encoding_type = 'replace'
757764
758 def run(self, dir_list):765 @classmethod
759 for d in dir_list:766 def add_file_with_parents(cls, wt, relpath):
760 wt, dd = WorkingTree.open_containing(d)767 if wt.path2id(relpath) is not None:
761 base = os.path.dirname(dd)768 return
762 id = wt.path2id(base)769 cls.add_file_with_parents(wt, osutils.dirname(relpath))
763 if id != None:770 wt.add([relpath])
764 os.mkdir(d)771
765 wt.add([dd])772 @classmethod
766 if not is_quiet():773 def add_file_single(cls, wt, relpath):
767 self.outf.write(gettext('added %s\n') % d)774 wt.add([relpath])
775
776 def run(self, dir_list, parents=False):
777 if parents:
778 add_file = self.add_file_with_parents
779 else:
780 add_file = self.add_file_single
781 for dir in dir_list:
782 wt, relpath = WorkingTree.open_containing(dir)
783 if parents:
784 os.makedirs(dir)
768 else:785 else:
769 raise errors.NotVersionedError(path=base)786 os.mkdir(dir)
787 add_file(wt, relpath)
788 if not is_quiet():
789 self.outf.write(gettext('added %s\n') % dir)
770790
771791
772class cmd_relpath(Command):792class cmd_relpath(Command):
773793
=== modified file 'bzrlib/tests/blackbox/__init__.py'
--- bzrlib/tests/blackbox/__init__.py 2011-09-27 12:13:53 +0000
+++ bzrlib/tests/blackbox/__init__.py 2011-12-11 02:34:29 +0000
@@ -85,6 +85,7 @@
85 'test_merge',85 'test_merge',
86 'test_merge_directive',86 'test_merge_directive',
87 'test_missing',87 'test_missing',
88 'test_mkdir',
88 'test_modified',89 'test_modified',
89 'test_mv',90 'test_mv',
90 'test_nick',91 'test_nick',
9192
=== added file 'bzrlib/tests/blackbox/test_mkdir.py'
--- bzrlib/tests/blackbox/test_mkdir.py 1970-01-01 00:00:00 +0000
+++ bzrlib/tests/blackbox/test_mkdir.py 2011-12-11 02:34:29 +0000
@@ -0,0 +1,48 @@
1# Copyright (C) 2011 Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17
18"""Black-box tests for bzr mkdir.
19"""
20
21import os
22from bzrlib import tests
23
24
25class TestMkdir(tests.TestCaseWithTransport):
26
27 def test_mkdir(self):
28 tree = self.make_branch_and_tree('.')
29 self.run_bzr(['mkdir', 'somedir'])
30
31 self.assertEquals(tree.kind(tree.path2id('somedir')), "directory")
32
33 def test_mkdir_multi(self):
34 tree = self.make_branch_and_tree('.')
35 self.run_bzr(['mkdir', 'somedir', 'anotherdir'])
36 self.assertEquals(tree.kind(tree.path2id('somedir')), "directory")
37 self.assertEquals(tree.kind(tree.path2id('anotherdir')), "directory")
38
39 def test_mkdir_parents(self):
40 tree = self.make_branch_and_tree('.')
41 self.run_bzr(['mkdir', '-p', 'somedir/foo'])
42 self.assertEquals(tree.kind(tree.path2id('somedir/foo')), "directory")
43
44 def test_mkdir_parents_with_unversioned_parent(self):
45 tree = self.make_branch_and_tree('.')
46 os.mkdir('somedir')
47 self.run_bzr(['mkdir', '-p', 'somedir/foo'])
48 self.assertEquals(tree.kind(tree.path2id('somedir/foo')), "directory")
049
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt 2011-12-09 12:04:25 +0000
+++ doc/en/release-notes/bzr-2.5.txt 2011-12-11 02:34:29 +0000
@@ -20,6 +20,10 @@
2020
21.. New commands, options, etc that users may wish to try out.21.. New commands, options, etc that users may wish to try out.
2222
23* "bzr mkdir" now includes -p (--parents) option for recursively adding
24 parent directories.
25 (Jared Hance, Jelmer Vernooij, #253529)
26
23Improvements27Improvements
24************28************
2529