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

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

It's both: it's duplication that's unnecessary, and also it's now the
wrong condition: you should be able to copy between repositories at
the same location if they have different fallbacks.

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

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