Merge lp:~mathepic/bzr/mkdir-recursive into lp:bzr

Proposed by Jared Hance
Status: Superseded
Proposed branch: lp:~mathepic/bzr/mkdir-recursive
Merge into: lp:bzr
Diff against target: 49 lines
2 files modified
NEWS (+4/-0)
bzrlib/builtins.py (+19/-3)
To merge this branch: bzr merge lp:~mathepic/bzr/mkdir-recursive
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Ian Clatworthy Needs Fixing
bzr-core Pending
Review via email: mp+13380@code.launchpad.net

This proposal has been superseded by a proposal from 2011-12-10.

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

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 :

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 :

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

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 :

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 :

Okay, fixed.

Revision history for this message
Martin Pool (mbp) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-10-23 16:31:03 +0000
3+++ NEWS 2009-10-25 13:31:12 +0000
4@@ -22,6 +22,10 @@
5 Bug Fixes
6 *********
7
8+* "bzr mkdir" now includes -p (--parents) option for recursively adding
9+ parent directories.
10+ (Jared Hance, #253529)
11+
12 * ``bzr+http`` servers no longer give spurious jail break errors when
13 serving branches inside a shared repository. (Andrew Bennetts, #348308)
14
15
16=== modified file 'bzrlib/builtins.py'
17--- bzrlib/builtins.py 2009-10-12 22:09:19 +0000
18+++ bzrlib/builtins.py 2009-10-25 13:31:12 +0000
19@@ -677,13 +677,29 @@
20 """
21
22 takes_args = ['dir+']
23+ takes_options = [
24+ Option(
25+ 'parents',
26+ help='No error if existing, make parent directories as needed.',
27+ short_name='p'
28+ )
29+ ]
30 encoding_type = 'replace'
31
32- def run(self, dir_list):
33+ def run(self, dir_list, parents=False):
34 for d in dir_list:
35- os.mkdir(d)
36+ if parents:
37+ os.mkdir(d)
38+ else:
39+ os.makedirs(d)
40+
41 wt, dd = WorkingTree.open_containing(d)
42- wt.add([dd])
43+
44+ # This should work since the target directory of "bzr mkdir"
45+ # is always empty - Therefore, no files except the directory
46+ # will be added, making this equivalent to recursively adding
47+ # the directories, if needed.
48+ wt.smart_add([dd])
49 self.outf.write('added %s\n' % d)
50
51