[merge][rfc] New command: 'bzr remove-tree'
John Arbash Meinel
john at arbash-meinel.com
Fri Nov 10 23:20:41 GMT 2006
Daniel Silverstone wrote:
> There is a lot of conversation about the correct name for this command
> but I coded up the minor builtin 'remove-tree' and added blackbox tests
> and a NEWS item for it.
>
> There is talk of making it a hidden command until a better name comes
> up, but I'm not clever enough to do that yet I don't think (or at least
> I don't know how to do it :-)
>
> You can find it at http://bzr.digital-scurf.org/trees/dsilvers/bzr.dev
>
> Also, the bundle is attached.
>
> I'd like opinions on the UI for it, and its functionality limits.
>
> D.
I think the command is okay. We can start it off as hidden, and then
make it unhidden later if there is a lot of requests for it.
To make a function hidden, you do:
class cmd_foo(...):
hidden = True
It looks like you copied some other tests which were written in an older
style. So I'll have quite a few points. It isn't strictly wrong, but we
are trying to write tests which are clearer to understand.
v- I think we avoid -*- lines, especially since we avoid adding
non-ascii characters into the source code.
> +# Copyright (C) 2005 Canonical Ltd
> +# -*- coding: utf-8 -*-
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
> +
v- If you only have 1 line, we try to put the closing """ on the same line.
> +"""Black-box tests for bzr remove-tree.
> +"""
> +
> +import os
> +
> +from bzrlib.tests.blackbox import ExternalBase
-- two spaces between module scope definitions (so you need another
blank space here)
> +
> +class TestRemoveTree(ExternalBase):
> +
v- We have the helper functions "self.failIfExists()" and
"self.failUnlessExists()" for this sort of thing.
> + def _present(self, f):
> + self.assertEquals(os.path.exists(f), True)
> +
> + def _absent(self, f):
> + self.assertEquals(os.path.exists(f), False)
-- One space between class scope definitions. (so remove one space here)
> +
> +
> + def test_remove_tree(self):
> +
> + def bzr(*args, **kwargs):
> + return self.run_bzr(*args, **kwargs)[0]
> +
> + os.mkdir('branch1')
> + os.chdir('branch1')
> + bzr('init')
> + f=open('foo','wb')
> + f.write("foo\n")
> + f.close()
> + bzr('add', 'foo')
> +
> + bzr('commit', '-m', '1')
> +
> + os.chdir("..")
> +
> + self._present("branch1/foo")
> + bzr('branch', 'branch1', 'branch2')
> + self._present("branch2/foo")
> + bzr('checkout', 'branch1', 'branch3')
> + self._present("branch3/foo")
> + bzr('checkout', '--lightweight', 'branch1', 'branch4')
> + self._present("branch4/foo")
> +
^- We've switched to setting up trees and branches using the bzrlib api,
and then only call run_bzr() for the thing we are actually testing. So
instead this should be:
tree = self.make_branch_and_tree('branch1')
self.build_tree(['branch1/foo'])
tree.add('foo')
tree.commit('1')
self.failUnlessExists('branch1/foo')
tree2 = tree.bzrdir.sprout('branch2')
self.failUnlessExists('branch2/foo')
tree3 = tree.branch.create_checkout('branch3', lightweight=False)
self.failUnlessExists('branch3/foo')
tree4 = tree.branch.create_checkout('branch4', lightweight=True)
self.failUnlessExists('branch4/foo')
> + # branch1 == branch
> + # branch2 == branch of branch1
> + # branch3 == checkout of branch1
> + # branch4 == lightweight checkout of branch1
> +
v- And these become
bzr('remove-tree', working_dir='branch1')
self.failIfExists('branch1/foo')
> + # bzr remove-tree (CWD)
> + os.chdir("branch1")
> + bzr('remove-tree')
> + os.chdir("..")
> + self._absent("branch1/foo")
> +
> + # bzr remove-tree (path)
> + bzr('remove-tree', 'branch2')
> + self._absent("branch2/foo")
> +
> + # bzr remove-tree (checkout)
> + bzr('remove-tree', 'branch3')
> + self._absent("branch3/foo")
> +
v- For this, I would probably use 'self.run_bzr_error()' so that you can
make sure that bzr is failing for the correct reason, and printing a
reasonable error message to the user.
> + # bzr remove-tree (lightweight checkout, refuse)
> + bzr('remove-tree', 'branch4', retcode=3)
> + self._present("branch4/foo")
^- Also, you are really doing 4 different tests (original branch, copy
of branch, heavy checkout, lightweight checkout).
So it should really be 4 tests. You can use def setUp(self) as a place
to create the initial tree along with a single commit, and then you have
4 relatively short test functions, which make it clear what the
dependencies are and are not.
>
> === modified file NEWS
> --- NEWS
> +++ NEWS
> @@ -2,6 +2,10 @@
>
> IMPROVEMENTS:
>
> + * New command ``bzr remove-tree`` allows the removal of the working
> + tree from a branch.
> + (Daniel Silverstone)
> +
> * ``bzr export`` allows an optional branch parameter, to export a bzr
> tree from some other url. For example:
> ``bzr export bzr.tar.gz http://bazaar-vcs.org/bzr/bzr.dev``
>
> === modified file bzrlib/builtins.py
> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
> @@ -213,6 +213,29 @@
> self.outf.write(b.repository.get_revision_xml(rev_id).decode('utf-8'))
>
>
> +class cmd_remove_tree(Command):
> + """Remove the working tree from a given branch/checkout.
> +
> + Since a lightweight checkout is little more than a working tree
> + this will refuse to run against one.
> + """
> +
> + takes_args = ['location?']
> +
v- 'location' should just be '.' not u'.'.
I also prefer:
d = bzrdir.BzrDir.open(location)
I don't think we want 'open_containing()', because it is kind of silly
to remove a working tree that you are currently deeply nested in. Since
it will try to remove your current working directory.
And I think always using open_containing() will actually cause problems
where a user thinks they are removing one tree, but due to a typo, they
end up removing a different one.
For try/except functions, we prefer to catch them in a small area,
rather than over a lot of functions. So it should be:
try:
working = d.open_workingtree()
except errors.NoWorkingTree:
raise errors.BzrCommandError('No working tree to remove')
except errors.NotLocalUrl:
raise errors.BzrCommandError('Cannot remove the working tree of a'
' remote path')
working_path = working.bzrdir.root_transport.base
branch_path = ...
if working != branch:
raise errors.BzrCommandError('Cannot remove ...')
> + def run(self, location=u'.'):
> + d = bzrdir.BzrDir.open_containing(location)[0]
> + try:
> + working = d.open_workingtree()
> + working_path = working.bzrdir.root_transport.base
> + branch_path = working.branch.bzrdir.root_transport.base
> + if working_path != branch_path:
> + raise errors.BzrCommandError("Cannot remove working tree from lightweight checkout")
> + except (errors.NoWorkingTree, errors.NotLocalUrl):
> + raise errors.BzrCommandError("No working tree to remove")
> +
> + d.destroy_workingtree()
> +
> +
You also should wrap things at 79 characters.
So there are a few things to be done, but I think you have the general
idea. Your test coverage is good, it should just be refactored into
multiple small tests.
I'd be happy to merge a cleaned up patch.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061110/0d7b34d6/attachment.pgp
More information about the bazaar
mailing list