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

Proposed by Martin Pool
Status: Merged
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/391411-reconfigure-stacked
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 656 lines
To merge this branch: bzr merge lp:~mbp/bzr/391411-reconfigure-stacked
Reviewer Review Type Date Requested Status
Ian Clatworthy Needs Fixing
Review via email: mp+8527@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote :

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/>

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :
Download full text (4.0 KiB)

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...

Read more...

Revision history for this message
Martin Pool (mbp) wrote :

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/>

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

See comments above from Robert and I.

review: Needs Fixing
Revision history for this message
Martin Pool (mbp) wrote :

> 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

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :

> 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...

Revision history for this message
Martin Pool (mbp) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2009-07-23 17:00:27 +0000
+++ NEWS 2009-07-24 03:35:23 +0000
@@ -16,8 +16,12 @@
16New Features16New Features
17************17************
1818
19* ``merge --interactive`` applies a user-selected portion of the merge. The UI19* ``bzr merge --interactive`` applies a user-selected portion of the
20 is similar to ``shelve``. (Aaron Bentley)20 merge. The UI is similar to ``shelve``. (Aaron Bentley)
21
22* ``bzr reconfigure`` now takes options ``--stacked-on URL`` and
23 ``--unstacked`` to change stacking of a branch.
24 (Martin Pool, #391411)
2125
22Bug Fixes26Bug Fixes
23*********27*********
@@ -172,7 +176,7 @@
172176
173* ``bzr push`` now aborts if uncommitted changes (including pending merges)177* ``bzr push`` now aborts if uncommitted changes (including pending merges)
174 are present in the working tree (if one is present) and no revision is178 are present in the working tree (if one is present) and no revision is
175 speified. The configuration option ``push_strict`` can be used to set the179 specified. The configuration option ``push_strict`` can be used to set the
176 default for this behavior. (Vincent Ladeuil, #284038, #322808, #65286)180 default for this behavior. (Vincent Ladeuil, #284038, #322808, #65286)
177181
178* ``bzr revno`` and ``bzr revision-info`` now have a ``--tree`` option to182* ``bzr revno`` and ``bzr revision-info`` now have a ``--tree`` option to
179183
=== 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 @@
35 symbol_versioning,35 symbol_versioning,
36 transport,36 transport,
37 tsort,37 tsort,
38 ui,
38 urlutils,39 urlutils,
39 )40 )
40from bzrlib.config import BranchConfig, TransportConfig41from bzrlib.config import BranchConfig, TransportConfig
@@ -662,6 +663,9 @@
662 """663 """
663 if not self._format.supports_stacking():664 if not self._format.supports_stacking():
664 raise errors.UnstackableBranchFormat(self._format, self.base)665 raise errors.UnstackableBranchFormat(self._format, self.base)
666 # XXX: Changing from one fallback repository to another does not check
667 # that all the data you need is present in the new fallback.
668 # Possibly it should.
665 self._check_stackable_repo()669 self._check_stackable_repo()
666 if not url:670 if not url:
667 try:671 try:
@@ -669,27 +673,67 @@
669 except (errors.NotStacked, errors.UnstackableBranchFormat,673 except (errors.NotStacked, errors.UnstackableBranchFormat,
670 errors.UnstackableRepositoryFormat):674 errors.UnstackableRepositoryFormat):
671 return675 return
672 url = ''676 self._unstack()
673 # XXX: Lock correctness - should unlock our old repo if we were
674 # locked.
675 # repositories don't offer an interface to remove fallback
676 # repositories today; take the conceptually simpler option and just
677 # reopen it.
678 self.repository = self.bzrdir.find_repository()
679 self.repository.lock_write()
680 # for every revision reference the branch has, ensure it is pulled
681 # in.
682 source_repository = self._get_fallback_repository(old_url)
683 for revision_id in chain([self.last_revision()],
684 self.tags.get_reverse_tag_dict()):
685 self.repository.fetch(source_repository, revision_id,
686 find_ghosts=True)
687 else:677 else:
688 self._activate_fallback_location(url)678 self._activate_fallback_location(url)
689 # write this out after the repository is stacked to avoid setting a679 # write this out after the repository is stacked to avoid setting a
690 # stacked config that doesn't work.680 # stacked config that doesn't work.
691 self._set_config_location('stacked_on_location', url)681 self._set_config_location('stacked_on_location', url)
692682
683 def _unstack(self):
684 """Change a branch to be unstacked, copying data as needed.
685
686 Don't call this directly, use set_stacked_on_url(None).
687 """
688 pb = ui.ui_factory.nested_progress_bar()
689 try:
690 pb.update("Unstacking")
691 # The basic approach here is to fetch the tip of the branch,
692 # including all available ghosts, from the existing stacked
693 # repository into a new repository object without the fallbacks.
694 #
695 # XXX: See <https://launchpad.net/bugs/397286> - this may not be
696 # correct for CHKMap repostiories
697 old_repository = self.repository
698 if len(old_repository._fallback_repositories) != 1:
699 raise AssertionError("can't cope with fallback repositories "
700 "of %r" % (self.repository,))
701 # unlock it, including unlocking the fallback
702 old_repository.unlock()
703 old_repository.lock_read()
704 try:
705 # Repositories don't offer an interface to remove fallback
706 # repositories today; take the conceptually simpler option and just
707 # reopen it. We reopen it starting from the URL so that we
708 # get a separate connection for RemoteRepositories and can
709 # stream from one of them to the other. This does mean doing
710 # separate SSH connection setup, but unstacking is not a
711 # common operation so it's tolerable.
712 new_bzrdir = bzrdir.BzrDir.open(self.bzrdir.root_transport.base)
713 new_repository = new_bzrdir.find_repository()
714 self.repository = new_repository
715 if self.repository._fallback_repositories:
716 raise AssertionError("didn't expect %r to have "
717 "fallback_repositories"
718 % (self.repository,))
719 # this is not paired with an unlock because it's just restoring
720 # the previous state; the lock's released when set_stacked_on_url
721 # returns
722 self.repository.lock_write()
723 # XXX: If you unstack a branch while it has a working tree
724 # with a pending merge, the pending-merged revisions will no
725 # longer be present. You can (probably) revert and remerge.
726 #
727 # XXX: This only fetches up to the tip of the repository; it
728 # doesn't bring across any tags. That's fairly consistent
729 # with how branch works, but perhaps not ideal.
730 self.repository.fetch(old_repository,
731 revision_id=self.last_revision(),
732 find_ghosts=True)
733 finally:
734 old_repository.unlock()
735 finally:
736 pb.finished()
693737
694 def _set_tags_bytes(self, bytes):738 def _set_tags_bytes(self, bytes):
695 """Mirror method for _get_tags_bytes.739 """Mirror method for _get_tags_bytes.
696740
=== 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 @@
5259 ),5259 ),
5260 Option('bind-to', help='Branch to bind checkout to.', type=str),5260 Option('bind-to', help='Branch to bind checkout to.', type=str),
5261 Option('force',5261 Option('force',
5262 help='Perform reconfiguration even if local changes'5262 help='Perform reconfiguration even if local changes'
5263 ' will be lost.')5263 ' will be lost.'),
5264 Option('stacked-on',
5265 help='Reconfigure a branch to be stacked on another branch.',
5266 type=unicode,
5267 ),
5268 Option('unstacked',
5269 help='Reconfigure a branch to be unstacked. This '
5270 'may require copying substantial data into it.',
5271 ),
5264 ]5272 ]
52655273
5266 def run(self, location=None, target_type=None, bind_to=None, force=False):5274 def run(self, location=None, target_type=None, bind_to=None, force=False,
5275 stacked_on=None,
5276 unstacked=None):
5267 directory = bzrdir.BzrDir.open(location)5277 directory = bzrdir.BzrDir.open(location)
5278 if stacked_on and unstacked:
5279 raise BzrCommandError("Can't use both --stacked-on and --unstacked")
5280 elif stacked_on is not None:
5281 reconfigure.ReconfigureStackedOn().apply(directory, stacked_on)
5282 elif unstacked:
5283 reconfigure.ReconfigureUnstacked().apply(directory)
5284 # At the moment you can use --stacked-on and a different
5285 # reconfiguration shape at the same time; there seems no good reason
5286 # to ban it.
5268 if target_type is None:5287 if target_type is None:
5269 raise errors.BzrCommandError('No target configuration specified')5288 if stacked_on or unstacked:
5289 return
5290 else:
5291 raise errors.BzrCommandError('No target configuration '
5292 'specified')
5270 elif target_type == 'branch':5293 elif target_type == 'branch':
5271 reconfiguration = reconfigure.Reconfigure.to_branch(directory)5294 reconfiguration = reconfigure.Reconfigure.to_branch(directory)
5272 elif target_type == 'tree':5295 elif target_type == 'tree':
52735296
=== 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 @@
1# Copyright (C) 2005, 2006 Canonical Ltd1# Copyright (C) 2005, 2006, 2009 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -19,7 +19,10 @@
1919
20import os20import os
2121
22from bzrlib import errors22from bzrlib import (
23 errors,
24 ui,
25 )
23from bzrlib.bundle.serializer import (BundleSerializer,26from bzrlib.bundle.serializer import (BundleSerializer,
24 _get_bundle_header,27 _get_bundle_header,
25 )28 )
@@ -27,9 +30,7 @@
27from bzrlib.bundle.bundle_data import (RevisionInfo, BundleInfo, BundleTree)30from bzrlib.bundle.bundle_data import (RevisionInfo, BundleInfo, BundleTree)
28from bzrlib.diff import internal_diff31from bzrlib.diff import internal_diff
29from bzrlib.osutils import pathjoin32from bzrlib.osutils import pathjoin
30from bzrlib.progress import DummyProgress
31from bzrlib.revision import NULL_REVISION33from bzrlib.revision import NULL_REVISION
32import bzrlib.ui
33from bzrlib.testament import StrictTestament34from bzrlib.testament import StrictTestament
34from bzrlib.timestamp import (35from bzrlib.timestamp import (
35 format_highres_date,36 format_highres_date,
@@ -119,12 +120,11 @@
119 source.lock_read()120 source.lock_read()
120 try:121 try:
121 self._write_main_header()122 self._write_main_header()
122 pb = DummyProgress()123 pb = ui.ui_factory.nested_progress_bar()
123 try:124 try:
124 self._write_revisions(pb)125 self._write_revisions(pb)
125 finally:126 finally:
126 pass127 pb.finished()
127 #pb.finished()
128 finally:128 finally:
129 source.unlock()129 source.unlock()
130130
@@ -183,7 +183,7 @@
183183
184 i_max = len(self.revision_ids)184 i_max = len(self.revision_ids)
185 for i, rev_id in enumerate(self.revision_ids):185 for i, rev_id in enumerate(self.revision_ids):
186 pb.update("Generating revsion data", i, i_max)186 pb.update("Generating revision data", i, i_max)
187 rev = self.source.get_revision(rev_id)187 rev = self.source.get_revision(rev_id)
188 if rev_id == last_rev_id:188 if rev_id == last_rev_id:
189 rev_tree = last_rev_tree189 rev_tree = last_rev_tree
190190
=== 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 @@
59 symbol_versioning.deprecated_in((1, 14, 0))59 symbol_versioning.deprecated_in((1, 14, 0))
60 % "pb parameter to RepoFetcher.__init__")60 % "pb parameter to RepoFetcher.__init__")
61 # and for simplicity it is in fact ignored61 # and for simplicity it is in fact ignored
62 if to_repository.has_same_location(from_repository):62 # repository.fetch has the responsibility for short-circuiting
63 # repository.fetch should be taking care of this case.63 # attempts to copy between a repository and itself.
64 raise errors.BzrError('RepoFetcher run '
65 'between two objects at the same location: '
66 '%r and %r' % (to_repository, from_repository))
67 self.to_repository = to_repository64 self.to_repository = to_repository
68 self.from_repository = from_repository65 self.from_repository = from_repository
69 self.sink = to_repository._get_sink()66 self.sink = to_repository._get_sink()
7067
=== 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 @@
23-Dknit Trace knit operations.23-Dknit Trace knit operations.
24-Dlock Trace when lockdir locks are taken or released.24-Dlock Trace when lockdir locks are taken or released.
25-Dmerge Emit information for debugging merges.25-Dmerge Emit information for debugging merges.
26-Dunlock Some errors during unlock are treated as warnings.
26-Dpack Emit information about pack operations.27-Dpack Emit information about pack operations.
27-Dsftp Trace SFTP internals.28-Dsftp Trace SFTP internals.
2829
=== 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 @@
37import errno37import errno
38import os38import os
39import sys39import sys
40import warnings
4041
41from bzrlib import (42from bzrlib import (
43 debug,
42 errors,44 errors,
43 osutils,45 osutils,
44 trace,46 trace,
@@ -86,6 +88,23 @@
86 self.lock_url, self.details)88 self.lock_url, self.details)
8789
8890
91def cant_unlock_not_held(locked_object):
92 """An attempt to unlock failed because the object was not locked.
93
94 This provides a policy point from which we can generate either a warning
95 or an exception.
96 """
97 # This is typically masking some other error and called from a finally
98 # block, so it's useful to have the option not to generate a new error
99 # here. You can use -Werror to make it fatal. It should possibly also
100 # raise LockNotHeld.
101 if 'unlock' in debug.debug_flags:
102 warnings.warn("%r is already unlocked" % (locked_object,),
103 stacklevel=3)
104 else:
105 raise errors.LockNotHeld(locked_object)
106
107
89try:108try:
90 import fcntl109 import fcntl
91 have_fcntl = True110 have_fcntl = True
92111
=== 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 @@
24from bzrlib import (24from bzrlib import (
25 counted_lock,25 counted_lock,
26 errors,26 errors,
27 lock,
27 osutils,28 osutils,
28 transactions,29 transactions,
29 urlutils,30 urlutils,
@@ -308,7 +309,7 @@
308309
309 def unlock(self):310 def unlock(self):
310 if not self._lock_mode:311 if not self._lock_mode:
311 raise errors.LockNotHeld(self)312 return lock.cant_unlock_not_held(self)
312 if self._lock_warner.lock_count > 1:313 if self._lock_warner.lock_count > 1:
313 self._lock_warner.lock_count -= 1314 self._lock_warner.lock_count -= 1
314 else:315 else:
315316
=== 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 @@
293 self._fake_read_lock = False293 self._fake_read_lock = False
294 return294 return
295 if not self._lock_held:295 if not self._lock_held:
296 raise LockNotHeld(self)296 return lock.cant_unlock_not_held(self)
297 if self._locked_via_token:297 if self._locked_via_token:
298 self._locked_via_token = False298 self._locked_via_token = False
299 self._lock_held = False299 self._lock_held = False
300300
=== 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 @@
1# Copyright (C) 2007 Canonical Ltd1# Copyright (C) 2007, 2009 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -14,14 +14,62 @@
14# along with this program; if not, write to the Free Software14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
1616
17"""Reconfigure a bzrdir into a new tree/branch/repository layout"""17"""Reconfigure a bzrdir into a new tree/branch/repository layout.
18
19Various types of reconfiguration operation are available either by
20constructing a class or using a factory method on Reconfigure.
21"""
22
1823
19from bzrlib import (24from bzrlib import (
20 branch,25 branch,
21 bzrdir,26 bzrdir,
22 errors,27 errors,
28 trace,
29 ui,
30 urlutils,
23 )31 )
2432
33
34# TODO: common base class for all reconfigure operations, making no
35# assumptions about what kind of change will be done.
36
37
38class ReconfigureStackedOn(object):
39 """Reconfigures a branch to be stacked on another branch."""
40
41 def apply(self, bzrdir, stacked_on_url):
42 branch = bzrdir.open_branch()
43 # it may be a path relative to the cwd or a url; the branch wants
44 # a path relative to itself...
45 on_url = urlutils.relative_url(branch.base,
46 urlutils.normalize_url(stacked_on_url))
47 branch.lock_write()
48 try:
49 branch.set_stacked_on_url(on_url)
50 if not trace.is_quiet():
51 ui.ui_factory.note(
52 "%s is now stacked on %s\n"
53 % (branch.base, branch.get_stacked_on_url()))
54 finally:
55 branch.unlock()
56
57
58class ReconfigureUnstacked(object):
59
60 def apply(self, bzrdir):
61 branch = bzrdir.open_branch()
62 branch.lock_write()
63 try:
64 branch.set_stacked_on_url(None)
65 if not trace.is_quiet():
66 ui.ui_factory.note(
67 "%s is now not stacked\n"
68 % (branch.base,))
69 finally:
70 branch.unlock()
71
72
25class Reconfigure(object):73class Reconfigure(object):
2674
27 def __init__(self, bzrdir, new_bound_location=None):75 def __init__(self, bzrdir, new_bound_location=None):
2876
=== 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 @@
1# Copyright (C) 2006, 2007, 2008 Canonical Ltd1# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -14,9 +14,6 @@
14# along with this program; if not, write to the Free Software14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
1616
17# TODO: At some point, handle upgrades by just passing the whole request
18# across to run on the server.
19
20import bz217import bz2
2118
22from bzrlib import (19from bzrlib import (
@@ -27,6 +24,7 @@
27 debug,24 debug,
28 errors,25 errors,
29 graph,26 graph,
27 lock,
30 lockdir,28 lockdir,
31 repository,29 repository,
32 revision,30 revision,
@@ -813,7 +811,23 @@
813 result.add(_mod_revision.NULL_REVISION)811 result.add(_mod_revision.NULL_REVISION)
814 return result812 return result
815813
814 def _has_same_fallbacks(self, other_repo):
815 """Returns true if the repositories have the same fallbacks."""
816 # XXX: copied from Repository; it should be unified into a base class
817 # <https://bugs.edge.launchpad.net/bzr/+bug/401622>
818 my_fb = self._fallback_repositories
819 other_fb = other_repo._fallback_repositories
820 if len(my_fb) != len(other_fb):
821 return False
822 for f, g in zip(my_fb, other_fb):
823 if not f.has_same_location(g):
824 return False
825 return True
826
816 def has_same_location(self, other):827 def has_same_location(self, other):
828 # TODO: Move to RepositoryBase and unify with the regular Repository
829 # one; unfortunately the tests rely on slightly different behaviour at
830 # present -- mbp 20090710
817 return (self.__class__ is other.__class__ and831 return (self.__class__ is other.__class__ and
818 self.bzrdir.transport.base == other.bzrdir.transport.base)832 self.bzrdir.transport.base == other.bzrdir.transport.base)
819833
@@ -1025,7 +1039,7 @@
10251039
1026 def unlock(self):1040 def unlock(self):
1027 if not self._lock_count:1041 if not self._lock_count:
1028 raise errors.LockNotHeld(self)1042 return lock.cant_unlock_not_held(self)
1029 self._lock_count -= 11043 self._lock_count -= 1
1030 if self._lock_count > 0:1044 if self._lock_count > 0:
1031 return1045 return
@@ -1230,7 +1244,9 @@
1230 raise errors.InternalBzrError(1244 raise errors.InternalBzrError(
1231 "May not fetch while in a write group.")1245 "May not fetch while in a write group.")
1232 # fast path same-url fetch operations1246 # fast path same-url fetch operations
1233 if self.has_same_location(source) and fetch_spec is None:1247 if (self.has_same_location(source)
1248 and fetch_spec is None
1249 and self._has_same_fallbacks(source)):
1234 # check that last_revision is in 'from' and then return a1250 # check that last_revision is in 'from' and then return a
1235 # no-operation.1251 # no-operation.
1236 if (revision_id is not None and1252 if (revision_id is not None and
12371253
=== 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 @@
848######################################################################848######################################################################
849# Repositories849# Repositories
850850
851
851class Repository(object):852class Repository(object):
852 """Repository holding history for one or more branches.853 """Repository holding history for one or more branches.
853854
@@ -1183,8 +1184,25 @@
1183 self._inventory_entry_cache = fifo_cache.FIFOCache(10*1024)1184 self._inventory_entry_cache = fifo_cache.FIFOCache(10*1024)
11841185
1185 def __repr__(self):1186 def __repr__(self):
1186 return '%s(%r)' % (self.__class__.__name__,1187 if self._fallback_repositories:
1187 self.base)1188 return '%s(%r, fallback_repositories=%r)' % (
1189 self.__class__.__name__,
1190 self.base,
1191 self._fallback_repositories)
1192 else:
1193 return '%s(%r)' % (self.__class__.__name__,
1194 self.base)
1195
1196 def _has_same_fallbacks(self, other_repo):
1197 """Returns true if the repositories have the same fallbacks."""
1198 my_fb = self._fallback_repositories
1199 other_fb = other_repo._fallback_repositories
1200 if len(my_fb) != len(other_fb):
1201 return False
1202 for f, g in zip(my_fb, other_fb):
1203 if not f.has_same_location(g):
1204 return False
1205 return True
11881206
1189 def has_same_location(self, other):1207 def has_same_location(self, other):
1190 """Returns a boolean indicating if this repository is at the same1208 """Returns a boolean indicating if this repository is at the same
@@ -1529,7 +1547,11 @@
1529 raise errors.InternalBzrError(1547 raise errors.InternalBzrError(
1530 "May not fetch while in a write group.")1548 "May not fetch while in a write group.")
1531 # fast path same-url fetch operations1549 # fast path same-url fetch operations
1532 if self.has_same_location(source) and fetch_spec is None:1550 # TODO: lift out to somewhere common with RemoteRepository
1551 # <https://bugs.edge.launchpad.net/bzr/+bug/401646>
1552 if (self.has_same_location(source)
1553 and fetch_spec is None
1554 and self._has_same_fallbacks(source)):
1533 # check that last_revision is in 'from' and then return a1555 # check that last_revision is in 'from' and then return a
1534 # no-operation.1556 # no-operation.
1535 if (revision_id is not None and1557 if (revision_id is not None and
15361558
=== 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 @@
1# Copyright (C) 2007 Canonical Ltd1# Copyright (C) 2007, 2009 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -20,6 +20,7 @@
20 tests,20 tests,
21 workingtree,21 workingtree,
22 )22 )
23from bzrlib.branchbuilder import BranchBuilder
2324
2425
25class TestReconfigure(tests.TestCaseWithTransport):26class TestReconfigure(tests.TestCaseWithTransport):
@@ -182,3 +183,59 @@
182183
183 def test_lightweight_rich_root_pack_checkout_to_tree(self, format=None):184 def test_lightweight_rich_root_pack_checkout_to_tree(self, format=None):
184 self.test_lightweight_format_checkout_to_tree('rich-root-pack')185 self.test_lightweight_format_checkout_to_tree('rich-root-pack')
186
187
188class TestReconfigureStacking(tests.TestCaseWithTransport):
189
190 def test_reconfigure_stacking(self):
191 """Test a fairly realistic scenario for stacking:
192
193 * make a branch with some history
194 * branch it
195 * make the second branch stacked on the first
196 * commit in the second
197 * then make the second unstacked, so it has to fill in history from
198 the original fallback lying underneath its original content
199
200 See discussion in <https://bugs.edge.launchpad.net/bzr/+bug/391411>
201 """
202 # there are also per_branch tests that exercise remote operation etc
203 tree_1 = self.make_branch_and_tree('b1', format='2a')
204 self.build_tree(['b1/foo'])
205 tree_1.add(['foo'])
206 tree_1.commit('add foo')
207 branch_1 = tree_1.branch
208 # now branch and commit again
209 bzrdir_2 = tree_1.bzrdir.sprout('b2')
210 tree_2 = bzrdir_2.open_workingtree()
211 branch_2 = tree_2.branch
212 # now reconfigure to be stacked
213 out, err = self.run_bzr('reconfigure --stacked-on b1 b2')
214 self.assertContainsRe(out,
215 '^.*/b2/ is now stacked on ../b1\n$')
216 self.assertEquals('', err)
217 # can also give the absolute URL of the branch, and it gets stored
218 # as a relative path if possible
219 out, err = self.run_bzr('reconfigure --stacked-on %s b2'
220 % (self.get_url('b1'),))
221 self.assertContainsRe(out,
222 '^.*/b2/ is now stacked on ../b1\n$')
223 self.assertEquals('', err)
224 # It should be given a relative URL to the destination, if possible,
225 # because that's most likely to work across different transports
226 self.assertEquals(branch_2.get_stacked_on_url(),
227 '../b1')
228 # commit, and it should be stored into b2's repo
229 self.build_tree_contents([('foo', 'new foo')])
230 tree_2.commit('update foo')
231 # Now turn it off again
232 out, err = self.run_bzr('reconfigure --unstacked b2')
233 self.assertContainsRe(out,
234 '^.*/b2/ is now not stacked\n$')
235 self.assertEquals('', err)
236 self.assertRaises(errors.NotStacked,
237 branch_2.get_stacked_on_url)
238
239 # XXX: Needs a test for reconfiguring stacking and shape at the same time;
240 # no branch at location; stacked-on is not a branch; quiet mode.
241 # -- mbp 20090706
185242
=== 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 @@
184 trunk_revid = trunk_tree.commit('revision on mainline')184 trunk_revid = trunk_tree.commit('revision on mainline')
185 # and make branch from it which is stacked185 # and make branch from it which is stacked
186 try:186 try:
187 new_dir = trunk_tree.bzrdir.sprout('newbranch', stacked=True)187 new_dir = trunk_tree.bzrdir.sprout(self.get_url('newbranch'),
188 stacked=True)
188 except unstackable_format_errors, e:189 except unstackable_format_errors, e:
189 raise TestNotApplicable(e)190 raise TestNotApplicable(e)
190 # stacked repository191 # stacked repository
191 self.assertRevisionNotInRepository('newbranch', trunk_revid)192 self.assertRevisionNotInRepository('newbranch', trunk_revid)
193 # TODO: we'd like to commit in the stacked repository; that requires
194 # some care (maybe a BranchBuilder) if it's remote and has no
195 # workingtree
196 ##newbranch_revid = new_dir.open_workingtree().commit('revision in '
197 ##'newbranch')
192 # now when we unstack that should implicitly fetch, to make sure that198 # now when we unstack that should implicitly fetch, to make sure that
193 # the branch will still work199 # the branch will still work
194 new_branch = new_dir.open_branch()200 new_branch = new_dir.open_branch()
195201
=== 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 @@
83stacked-on branch needs to be available for almost all operations. This is83stacked-on branch needs to be available for almost all operations. This is
84not an issue when both branches are local or both branches are on the84not an issue when both branches are local or both branches are on the
85same server.85same server.
86
87
88Changing branch stacking
89------------------------
90
91Stacking of existing branches can be changed using the ``bzr reconfigure``
92command to either stack on an existing branch, or to turn off stacking.
93Be aware that when ``bzr reconfigure --unstacked`` is used, bzr will
94copy all the referenced data from the stacked-on repository into the
95previously stacked repository. For large repositories this may take
96considerable time and may substantially increase the size of the
97repository.