Merge lp:~mwhudson/launchpad/no-hosted-area-formats-from-codehosting into lp:launchpad

[Michael Hudson-Doyle]

Hi Tim,

This branch changes the branchChanged endpoint to record the branch formats and the codehosting server to record them, so not creating scan jobs when the tip revision doesn't change doesn't mean a stale format showing in the UI.

Cheers,
mwh

[Tim Penhey (thumper)]

I think it would be better for the branchChanged method to take
the format strings as three separate parameters rather than a tuple that
is unpacked.

We don't really need to test the branchChanged for packs and knits.

We should remove the code from the scanner that records the
formats as part of this (or the next pipe if you wish).

review: Needs Fixing

[Michael Hudson-Doyle (mwhudson)]

On 19/04/10 16:41, Tim Penhey wrote:
> Review: Needs Fixing
> I think it would be better for the branchChanged method to take
> the format strings as three separate parameters rather than a tuple that
> is unpacked.

Yeah, I think you're right.

> We don't really need to test the branchChanged for packs and knits.

OK.

> We should remove the code from the scanner that records the
> formats as part of this (or the next pipe if you wish).

I think we should do that after the pipe that records the formats from
the branch puller, which is a couple of pipes down the line yet...

Interdiff attached.

Cheers,
mwh

[Tim Penhey (thumper)]

Good now.

review: Approve

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2010-04-27 02:16:59 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2010-04-27 02:17:19 +0000
@@ -34,6 +34,7 @@
 from canonical.launchpad.xmlrpc.helpers import return_fault
 
 from lp.code.errors import UnknownBranchTypeError
+from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.enums import BranchType
 from lp.code.interfaces.branch import BranchCreationException
 from lp.code.interfaces.branchjob import IBranchScanJobSource
@@ -248,7 +249,8 @@
             return True
         return run_with_login(login_id, request_mirror)
 
-    def branchChanged(self, branch_id, stacked_on_location, last_revision_id):
+    def branchChanged(self, branch_id, stacked_on_location, last_revision_id,
+                      control_string, branch_string, repository_string):
         """See `IBranchFileSystem`."""
         branch_set = removeSecurityProxy(getUtility(IBranchLookup))
         branch = branch_set.get(branch_id)
@@ -268,6 +270,22 @@
         if branch.last_mirrored_id != last_revision_id:
             branch.last_mirrored_id = last_revision_id
             getUtility(IBranchScanJobSource).create(branch)
+
+        def match_title(enum, title, default):
+            for value in enum.items:
+                if value.title == title:
+                    return value
+            else:
+                return default
+
+        branch.control_format = match_title(
+            ControlFormat, control_string, ControlFormat.UNRECOGNIZED)
+        branch.branch_format = match_title(
+            BranchFormat, branch_string, BranchFormat.UNRECOGNIZED)
+        branch.repository_format = match_title(
+            RepositoryFormat, repository_string,
+            RepositoryFormat.UNRECOGNIZED)
+
         return True
 
     def _serializeBranch(self, requester, branch, trailing_path):
 
=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-27 02:16:59 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-27 02:17:19 +0000
@@ -9,6 +9,7 @@
 import pytz
 import unittest
 
+from bzrlib import bzrdir
 from bzrlib.tests import multiply_tests
 from bzrlib.urlutils import escape
 
@@ -28,6 +29,7 @@
 from canonical.launchpad.xmlrpc import faults
 from canonical.testing import DatabaseFunctionalLayer, FunctionalLayer
 
+from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.enums import BranchType
 from lp.code.errors import UnknownBranchTypeError
 from lp.code.interfaces.branch import BRANCH_NAME_VALIDATION_ERROR_MESSAGE
@@ -712,11 +714,24 @@
         self.assertSqlAttributeEqualsDate(
             branch, 'next_mirror_time', UTC_NOW)
 
+    def getFormatStringsForFormatName(self, format_name):
+        default_format = bzrdir.format_registry.get(format_name)()
+        control_string = default_format.get_format_string()
+        branch_string = default_format.get_branch_format().get_format_string()
+        repository_string = \
+            default_format.repository_format.get_format_string()
+        return (control_string, branch_string, repository_string)
+
+    @property
+    def arbitrary_format_strings(self):
+        return self.getFormatStringsForFormatName('default')
+
     def test_branchChanged_sets_last_mirrored_id(self):
         # branchChanged sets the last_mirrored_id attribute on the branch.
         revid = self.factory.getUniqueString()
         branch = self.factory.makeAnyBranch()
