Code review comment for lp:~mbp/bzr/391411-reconfigure-stacked

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