[Merge][#111665] bzr rm should remove clean subtrees

[Alexander Belchenko]

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

bb:resubmit

> === modified file 'bzrlib/tests/workingtree_implementations/test_remove.py'
> --- bzrlib/tests/workingtree_implementations/test_remove.py	2007-06-28 06:11:06 +0000
> +++ bzrlib/tests/workingtree_implementations/test_remove.py	2007-08-11 11:29:05 +0000
> @@ -17,25 +17,30 @@

>  class TestRemove(TestCaseWithWorkingTree):
>      """Tests WorkingTree.remove"""

> -    def getTree(self):
> +    def getTree(self, files):

> +    def getCommittedTree(self, files, message="Committing"):

^-- PEP-8 suggests to use names_with_underscores for functions and methods,
and keep mixedCase only in sources where already too much such names
for backward compatibility.
I'm not sure what is our case: unittest methods heavily used mixedCase,
but we use test_names_with_underscores. May be it's good to name
helper functions in the same style: get_tree, get_committed_tree?
I know that getTree was here before, it's just some thoughts about.
This comment is only comment, it's not blocker to merge your patch.

> +    def test_remove_unknown_ignored_files(self):
> +        """unknown ignored files should be deleted."""

^-- s/unknown/Unknown/ please.

> +        tree = self.getCommittedTree(['b/'])
> +        ignores.add_runtime_ignores(["*ignored*"])
> +
> +        self.build_tree(['unknown_ignored_file'])

> +        self.assertTrue(tree.is_ignored('unknown_ignored_file') != None)

^-- self.assertNotEquals(None, tree.is_ignored('unknown_ignored_file'))
is much better IMO.

> +        tree.remove('unknown_ignored_file', keep_files=False)

> +        self.assertNotInWorkingTree('unknown_ignored_file')
> +        self.failIfExists('unknown_ignored_file')

^-- I'm not sure, but in these tests such pair of checks is used very often.
May be it's worth to refactor them out as handy method?

> +
> +        self.build_tree(['b/unknown_ignored_file', 'b/unknown_ignored_dir/'])
> +        self.assertTrue(tree.is_ignored('b/unknown_ignored_file') != None)
> +        self.assertTrue(tree.is_ignored('b/unknown_ignored_dir') != None)

^-- the same here, self.assertNotEquals(None, ...)

>      def test_remove_directory_with_changed_file(self):
>          """Refuse to delete directories with changed files."""
> -        tree = self.getTree()
> -        tree.add(TestRemove.b_c)
> -        tree.commit("make sure b and c are versioned")
> +        files = ['b/', 'b/c']
> +        tree = self.getCommittedTree(files)
>          self.build_tree_contents([('b/c', "some other new content!")])
> -        self.assertInWorkingTree(TestRemove.b_c)
> +
>          err = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,
> -            TestRemove.b, keep_files=False)
> +            'b', keep_files=False)
>          self.assertContainsRe(err.changes_as_text, '(?s)modified:.*b/c')
> -        self.assertInWorkingTree(TestRemove.b_c)
> -        self.failUnlessExists(TestRemove.b_c)
> +        self.assertInWorkingTree(files)
> +        self.failUnlessExists(files)
>  
>          #see if we can force it now..

^-- may I ask you to insert one space between # and comment text?
For readability.

> +    def test_remove_directory_with_renames(self):
> +        """Delete directory with renames in or out."""
> +
> +        files = ['a/', 'a/file', 'a/directory/', 'b/']
> +        files_to_move = ['a/file', 'a/directory/']
> +
> +        tree = self.getCommittedTree(files)
> +        #move stuff from a=>b

^-- the same here, please.

> @@ -226,3 +283,4 @@
>          tree.commit('add file')
>          tree.remove('dir/', keep_files=False)
>          self.failIfExists('tree/dir/file')
> +        self.assertNotInWorkingTree('tree/dir/file', 'tree')
> \ No newline at end of file

^-- :-) newline at the end of file, please.

> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py	2007-07-25 21:25:00 +0000
> +++ bzrlib/workingtree.py	2007-08-11 11:29:05 +0000

> @@ -1806,11 +1804,16 @@
>                      directory):
>                  for relpath, basename, kind, lstat, abspath in file_infos:
>                      if kind == 'file':
> -                        if self.path2id(relpath): #is it versioned?
> +                        #is it versioned or ignored?

^-- space between # and comment text, please.

> +                        if self.path2id(relpath) or self.is_ignored(relpath):
> +                            # add subtree content for deletion
>                              new_files.add(relpath)
>                          else:
>                              unknown_files_in_directory.add(
>                                  (relpath, None, kind))
> +                    else:
> +                        # make sure we delete subtrees
> +                        new_files.add(relpath)

^-- I don't understand this part. You're explicitly checking
for versioned and ignored files, but skip such check for
directories and symlinks. Why?

>  
>          for filename in files:
>              # Get file name into canonical form.
> @@ -1820,6 +1823,7 @@
>                  new_files.add(filename)
>                  if osutils.isdir(abspath):
>                      recurse_directory_to_add_files(filename)
> +
>          files = [f for f in new_files]

^-- does list comprehension is faster than slicing operation?
My simple test shows that is not:

E:\Bazaar\mydev\selftest.miss.dep>python -m timeit "a = range(1000); b = [i for i in a]"
1000 loops, best of 3: 381 usec per loop

E:\Bazaar\mydev\selftest.miss.dep>python -m timeit "a = range(1000); b = a[:]"
10000 loops, best of 3: 86.7 usec per loop


If my test is reliable then I suggest to change the line above to

          files = new_files[:]


[µ]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGwBjDzYr338mxwCURAmljAKCOZpaPmosifn0uq8vaR821aOoKCwCbB3uE
YVo7v68aSfbkOmbyYVjHNNU=
=5Z8W
-----END PGP SIGNATURE-----