Merge lp:~jameinel/bzr/1.16-remote-format-stacking into lp:~bzr/bzr/trunk-old

[John A Meinel]

[John A Meinel (jameinel)]

This should fix some of the remaining issues with stacking and non-default formats.

This isn't a 'perfect' fix, as on the server it will still give warnings like:
Source branch format does not support stacking, using format:
  Branch format 7

That is because it still is trying to upgrade the 'default format branch' because it doesn't support stacking, even though 'initialize_on_transport_ex' *isn't* trying to create a branch anyway.

However, stacking *works* now, rather than failing because the repository format is accidentally downgraded because the default branch format doesn't support stacking...

It also has that doing 'bzr branch dev6/format_6_branch --stacked test' also now still works, since it only upgrades the branch format to format 7, and doesn't try to downgrade the repository format.

It has the advantage that 'acquire_repository()' doesn't try to hard-code its own set of formats, the formats are only encoded in BzrDirFormat1.require_stacking. Though this means that we now auto-upgrade to 1.6 rather than the 1.9 that was hard-coded in acquire_repository. (I figured better to be consistent with the older code.)

[Robert Collins (lifeless)]

 review approve

review: Approve

[Andrew Bennetts (spiv)]

 review approve

Basically good, but I have some comments:

John A Meinel wrote:
[...]
> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py 2009-06-03 04:42:02 +0000
> +++ bzrlib/bzrdir.py 2009-06-03 20:03:55 +0000
> @@ -2358,26 +2358,95 @@
> def set_branch_format(self, format):
> self._branch_format = format
>
> - def require_stacking(self):
> + def require_stacking(self, stack_on=None, possible_transports=[]):

Don't use [] as a default argument, as it is mutable. Do
possible_transports=None, as we do everywhere else.

(It's probably safe here, but it looks dangerous and is easy to avoid.)

[...]
> + try:
> + target_dir = BzrDir.open(stack_on,
> + possible_transports=possible_transports)
> + except errors.NotBranchError:
> + # Nothing there, don't change formats
> + target[:] = [None, True, False]
> + return target
> + except errors.JailBreak:
> + # JailBreak, JFDI and upgrade anyway
> + target[:] = [None, True, True]
> + return target
> + try:
> + target_branch = target_dir.open_branch()
> + except errors.NotBranchError:
> + # No branch, don't upgrade formats
> + target[:] = [None, True, False]
> + return target
> + target[:] = [target_branch, True, False]
> + return target

I wonder what happens if a repository is already present, but not a branch?
Or if target_dir is contained in a shared repo?

> + if not (self.repository_format.supports_external_lookups):
> + # We need to upgrade the Repository.
> + target_branch, _, do_upgrade = get_target_branch()
> + if target_branch is None:
> + # We don't have a target branch, should we upgrade anyway?
> + if do_upgrade:
> + # stack_on is inaccessible, JFDI.
> + # TODO: bad monkey, hard-coded formats...
> + if self.repository_format.rich_root_data:
> + new_repo_format = pack_repo.RepositoryFormatKnitPack5RichRoot()
> + else:
> + new_repo_format = pack_repo.RepositoryFormatKnitPack5()

At least this won't accidentally downgrade the source format, I guess.
Personally I'd lean towards making 1.9 rather than 1.6 now, on the basis
that 1.9 isn't that new anymore (and relatively speaking is not much newer
than 1.6 anymore), but hopefully this isn't a big deal either way.

review: Approve

[John A Meinel (jameinel)]

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

Andrew Bennetts wrote:
> Review: Approve
> review approve

...

