launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-375013 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #375013 in Launchpad itself: "Cannot commit directly to a stacked branch"
  https://bugs.launchpad.net/launchpad/+bug/375013

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-375013/+merge/79378

= Summary =

The translations-to-branch export script still has a workaround for an old bzr bug: it unstacks any stacked branches it wants to export to, because DirectBranchCommit internally amounts to a kind of lightweight checkout.  Lightweight checkouts couldn't commit to stacked branches.

Of course that costs storage and other resources that we'd rather conserve.  So now that all production systems have the bzr fix deployed, I'm retiring the workaround.


== Pre-implementation notes ==

This ancient workaround was brought back to my attention by a separate bug involving attempts to write to the slave database.


== Implementation details ==

This is all pretty straightforward because it was designed in from the start.  Find the XXX comments, follow the instructions, done.


== Tests ==

I ran the now-obsolete test against the updated code before retiring it.  (It did require replacing a call to _prepareBranchCommit with one to _makeDirectBranchCommit, but that matches the main code change).


== Demo and Q/A ==

Push a branch to a codehosting test server; make sure it's stacked.  Run the branch scanner.  Set the branch as a translations export branch.  Import a template, provide some translations.  Run the translations-to-branch export script on it.  Rather than trying to unstack the branch, it should export and commit normally.  Check the branch for the presence of the translations you entered.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/scripts/tests/test_translations_to_branch.py
  lib/lp/translations/scripts/translations_to_branch.py

./lib/lp/translations/scripts/translations_to_branch.py
      46: 'lp' imported but unused
-- 
https://code.launchpad.net/~jtv/launchpad/bug-375013/+merge/79378
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-375013 into lp:launchpad.

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2011-05-26 15:44:09 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2011-10-14 09:34:27 +0000
@@ -1,14 +1,14 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Acceptance test for the translations-export-to-branch script."""
 
 import datetime
-import pytz
 import re
 from textwrap import dedent
 
 from bzrlib.errors import NotBranchError
+import pytz
 from testtools.matchers import MatchesRegex
 import transaction
 from zope.component import getUtility
@@ -360,44 +360,3 @@
             list(exporter._findChangedPOFiles(
                 pofile.potemplate.productseries,
                 date_in_the_future)))
-
-
-class TestExportToStackedBranch(TestCaseWithFactory):
-    """Test workaround for bzr bug 375013."""
-    # XXX JeroenVermeulen 2009-10-02 bug=375013: Once bug 375013 is
-    # fixed, this entire test can go.
-    layer = ZopelessAppServerLayer
-
-    def _setUpBranch(self, db_branch, tree, message):
-        """Set the given branch and tree up for use."""
-        bzr_branch = tree.branch
-        last_revno, last_revision_id = bzr_branch.last_revision_info()
-        removeSecurityProxy(db_branch).last_scanned_id = last_revision_id
-
-    def setUp(self):
-        super(TestExportToStackedBranch, self).setUp()
-        self.useBzrBranches()
-
-        base_branch, base_tree = self.create_branch_and_tree(
-            'base', name='base')
-        self._setUpBranch(base_branch, base_tree, "Base branch.")
-
-        stacked_branch, stacked_tree = self.create_branch_and_tree(
-            'stacked', name='stacked')
-        stacked_tree.branch.set_stacked_on_url('/' + base_branch.unique_name)
-        stacked_branch.stacked_on = base_branch
-        self._setUpBranch(stacked_branch, stacked_tree, "Stacked branch.")
-
-        self.stacked_branch = stacked_branch
-
-    def test_export_to_shared_branch(self):
-        # The script knows how to deal with stacked branches.
-        # Otherwise, this would fail.
-        script = ExportTranslationsToBranch('reupload', test_args=['-q'])
-        committer = script._prepareBranchCommit(self.stacked_branch)
-        try:
-            self.assertNotEqual(None, committer)
-            committer.writeFile('x.txt', 'x')
-            committer.commit("x!")
-        finally:
-            committer.unlock()

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2011-10-10 16:53:10 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2011-10-14 09:34:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Export translation snapshots to bzr branches where requested."""
@@ -35,8 +35,6 @@
     SLAVE_FLAVOR,
     )
 from lp.app.enums import ServiceUsage
-# Load the normal plugin set. Lint complains but keep this in.
-import lp.codehosting
 from lp.code.errors import StaleLastMirrored
 from lp.code.interfaces.branch import get_db_branch_info
 from lp.code.interfaces.branchjob import IRosettaUploadJobSource
@@ -44,6 +42,8 @@
     ConcurrentUpdateError,
     DirectBranchCommit,
     )
+# Load the normal plugin set. Lint complains but keep this in.
+import lp.codehosting
 from lp.codehosting.vfs import get_rw_server
 from lp.services.mail.sendmail import (
     format_address,
@@ -109,33 +109,6 @@
             db_branch.owner.name)
         return DirectBranchCommit(db_branch, committer_id=committer_id)
 
-    def _prepareBranchCommit(self, db_branch):
-        """Prepare branch for use with `DirectBranchCommit`.
-
-        Create a `DirectBranchCommit` for `db_branch`.  If `db_branch`
-        is not in a format we can commit directly to, try to deal with
-        that.
-
-        :param db_branch: A `Branch`.
-        :return: `DirectBranchCommit`.
-        """
-        # XXX JeroenVermeulen 2009-09-30 bug=375013: It should become
-        # possible again to commit to these branches at some point.
-        # When that happens, remove this workaround and just call
-        # _makeDirectBranchCommit directly.
-        if db_branch.stacked_on:
-            bzrbranch = db_branch.getBzrBranch()
-            self.logger.info("Unstacking branch to work around bug 375013.")
-            bzrbranch.set_stacked_on_url(None)
-            self.logger.info("Done unstacking branch.")
-
-            # This may have taken a while, so commit for good
-            # manners.
-            if self.txn:
-                self.txn.commit()
-
-        return self._makeDirectBranchCommit(db_branch)
-
     def _commit(self, source, committer):
         """Commit changes to branch.  Check for race conditions."""
         self._checkForObjections(source)
@@ -194,9 +167,10 @@
         """
         self.logger.info("Exporting %s." % source.title)
         self._checkForObjections(source)
+        branch = source.translations_branch
 
         try:
-            committer = self._prepareBranchCommit(source.translations_branch)
+            committer = self._makeDirectBranchCommit(branch)
         except StaleLastMirrored as e:
             source.translations_branch.branchChanged(
                 **get_db_branch_info(**e.info))