[MERGE] bzr rm improvements

Andrew Bennetts andrew at canonical.com
Wed Aug 13 09:14:00 BST 2008


Robert Collins wrote:
> This makes some some improvements to bzr rm:
>  - its aliased to del for svn users
>  - just running 'bzr rm' will remove all missing files.
> 
> It depends on the InterTree fix I just posted, because, well, it just
> does ok?

I like this change!

bb:tweak

> === modified file 'bzrlib/builtins.py'
[...]
>  class cmd_remove(Command):
>      """Remove files or directories.
>  
> -    This makes bzr stop tracking changes to the specified files and
> -    delete them if they can easily be recovered using revert.
> -
> -    You can specify one or more files, and/or --new.  If you specify --new,
> -    only 'added' files will be removed.  If you specify both, then new files
> -    in the specified directories will be removed.  If the directories are
> -    also new, they will also be removed.
> +    This makes bzr stop tracking changes to the specified files and delete them

s/delete/deletes

> === modified file 'bzrlib/tests/blackbox/test_remove.py'
[...]
> -    def _make_add_and_assert_tree(self, files):
> +    def _make_add_and_assert_tree(self, paths):
>          tree = self.make_branch_and_tree('.')
> -        self.build_tree(files)
> -        for f in files:
> -            id=str(f).replace('/', '_') + _id
> -            tree.add(f, id)
> -            self.assertEqual(tree.path2id(f), id)
> -            self.failUnlessExists(f)
> -            self.assertInWorkingTree(f)
> +        tree.lock_write()
> +        try:
> +            self.build_tree(paths)
> +            for path in paths:
> +                file_id=str(path).replace('/', '_') + _id
> +                tree.add(path, file_id)
> +        finally:
> +            tree.unlock()
>          return tree

If this method is no longer making any assertions, then it shouldn't have
“and_assert” in the name.  (I do agree with removing the asserts from this
helper method, I just want the name to be updated.)

[...]
>      def test_remove_no_files_specified(self):
[...]
> -        self.run_bzr_error(["bzr: ERROR: Specify one or more files to remove, "
> -            "or use --new."], 'remove')
> -
> -        self.run_bzr_error(["bzr: ERROR: No matching files."], 'remove --new')
> -
> -        self.run_bzr_error(["bzr: ERROR: No matching files."],
> -            'remove --new .')

Hmm, it looks like you're losing some test coverage here.  There's nowhere else
in this file that exercises the “No matching files” error path, or the use of
“remove --new” with no other arguments (although this latter condition may be
less interesting?).

So, please add a test that exercises the “No matching files” case of “bzr remove
--new”.

-Andrew.




More information about the bazaar mailing list