[MERGE] New reconfigure command

[Ian Clatworthy]

Aaron Bentley wrote:
> This patch implements a "reconfigure" command.  It can convert from
[snip]

Sweet. Nice to see this.

> There are other things it could support in the future
[snip]
> But I thought the patch was long enough already, so I decided to stop here.

Agreed. I like the way you've structured this but it's worth getting the
core landed and delivering the rest in later patches.

I haven't reviewed all the tests yet and I need more time to think about
 some other bits a bit more. Even so, I hope there's enough feedback
below to be helpful.

bb: resubmit

> +    @staticmethod
> +    def to_branch(bzrdir):
> +        reconfiguration = Reconfigure(bzrdir)
> +        reconfiguration.select_changes(tree=False, branch=True, bound=False)
> +        if not reconfiguration.planned_changes():
> +            raise errors.AlreadyBranch(bzrdir)
> +        return reconfiguration
> +
> +    @staticmethod
> +    def to_tree(bzrdir):
> +        reconfiguration = Reconfigure(bzrdir)
> +        reconfiguration.select_changes(tree=True, branch=True, bound=False)
> +        if not reconfiguration.planned_changes():
> +            raise errors.AlreadyTree(bzrdir)
> +        return reconfiguration
> +
> +    @staticmethod
> +    def to_checkout(bzrdir, bound_location=None):
> +        reconfiguration = Reconfigure(bzrdir, bound_location)
> +        reconfiguration.select_changes(tree=True, branch=True, bound=True)
> +        if not reconfiguration.planned_changes():
> +            raise errors.AlreadyCheckout(bzrdir)
> +        return reconfiguration

I'd like some docstrings for these functions explaining that a
Reconfigure object is returned and enumerating the Exceptions raised
otherwise.

> +    def select_changes(self, tree, branch, bound):
> +        """Determine which changes are needed to assume the configuration"""

I'd like to suggest that tree and branch be named want_tree and
want_branch to make it clear they are booleans, not objects. I also
think this method should be private for now, particularly as the full
scope of Reconfigure objects will grow and this API will most like
change accordingly.

> +    def planned_changes(self):
> +        """Return True if changes are planned, False otherwise"""

I'd prefer the name changes_are_needed() to stress the boolean nature.
"planned_changes()" sounds like it ought to return the set of planned
changes. Suggest making this method private as well (though don't mind
if you think otherwise given it's parameterless).

> +    def apply(self, force=False):

Needs a doc string.

> +        if self.destroy_reference:
> +            reference_info = self.referenced_branch.last_revision_info()
> +            self.bzrdir.destroy_branch()
> +        if self.create_branch:
> +            local_branch = self.bzrdir.create_branch()
> +            local_branch.set_last_revision_info(*reference_info)
> +        else:
> +            local_branch = self.local_branch

Practically this might always be safe? Technically, if create_branch is
True and destroy_reference isn't, *reference_info won't be set will it?

> +class cmd_reconfigure(Command):
> +    """Reconfigure the type of a bzr directory"""
> +
> +    takes_args = ['location?']
> +    takes_options = [RegistryOption.from_kwargs('target_type',
> +                     title='Target type',
> +                     help='The type to reconfigure the directory to.',
> +                     value_switches=True, enum_switch=False,
> +                     branch='Reconfigure to a branch.',
> +                     tree='Reconfigure to a tree.',
> +                     checkout='Reconfigure to a checkout.'),
> +                     Option('bind-to', help='Branch to bind checkout to.',
> +                            type=str),
> +                     Option('force',
> +                        help='Perform reconfiguration even if local changes'
> +                        ' will be lost.')
> +                     ]

I think this needs a little more doc for the help and reference manual.
In particular, the doc needs to be clearer on:

1. What the default target_type will be if none is given on the command
   line.
2. The rules used to bind a checkout if --bind-to is not given.

> +    def run(self, location=None, target_type=None, bind_to=None, force=False):
> +        directory = bzrdir.BzrDir.open(location)
> +        if target_type == 'branch':
> +            reconfiguration = reconfigure.Reconfigure.to_branch(directory)
> +        elif target_type == 'tree':
> +            reconfiguration = reconfigure.Reconfigure.to_tree(directory)
> +        elif target_type == 'checkout':
> +            reconfiguration = reconfigure.Reconfigure.to_checkout(directory,
> +                                                                  bind_to)
> +        reconfiguration.apply(force)

I'd like to see an else block trapping and reporting an unknown target
type. Without it, reconfigure.apply can be called without the
reconfiguration object being set.

> +    def destroy_branch(self):
> +        raise NotImplementedError(self.destroy_branch)
> +

One line doc string please.

> +    def destroy_branch(self):
> +        """See BzrDir.destroy_workingtree."""
> +        raise errors.UnsupportedOperation(self.destroy_branch, self)
> +

No, see BzrDir.destroy_branch. Well, once the one line doc string is
there. :-)

> +    def destroy_branch(self):
> +        """See BzrDir.create_branch."""
> +        self.transport.delete_tree('branch')
> +

Ditto.

>
> +class UncommittedChanges(BzrError):
> +
> +    _fmt = 'Working tree "%(display_url)s" has uncommitted changes.'
> +
> +    def __init__(self, tree):
> +        import bzrlib.urlutils as urlutils
> +        display_url = urlutils.unescape_for_display(
> +            tree.bzrdir.root_transport.base, 'ascii')
> +        BzrError.__init__(self, tree=tree, display_url=display_url)

Is there any benefit to passing the tree parameter to __init__ here?
It's not used in the message.

> === modified file 'bzrlib/merge.py'
> --- bzrlib/merge.py	2007-08-03 01:46:21 +0000
> +++ bzrlib/merge.py	2007-09-08 06:02:59 +0000
> @@ -209,7 +209,7 @@
>          if check_clean:
>              self.compare_basis()
>              if self.this_basis != self.this_rev_id:
> -                raise BzrCommandError("Working tree has uncommitted changes.")
> +                raise errors.UncommittedChanges(self.this_tree)

Just a minor note: the message here will now be slightly different
because the working tree URL will be included, yes? I didn't see any
updates to tests/blackbox/test_merge.py though. Perhaps it doesn't break
any tests? I'm mentioning it in case it does or in case you're motivated
to add one so it breaks the next time the string is changed. :-)

> === modified file 'bzrlib/tests/__init__.py'
> === modified file 'bzrlib/tests/blackbox/__init__.py'
> === modified file 'bzrlib/tests/bzrdir_implementations/test_bzrdir.py'

These test changes all reviewed and fine. Haven't reviewed the
reconfigure tests (yet) though. I'll review those when you resubmit.

Great to see this coming along in the pipeline.

Ian C.