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

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