[MERGE][#251871] Use source_branch.sprout in BzrDir.sprout when possible.

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
| Hi,
|
| This patch fixes bzr.dev so that branching with the bzr-svn plugin
(using the
| latest tip of the stable branch) works again.  When sprouting from an
SvnBranch,
| calling source._format.initialise(result) was failing because
SvnBranchFormat
| simply raises NotImplementedError, which is somewhat reasonable as
generally you
| cannot create them at arbitrary locations outside an SVN repository.
(Perhaps
| IncompatibleFormat or UninitializableFormat would be a more
appropriate error
| for bzr-svn to raise, but it doesn't really matter.)
|
| Basically, this patch just reverts BzrDir.sprout to calling
source_branch.sprout
| when it can, which is how things used to work.
|
| There's a bit of tension in BzrDir.sprout about which format to use
for the
| result_branch.  Generally it wants to preserve the source format, except:
|
|  * sometimes we require a stacking-compatible format, even if the
source isn't
|    stacking compatible.
|  * sometimes we might be cloning from a bzrdir with no branch
(although comments
|    in the code speculate that this may be a dead code path...)

I know we have an open bug for someone who wants to:

~  bzr branch -r revid:XXX repo repo/new-branch

It works if you are doing:

~  bzr branch -r revid:XXX repo/random_branch repo/new-branch

I agree, though, that we don't really know what to use in that case.


|  * sometimes, as with bzr-svn, the source format isn't actually
initializable.
|    This could happen with very old bzr formats too I suppose (those
that are no
|    longer supported for writing, just reading).
|
| This patch preserves the existing logic for the force-stacking and
| no-source-branch cases: they are special-cased and don't call
| source_branch.sprout.  The general case now relies on
source_branch.sprout to
| make an appropriate choice of format, which is how bzr-svn decides to
create
| rich-root-pack branches for SvnBranches.
|
| I'm pretty sure we'll be touching this code again at some point in the
future.
| For instance the duplication of Branch.sprout logic later in the
function smells
| a bit (as the comment acknowledges).  But this seems like a simple and
correct
| change for now, even if it doesn't get rid of the underlying
messiness.  It's a
| bit late in the release cycle for deep changes anyway...

I wonder about passing the stacking information into Branch.sprout . It
might be a nice way to have rich-root-pack create a 1.6-rich-root
target, rather than the default format.

|
| This patch currently lacks a test: perhaps an appropriate test would
be that
| sprouting a bzrdir works when the a source branch has:
|   * an uninitialisable format
|   * a working sprout method
|
| I'm have a niggling feeling that's too specific, but perhaps it's fine.
|
| I've tested this branch quite a bit manually.  With the attached
bundle, and
| this small patch to bzr-svn I can branch and branch --stacked direct
from an SVN
| repo and SVN checkouts:
|
| === modified file 'workingtree.py'
| --- workingtree.py	2008-07-23 16:06:27 +0000
| +++ workingtree.py	2008-07-31 06:31:24 +0000
| @@ -21,7 +21,7 @@
|  from bzrlib.bzrdir import BzrDirFormat, BzrDir
|  from bzrlib.errors import (InvalidRevisionId, NotBranchError, NoSuchFile,
|                             NoRepositoryPresent, BzrError,
UninitializableFormat,
| -                           OutOfDateTree)
| +                           OutOfDateTree, NoSuchRevisionInTree)
|  from bzrlib.inventory import Inventory, InventoryFile, InventoryLink
|  from bzrlib.lockable_files import TransportLock, LockableFiles
|  from bzrlib.lockdir import LockDir
| @@ -581,6 +581,9 @@
|
|          return self.base_tree
|
| +    def revision_tree(self, revision_id):
| +        raise NoSuchRevisionInTree(self, revision_id)
| +
|
|
| That patch fixes a traceback when branching from a checkout.
|
| (Btw, it appears that "bzr info -v" inside a branch stacked on an
SvnBranch
| fails, not sure yet if it's a bzr-svn issue or a bzr issue.)
|
| -Andrew.

Well, sounds like a bug in bzr-svn, as WorkingTree3 already has a proper
implementation of 'revision_tree':

def revision_tree(self, revision_id):
~    """See Tree.revision_tree.

~    WorkingTree can supply revision_trees for the basis revision only
~    because there is only one cached inventory in the bzr directory.
~    """
~    if revision_id == self.last_revision():
~        try:
~            xml = self.read_basis_inventory()
~        except errors.NoSuchFile:
~            pass
~        else:
~            try:
~                inv = xml7.serializer_v7.read_inventory_from_string(xml)
~                # dont use the repository revision_tree api because we want
~                # to supply the inventory.
~                if inv.revision_id == revision_id:
~                    return
revisiontree.RevisionTree(self.branch.repository,
~                        inv, revision_id)
~            except errors.BadInventoryFormat:
~                pass
~    # raise if there was no inventory, or if we read the wrong inventory.
~    raise errors.NoSuchRevisionInTree(self, revision_id)


Basically, if we have cached the xml, we don't need to extract it from
the repository, which *did* make some things a lot faster.
WorkingTree4 has a newer form which can use the dirstate itself for
this, and doing so enables the faster code-path for 'iter_changes()'.
(Though we also have basis_tree() to do this.)

BB:tweak

I'm happy with the patch, I would just like to see a test for it if
possible.

I think adding a test for a Branch.sprout() that creates a different
branch format (this could be hacked together in the test suite) would be
a reasonable way to test that BzrDir.sprout() is calling the right thing
when it can.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkiSGwIACgkQJdeBCYSNAAPmNQCeM3x56Xi3ZnvKkByaDajDhLTq
kHsAoMvQeZbZoeO4AkSBoSIrk99fvJUH
=L1xq
-----END PGP SIGNATURE-----