Merge lp:~mbp/bzr/391411-reconfigure-stacked into lp:~bzr/bzr/trunk-old

[Martin Pool]

[Martin Pool (mbp)]

This adds reconfigure --stacked-on and --unstacked options, and fixes a bug in fetching data when unstacking that made it break in all but trivial cases of empty branches.

I've included here part of the -Dunlock code because it made debugging this much easier.

It also adds a RepositoryBase so that we can have at least some common code with RemoteRepository.

[Ian Clatworthy (ian-clatworthy)]

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/391411-reconfigure-stacked into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This adds reconfigure --stacked-on and --unstacked options, and fixes a bug in fetching data when unstacking that made it break in all but trivial cases of empty branches.
>
> I've included here part of the -Dunlock code because it made debugging this much easier.
>
> It also adds a RepositoryBase so that we can have at least some common code with RemoteRepository.
>
Thanks for working on this. It certainly fills in an important hole. I
suspect it needs another round of polish though before it's good enough
to land.

> + Option('unstacked',
> + help='Reconfigure a branch to be unstacked. This '
> + 'may requiring copying substantial substantial data into it.',
>

s/requiring/require/
substantial x 2

> + stacked_on=None,
> + unstacked=None):
> directory = bzrdir.BzrDir.open(location)
> + if stacked_on is not None:
>

I think we ought to guard against both --stacked-on and --unstacked
being both passed. (At the moment, we assume stacked-on is what the user
meant. If we want to keep that assumption, we need to document it in the
help at a minimum.) A blackbox test is required for this case too.

> -Dmerge Emit information for debugging merges.
> +-Dunlock Some errors during unlock are treated as warnings.
> -Dpack Emit information about pack operations.
>

Looks like a tabbing or whitespace issue here.

> + # block, so it's useful to have tho option not to generate a new error
>

s/tho/the/

> @@ -18,6 +18,7 @@
> # across to run on the server.
>
> import bz2
> +import warnings
>
> from bzrlib import (
> bencode,
> @@ -27,11 +28,13 @@
> debug,
> errors,
> graph,
> + lock,
> lockdir,
> repository,
> revision,
> revision as _mod_revision,
> symbol_versioning,
> + trace,
>

The new imports of warnings and trace are redundant here.

> + def has_same_fallbacks(self, other_repo):
> + my_fb = self._fallback_repositories
> + other_fb = other_repo._fallback_repositories
> + if len(my_fb) != len(other_fb):
> + return False
> + for f, g in zip(my_fb, other_fb):
> + if not f.has_same_location(g):
> + return False
> + return True
>

This needs a docstring. It's also a public API (currently) so it needs
API tests written for it and/or needs to be made private. (BTW, I don't
think _fallback_repositories can ever be None. If it can, the above code
needs to be slightly more defensive as well.)

> +Be aware that when ``bzr reconfigure --unstacked`` is used, bzr will
> +to copy all the referenced data from the stacked-on repository into the
s/to copy/copy/

In summary, it's mainly small tweaks above but I suspect you want a few
more tests as well.

Ian C.

[Martin Pool (mbp)]

2009/7/14 Ian Clatworthy <email address hidden>:
>> +                'may requiring copying substantial substantial data into it.',

Well, it's _really_ substantial :)

Thanks for the review, I'll do those tweaks.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Fri, 2009-07-10 at 08:55 +0000, Martin Pool wrote:

> @@ -662,6 +663,9 @@
> """
> if not self._format.supports_stacking():
> raise errors.UnstackableBranchFormat(self._format,
> self.base)
> + # XXX: Changing from one fallback repository to another does
> not check
> + # that all the data you need is present in the new fallback.
> + # Possibly it should.

I think this would be better as a bug 'reconfigure --stacked-on-url does
not check new location has sufficient data'.

> - # in.
> - source_repository =
> self._get_fallback_repository(old_url)
> - for revision_id in chain([self.last_revision()],
> - self.tags.get_reverse_tag_dict()):
> - self.repository.fetch(source_repository, revision_id,
> - find_ghosts=True)

...
I think it would be better to /try/ to copy the tags. That should be
easy to do (fetch the tip, then for each tag try and catch
NoSuchRevision).

> + self.repository.fetch(old_repository,
> + revision_id=self.last_revision(),
> + find_ghosts=True)
> + finally:
> + old_repository.unlock()
> + finally:
> + pb.finished()
>
> def _set_tags_bytes(self, bytes):
> """Mirror method for _get_tags_bytes.
...
> - def run(self, location=None, target_type=None, bind_to=None,
> force=False):
> + def run(self, location=None, target_type=None, bind_to=None,
> force=False,
> +... 'specified')
> elif target_type == 'branch':
> reconfiguration =
> reconfigure.Reconfigure.to_branch(directory)
> elif target_type == 'tree':

Your changes to the reconfigure command add quite a few lines of logic,
but most of reconfigure is meant to be in the reconfigure module where
it is accessible to GUI's and so on. I think that this should be moved
there before landing, as otherwise the branch 'reduces clarity of
layering'.

> === modified file 'bzrlib/fetch.py'
> --- bzrlib/fetch.py 2009-06-17 17:57:15 +0000
> +++ bzrlib/fetch.py 2009-07-09 08:59:51 +0000
> @@ -59,11 +59,8 @@
> symbol_versioning.deprecated_in((1, 14, 0))
> % "pb parameter to RepoFetcher.__init__")
> # and for simplicity it is in fact ignored
> - if to_repository.has_same_location(from_repository):
> - # repository.fetch should be taking care of this case.
> - raise errors.BzrError('RepoFetcher run '
> - 'between two objects at the same location: '
> - '%r and %r' % (to_repository, from_repository))
> + # repository.fetch has the responsibility for
> short-circuiting
> + # attempts to copy between a repository and itself.

