Merge lp:~amanica/bzr/rm-no-backup-400554 into lp:bzr

[Marius Kruger]

add 'bzr rm --no-backup' and deprecate the --force opting to be consitent with revert

This fixes the last bit of:
https://bugs.launchpad.net/bzr/+bug/400554
by adding a --no-backup to `bzr rm` and deprecating --force.
--no-backup is consistent with `bzr revert`
so now we don't need the big --force hammer any more.

* I didn't change the internal parameter names because I don't know if its worth the effort
  of renaming it.
* The --force option still appears in the help because I can't see how to
  hide one RegistryOption but still work as a enumish thing. If we have to
  hide it, we should first implement this feature in another merge proposal.
* --force now prints out a deprecation warning, encouraging the user to rather use
  --no-backup.
* Maybe I'm not making it clear enough that --no-backup will delete
  everything you tell it to, with no going back.

[Martin Pool (mbp)]

Thanks for that, Marius.

62 + self.run_bzr(['remove', '--no-backup'] + list(files),
63 + error_regexes=["deleted a", "deleted b",
64 + "removed b/c", "deleted d"])
65 + self.assertFilesDeleted(files)

This code is fine, but it would be easier to read and write if you used run_script instead, and no slower.

review: Approve

[Marius Kruger (amanica)]

On 1 October 2010 07:15, Martin Pool <email address hidden> wrote:
> Review: Approve
> Thanks for that, Marius.
pleasure ;-)

> 62      +        self.run_bzr(['remove', '--no-backup'] + list(files),
> 63      +                     error_regexes=["deleted a", "deleted b",
> 64      +                                    "removed b/c", "deleted d"])
> 65      +        self.assertFilesDeleted(files)
>
> This code is fine, but it would be easier to read and write if you used run_script instead, and no slower.

ok I changed the 2 tests that this patch touches to use ScriptRunner.
I can do a follow-on if you'd like the rest of test_remove converted.

With ScriptRunner it does read better.
Previously we did try to make the tests less fragile by not
matching everything exactly eg. the files may be deleted in
a different order. Maybe because this is a blackbox test
for *remove* its ok to test every character.

[Martin Packman (gz)]

I actually prefer the non-scriptrunner version, but if we're really not worried about directory iteration order on the output I guess either is fine.

review: Approve

[Marius Kruger (amanica)]

On 4 October 2010 10:06, Martin [gz] <email address hidden> wrote:
> Review: Approve
> I actually prefer the non-scriptrunner version, but if we're really not worried about directory iteration order on the output I guess either is fine.

yes there are pros and cons.
Directory iteration order may have been a valid issue if we were not
sorting them;
it would have been nice though if you can somehow tell the script
runner that certain lines are allowed to be in a different order..
I'll let whoever merge this decide which way to go since its approved
either way :-)

[Martin Pool (mbp)]

review: Approve

[Martin Pool (mbp)]

On 4 October 2010 19:06, Martin [gz] <email address hidden> wrote:
> Review: Approve
> I actually prefer the non-scriptrunner version, but if we're really not worried about directory iteration order on the output I guess either is fine.

Because of it not caring about the ordering, or for some other reason?

--
Martin

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-10-01 08:49:39 +0000
+++ NEWS	2010-10-02 18:11:43 +0000
@@ -51,6 +51,11 @@
 Improvements
 ************
 
+* ``bzr remove`` now takes a ``--no-backup`` option for when you don't want it
+  to backup anything, just delete it. This option used to be called ``--force``
+  which is now deprecated.
+  (Marius Kruger, #400554)
+
 Documentation
 *************
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-10-01 08:49:39 +0000
+++ bzrlib/builtins.py	2010-10-02 18:11:43 +0000
@@ -1505,13 +1505,20 @@
             title='Deletion Strategy', value_switches=True, enum_switch=False,
             safe='Backup changed files (default).',
             keep='Delete from bzr but leave the working copy.',
+            no_backup='Don\'t backup changed files.',
             force='Delete all the specified files, even if they can not be '
-                'recovered and even if they are non-empty directories.')]
+                'recovered and even if they are non-empty directories. '
+                '(deprecated, use no-backup)')]
     aliases = ['rm', 'del']
     encoding_type = 'replace'
 
     def run(self, file_list, verbose=False, new=False,
         file_deletion_strategy='safe'):
+        if file_deletion_strategy == 'force':
+            note("(The --force option is deprecated, rather use --no-backup "
+                "in future.)")
+            file_deletion_strategy = 'no-backup'
+
         tree, file_list = WorkingTree.open_containing_paths(file_list)
 
         if file_list is not None:
@@ -1538,7 +1545,7 @@
             file_deletion_strategy = 'keep'
         tree.remove(file_list, verbose=verbose, to_file=self.outf,
             keep_files=file_deletion_strategy=='keep',
-            force=file_deletion_strategy=='force')
+            force=(file_deletion_strategy=='no-backup'))
 
 
 class cmd_file_id(Command):

=== modified file 'bzrlib/tests/blackbox/test_remove.py'
--- bzrlib/tests/blackbox/test_remove.py	2010-07-09 23:09:39 +0000
+++ bzrlib/tests/blackbox/test_remove.py	2010-10-02 18:11:43 +0000
@@ -18,8 +18,12 @@
 import os
 import sys
 
-from bzrlib.tests import SymlinkFeature, TestSkipped
-from bzrlib.tests import TestCaseWithTransport
+from bzrlib.tests import (
+    script,
+    SymlinkFeature,
+    TestCaseWithTransport,
+    TestSkipped,
+    )
 from bzrlib.workingtree import WorkingTree
 from bzrlib import osutils
 
@@ -200,12 +204,29 @@
         self.run_bzr('remove --keep a', error_regexes=["a is not versioned."])
         self.assertFilesUnversioned(files)
 
+    def test_remove_no_backup_unversioned_files(self):
+        self.build_tree(files)
+        tree = self.make_branch_and_tree('.')
+        script.ScriptRunner().run_script(self, '''
+        $ bzr remove --no-backup a b/ b/c d/
+        2>deleted d
+        2>removed b/c (but kept a copy: b/c.~1~)
+        2>deleted b
+        2>deleted a
+        ''')
+        self.assertFilesDeleted(files)
+
     def test_remove_force_unversioned_files(self):
         self.build_tree(files)
         tree = self.make_branch_and_tree('.')
-        self.run_bzr(['remove', '--force'] + list(files),
-                     error_regexes=["deleted a", "deleted b",
-                                    "removed b/c", "deleted d"])
+        script.ScriptRunner().run_script(self, '''
+        $ bzr remove --force a b/ b/c d/
+        2>(The --force option is deprecated, rather use --no-backup in future.)
+        2>deleted d
+        2>removed b/c (but kept a copy: b/c.~1~)
+        2>deleted b
+        2>deleted a
+        ''')
         self.assertFilesDeleted(files)
 
     def test_remove_deleted_files(self):