-        self.branchfs.branchChanged(branch.id, '', revid)
+        self.branchfs.branchChanged(
+            branch.id, '', revid, *self.arbitrary_format_strings)
         self.assertEqual(revid, branch.last_mirrored_id)
 
     def test_branchChanged_sets_stacked_on(self):
@@ -724,7 +739,9 @@
         # passed in.
         branch = self.factory.makeAnyBranch()
         stacked_on = self.factory.makeAnyBranch()
-        self.branchfs.branchChanged(branch.id, stacked_on.unique_name, '')
+        self.branchfs.branchChanged(
+            branch.id, stacked_on.unique_name, '',
+            *self.arbitrary_format_strings)
         self.assertEqual(stacked_on, branch.stacked_on)
 
     def test_branchChanged_unsets_stacked_on(self):
@@ -732,14 +749,16 @@
         # passed in as the stacked_on location.
         branch = self.factory.makeAnyBranch()
         removeSecurityProxy(branch).stacked_on = self.factory.makeAnyBranch()
-        self.branchfs.branchChanged(branch.id, '', '')
+        self.branchfs.branchChanged(
+            branch.id, '', '', *self.arbitrary_format_strings)
         self.assertIs(None, branch.stacked_on)
 
     def test_branchChanged_sets_last_mirrored(self):
         # branchChanged sets the last_mirrored attribute on the branch to the
         # current time.
         branch = self.factory.makeAnyBranch()
-        self.branchfs.branchChanged(branch.id, '', '')
+        self.branchfs.branchChanged(
+            branch.id, '', '', *self.arbitrary_format_strings)
         if self.frontend == LaunchpadDatabaseFrontend:
             self.assertSqlAttributeEqualsDate(
                 branch, 'last_mirrored', UTC_NOW)
@@ -751,7 +770,8 @@
         # mirror_status_message is set to indicate the problem and stacked_on
         # set to None.
         branch = self.factory.makeAnyBranch()
-        self.branchfs.branchChanged(branch.id, '~does/not/exist', '')
+        self.branchfs.branchChanged(
+            branch.id, '~does/not/exist', '', *self.arbitrary_format_strings)
         self.assertIs(None, branch.stacked_on)
         self.assertTrue('~does/not/exist' in branch.mirror_status_message)
 
@@ -760,7 +780,8 @@
         # mirror_status_message.
         branch = self.factory.makeAnyBranch()
         removeSecurityProxy(branch).mirror_status_message = 'foo'
-        self.branchfs.branchChanged(branch.id, '', '')
+        self.branchfs.branchChanged(
+            branch.id, '', '', *self.arbitrary_format_strings)
         self.assertIs(None, branch.mirror_status_message)
 
     def test_branchChanged_fault_on_unknown_id(self):
@@ -768,7 +789,8 @@
         # "NoBranchWithID" is returned.
         unused_id = -1
         expected_fault = faults.NoBranchWithID(unused_id)
-        received_fault = self.branchfs.branchChanged(unused_id, '', '')
+        received_fault = self.branchfs.branchChanged(
+            unused_id, '', '', *self.arbitrary_format_strings)
         self.assertEqual(
             (expected_fault.faultCode, expected_fault.faultString),
             (received_fault.faultCode, received_fault.faultString))
@@ -780,7 +802,8 @@
         branch = self.factory.makeAnyBranch()
         jobs = list(getUtility(IBranchScanJobSource).iterReady())
         self.assertEqual(0, len(jobs))
-        self.branchfs.branchChanged(branch.id, '', 'rev1')
+        self.branchfs.branchChanged(
+            branch.id, '', 'rev1', *self.arbitrary_format_strings)
         jobs = list(getUtility(IBranchScanJobSource).iterReady())
         self.assertEqual(1, len(jobs))
 
@@ -791,10 +814,43 @@
         removeSecurityProxy(branch).last_mirrored_id = 'rev1'
         jobs = list(getUtility(IBranchScanJobSource).iterReady())
         self.assertEqual(0, len(jobs))
-        self.branchfs.branchChanged(branch.id, '', 'rev1')
+        self.branchfs.branchChanged(
+            branch.id, '', 'rev1', *self.arbitrary_format_strings)
         jobs = list(getUtility(IBranchScanJobSource).iterReady())
         self.assertEqual(0, len(jobs))
 