Is the above change needed? We added it when we refactored - if you're
removing this because you noticed it, thats fine. If you're removing it
because it was firing, thats a problem.

I don't think RepositoryBase is needed. Thats what Repository is.

-Rob

[Martin Pool (mbp)]

2009/7/15 Robert Collins <email address hidden>:
> On Fri, 2009-07-10 at 08:55 +0000, Martin Pool wrote:
>
>> @@ -662,6 +663,9 @@
>>          """
>>          if not self._format.supports_stacking():
>>              raise errors.UnstackableBranchFormat(self._format,
>> self.base)
>> +        # XXX: Changing from one fallback repository to another does
>> not check
>> +        # that all the data you need is present in the new fallback.
>> +        # Possibly it should.
>
> I think this would be better as a bug 'reconfigure --stacked-on-url does
> not check new location has sufficient data'.

Agree.

>> -            # in.
>> -            source_repository =
>> self._get_fallback_repository(old_url)
>> -            for revision_id in chain([self.last_revision()],
>> -                self.tags.get_reverse_tag_dict()):
>> -                self.repository.fetch(source_repository, revision_id,
>> -                    find_ghosts=True)
>
> ...
> I think it would be better to /try/ to copy the tags. That should be
> easy to do (fetch the tip, then for each tag try and catch
> NoSuchRevision).

I was a bit concerned this would make reconfigure be O(n_tags). I
suppose the common case is they will already have been copied and
we'll have enough of the graph cached to tell this quickly. It might
be nice if fetch could be give multiple revids and just tell you which
ones failed to be copied.

>
>> +                self.repository.fetch(old_repository,
>> +                    revision_id=self.last_revision(),
>> +                    find_ghosts=True)
>> +            finally:
>> +                old_repository.unlock()
>> +        finally:
>> +            pb.finished()
>>
>>      def _set_tags_bytes(self, bytes):
>>          """Mirror method for _get_tags_bytes.
> ...
>> -    def run(self, location=None, target_type=None, bind_to=None,
>> force=False):
>> +    def run(self, location=None, target_type=None, bind_to=None,
>> force=False,
>> +...                    'specified')
>>          elif target_type == 'branch':
>>              reconfiguration =
>> reconfigure.Reconfigure.to_branch(directory)
>>          elif target_type == 'tree':
>
> Your changes to the reconfigure command add quite a few lines of logic,
> but most of reconfigure is meant to be in the reconfigure module where
> it is accessible to GUI's and so on. I think that this should be moved
> there before landing, as otherwise the branch 'reduces clarity of
> layering'.

OK

>
>> === modified file 'bzrlib/fetch.py'
>> --- bzrlib/fetch.py     2009-06-17 17:57:15 +0000
>> +++ bzrlib/fetch.py     2009-07-09 08:59:51 +0000
>> @@ -59,11 +59,8 @@
>>                  symbol_versioning.deprecated_in((1, 14, 0))
>>                  % "pb parameter to RepoFetcher.__init__")
>>              # and for simplicity it is in fact ignored
>> -        if to_repository.has_same_location(from_repository):
>> -            # repository.fetch should be taking care of this case.
>> -            raise errors.BzrError('RepoFetcher run '
>> -                    'between two objects at the same location: '
>> -                    '%r and %r' % (to_repository, from_repository))
>> +        # repository.fetch has the r...

[Martin Pool (mbp)]

2009/7/15 Martin Pool <email address hidden>:

