<div class="gmail_quote">2009/2/9 Aaron Bentley <<a href="mailto:aaron@aaronbentley.com">aaron@aaronbentley.com</a>> <div class="Ih2E3d"> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d">Marius Kruger wrote: >     Actually, considering the change is just >     Repository.set_make_working_trees, it might be simpler to just invoke it >     directly. > > > done. </div>No, I meant not using Reconfigure at all.  If you're using Reconfigure, then the change should be caused by apply.</blockquote><div> ok. Because Reconfigure gives other advantages like checking if we have a repository present, I chose to keep on using it. So I moved the actual change to apply() again.   <blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote">>     > 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. > > > I left them in, as they were already done, and they do add some value: > they test that we support the flags and that the user gets good error > messages. It's okay to leave them it, but the error messages should be tested in the unit tests as well. </blockquote> done. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div class="Ih2E3d"> >     > @@ -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. > > > AFAICT,  newly-created repositories are always shared=False, which is > something the new actions claim not to support. > So since changes are now done in set_repository_trees() directly, > we will ignore newly-created repositories, which is fine IMO, > as I can't see us getting into such a situation from the commandline. </div>This is an API, not the commandline.  We may have other clients that use it differently.  We may extend the commandline later. It does not make sense to write fragile code when it's just as easy to write robust code.</blockquote><div> fair enough. sorry. done: using 'repo' now.  </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> > # Bazaar merge directive format 2 (Bazaar 0.90) > # revision_id: amanic@gmail.com-20090206014740-6gbah25yvrkocm6t > # target_branch: ../bzr.dev/ ^^^ Please set the public_branch for your local mirror</blockquote><div> eish <\me scratches head>. `bzr send` seems to be a black art with this public_branch location that I just don't seem to get right :(  It seems to work great, but not being able to set it directly with `reconfigure` or something makes it a bit obscure. But that is another story. fixed now. thanks. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> > === modified file 'bzrlib/builtins.py' > --- bzrlib/builtins.py        2009-02-02 05:43:13 +0000 > +++ bzrlib/builtins.py        2009-02-05 23:31:14 +0000 > @@ -4851,7 +4851,11 @@ >                       ' checkout (with no local history).', >                       standalone='Reconfigure to be a standalone branch ' >                          '(i.e. stop using shared repository).', > -                     use_shared='Reconfigure to use a shared repository.'), > +                     use_shared='Reconfigure to use a shared repository.', > +                     with_trees='Reconfigure repository to create ' > +                        'working trees on branches by default', > +                     with_no_trees='Reconfigure repository to not create ' > +                        'working trees on branches by default'), >                       Option('bind-to', help='Branch to bind checkout to.', >                              type=str), >                       Option('force', > @@ -4877,6 +4881,12 @@ >              reconfiguration = reconfigure.Reconfigure.to_use_shared(directory) >          elif target_type == 'standalone': >              reconfiguration = reconfigure.Reconfigure.to_standalone(directory) > +        elif target_type == 'with-trees': > +            reconfiguration = reconfigure.Reconfigure.set_repository_trees( > +                directory, True) > +        elif target_type == 'with-no-trees': > +            reconfiguration = reconfigure.Reconfigure.set_repository_trees( > +                directory, False) >          reconfiguration.apply(force) ^^^ It doesn't make sense to do reconfiguration.apply for with-trees or with-no-trees.</blockquote><div> we now do the actual change in apply() again.  </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> > === modified file 'bzrlib/errors.py' > --- bzrlib/errors.py  2009-01-27 05:04:43 +0000 > +++ bzrlib/errors.py  2009-02-05 23:31:14 +0000 > @@ -2744,6 +2744,18 @@ >      _fmt = "'%(display_url)s' is already standalone." > > > +class AlreadyWithTrees(BzrDirError): > + > +    _fmt = "Shared repository '%(display_url)s' already creates " \ > +           "working trees." We generally use parentheses to split lines, e.g.:    _fmt = ("Shared repository '%(display_url)s' already creates"            " working trees.")</blockquote><div> done.  </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> > === modified file 'bzrlib/tests/test_reconfigure.py' > --- bzrlib/tests/test_reconfigure.py  2008-04-25 22:16:00 +0000 > +++ bzrlib/tests/test_reconfigure.py  2009-01-08 08:13:29 +0000 > @@ -377,3 +377,33 @@ >      def test_unsynced_branch_to_lightweight_checkout_forced(self): >          reconfiguration = self.make_unsynced_branch_reconfiguration() >          reconfiguration.apply(force=True) > + > +    def make_repository_with_without_trees(self, with_trees): <div class="Ih2E3d">> +        repo = self.make_repository('repo', shared=True) </div>> +        repo.set_make_working_trees(with_trees) > +        return repo > + > +    def test_make_with_trees(self): > +        repo = self.make_repository_with_without_trees(False) > +        reconfiguration = reconfigure.Reconfigure.set_repository_trees( > +            repo.bzrdir, True) > +        reconfiguration.apply() > +        self.assertIs(True, repo.make_working_trees()) > + > +    def test_make_without_trees(self): > +        repo = self.make_repository_with_without_trees(True) > +        reconfiguration = reconfigure.Reconfigure.set_repository_trees( > +            repo.bzrdir, False) > +        reconfiguration.apply() > +        self.assertIs(False, repo.make_working_trees()) > + > +    def test_make_with_trees_already_with_trees(self): > +        repo = self.make_repository_with_without_trees(True) > +        self.assertRaises(errors.AlreadyWithTrees, > +            reconfigure.Reconfigure.set_repository_trees, repo.bzrdir, True) You should also test the text of the error, like so:    e = self.assertRaises(errors.AlreadyWithTrees,        reconfigure.Reconfigure.set_repository_trees, repo.bzrdir, True)    self.assertEqual('Already has trees!', str(e))</blockquote><div> done for both tests (with and without).   </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> > + > +    def test_make_without_trees_already_no_trees(self): > +        repo = self.make_repository_with_without_trees(False) > +        self.assertRaises(errors.AlreadyWithNoTrees, > +            reconfigure.Reconfigure.set_repository_trees, repo.bzrdir, False) > + You also need to test the case where errors.ReconfigurationNotSupported is raised.</blockquote><div> </div></div>done. thanks a lot for the review and spoon feeding me the fixes:) -- <| regards U| Marius H| <><