+    def test_branchChanged_2a_format(self):
+        branch = self.factory.makeAnyBranch()
+        self.branchfs.branchChanged(
+            branch.id, '', 'rev1', *self.getFormatStringsForFormatName('2a'))
+        self.assertEqual(
+            (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_7,
+             RepositoryFormat.BZR_CHK_2A),
+            (branch.control_format, branch.branch_format,
+             branch.repository_format))
+
+    def test_branchChanged_packs_format(self):
+        branch = self.factory.makeAnyBranch()
+        self.branchfs.branchChanged(
+            branch.id, '', 'rev1',
+            *self.getFormatStringsForFormatName('pack-0.92'))
+        self.assertEqual(
+            (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_6,
+             RepositoryFormat.BZR_KNITPACK_1),
+            (branch.control_format, branch.branch_format,
+             branch.repository_format))
+
+    def test_branchChanged_knits_format(self):
+        branch = self.factory.makeAnyBranch()
+        self.branchfs.branchChanged(
+            branch.id, '', 'rev1',
+            *self.getFormatStringsForFormatName('knit'))
+        self.assertEqual(
+            (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_5,
+             RepositoryFormat.BZR_KNIT_1),
+            (branch.control_format, branch.branch_format,
+             branch.repository_format))
+
     def assertCannotTranslate(self, requester, path):
         """Assert that we cannot translate 'path'."""
         fault = self.branchfs.translatePath(requester.id, path)
 
=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2010-04-27 02:16:59 +0000
+++ lib/lp/codehosting/inmemory.py	2010-04-27 02:17:19 +0000
@@ -21,6 +21,7 @@
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.launchpad.xmlrpc import faults
 
+from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.errors import UnknownBranchTypeError
 from lp.code.model.branchnamespace import BranchNamespaceSet
 from lp.code.model.branchtarget import (
@@ -618,7 +619,8 @@
     def requestMirror(self, requester_id, branch_id):
         self._branch_set.get(branch_id).requestMirror()
 
-    def branchChanged(self, branch_id, stacked_on_location, last_revision_id):
+    def branchChanged(self, branch_id, stacked_on_location, last_revision_id,
+                      control_string, branch_string, repository_string):
         branch = self._branch_set._find(id=branch_id)
         if branch is None:
             return faults.NoBranchWithID(branch_id)
@@ -638,6 +640,22 @@
         branch.last_mirrored = UTC_NOW
         if branch.last_mirrored_id != last_revision_id:
             branch.last_mirrored_id = last_revision_id
+
+        def match_title(enum, title, default):
+            for value in enum.items:
+                if value.title == title:
+                    return value
+            else:
+                return default
+
+        branch.control_format = match_title(
+            ControlFormat, control_string, ControlFormat.UNRECOGNIZED)
+        branch.branch_format = match_title(
+            BranchFormat, branch_string, BranchFormat.UNRECOGNIZED)
+        branch.repository_format = match_title(
+            RepositoryFormat, repository_string,
+            RepositoryFormat.UNRECOGNIZED)
+
         return True
 
     def _canRead(self, person_id, branch):
 
=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
--- lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-04-27 02:16:59 +0000
+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-04-27 02:17:19 +0000
@@ -547,8 +547,7 @@
         # We can scan a stacked branch that's stacked on a branch that has an
         # lp-mirrored:// URL.
         db_stacked_on_branch = self.factory.makeAnyBranch()
-        stacked_on_tree = self.makeBzrBranchAndTree(
-            db_stacked_on_branch, format='1.6')
+        self.makeBzrBranchAndTree(db_stacked_on_branch, format='1.6')
         db_stacked_branch = self.factory.makeAnyBranch()
         stacked_tree = self.makeBzrBranchAndTree(
             db_stacked_branch, format='1.6')
 
=== modified file 'lib/lp/codehosting/tests/test_acceptance.py'
--- lib/lp/codehosting/tests/test_acceptance.py	2010-04-27 02:16:59 +0000
+++ lib/lp/codehosting/tests/test_acceptance.py	2010-04-27 02:17:19 +0000
@@ -23,6 +23,7 @@
 from canonical.testing import ZopelessAppServerLayer
 from canonical.testing.profiled import profiled
 
+from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.enums import BranchType
 from lp.code.interfaces.branch import IBranchSet
 from lp.code.interfaces.branchnamespace import get_branch_namespace
@@ -321,18 +322,19 @@
             url=url)
 
     def test_push_to_new_branch(self):
-        """
-        The bzr client should be able to read and write to the codehosting
-        server just like another other server.  This means that actions
-        like:
-            * `bzr push bzr+ssh://testinstance/somepath`
-            * `bzr log sftp://testinstance/somepath`
-        (and/or their bzrlib equivalents) and so on should work, so long as
-        the user has permission to read or write to those URLs.
-        """
         remote_url = self.getTransportURL('~testuser/+junk/test-branch')
         self.push(self.local_branch_path, remote_url)
         self.assertBranchesMatch(self.local_branch_path, remote_url)