>> I don't think RepositoryBase is needed. Thats what Repository is.
>
> It would be nice if Repository was actually the base.  At the moment
> RemoteRepository does not inherit from it, and there seems to be some
> amount of copy-and-paste between them because of this.  Perhaps that
> should be a separate bug and I should continue the copy/paste for now.

I may be wrong but I anticipate some trouble if I change this; I
recall Spiv talking for a while about whether it should be a subclass
or not...

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Tue, 2009-07-14 at 23:48 +0000, Martin Pool wrote:
>
> > It would be nice if Repository was actually the base. At the moment
> > RemoteRepository does not inherit from it, and there seems to be
> some
> > amount of copy-and-paste between them because of this. Perhaps that
> > should be a separate bug and I should continue the copy/paste for
> now.
>
> I may be wrong but I anticipate some trouble if I change this; I
> recall Spiv talking for a while about whether it should be a subclass
> or not...

Spiv and I have repeatedly lamented them not being subclasses; however
we haven't had time to make it so. It may be easy.

-Rob

[Ian Clatworthy (ian-clatworthy)]

See comments above from Robert and I.

review: Needs Fixing

[Martin Pool (mbp)]

> Spiv and I have repeatedly lamented them not being subclasses; however
> we haven't had time to make it so. It may be easy.

https://bugs.edge.launchpad.net/bzr/+bug/401622 for doing this

[Martin Pool (mbp)]

Also bug 401642 for checking when reconfiguring to a different fallback, and bug 401646 for copying data referenced by tags.

[Martin Pool (mbp)]

> On Fri, 2009-07-10 at 08:55 +0000, Martin Pool wrote:
>
> > @@ -662,6 +663,9 @@
> > """
> > if not self._format.supports_stacking():
> > raise errors.UnstackableBranchFormat(self._format,
> > self.base)
> > + # XXX: Changing from one fallback repository to another does
> > not check
> > + # that all the data you need is present in the new fallback.
> > + # Possibly it should.
>
> I think this would be better as a bug 'reconfigure --stacked-on-url does
> not check new location has sufficient data'.
>
> > - # in.
> > - source_repository =
> > self._get_fallback_repository(old_url)
> > - for revision_id in chain([self.last_revision()],
> > - self.tags.get_reverse_tag_dict()):
> > - self.repository.fetch(source_repository, revision_id,
> > - find_ghosts=True)
>
> ...
> I think it would be better to /try/ to copy the tags. That should be
> easy to do (fetch the tip, then for each tag try and catch
> NoSuchRevision).
>
> > + self.repository.fetch(old_repository,
> > + revision_id=self.last_revision(),
> > + find_ghosts=True)
> > + finally:
> > + old_repository.unlock()
> > + finally:
> > + pb.finished()
> >
> > def _set_tags_bytes(self, bytes):
> > """Mirror method for _get_tags_bytes.
> ...
> > - def run(self, location=None, target_type=None, bind_to=None,
> > force=False):
> > + def run(self, location=None, target_type=None, bind_to=None,
> > force=False,
> > +... 'specified')
> > elif target_type == 'branch':
> > reconfiguration =
> > reconfigure.Reconfigure.to_branch(directory)
> > elif target_type == 'tree':
>

> Your changes to the reconfigure command add quite a few lines of logic,
> but most of reconfigure is meant to be in the reconfigure module where
> it is accessible to GUI's and so on. I think that this should be moved
> there before landing, as otherwise the branch 'reduces clarity of
> layering'.

That's a very good point about putting this outside of builtins.py where it can be reused.

I have some qualms about putting it into class Reconfigure though; this seems like the kind of thing we don't have an entirely satisfactory style for. Perhaps in this case the different types of reconfiguration should be different subclasses, rather than a bunch of variables on the instance...

[Martin Pool (mbp)]

> I have some qualms about putting it into class Reconfigure though; this seems
> like the kind of thing we don't have an entirely satisfactory style for.
> Perhaps in this case the different types of reconfiguration should be
> different subclasses, rather than a bunch of variables on the instance...

I've put it into new Reconfigure-like classes to do this, and having done the other tweaks will submit this.

=== modified file 'NEWS'
--- NEWS	2009-07-23 17:00:27 +0000
+++ NEWS	2009-07-24 03:35:23 +0000
@@ -16,8 +16,12 @@
 New Features
 ************
 
