[MERGE] bzr rm should remove clean subtrees

[Martin Pool]

Martin Pool has voted resubmit.
Status is now: Resubmit
Comment:

      def test_remove_nonempty_directory_with_unknowns(self):
-        """Unchanged non-empty directories should be deleted."""
+        """Unchanged non-empty directories should not be deleted."""

The docstring seems to disagree with the method name...

      def test_force_remove_nonempty_directory(self):
+        """Unchanged non-empty directories should be deleted when 
forced."""

The point seems to be not so much that it's nonempty but that it has 
unknown files.

+        self.assertInWorkingTree(files)
+        self.failUnlessExists(files)
+        self.build_tree(['b/unknown_directory/', 
'b/unknown_directory/ud2/'])
+        tree.remove('b', keep_files=False)
+        self.assertNotInWorkingTree('b')
+        self.failIfExists('b')

This seems to be saying that we will delete directories which contain 
unknown non-files under them.  Is that correct?  That seems a bit 
dangerous; we should probably not delete directories that have any 
unknown things under them, whether files or directories.   Suppose I 
have a new directory that I meant to add but have not that contains a 
lot of precious files.

 >the following looked a little controversial, but I'll be happy to do it 
in
>a followup bundle if you guys think that this is the desired behavior:
>* remove unversioned ignored files  (without force)

I know some people have concerns about files that are ignored but 
precious (that the user might want to retain).  It's hard to get these 
always right without introducing more than just the three categories of 
versioned/unknown/ignored.

You can make a case that if the user tells Bazaar to ignore a file it 
should just ignore it when considering whether a directory should be 
deleted.

On the whole I would be ok to delete them when removing a directory - 
after all they did say to delete the directory.  On projects with *.pyc 
or *.o files many directories will contain them.

                  if osutils.lexists(abs_path):
                      if (osutils.isdir(abs_path) and
                          len(os.listdir(abs_path)) > 0):
-                        message="%s is not empty directory "\
-                            "and won't be deleted." % (f,)
+                        if force:
+                            osutils.rmtree(abs_path)
+                        else:
+                            message = "%s is not an empty directory "\
+                                "and won't be deleted." % (f,)

How does this work when removing the subtree?  Have the contents been 
removed by the time we get here?

@@ -1834,26 +1839,29 @@
                       kind, executable) in 
self._iter_changes(self.basis_tree(),
                           include_unchanged=True, 
require_versioned=False,
                           want_unversioned=True, specific_files=files):
-                    # check if it's unknown OR changed but not deleted:
-                    if (versioned == (False, False)
-                        or (content_change and kind[1] != None)):
+                    # check if it's an unknown OR changed file:
+                    if kind[1] == 'file' and ( versioned == (False, 
False)
+                        or content_change ):
                          has_changed_files = True
                          break

I'm not sure why we're checking the kind?


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C418c22640708051147o2b78ea05h18ef10b5d457b51%40mail.gmail.com%3E