+        LaunchpadZopelessTestSetup().txn.begin()
+        db_branch = getUtility(IBranchSet).getByUniqueName(
+            '~testuser/+junk/test-branch')
+        self.assertEqual(
+            RepositoryFormat.BZR_CHK_2A, db_branch.repository_format)
+        self.assertEqual(
+            BranchFormat.BZR_BRANCH_7, db_branch.branch_format)
+        self.assertEqual(
+            ControlFormat.BZR_METADIR_1, db_branch.control_format)
+        LaunchpadZopelessTestSetup().txn.commit()
 
     def test_push_to_existing_branch(self):
         """Pushing to an existing branch must work."""
 
=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2010-04-27 02:16:59 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2010-04-27 02:17:19 +0000
@@ -675,13 +675,21 @@
                     ignore_fallbacks=True)
                 last_revision = branch.last_revision()
                 stacked_on_url = self._normalize_stacked_on_url(branch)
+                # XXX: Aaron Bentley 2008-06-13
+                # Bazaar does not provide a public API for learning about
+                # format markers.  Fix this in Bazaar, then here.
+                control_string = branch.bzrdir._format.get_format_string()
+                branch_string = branch._format.get_format_string()
+                repository_string = \
+                    branch.repository._format.get_format_string()
             finally:
                 if jail_info.transports:
                     jail_info.transports.remove(transport)
             if stacked_on_url is None:
                 stacked_on_url = ''
             return self._authserver.branchChanged(
-                data['id'], stacked_on_url, last_revision)
+                data['id'], stacked_on_url, last_revision,
+                control_string, branch_string, repository_string)
 
         # It gets really confusing if we raise an exception from this method
         # (the branch remains locked, but this isn't obvious to the client) so
 
=== modified file 'lib/lp/codehosting/vfs/branchfsclient.py'
--- lib/lp/codehosting/vfs/branchfsclient.py	2010-04-27 02:16:59 +0000
+++ lib/lp/codehosting/vfs/branchfsclient.py	2010-04-27 02:17:19 +0000
@@ -115,14 +115,16 @@
             self._branchfs_endpoint.callRemote, 'createBranch', self._user_id,
             branch_path)
 
-    def branchChanged(self, branch_id, stacked_on_url, last_revision_id):
+    def branchChanged(self, branch_id, stacked_on_url, last_revision_id,
+                      control_string, branch_string, repository_string):
         """Mark a branch as needing to be mirrored.
 
         :param branch_id: The database ID of the branch.
         """
         return defer.maybeDeferred(
             self._branchfs_endpoint.callRemote,
-            'branchChanged', branch_id, stacked_on_url, last_revision_id)
+            'branchChanged', branch_id, stacked_on_url, last_revision_id,
+            control_string, branch_string, repository_string)
 
     def translatePath(self, path):
         """Translate 'path'."""
 
=== modified file 'lib/lp/codehosting/vfs/tests/test_branchfs.py'
--- lib/lp/codehosting/vfs/tests/test_branchfs.py	2010-04-27 02:16:59 +0000
+++ lib/lp/codehosting/vfs/tests/test_branchfs.py	2010-04-27 02:17:19 +0000
@@ -11,7 +11,7 @@
 import unittest
 
 from bzrlib import errors