-* ``merge --interactive`` applies a user-selected portion of the merge.  The UI
-  is similar to ``shelve``.  (Aaron Bentley)
+* ``bzr merge --interactive`` applies a user-selected portion of the
+  merge.  The UI is similar to ``shelve``.  (Aaron Bentley)
+
+* ``bzr reconfigure`` now takes options ``--stacked-on URL`` and
+  ``--unstacked`` to change stacking of a branch.
+  (Martin Pool, #391411)
 
 Bug Fixes
 *********
@@ -172,7 +176,7 @@
 
 * ``bzr push`` now aborts if uncommitted changes (including pending merges)
   are present in the working tree (if one is present) and no revision is
-  speified. The configuration option ``push_strict`` can be used to set the
+  specified. The configuration option ``push_strict`` can be used to set the
   default for this behavior.  (Vincent Ladeuil, #284038, #322808, #65286)
 
 * ``bzr revno`` and ``bzr revision-info`` now have a ``--tree`` option to

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2009-07-01 10:30:41 +0000
+++ bzrlib/branch.py	2009-07-24 03:35:24 +0000
@@ -35,6 +35,7 @@
         symbol_versioning,
         transport,
         tsort,
+        ui,
         urlutils,
         )
 from bzrlib.config import BranchConfig, TransportConfig
@@ -662,6 +663,9 @@
         """
         if not self._format.supports_stacking():
             raise errors.UnstackableBranchFormat(self._format, self.base)
+        # XXX: Changing from one fallback repository to another does not check
+        # that all the data you need is present in the new fallback.
+        # Possibly it should.
         self._check_stackable_repo()
         if not url:
             try:
@@ -669,27 +673,67 @@
             except (errors.NotStacked, errors.UnstackableBranchFormat,
                 errors.UnstackableRepositoryFormat):
                 return
-            url = ''
-            # XXX: Lock correctness - should unlock our old repo if we were
-            # locked.
-            # repositories don't offer an interface to remove fallback
-            # repositories today; take the conceptually simpler option and just
-            # reopen it.
-            self.repository = self.bzrdir.find_repository()
-            self.repository.lock_write()
-            # for every revision reference the branch has, ensure it is pulled
-            # in.
-            source_repository = self._get_fallback_repository(old_url)
-            for revision_id in chain([self.last_revision()],
-                self.tags.get_reverse_tag_dict()):
-                self.repository.fetch(source_repository, revision_id,
-                    find_ghosts=True)
+            self._unstack()
         else:
             self._activate_fallback_location(url)
         # write this out after the repository is stacked to avoid setting a
         # stacked config that doesn't work.
         self._set_config_location('stacked_on_location', url)
 
+    def _unstack(self):
+        """Change a branch to be unstacked, copying data as needed.
+        
+        Don't call this directly, use set_stacked_on_url(None).
+        """
+        pb = ui.ui_factory.nested_progress_bar()
+        try:
+            pb.update("Unstacking")
+            # The basic approach here is to fetch the tip of the branch,
+            # including all available ghosts, from the existing stacked
+            # repository into a new repository object without the fallbacks. 
+            #
+            # XXX: See <https://launchpad.net/bugs/397286> - this may not be
+            # correct for CHKMap repostiories
+            old_repository = self.repository
+            if len(old_repository._fallback_repositories) != 1:
+                raise AssertionError("can't cope with fallback repositories "
+                    "of %r" % (self.repository,))
+            # unlock it, including unlocking the fallback
+            old_repository.unlock()
+            old_repository.lock_read()
+            try:
+                # Repositories don't offer an interface to remove fallback
+                # repositories today; take the conceptually simpler option and just
+                # reopen it.  We reopen it starting from the URL so that we
+                # get a separate connection for RemoteRepositories and can
+                # stream from one of them to the other.  This does mean doing
+                # separate SSH connection setup, but unstacking is not a
+                # common operation so it's tolerable.
+                new_bzrdir = bzrdir.BzrDir.open(self.bzrdir.root_transport.base)
+                new_repository = new_bzrdir.find_repository()
+                self.repository = new_repository
+                if self.repository._fallback_repositories:
+                    raise AssertionError("didn't expect %r to have "
+                        "fallback_repositories"
+                        % (self.repository,))
+                # this is not paired with an unlock because it's just restoring
+                # the previous state; the lock's released when set_stacked_on_url
+                # returns
+                self.repository.lock_write()
+                # XXX: If you unstack a branch while it has a working tree
+                # with a pending merge, the pending-merged revisions will no
+                # longer be present.  You can (probably) revert and remerge.
+                #
+                # XXX: This only fetches up to the tip of the repository; it
+                # doesn't bring across any tags.  That's fairly consistent
+                # with how branch works, but perhaps not ideal.
+                self.repository.fetch(old_repository,
+                    revision_id=self.last_revision(),
+                    find_ghosts=True)
+            finally:
+                old_repository.unlock()
+        finally:
+            pb.finished()
 
     def _set_tags_bytes(self, bytes):
         """Mirror method for _get_tags_bytes.

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-07-19 04:47:49 +0000
+++ bzrlib/builtins.py	2009-07-24 03:35:24 +0000
@@ -5259,14 +5259,37 @@
             ),
         Option('bind-to', help='Branch to bind checkout to.', type=str),
         Option('force',
-               help='Perform reconfiguration even if local changes'
-               ' will be lost.')
+            help='Perform reconfiguration even if local changes'
+            ' will be lost.'),
+        Option('stacked-on',
+            help='Reconfigure a branch to be stacked on another branch.',
+            type=unicode,
+            ),
+        Option('unstacked',
+            help='Reconfigure a branch to be unstacked.  This '
+                'may require copying substantial data into it.',
+            ),
         ]
 
