[MERGE REVIEW] Clean-tree

[John Arbash Meinel]

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

Aaron Bentley wrote:
> Martin recently suggested we might merge clean-tree from bzrtools into
> the mainline, so here it is.  I've, updated it slightly to use note()
> instead of print.
> 
> I've been talking with John Meinel, and he suggested making --detritus
> the default.  Is that something we should do?
> 
> Aaron


Of course we should make it the default :)




...
+import errno
+import os
+import shutil
+import sys
+
+from bzrlib.osutils import has_symlinks, isdir
+from bzrlib.trace import note
+from bzrlib.workingtree import WorkingTree
+

^ you need an extra space here

+def is_detritus(subp):
+    return subp.endswith('.THIS') or subp.endswith('.BASE') or\
+        subp.endswith('.OTHER') or subp.endswith('~') or
subp.endswith('.tmp')


For performance this probably should be turned into the regex:
_is_detritus_re = re.compile(
    r'.*(?:(?:\.THIS$)|(?:\.BASE$)|(?:\.OTHER$)|(?:~$))'
    )

The (?:) is so that you can create a group that python doesn't have to
remember later.

v- you also need a space between the functions here

+
+def iter_deletables(tree, unknown=True, ignored=False, detritus=False):
+    """Iterate through files that may be deleted"""
+    for subp in tree.extras():
+        if detritus and is_detritus(subp):
+            yield tree.abspath(subp), subp
+            continue
+        if tree.is_ignored(subp):
+            if ignored:
+                yield tree.abspath(subp), subp
+        else:
+            if unknown:
+                yield tree.abspath(subp), subp
+

v- Another blank space between functions.

+def clean_tree(directory, ignored=False, detritus=False, dry_run=False):
+    tree = WorkingTree.open_containing(directory)[0]
+    deletables = iter_deletables(tree, ignored=ignored, detritus=detritus)
+    delete_items(deletables, dry_run=dry_run)
+
+def delete_items(deletables, dry_run=False):
+    """Delete files in the deletables iterable"""
+    has_deleted = False
+    for path, subp in deletables:
+        if not has_deleted:
+            note("deleting paths:")
+            has_deleted = True
+        note('  ' + subp)
+        if not dry_run:
+            if isdir(path):
+                shutil.rmtree(path)
+            else:
+                os.unlink(path)
+    if not has_deleted:
+        note("No files deleted.")


^- We have a 'delete_any' function in bzrlib.osutils. Though it doesn't
use rmtree, preferring to just delete a single directory. But I guess
since unknowns() doesn't go into unknown directories, this is really how
you have to do it. I just flinch when I see "rmtree". But it seems
correct here.


=== added file bzrlib/tests/blackbox/test_clean_tree.py //
file-id:test_clean_t
... ree.py-20060601212211-50rds19mwol16lx4-1 //
last-changed:aaron.bentley at utor
... onto.ca-20060601212453-f46ae4b44434594f

^- It would seem that for my mail reader we could wrap a little bit
earlier. I guess you wrap at 79, and the default for Thunderbird is to
wrap at 72 in text mode. I might just bump up thunderbird to 79/80. But
since it is common to wrap at 72 chars for mail readers, it might be
nice to make bundles that don't wrap as much.


...

+
+import os
+
+from bzrlib.tests.blackbox import ExternalBase
+
+

v- I realize this test probably predates it, but is there a reason you
are using 'touch' instead of 'build_tree'? Also we have
'self.failUnlessExists'. It would be nice if this was cleaned up.


+class TestCleanTree(ExternalBase):
+    @staticmethod
+    def touch(filename):
+        file(filename, 'wb').write('')
+
+    def test_clean_tree(self):
+        self.runbzr('init')
+        self.touch('name')
+        self.touch('name~')
+        assert os.path.lexists('name~')
+        self.touch('name.pyc')
+        self.runbzr('clean-tree')
+        assert os.path.lexists('name~')
+        assert not os.path.lexists('name')
+        self.runbzr('clean-tree --detritus')
+        assert not os.path.lexists('name~')
+        assert os.path.lexists('name.pyc')
+        self.runbzr('clean-tree --ignored')
+        assert not os.path.lexists('name.pyc')


v- More 'assert os.path.exists()' stuff, rather than
'self.failUnlessExists()'.

+class TestCleanTree(TestCaseInTempDir):
+    def test_symlinks(self):
+        if has_symlinks() is False:
+            return
+        os.mkdir('branch')
+        BzrDir.create_standalone_workingtree('branch')
+        os.symlink(os.path.realpath('no-die-please'), 'branch/die-please')
+        os.mkdir('no-die-please')
+        assert os.path.exists('branch/die-please')
+        os.mkdir('no-die-please/child')
+
+        clean_tree('branch')
+        assert os.path.exists('no-die-please')
+        assert os.path.exists('no-die-please/child')
+
+    def test_iter_deletable(self):
+        """Files are selected for deletion appropriately"""
+        os.mkdir('branch')
+        tree = BzrDir.create_standalone_workingtree('branch')
+        file('branch/file.BASE', 'wb').write('contents')
+        self.assertEqual(len(list(iter_deletables(tree))), 1)
+        file('branch/file~', 'wb').write('contents')
+        file('branch/file.pyc', 'wb').write('contents')
+        dels = [r for a,r in iter_deletables(tree)]
+        assert 'file~' not in dels
+        assert 'file.pyc' not in dels
+        dels = [r for a,r in iter_deletables(tree, detritus=True)]
+        assert 'file~' in dels
+        assert 'file.pyc' not in dels
+        dels = [r for a,r in iter_deletables(tree, ignored=True)]
+        assert 'file~' in dels
+        assert 'file.BASE' in dels
+        assert 'file.pyc' in dels
+        dels = [r for a,r in iter_deletables(tree, unknown=False)]
+        assert 'file.BASE' not in dels


I didn't review your history.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.0 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEgUZOJdeBCYSNAAMRAqf6AKCBmSwZDPLRNevwNVkorklo0D8WagCgng4O
Z2lppqGsyBZAg6aBd+k1Ujk=
=lsWx
-----END PGP SIGNATURE-----