Merge lp:~junak-michal/bzr/giveback into lp:bzr

[Michal Junák]

[Michal Junák (junak-michal)]

#511987 bugfix

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Michal Junák wrote:
> Michal Junák has proposed merging lp:~junak-michal/bzr/giveback into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> #511987 bugfix
>

I think there are some logic failures in this patch, though it is
getting closer.

1) isinstance(subdir_object, InventoryFile) is probably better done as:
  if subdir_object.kind == 'file'

2) inv.path2id(path) will return None if the object isn't present, so
you don't need a 'has_filename' check on top of getting the file_id.
(you can do "if file_id is None" instead.)

3) The docstring should be updated. It clearly states that 'subdir' is
meant to be the path to a directory. So it should probably say ":param
subdir: None or the path of an entry to start exporting.

4) This doesn't handle symlinks, and it seems reasonably easy to do so.
Namely, instead of checking if the object is a file, you could instead
check if the object is not a directory. So:

if not isinstance(...)

would become

if subdir_object is not None and subdir_object.kind != 'directory':
  yield, return
else:
  entries = ...

 review: needs_fixing

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktirt8ACgkQJdeBCYSNAAPqqACghwnPBXwjMp6UHD8Fvl1h/7t9
jPkAoM58UhjBuScNpHspp6HGE0deHw+Z
=+A6y
-----END PGP SIGNATURE-----

review: Needs Fixing

[Robert Collins (lifeless)]

In addition to John's comments:

Please don't add unnecessary vertical whitespace here:

     """
+
     inv = tree.inventory
     if subdir is None:
- s

and here:

+ yield subdir_object.name, subdir_object
+ return
+
     if subdir is None:

Thanks.
-Rob

[Michal Junák (junak-michal)]

> I think there are some logic failures in this patch, though it is
> getting closer.
>
> 1) isinstance(subdir_object, InventoryFile) is probably better done as:
> if subdir_object.kind == 'file'
>
> 2) inv.path2id(path) will return None if the object isn't present, so
> you don't need a 'has_filename' check on top of getting the file_id.
> (you can do "if file_id is None" instead.)
>
> 3) The docstring should be updated. It clearly states that 'subdir' is
> meant to be the path to a directory. So it should probably say ":param
> subdir: None or the path of an entry to start exporting.
>
> 4) This doesn't handle symlinks, and it seems reasonably easy to do so.
> Namely, instead of checking if the object is a file, you could instead
> check if the object is not a directory. So:
>
> if not isinstance(...)
>
> would become
>
> if subdir_object is not None and subdir_object.kind != 'directory':
> yield, return
> else:
> entries = ...
>
> review: needs_fixing
>
> John
> =:->

2) Well, it was stupid to search through twice. None-check added.
3) Docstring changed. I was thinking of renaming subdir variable too, but no better name crossed my mind.
4) Even though exporting the symlink won't cause a crash in the new version, if someone exports the symlink, they may not get what they anticipated.

And whitespaces deleted

[Michal Junák (junak-michal)]

I' ve repaired the bugfix by your suggestions. I' am new in launchpad, but I hope that resubmitting my proposal is the right way to let you know.

[Ian Clatworthy (ian-clatworthy)]

This looks ok now. BTW, it's good to add an entry to NEWS taking credit for fixing bugs and making improvements. In this case, I'll add one for you while I'm merging (after it gets a second approval).

review: Approve

[Martin Pool (mbp)]

looks good. contributor agreement done?

review: Approve

[Vincent Ladeuil (vila)]

@Michal,

Thanks for your patch, can you please follow the instructions at
http://www.canonical.com/contributors so we can merge it ?

review: Needs Information

[John A Meinel (jameinel)]

We should merge lp:~jameinel/bzr/export-file-511987

I merged with bzr.dev, resolved a small conflict in test_export, and added a NEWS entry to give Michal credit for the bugfix.

Just waiting on the contributor agreement getting signed.

review: Approve

[Michal Junák (junak-michal)]

Shame on me, I haven't noticed I have to accept contributor agreement, sorry for that. I did it by sending email as Vincent advised me, I hope it is alright now.

[Vincent Ladeuil (vila)]

Martin, did you get Michal mail regarding the contributor agreement ?
It doesn't appear to be a https://edge.launchpad.net/~contributor-agreement-canonical member yet.

review: Needs Information

[Martin Pool (mbp)]

2010/2/6 Michal Junák <email address hidden>:
> Shame on me, I haven't noticed I have to accept contributor agreement, sorry for that. I did it by sending email as Vincent advised me, I hope it is alright now.

Sorry Michal, I didn't get that. Did you cc me?

--
Martin <http://launchpad.net/~mbp/>

=== modified file 'bzrlib/export/__init__.py'
--- bzrlib/export/__init__.py	2009-03-23 14:59:43 +0000
+++ bzrlib/export/__init__.py	2010-01-30 09:38:13 +0000
@@ -138,14 +138,23 @@
     """Iter the entries for tree suitable for exporting.
 
     :param tree: A tree object.
-    :param subdir: None or the path of a directory to start exporting from.
+    :param subdir: None or the path of an entry to start exporting from.
     """
     inv = tree.inventory
     if subdir is None:
-        subdir_id = None
+        subdir_object = None
     else:
         subdir_id = inv.path2id(subdir)
-    entries = inv.iter_entries(subdir_id)
+        if subdir_id is not None:
+            subdir_object = inv[subdir_id]
+        # XXX: subdir is path not an id, so NoSuchId isn't proper error
+        else:
+            raise errors.NoSuchId(tree, subdir)
+    if subdir_object is not None and subdir_object.kind != 'directory':
+        yield subdir_object.name, subdir_object
+        return
+    else:
+        entries = inv.iter_entries(subdir_object)
     if subdir is None:
         entries.next() # skip root
     for entry in entries:

=== modified file 'bzrlib/tests/test_export.py'
--- bzrlib/tests/test_export.py	2009-07-29 13:46:55 +0000
+++ bzrlib/tests/test_export.py	2010-01-30 09:38:13 +0000
@@ -62,3 +62,14 @@
         wt.commit('1')
         self.build_tree(['target/', 'target/foo'])
         self.assertRaises(errors.BzrError, export.export, wt, 'target', format="dir")
+
+    def test_dir_export_existing_single_file(self):
+        self.build_tree(['dir1/', 'dir1/dir2/', 'dir1/first', 'dir1/dir2/second'])
+        wtree = self.make_branch_and_tree('dir1')
+        wtree.add(['dir2', 'first', 'dir2/second'])
+        wtree.commit('1')
+        export.export(wtree, 'target1', format='dir', subdir='first')
+        self.failUnlessExists('target1/first')
+        export.export(wtree, 'target2', format='dir', subdir='dir2/second')
+        self.failUnlessExists('target2/second')
+