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

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.