-    def run(self, location=None, target_type=None, bind_to=None, force=False):
+    def run(self, location=None, target_type=None, bind_to=None, force=False,
+            stacked_on=None,
+            unstacked=None):
         directory = bzrdir.BzrDir.open(location)
+        if stacked_on and unstacked:
+            raise BzrCommandError("Can't use both --stacked-on and --unstacked")
+        elif stacked_on is not None:
+            reconfigure.ReconfigureStackedOn().apply(directory, stacked_on)
+        elif unstacked:
+            reconfigure.ReconfigureUnstacked().apply(directory)
+        # At the moment you can use --stacked-on and a different
+        # reconfiguration shape at the same time; there seems no good reason
+        # to ban it.
         if target_type is None:
-            raise errors.BzrCommandError('No target configuration specified')
+            if stacked_on or unstacked:
+                return
+            else:
+                raise errors.BzrCommandError('No target configuration '
+                    'specified')
         elif target_type == 'branch':
             reconfiguration = reconfigure.Reconfigure.to_branch(directory)
         elif target_type == 'tree':

=== modified file 'bzrlib/bundle/serializer/v08.py'
--- bzrlib/bundle/serializer/v08.py	2009-05-06 05:36:28 +0000
+++ bzrlib/bundle/serializer/v08.py	2009-07-24 03:35:25 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006 Canonical Ltd
+# Copyright (C) 2005, 2006, 2009 Canonical Ltd
 #
 # 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
@@ -19,7 +19,10 @@
 
 import os
 
-from bzrlib import errors
+from bzrlib import (
+    errors,
+    ui,
+    )
 from bzrlib.bundle.serializer import (BundleSerializer,
                                       _get_bundle_header,
                                      )
@@ -27,9 +30,7 @@
 from bzrlib.bundle.bundle_data import (RevisionInfo, BundleInfo, BundleTree)
 from bzrlib.diff import internal_diff
 from bzrlib.osutils import pathjoin
-from bzrlib.progress import DummyProgress
 from bzrlib.revision import NULL_REVISION
-import bzrlib.ui
 from bzrlib.testament import StrictTestament
 from bzrlib.timestamp import (
     format_highres_date,
@@ -119,12 +120,11 @@
         source.lock_read()
         try:
             self._write_main_header()
-            pb = DummyProgress()
+            pb = ui.ui_factory.nested_progress_bar()
             try:
                 self._write_revisions(pb)
             finally:
-                pass
-                #pb.finished()
+                pb.finished()
         finally:
             source.unlock()
 
@@ -183,7 +183,7 @@
 
         i_max = len(self.revision_ids)
         for i, rev_id in enumerate(self.revision_ids):
-            pb.update("Generating revsion data", i, i_max)
+            pb.update("Generating revision data", i, i_max)
             rev = self.source.get_revision(rev_id)
             if rev_id == last_rev_id:
                 rev_tree = last_rev_tree

=== modified file 'bzrlib/fetch.py'
--- bzrlib/fetch.py	2009-06-17 17:57:15 +0000
+++ bzrlib/fetch.py	2009-07-24 03:35:24 +0000
@@ -59,11 +59,8 @@
                 symbol_versioning.deprecated_in((1, 14, 0))
                 % "pb parameter to RepoFetcher.__init__")
             # and for simplicity it is in fact ignored
-        if to_repository.has_same_location(from_repository):
-            # repository.fetch should be taking care of this case.
-            raise errors.BzrError('RepoFetcher run '
-                    'between two objects at the same location: '
-                    '%r and %r' % (to_repository, from_repository))
+        # repository.fetch has the responsibility for short-circuiting
+        # attempts to copy between a repository and itself.
         self.to_repository = to_repository
         self.from_repository = from_repository
         self.sink = to_repository._get_sink()