>> - def require_stacking(self):
>> + def require_stacking(self, stack_on=None, possible_transports=[]):
>
> Don't use [] as a default argument, as it is mutable. Do
> possible_transports=None, as we do everywhere else.
>
> (It's probably safe here, but it looks dangerous and is easy to avoid.)
>

Sure. I wasn't sure if passing None down the stack was supported, but it
seems to be fine. And yes, I know about the pitfalls of mutating default
parameters, but I'm happy to avoid it entirely.

> [...]
>> + try:
>> + target_dir = BzrDir.open(stack_on,
>> + possible_transports=possible_transports)
>> + except errors.NotBranchError:
>> + # Nothing there, don't change formats
>> + target[:] = [None, True, False]
>> + return target
>> + except errors.JailBreak:
>> + # JailBreak, JFDI and upgrade anyway
>> + target[:] = [None, True, True]
>> + return target
>> + try:
>> + target_branch = target_dir.open_branch()
>> + except errors.NotBranchError:
>> + # No branch, don't upgrade formats
>> + target[:] = [None, True, False]
>> + return target
>> + target[:] = [target_branch, True, False]
>> + return target
>
> I wonder what happens if a repository is already present, but not a branch?
> Or if target_dir is contained in a shared repo?

Interesting thought. You actually can only stack on another branch. The
code in question is:
   def _get_fallback_repository(self, url):
       """Get the repository we fallback to at url."""
       url = urlutils.join(self.base, url)
       a_bzrdir = bzrdir.BzrDir.open(url,
           possible_transports=[self.bzrdir.root_transport])
       return a_bzrdir.open_branch().repository

The trick is here ^^^^^^^^^^^^^

So if the target_dir doesn't contain a branch, stacking is going to
fail, anyway. This is mostly about people using a pack-0.92 copy of
bzr.dev, and pushing up to Launchpad when lp:bzr is in 1.9 format
anyway. It was considered "ok" to auto upgrade the public branch in that
case just at the request of Launchpad's default stacking policy. (I
think this had more fallout from Branch6 since upgrading your repo
didn't upgrade your branch format, etc.)

>
>> + if not (self.repository_format.supports_external_lookups):
>> + # We need to upgrade the Repository.
>> + target_branch, _, do_upgrade = get_target_branch()
>> + if target_branch is None:
>> + # We don't have a target branch, should we upgrade anyway?
>> + if do_upgrade:
>> + # stack_on is inaccessible, JFDI.
>> + # TODO: bad monkey, hard-coded formats...
>> + if self.repository_format.rich_root_data:
>> + new_repo_format = pack_repo.RepositoryFormatKnitPack5RichRoot()
>> + else:
>> + new_rep...

[Robert Collins (lifeless)]

On Thu, 2009-06-04 at 02:15 +0000, John A Meinel wrote:
>
> The former is giving a very nice message to the user "using a format
> which adds stacking support". The later is very unclear "why does it
> matter that it uses btree indexes?"
>
> Also, if they are using a format that *doesn't* support stacking, then
> 1.6 is the smallest possible upgrade. (Also, note that debian stable
> only has bzr 1.5...)

Well, I don't think the message is a strong argument. And stacking
before 1.9ish worked very badly anyway, which is why I'd used 1.9. But I
don't really care which is why I was approving your patch as-was.

-Rob

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2009-06-03 04:42:02 +0000
+++ bzrlib/bzrdir.py	2009-06-04 02:35:11 +0000
@@ -2358,26 +2358,95 @@
     def set_branch_format(self, format):
         self._branch_format = format
 
-    def require_stacking(self):
+    def require_stacking(self, stack_on=None, possible_transports=None):
+        """We have a request to stack, try to ensure the formats support it.
+
+        :param stack_on: If supplied, it is the URL to a branch that we want to
+            stack on. Check to see if that format supports stacking before
+            forcing an upgrade.
+        """
+        # Stacking is desired. requested by the target, but does the place it
+        # points at support stacking? If it doesn't then we should
+        # not implicitly upgrade. We check this here.
+        new_repo_format = None
+        new_branch_format = None
+
+        # a bit of state for get_target_branch so that we don't try to open it
+        # 2 times, for both repo *and* branch
+        target = [None, False, None] # target_branch, checked, upgrade anyway
+        def get_target_branch():
+            if target[1]:
+                # We've checked, don't check again
+                return target
+            if stack_on is None:
+                # No target format, that means we want to force upgrading
+                target[:] = [None, True, True]
+                return target
+            try:
+                target_dir = BzrDir.open(stack_on,
+                    possible_transports=possible_transports)
+            except errors.NotBranchError:
+                # Nothing there, don't change formats
+                target[:] = [None, True, False]
+                return target
+            except errors.JailBreak:
+                # JailBreak, JFDI and upgrade anyway
+                target[:] = [None, True, True]
+                return target
+            try:
+                target_branch = target_dir.open_branch()
+            except errors.NotBranchError:
+                # No branch, don't upgrade formats
+                target[:] = [None, True, False]
+                return target
+            target[:] = [target_branch, True, False]
+            return target
+
+        if not (self.repository_format.supports_external_lookups):
+            # We need to upgrade the Repository.
+            target_branch, _, do_upgrade = get_target_branch()
+            if target_branch is None:
+                # We don't have a target branch, should we upgrade anyway?
+                if do_upgrade:
+                    # stack_on is inaccessible, JFDI.
+                    # TODO: bad monkey, hard-coded formats...
+                    if self.repository_format.rich_root_data:
+                        new_repo_format = pack_repo.RepositoryFormatKnitPack5RichRoot()
+                    else:
+                        new_repo_format = pack_repo.RepositoryFormatKnitPack5()
+            else:
+                # If the target already supports stacking, then we know the
+                # project is already able to use stacking, so auto-upgrade
+                # for them
+                new_repo_format = target_branch.repository._format
+                if not new_repo_format.supports_external_lookups:
+                    # target doesn't, source doesn't, so don't auto upgrade
+                    # repo
+                    new_repo_format = None
+            if new_repo_format is not None:
+                self.repository_format = new_repo_format
+                note('Source repository format does not support stacking,'
+                     ' using format:\n  %s',
+                     new_repo_format.get_format_description())
+
         if not self.get_branch_format().supports_stacking():
-            # We need to make a stacked branch, but the default format for the
-            # target doesn't support stacking.  So force a branch that *can*
-            # support stacking.
-            from bzrlib.branch import BzrBranchFormat7
-            branch_format = BzrBranchFormat7()
-            self.set_branch_format(branch_format)
-            mutter("using %r for stacking" % (branch_format,))
-            from bzrlib.repofmt import pack_repo
-            if self.repository_format.rich_root_data:
-                bzrdir_format_name = '1.6.1-rich-root'
-                repo_format = pack_repo.RepositoryFormatKnitPack5RichRoot()
+            # We just checked the repo, now lets check if we need to
+            # upgrade the branch format
+            target_branch, _, do_upgrade = get_target_branch()
+            if target_branch is None:
+                if do_upgrade:
+                    # TODO: bad monkey, hard-coded formats...
+                    new_branch_format = branch.BzrBranchFormat7()
             else:
-                bzrdir_format_name = '1.6'
-                repo_format = pack_repo.RepositoryFormatKnitPack5()
-            note('Source format does not support stacking, using format:'
-                 ' \'%s\'\n  %s\n',
-                 bzrdir_format_name, repo_format.get_format_description())
-            self.repository_format = repo_format
+                new_branch_format = target_branch._format
+                if not new_branch_format.supports_stacking():
+                    new_branch_format = None
+            if new_branch_format is not None:
+                # Does support stacking, use its format.
+                self.set_branch_format(new_branch_format)
+                note('Source branch format does not support stacking,'
+                     ' using format:\n  %s',
+                     new_branch_format.get_format_description())
 
     def get_converter(self, format=None):
         """See BzrDirFormat.get_converter()."""
@@ -3532,54 +3601,9 @@
         """
         stack_on = self._get_full_stack_on()
         if stack_on:
-            # Stacking is desired. requested by the target, but does the place it
-            # points at support stacking? If it doesn't then we should
-            # not implicitly upgrade. We check this here.
             format = self._bzrdir._format
-            if not (format.repository_format.supports_external_lookups
-                and format.get_branch_format().supports_stacking()):
-                # May need to upgrade - but only do if the target also
-                # supports stacking. Note that this currently wastes
-                # network round trips to check - but we only do this
-                # when the source can't stack so it will fade away
-                # as people do upgrade.
-                branch_format = None
-                repo_format = None
-                try:
-                    target_dir = BzrDir.open(stack_on,
-                        possible_transports=[self._bzrdir.root_transport])
-                except errors.NotBranchError:
-                    # Nothing there, don't change formats
-                    pass
-                except errors.JailBreak:
-                    # stack_on is inaccessible, JFDI.
-                    if format.repository_format.rich_root_data:
-                        repo_format = pack_repo.RepositoryFormatKnitPack6RichRoot()
-                    else:
-                        repo_format = pack_repo.RepositoryFormatKnitPack6()
-                    branch_format = branch.BzrBranchFormat7()
-                else:
-                    try:
-                        target_branch = target_dir.open_branch()
-                    except errors.NotBranchError:
-                        # No branch, don't change formats
-                        pass
-                    else:
-                        branch_format = target_branch._format
-                        repo_format = target_branch.repository._format
-                        if not (branch_format.supports_stacking()
-                            and repo_format.supports_external_lookups):
-                            # Doesn't stack itself, don't force an upgrade
-                            branch_format = None
-                            repo_format = None
-                if branch_format and repo_format:
-                    # Does support stacking, use its format.
-                    format.repository_format = repo_format
-                    format.set_branch_format(branch_format)
-                    note('Source format does not support stacking, '
-                        'using format: \'%s\'\n  %s\n',
-                        branch_format.get_format_description(),
-                        repo_format.get_format_description())
+            format.require_stacking(stack_on=stack_on,
+                                    possible_transports=[self._bzrdir.root_transport])
             if not self._require_stacking:
                 # We have picked up automatic stacking somewhere.
                 note('Using default stacking branch %s at %s', self._stack_on,

=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2009-04-15 04:45:06 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2009-06-04 02:35:11 +0000
@@ -237,9 +237,10 @@
             ['branch', '--stacked', 'trunk', 'shallow'])
         # We should notify the user that we upgraded their format
         self.assertEqualDiff(
-            'Source format does not support stacking, using format: \'1.6\'\n'
+            'Source repository format does not support stacking, using format:\n'
             '  Packs 5 (adds stacking support, requires bzr 1.6)\n'
-            '\n'
+            'Source branch format does not support stacking, using format:\n'
+            '  Branch format 7\n'
             'Created new stacked branch referring to %s.\n' % (trunk.base,),
             err)
 
@@ -249,10 +250,10 @@
             ['branch', '--stacked', 'trunk', 'shallow'])
         # We should notify the user that we upgraded their format
         self.assertEqualDiff(
-            'Source format does not support stacking, using format:'
-            ' \'1.6.1-rich-root\'\n'
+            'Source repository format does not support stacking, using format:\n'
             '  Packs 5 rich-root (adds stacking support, requires bzr 1.6.1)\n'
-            '\n'
+            'Source branch format does not support stacking, using format:\n'
+            '  Branch format 7\n'
             'Created new stacked branch referring to %s.\n' % (trunk.base,),
             err)
 

=== modified file 'bzrlib/tests/per_repository_reference/test_initialize.py'
--- bzrlib/tests/per_repository_reference/test_initialize.py	2009-05-29 09:56:12 +0000
+++ bzrlib/tests/per_repository_reference/test_initialize.py	2009-06-04 02:35:11 +0000
@@ -52,8 +52,4 @@
         trans = self.make_smart_server('stacked')
         repo = self.initialize_and_check_on_transport(base, trans)
         network_name = base.repository._format.network_name()
-        if network_name != repo._format.network_name():
-            raise tests.KnownFailure('Remote initialize_on_transport_ex()'
-                ' tries to "upgrade" the format because it doesn\'t have a'
-                ' branch format, and hard-codes the new repository format.')
         self.assertEqual(network_name, repo._format.network_name())