[MERGE] bug #54172: revert handles new directories nicely

[John Arbash Meinel]

Aaron Bentley wrote:
> Hi all,
> 
> This bundle fixes a bug in revert that caused newly-added directories to
> be kept, when they should have been deleted.  The problem is that
> directories (and symlinks) don't have a hash, so they aren't inserted
> into the merge-modified list.  Now, entries that don't have hashes are
> also removed.
> 
> Aaron

=== modified file NEWS
--- NEWS
+++ NEWS
@@ -3,6 +3,7 @@
   IMPROVEMENTS:

   BUG FIXES:
+  * revert now removes newly-added directories (Aaron Bentley, #54172)

^- the indentation here is incorrect. I believe we indent to 4 spaces.
But you can merge the latest bzr.dev and you'll have to resolve a
conflict anyway. :)



...

                 if file_id not in target_tree:
                     trans_id = tt.trans_id_tree_file_id(file_id)
                     tt.unversion_file(trans_id)
-                    if file_id in merge_modified:
+                    try:
+                        file_kind = working_tree.kind(file_id)
+                    except NoSuchFile:
+                        file_kind = None
+                    if file_kind != 'file' and file_kind is not None:
+                        keep_contents = False
+                        delete_merge_modified = False

^- so this is saying if the object does exist in the working tree
(file_kind is not None) and it isn't a file, then don't keep the
contents, and don't delete it from the file system (because it doesn't
exist anyway, right?)


+                    else:
+                        if (file_id in merge_modified and
+                            merge_modified[file_id] ==
+                            working_tree.get_file_sha1(file_id)):
+                            keep_contents = False
+                            delete_merge_modified = True

^- I'm a little concerned that (file_kind is None) will sneak in here,
and then the get_file_sha1(file_id) will do something weird, because
id2path will raise a BzrError claiming the file_id doesn't exist in the
inventory. But checks for this might exist elsewhere.

+                        else:
+                            keep_contents = True
+                            delete_merge_modified = False
+                    if not keep_contents:
                         tt.delete_contents(trans_id)
+                    if delete_merge_modified:
                         del merge_modified[file_id]
         finally:
             child_pb.finished()


Otherwise it looks good. +1 from me if you can confirm that if file_kind
 is None it won't mess things up.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060828/dddb65a2/attachment.pgp