=== modified file 'bzrlib/help_topics/en/debug-flags.txt'
--- bzrlib/help_topics/en/debug-flags.txt	2009-07-16 00:01:17 +0000
+++ bzrlib/help_topics/en/debug-flags.txt	2009-07-24 03:35:25 +0000
@@ -23,5 +23,6 @@
 -Dknit            Trace knit operations.
 -Dlock            Trace when lockdir locks are taken or released.
 -Dmerge           Emit information for debugging merges.
+-Dunlock          Some errors during unlock are treated as warnings.
 -Dpack            Emit information about pack operations.
 -Dsftp            Trace SFTP internals.

=== modified file 'bzrlib/lock.py'
--- bzrlib/lock.py	2009-07-07 09:00:59 +0000
+++ bzrlib/lock.py	2009-07-24 03:35:24 +0000
@@ -37,8 +37,10 @@
 import errno
 import os
 import sys
+import warnings
 
 from bzrlib import (
+    debug,
     errors,
     osutils,
     trace,
@@ -86,6 +88,23 @@
                              self.lock_url, self.details)
 
 
+def cant_unlock_not_held(locked_object):
+    """An attempt to unlock failed because the object was not locked.
+
+    This provides a policy point from which we can generate either a warning 
+    or an exception.
+    """
+    # This is typically masking some other error and called from a finally
+    # block, so it's useful to have the option not to generate a new error
+    # here.  You can use -Werror to make it fatal.  It should possibly also
+    # raise LockNotHeld.
+    if 'unlock' in debug.debug_flags:
+        warnings.warn("%r is already unlocked" % (locked_object,),
+            stacklevel=3)
+    else:
+        raise errors.LockNotHeld(locked_object)
+
+
 try:
     import fcntl
     have_fcntl = True

