Merge lp:~bialix/bzr/clean-tree-bzrdir into lp:bzr

[Alexander Belchenko]

``bzr clean-tree`` should not delete nested bzrdirs. (bialix, #572098)

This is minimal patch to address the problem highlighted in the bug report https://bugs.launchpad.net/bzr/+bug/572098. I've chatted with Aaron some time ago and he has agreed clean-tree should not delete nested bzrdirs by default.

I'd like to get this patch backported to 2.1 series as well, because it's utterly important for users of bzr-externals and scmproj.

Next my step will be to provide additional command-line flag to force deleting of bzrdirs if user wants it.

[Parth Malwankar (parthm)]

I was wondering if it makes sense to issue an info message in verbose mode saying something like '"foo" branch is skipped. "bar" branch is skipped... Please consider adding to ignored list.' rather than silently skipping.

run_bzr calls can probably be updated to take a list instead of a string. IIUC thats the newer recommended way.

[Alexander Belchenko (bialix)]

Parth Malwankar пишет:
> I was wondering if it makes sense to issue an info message in verbose mode saying something like '"foo" branch is skipped. "bar" branch is skipped... Please consider adding to ignored list.' rather than silently skipping.

"Please adding to ignore list"? I don't think so.
In my case they're already added to ignore list, and my intent is to run `bzr clean-tree --ignored`
without worrying about nested branches.

I'm a little ambivalent on wording though: nested bzrdir could be repository, checkout, lightweight
checkout, and not only a branch. So if I'll say '"foo" branch is skipped' while it's not branch,
then I will lie to user. I don't want to make bzr liar. I've asked Martin Pool is there any English
word for calling any bzr object (branch/repo/checkout/light checkout) without specifying its
existing type. I'm leaning towards "bzr object" but I'm not sure it's right. What about svn working
copy which could be handled by bzr-svn?

> run_bzr calls can probably be updated to take a list instead of a string. IIUC thats the newer recommended way.

I've used the same style as of other tests in that file. I don't mind to change it if this is main
blocker. Should I update existing tests in that file as well?

--
All the dude wanted was his rug back

[Parth Malwankar (parthm)]

> Parth Malwankar пишет:
> > I was wondering if it makes sense to issue an info message in verbose mode
> saying something like '"foo" branch is skipped. "bar" branch is skipped...
> Please consider adding to ignored list.' rather than silently skipping.
>
> "Please adding to ignore list"? I don't think so.
> In my case they're already added to ignore list, and my intent is to run `bzr
> clean-tree --ignored`
> without worrying about nested branches.
>

Makes sense.

> I'm a little ambivalent on wording though: nested bzrdir could be repository,
> checkout, lightweight
> checkout, and not only a branch. So if I'll say '"foo" branch is skipped'
> while it's not branch,
> then I will lie to user. I don't want to make bzr liar. I've asked Martin Pool
> is there any English
> word for calling any bzr object (branch/repo/checkout/light checkout) without
> specifying its
> existing type. I'm leaning towards "bzr object" but I'm not sure it's right.
> What about svn working
> copy which could be handled by bzr-svn?
>

I don't really have a good suggestion for the message.
Maybe something like 'skipped separately versioned directory "foo"'?

> > run_bzr calls can probably be updated to take a list instead of a string.
> IIUC thats the newer recommended way.
>
> I've used the same style as of other tests in that file. I don't mind to
> change it if this is main
> blocker. Should I update existing tests in that file as well?
>

I don't think its a blocker. I haven't really been touching
existing test, only following this for new ones.

[Vincent Ladeuil (vila)]

I think there is potentially a huge performance impact on big trees here.

For all *files* you check that the *directory* is not a branch.

The correct way to do this would be to prune the directory and *all* of its content when it's first processed.

Since that requires changing tree.extras() and would widen the scope of this proposal significantly,
I'm not asking you to take this into account, but we need a big flashing red comment in filter_out_nested_bzrdirs to mention it.

And since fixing it will make this function obsolete, please make it private by adding a leading '_'
so we don't have to enter into the deprecation dance for it when it occurs.

Consider this proposal approved if you agree with this tweak.

Some nits:
73 + # bug https://bugs.launchpad.net/bzr/+bug/572098

A couple of words to explain the bug would help to understand the test when offline :)

78 + self.run_bzr('clean-tree --unknown --force')
79 + self.failUnlessExists('foo')
80 + self.failUnlessExists('bar')
81 + self.run_bzr('clean-tree --ignored --force')
82 + self.failUnlessExists('foo')
83 + self.failUnlessExists('bar')

You may want to make two different tests here and factor out the lines to prepare the working tree
(a dedicated class sounds overkill).

review: Needs Fixing

[Alexander Belchenko (bialix)]

Updated version of the patch is available, please review.

[Vincent Ladeuil (vila)]

The 'commit message' in the mp is the one used by pqm, please make it one-line long instead
(it's actual content is really your COVER letter here).

Regarding the FIXME you added, @display_command is the way to add outf support, but here, you may want to
use ui.ui_factory_show_message() instead.

[Vincent Ladeuil (vila)]

review: Approve

[Alexander Belchenko (bialix)]

Vincent Ladeuil пишет:
> The 'commit message' in the mp is the one used by pqm, please make it one-line long instead
> (it's actual content is really your COVER letter here).

I don't understand you.

> Regarding the FIXME you added, @display_command is the way to add outf support, but here, you may want to
> use ui.ui_factory_show_message() instead.

I'm not sure what I want here, but anyway it will require API change,
and I'd like to backport this patch to 2.1 first before introducing more
invasive changes.

--
All the dude wanted was his rug back

[Parth Malwankar (parthm)]

Looks good to me.

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-05-03 04:08:50 +0000
+++ NEWS	2010-05-04 10:32:59 +0000
@@ -38,6 +38,10 @@
   better with sudo.
   (Martin <[e-mail]>, Parth Malwankar, #376388)
 
+* ``bzr clean-tree`` should not delete nested bzrdirs. Required for proper
+  support of bzr-externals and scmproj plugins.
+  (Alexander Belchenko, bug #572098)
+
 * ``bzr log --exclude-common-ancestry -r X..Y`` displays the revisions that
   are part of Y ancestry but not part of X ancestry (aka the graph
   difference).

=== modified file 'bzrlib/clean_tree.py'
--- bzrlib/clean_tree.py	2010-04-30 11:03:59 +0000
+++ bzrlib/clean_tree.py	2010-05-04 10:32:59 +0000
@@ -18,6 +18,7 @@
 import os
 import shutil
 
+from bzrlib import bzrdir, errors
 from bzrlib.osutils import isdir
 from bzrlib.trace import note
 from bzrlib.workingtree import WorkingTree
@@ -51,11 +52,14 @@
     try:
         deletables = list(iter_deletables(tree, unknown=unknown,
             ignored=ignored, detritus=detritus))
+        deletables = _filter_out_nested_bzrdirs(deletables)
         if len(deletables) == 0:
             note('Nothing to delete.')
             return 0
         if not no_prompt:
             for path, subp in deletables:
+                # FIXME using print is very bad idea
+                # clean_tree should accept to_file argument to write the output
                 print subp
             val = raw_input('Are you sure you wish to delete these [y/N]?')
             if val.lower() not in ('y', 'yes'):
@@ -66,6 +70,27 @@
         tree.unlock()
 
 
+def _filter_out_nested_bzrdirs(deletables):
+    result = []
+    for path, subp in deletables:
+        # bzr won't recurse into unknowns/ignored directories by default
+        # so we don't pay a penalty for checking subdirs of path for nested
+        # bzrdir.
+        # That said we won't detect the branch in the subdir of non-branch
+        # directory and therefore delete it. (worth to FIXME?)
+        if isdir(path):
+            try:
+                bzrdir.BzrDir.open(path)
+            except errors.NotBranchError:
+                result.append((path,subp))
+            else:
+                # TODO may be we need to notify user about skipped directories?
+                pass
+        else:
+            result.append((path,subp))
+    return result
+
+
 def delete_items(deletables, dry_run=False):
     """Delete files in the deletables iterable"""
     has_deleted = False

=== modified file 'bzrlib/tests/blackbox/test_clean_tree.py'
--- bzrlib/tests/blackbox/test_clean_tree.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/blackbox/test_clean_tree.py	2010-05-04 10:32:59 +0000
@@ -21,6 +21,7 @@
 
 import os
 
+from bzrlib import ignores
 from bzrlib.tests import TestCaseWithTransport
 
 
@@ -62,3 +63,18 @@
         self.failIfExists('name')
         self.failIfExists('name~')
         self.failIfExists('name.pyc')
+
+    def test_clean_tree_nested_bzrdir(self):
+        # clean-tree should not blindly delete nested bzrdirs (branches)
+        # bug https://bugs.launchpad.net/bzr/+bug/572098
+        # so it will play well with scmproj/bzr-externals plugins.
+        wt1 = self.make_branch_and_tree('.')
+        wt2 = self.make_branch_and_tree('foo')
+        wt3 = self.make_branch_and_tree('bar')
+        ignores.tree_ignores_add_patterns(wt1, ['./foo'])
+        self.run_bzr(['clean-tree', '--unknown', '--force'])
+        self.failUnlessExists('foo')
+        self.failUnlessExists('bar')
+        self.run_bzr(['clean-tree', '--ignored', '--force'])
+        self.failUnlessExists('foo')
+        self.failUnlessExists('bar')