-from bzrlib.bzrdir import BzrDir
+from bzrlib.bzrdir import BzrDir, format_registry
 from bzrlib.tests import (
     TestCase as BzrTestCase, TestCaseInTempDir, TestCaseWithTransport)
 from bzrlib.transport import (
@@ -821,12 +821,17 @@
         frontend = InMemoryFrontend()
         self.factory = frontend.getLaunchpadObjectFactory()
         self.authserver = frontend.getFilesystemEndpoint()
-        self.authserver.branchChanged = (
-            lambda *args: self._branch_changed_log.append(args))
+        self.authserver.branchChanged = self._replacement_branchChanged
         self.requester = self.factory.makePerson()
         self.backing_transport = MemoryTransport()
         self.disable_directory_isolation()
 
+    def _replacement_branchChanged(self, branch_id, stacked_on_url,
+                                   last_revision, *format_strings):
+        self._branch_changed_log.append(dict(
+            branch_id=branch_id, stacked_on_url=stacked_on_url,
+            last_revision=last_revision, format_strings=format_strings))
+
     def get_server(self):
         if self._server is None:
             self._server = LaunchpadServer(
@@ -844,8 +849,7 @@
         db_branch = self.factory.makeAnyBranch(
             branch_type=BranchType.HOSTED, owner=self.requester)
         self.make_branch(db_branch.unique_name)
-        self.assertEqual(
-            [(db_branch.id, '', 'null:')], self._branch_changed_log)
+        self.assertEqual(1, len(self._branch_changed_log))
 
     def test_branch_unlock_calls_branchChanged(self):
         # Unlocking a branch calls branchChanged on the branch filesystem
@@ -856,8 +860,7 @@
         del self._branch_changed_log[:]
         branch.lock_write()
         branch.unlock()
-        self.assertEqual(
-            [(db_branch.id, '', 'null:')], self._branch_changed_log)
+        self.assertEqual(1, len(self._branch_changed_log))
 
     def test_branch_unlock_reports_stacked_on_url(self):
         # Unlocking a branch reports the stacked on URL to the branch
@@ -872,9 +875,10 @@
         branch.lock_write()
         branch.set_stacked_on_url('/' + db_branch1.unique_name)
         branch.unlock()
+        self.assertEqual(1, len(self._branch_changed_log))
         self.assertEqual(
-            [(db_branch2.id, '/' + db_branch1.unique_name, 'null:')],
-            self._branch_changed_log)
+            '/' + db_branch1.unique_name,
+            self._branch_changed_log[0]['stacked_on_url'])
 
     def test_branch_unlock_reports_last_revision(self):
         # Unlocking a branch reports the tip revision of the branch to the
@@ -886,8 +890,10 @@
         del self._branch_changed_log[:]
         branch.lock_write()
         branch.unlock()
+        self.assertEqual(1, len(self._branch_changed_log))
         self.assertEqual(
-            [(db_branch.id, '', revid)], self._branch_changed_log)
+            revid,
+            self._branch_changed_log[0]['last_revision'])
 
     def test_branch_unlock_relativizes_absolute_stacked_on_url(self):
         # When a branch that has been stacked on the absolute URL of another
@@ -904,9 +910,10 @@
             'http://bazaar.launchpad.dev/~user/product/branch')
         branch.unlock()
         self.assertEqual('/~user/product/branch', branch.get_stacked_on_url())
+        self.assertEqual(1, len(self._branch_changed_log))
         self.assertEqual(
-            [(db_branch.id, '/~user/product/branch', 'null:')],
-            self._branch_changed_log)
+            '/~user/product/branch',
+            self._branch_changed_log[0]['stacked_on_url'])
 
     def test_branch_unlock_ignores_non_launchpad_stacked_url(self):
         # When a branch that has been stacked on the absolute URL of a branch
@@ -920,8 +927,9 @@
         branch.get_config().set_user_option(
             'stacked_on_location', stacked_on_url)
         branch.unlock()
+        self.assertEqual(1, len(self._branch_changed_log))
         self.assertEqual(
-            [(db_branch.id, stacked_on_url, 'null:')], self._branch_changed_log)
+            stacked_on_url, self._branch_changed_log[0]['stacked_on_url'])
         self.assertEqual(stacked_on_url, branch.get_stacked_on_url())
 
     def test_branch_unlock_ignores_odd_scheme_stacked_url(self):
@@ -937,11 +945,28 @@
         branch.get_config().set_user_option(
             'stacked_on_location', stacked_on_url)
         branch.unlock()
+        self.assertEqual(1, len(self._branch_changed_log))
         self.assertEqual(
-            [(db_branch.id, stacked_on_url, 'null:')],
-            self._branch_changed_log)
+            stacked_on_url, self._branch_changed_log[0]['stacked_on_url'])
         self.assertEqual(stacked_on_url, branch.get_stacked_on_url())
 
+    def assertFormatStringsPassed(self, branch):
+        self.assertEqual(1, len(self._branch_changed_log))
+        control_string = branch.bzrdir._format.get_format_string()
+        branch_string = branch._format.get_format_string()
+        repository_string = branch.repository._format.get_format_string()
+        self.assertEqual(
+            (control_string, branch_string, repository_string),
+            self._branch_changed_log[0]['format_strings'])
+
+    def test_format_2a(self):
+        # Creating a 2a branch reports the format to branchChanged.
+        db_branch = self.factory.makeAnyBranch(
+            branch_type=BranchType.HOSTED, owner=self.requester)
+        branch = self.make_branch(
+            db_branch.unique_name, format=format_registry.get('2a')())
+        self.assertFormatStringsPassed(branch)
+
 
 class TestLaunchpadTransportReadOnly(TrialTestCase, BzrTestCase):
     """Tests for read-only operations on the LaunchpadTransport."""