=== modified file 'bzrlib/lockable_files.py'
--- bzrlib/lockable_files.py	2009-03-25 04:20:12 +0000
+++ bzrlib/lockable_files.py	2009-07-24 03:35:24 +0000
@@ -24,6 +24,7 @@
 from bzrlib import (
     counted_lock,
     errors,
+    lock,
     osutils,
     transactions,
     urlutils,
@@ -308,7 +309,7 @@
 
     def unlock(self):
         if not self._lock_mode:
-            raise errors.LockNotHeld(self)
+            return lock.cant_unlock_not_held(self)
         if self._lock_warner.lock_count > 1:
             self._lock_warner.lock_count -= 1
         else:

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py	2009-05-08 15:39:11 +0000
+++ bzrlib/lockdir.py	2009-07-24 03:35:24 +0000
@@ -293,7 +293,7 @@
             self._fake_read_lock = False
             return
         if not self._lock_held:
-            raise LockNotHeld(self)
+            return lock.cant_unlock_not_held(self)
         if self._locked_via_token:
             self._locked_via_token = False
             self._lock_held = False

=== modified file 'bzrlib/reconfigure.py'
--- bzrlib/reconfigure.py	2009-07-10 08:33:11 +0000
+++ bzrlib/reconfigure.py	2009-07-24 03:35:24 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007 Canonical Ltd
+# Copyright (C) 2007, 2009 Canonical Ltd
 #
 # 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
@@ -14,14 +14,62 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
-"""Reconfigure a bzrdir into a new tree/branch/repository layout"""
+"""Reconfigure a bzrdir into a new tree/branch/repository layout.
+
+Various types of reconfiguration operation are available either by
+constructing a class or using a factory method on Reconfigure.
+"""
+
 
 from bzrlib import (
     branch,
     bzrdir,
     errors,
+    trace,
+    ui,
+    urlutils,
     )
 
+
+# TODO: common base class for all reconfigure operations, making no
+# assumptions about what kind of change will be done.
+
+
+class ReconfigureStackedOn(object):
+    """Reconfigures a branch to be stacked on another branch."""
+
+    def apply(self, bzrdir, stacked_on_url):
+        branch = bzrdir.open_branch()
+        # it may be a path relative to the cwd or a url; the branch wants
+        # a path relative to itself...
+        on_url = urlutils.relative_url(branch.base,
+            urlutils.normalize_url(stacked_on_url))
+        branch.lock_write()
+        try:
+            branch.set_stacked_on_url(on_url)
+            if not trace.is_quiet():
+                ui.ui_factory.note(
+                    "%s is now stacked on %s\n"
+                    % (branch.base, branch.get_stacked_on_url()))
+        finally:
+            branch.unlock()
+
+
+class ReconfigureUnstacked(object):
+
+    def apply(self, bzrdir):
+        branch = bzrdir.open_branch()
+        branch.lock_write()
+        try:
+            branch.set_stacked_on_url(None)
+            if not trace.is_quiet():
+                ui.ui_factory.note(
+                    "%s is now not stacked\n"
+                    % (branch.base,))
+        finally:
+            branch.unlock()
+
+
 class Reconfigure(object):
 
     def __init__(self, bzrdir, new_bound_location=None):

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2009-07-06 09:47:35 +0000
+++ bzrlib/remote.py	2009-07-24 03:35:24 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2008 Canonical Ltd
+# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
 #
 # 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
@@ -14,9 +14,6 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
-# TODO: At some point, handle upgrades by just passing the whole request
-# across to run on the server.
-
 import bz2
 
 from bzrlib import (
@@ -27,6 +24,7 @@
     debug,
     errors,
     graph,
+    lock,
     lockdir,
     repository,
     revision,
@@ -813,7 +811,23 @@
             result.add(_mod_revision.NULL_REVISION)
         return result
 
+    def _has_same_fallbacks(self, other_repo):
+        """Returns true if the repositories have the same fallbacks."""
+        # XXX: copied from Repository; it should be unified into a base class
+        # <https://bugs.edge.launchpad.net/bzr/+bug/401622>
+        my_fb = self._fallback_repositories
+        other_fb = other_repo._fallback_repositories
+        if len(my_fb) != len(other_fb):
+            return False
+        for f, g in zip(my_fb, other_fb):
+            if not f.has_same_location(g):
+                return False
+        return True
+
     def has_same_location(self, other):
+        # TODO: Move to RepositoryBase and unify with the regular Repository
+        # one; unfortunately the tests rely on slightly different behaviour at
+        # present -- mbp 20090710
         return (self.__class__ is other.__class__ and
                 self.bzrdir.transport.base == other.bzrdir.transport.base)
 
@@ -1025,7 +1039,7 @@
 
     def unlock(self):
         if not self._lock_count:
-            raise errors.LockNotHeld(self)
+            return lock.cant_unlock_not_held(self)
         self._lock_count -= 1
         if self._lock_count > 0:
             return
@@ -1230,7 +1244,9 @@
             raise errors.InternalBzrError(
                 "May not fetch while in a write group.")
         # fast path same-url fetch operations
-        if self.has_same_location(source) and fetch_spec is None:
+        if (self.has_same_location(source)
+            and fetch_spec is None
+            and self._has_same_fallbacks(source)):
             # check that last_revision is in 'from' and then return a
             # no-operation.
             if (revision_id is not None and

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2009-07-02 23:10:53 +0000
+++ bzrlib/repository.py	2009-07-24 03:35:25 +0000
@@ -848,6 +848,7 @@
 ######################################################################
 # Repositories
 
+
 class Repository(object):
     """Repository holding history for one or more branches.
 
@@ -1183,8 +1184,25 @@
         self._inventory_entry_cache = fifo_cache.FIFOCache(10*1024)
 
     def __repr__(self):
-        return '%s(%r)' % (self.__class__.__name__,
-                           self.base)
+        if self._fallback_repositories:
+            return '%s(%r, fallback_repositories=%r)' % (
+                self.__class__.__name__,
+                self.base,
+                self._fallback_repositories)
+        else:
+            return '%s(%r)' % (self.__class__.__name__,
+                               self.base)
+
+    def _has_same_fallbacks(self, other_repo):
+        """Returns true if the repositories have the same fallbacks."""
+        my_fb = self._fallback_repositories
+        other_fb = other_repo._fallback_repositories
+        if len(my_fb) != len(other_fb):
+            return False
+        for f, g in zip(my_fb, other_fb):
+            if not f.has_same_location(g):
+                return False
+        return True
 
     def has_same_location(self, other):
         """Returns a boolean indicating if this repository is at the same
@@ -1529,7 +1547,11 @@
             raise errors.InternalBzrError(
                 "May not fetch while in a write group.")
         # fast path same-url fetch operations
-        if self.has_same_location(source) and fetch_spec is None:
+        # TODO: lift out to somewhere common with RemoteRepository
+        # <https://bugs.edge.launchpad.net/bzr/+bug/401646>
+        if (self.has_same_location(source)
+            and fetch_spec is None
+            and self._has_same_fallbacks(source)):
             # check that last_revision is in 'from' and then return a
             # no-operation.
             if (revision_id is not None and

=== modified file 'bzrlib/tests/blackbox/test_reconfigure.py'
--- bzrlib/tests/blackbox/test_reconfigure.py	2009-05-07 05:08:46 +0000
+++ bzrlib/tests/blackbox/test_reconfigure.py	2009-07-24 03:35:25 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007 Canonical Ltd
+# Copyright (C) 2007, 2009 Canonical Ltd
 #
 # 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
@@ -20,6 +20,7 @@
     tests,
     workingtree,
     )
+from bzrlib.branchbuilder import BranchBuilder
 
 
 class TestReconfigure(tests.TestCaseWithTransport):
@@ -182,3 +183,59 @@
 
     def test_lightweight_rich_root_pack_checkout_to_tree(self, format=None):
         self.test_lightweight_format_checkout_to_tree('rich-root-pack')
+
+
+class TestReconfigureStacking(tests.TestCaseWithTransport):
+
+    def test_reconfigure_stacking(self):
+        """Test a fairly realistic scenario for stacking:
+
+         * make a branch with some history
+         * branch it
+         * make the second branch stacked on the first
+         * commit in the second
+         * then make the second unstacked, so it has to fill in history from
+           the original fallback lying underneath its original content
+
+        See discussion in <https://bugs.edge.launchpad.net/bzr/+bug/391411>
+        """
+        # there are also per_branch tests that exercise remote operation etc
+        tree_1 = self.make_branch_and_tree('b1', format='2a')
+        self.build_tree(['b1/foo'])
+        tree_1.add(['foo'])
+        tree_1.commit('add foo')
+        branch_1 = tree_1.branch
+        # now branch and commit again
+        bzrdir_2 = tree_1.bzrdir.sprout('b2')
+        tree_2 = bzrdir_2.open_workingtree()
+        branch_2 = tree_2.branch
+        # now reconfigure to be stacked
+        out, err = self.run_bzr('reconfigure --stacked-on b1 b2')
+        self.assertContainsRe(out,
+            '^.*/b2/ is now stacked on ../b1\n$')
+        self.assertEquals('', err)
+        # can also give the absolute URL of the branch, and it gets stored 
+        # as a relative path if possible
+        out, err = self.run_bzr('reconfigure --stacked-on %s b2'
+            % (self.get_url('b1'),))
+        self.assertContainsRe(out,
+            '^.*/b2/ is now stacked on ../b1\n$')
+        self.assertEquals('', err)
+        # It should be given a relative URL to the destination, if possible,
+        # because that's most likely to work across different transports
+        self.assertEquals(branch_2.get_stacked_on_url(),
+            '../b1')
+        # commit, and it should be stored into b2's repo
+        self.build_tree_contents([('foo', 'new foo')])
+        tree_2.commit('update foo')
+        # Now turn it off again
+        out, err = self.run_bzr('reconfigure --unstacked b2')
+        self.assertContainsRe(out,
+            '^.*/b2/ is now not stacked\n$')
+        self.assertEquals('', err)
+        self.assertRaises(errors.NotStacked,
+            branch_2.get_stacked_on_url)
+
+    # XXX: Needs a test for reconfiguring stacking and shape at the same time;
+    # no branch at location; stacked-on is not a branch; quiet mode.
+    # -- mbp 20090706

=== modified file 'bzrlib/tests/per_branch/test_stacking.py'
--- bzrlib/tests/per_branch/test_stacking.py	2009-07-10 05:49:34 +0000
+++ bzrlib/tests/per_branch/test_stacking.py	2009-07-24 03:35:25 +0000
@@ -184,11 +184,17 @@
         trunk_revid = trunk_tree.commit('revision on mainline')
         # and make branch from it which is stacked
         try:
-            new_dir = trunk_tree.bzrdir.sprout('newbranch', stacked=True)
+            new_dir = trunk_tree.bzrdir.sprout(self.get_url('newbranch'),
+                stacked=True)
         except unstackable_format_errors, e:
             raise TestNotApplicable(e)
         # stacked repository
         self.assertRevisionNotInRepository('newbranch', trunk_revid)
+        # TODO: we'd like to commit in the stacked repository; that requires
+        # some care (maybe a BranchBuilder) if it's remote and has no
+        # workingtree
+        ##newbranch_revid = new_dir.open_workingtree().commit('revision in '
+            ##'newbranch')
         # now when we unstack that should implicitly fetch, to make sure that
         # the branch will still work
         new_branch = new_dir.open_branch()

=== modified file 'doc/en/user-guide/stacked.txt'
--- doc/en/user-guide/stacked.txt	2008-07-17 06:00:08 +0000
+++ doc/en/user-guide/stacked.txt	2009-07-24 03:35:25 +0000
@@ -83,3 +83,15 @@
 stacked-on branch needs to be available for almost all operations. This is
 not an issue when both branches are local or both branches are on the
 same server.
+
+
+Changing branch stacking
+------------------------
+
+Stacking of existing branches can be changed using the ``bzr reconfigure``
+command to either stack on an existing branch, or to turn off stacking.
+Be aware that when ``bzr reconfigure --unstacked`` is used, bzr will
+copy all the referenced data from the stacked-on repository into the
+previously stacked repository.  For large repositories this may take
+considerable time and may substantially increase the size of the
+repository.