[MERGE] Allow changing --[no-]trees with reconfigure (bug 145033)

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

bb:resubmit

Matthew D. Fuller wrote:
>> In fact, I think I'll do just that.
> 
> And so I do.

And I agree it's the right thing to do.  It the same way that
- --use-shared and --standalone work.  _plan_changes is for changing a
bzrdir into a given configuration, such as a tree, branch, or repository.

Actually, considering the change is just
Repository.set_make_working_trees, it might be simpler to just invoke it
directly.

> Updated bundle attached.  Since I was touching it
> anyway, I also make the docstring fix Ian pointed out and merged it up
> with bzr.dev.
> 
> I did add a test that --with-no-trees doesn't screw with an existing
> working tree (the rewrite should make that impossible anyway).  I'm
> not real happy with the test; it sorta takes a long stroll around the
> garden to check if a door it just opened is open.  There has to be a
> less absurd way of writing it, but that's far past my competence.

Well, you can skip some of the blackbox tests.  The blackbox tests
should prove that it works at all, not duplicate the unit tests.

> === modified file 'bzrlib/reconfigure.py'
> --- bzrlib/reconfigure.py	2008-04-24 04:58:42 +0000
> +++ bzrlib/reconfigure.py	2009-01-19 08:48:21 +0000
> @@ -24,9 +24,10 @@
>  
>  class Reconfigure(object):
>  
> -    def __init__(self, bzrdir, new_bound_location=None):
> +    def __init__(self, bzrdir, new_bound_location=None, with_trees=None):
>          self.bzrdir = bzrdir
>          self.new_bound_location = new_bound_location
> +        self.with_trees = with_trees

What is the purpose of this variable?  AFAICT, it is only used in
set_repository_trees, and it is set there also.

>          try:
>              self.repository = self.bzrdir.find_repository()
>          except errors.NoRepositoryPresent:
> @@ -62,6 +63,8 @@
>          self._create_tree = False
>          self._create_repository = False
>          self._destroy_repository = False
> +        self._set_with_trees = False
> +        self._set_with_no_trees = False
>  
>      @staticmethod
>      def to_branch(bzrdir):
> @@ -140,6 +143,25 @@
>              raise errors.AlreadyStandalone(bzrdir)
>          return reconfiguration
>  
> +    @classmethod
> +    def set_repository_trees(klass, bzrdir, with_trees):
> +        """Adjust a repository's working tree presense default"""
> +        reconfiguration = klass(bzrdir, with_trees=with_trees)

You don't need to pass with_trees in-- just use with_trees instead of
reconfiguration.with_trees.

> +        if not reconfiguration.repository.is_shared():
> +            raise errors.ReconfigurationNotSupported(reconfiguration.bzrdir)
> +        if reconfiguration.with_trees is True and \

Please use a boolean test like

if reconfiguration.with_trees

Checking identity is especially bad because it misses other true values.

> +            not reconfiguration.repository.make_working_trees():
> +            reconfiguration._set_with_trees = True
> +        elif reconfiguration.with_trees is False and \

Again, use elif not reconfiguration.with_trees

> +            reconfiguration.repository.make_working_trees():
> +            reconfiguration._set_with_no_trees = True

Except, why not just use reconfiguration.with_trees directly in
Reconfiguration.apply?

i.e.

if self.with_trees is not None:
    if self.repository.set_make_working_trees(self.with_trees)

> @@ -302,3 +325,7 @@
>              local_branch.bind(branch.Branch.open(bind_location))
>          if self._destroy_repository:
>              self.bzrdir.destroy_repository()
> +        if self._set_with_trees:
> +            self.repository.set_make_working_trees(True)
> +        if self._set_with_no_trees:
> +            self.repository.set_make_working_trees(False)

self.repository refers to the current repository, but won't refer to a
newly-created repository.  You'll want to use the local variable 'repo'
instead.

> +    def test_make_without_trees_leaves_tree_alone(self):
> +        # There has to be a better way of doing this...
> +        repo = self.make_repository('repo', shared=True)
> +        branch = self.make_branch('repo/branch')
> +        self.run_bzr('reconfigure --use-shared',
> +            working_dir='repo/branch')
> +        tree = branch.bzrdir.create_workingtree()

Use BzrDir.create_branch_convenience instead of creating the branch and
tree and then making it use the shared repository.

> +        self.build_tree(['repo/branch/foo'])
> +        tree.add('foo')
> +        tree.commit('x')

You don't have to commit-- you can just supply --force

> +        self.run_bzr('reconfigure --with-no-trees',
> +            working_dir='repo/branch')
> +        self.failUnlessExists('repo/branch/foo')

But this whole thing should be a unit test so that you know that
workingtree.WorkingTree.open works at the end.

Aaron


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkl04NQACgkQ0F+nu1YWqI3QtwCff7yPivnNQ0Oil59O7AKlrvb1
6bMAniPbe9cJFOU7l5wmwJxOdhz6tTUT
=Iy2+
-----END PGP SIGNATURE-----