Merge lp:~jtv/launchpad/bug-439248 into lp:launchpad

[Jeroen T. Vermeulen]

[Jeroen T. Vermeulen (jtv)]

= Bug 439248 =

The plot so far:

The translations-to-branch script produces daily snapshots of a product
series' translations and commits them to a selected bzr branch.

It does this using the DirectBranchCommit. This helper class uses a
"shortcut" similar to a lightweight checkout to write files straight to
a bzr branch without having to pull down its full history first.

Branches in Launchpad are now stacked by deafult

The bzr folks have found that committing directly to a stacked branch
failed to preserve invariants. This is bzr bug 375013. Their interim
solution was to prohibit commits straight to stacked branches.

Et voilà: now some translations-to-branch exports are failing because
bzrlib refuses to commit (there is a helpful reference to bug 375013).

This branch works around that by unstacking the branch when needed.
Thanks are due to Robert Collins for proffering this solution.

Semantically it changes nothing for the user; the branch is just stored
in a less efficient way, which is mostly our own problem. The advantage
of this approach is that it's a one-time step per branch. I've already
contacted some users suggesting the command-line equivalent of this fix,
and some of them already seem to have fixed their exports in this way.

== Design considerations ==

I considered and rejected two alternatives in favour of this one:

 * Reverting to a more conventional way of writing to the branch. This
   means having two alternative code paths for doing the same job, and
   trying to hide the complexity behind DirectBranchCommit's simple API.
   It also means letting a limitation of exports scalability exist and
   grow unchecked.

 * Requiring users to convert their own branches manually. I don't
   think that would be acceptable; it's bad enough that "registering a
   branch" is not enough to create one. That means there already is a
   highly technical step to setting up a translations branch. We're not
   in the business of selling expensive consultancy services for this
   sort of chore.

Another design choice was whether to apply the fix in DirectBranchCommit
or in the export script that uses DirectBranchCommit. Here I opted for
keeping the workaround in the export script; making a helper class too
intelligent can lead to surprises for people who work intimately with
branches and have detailed expectations. Unstacking a branch may come
as a surprise there. A chat with Aaron Bentley validated this choice.

== Implementation ==

The export-to-branch script class has a factory method to facilitate
testing: _makeDirectBranchCommit. I added a shim _prepareBranchCommit
between that and the calling code, that checks whether the branch it's
looking at is stacked. If so, it drops the committer it created,
converts the branch to unstacked, and creates a new committer.

Why does it create up to two committers? Because the first will open
the underlying bzr branch, which otherwise we'd have to do once extra to
check for stacking. This way is shorter and more efficient for the
common case.

== Tests ==
{{{
./bin/test -vv -t directbranchcommit -t translations.to.branch
}}}

== Demo and Q/A ==

Pick a productseries with trans...

[Eleanor Berger (intellectronica)]

review: Approve

=== modified file 'lib/lp/code/tests/test_directbranchcommit.py'
--- lib/lp/code/tests/test_directbranchcommit.py	2009-09-09 08:17:59 +0000
+++ lib/lp/code/tests/test_directbranchcommit.py	2009-10-19 10:17:17 +0000
@@ -23,8 +23,7 @@
         self.useBzrBranches()
 
         self.series = self.factory.makeProductSeries()
-        self.db_branch, self.tree = self.create_branch_and_tree(
-            db_branch=self.db_branch, hosted=True)
+        self.db_branch, self.tree = self.create_branch_and_tree(hosted=True)
 
         self.series.translations_branch = self.db_branch
 

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2009-09-24 09:47:20 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2009-10-19 10:17:17 +0000
@@ -16,6 +16,9 @@
 
 from lp.testing import map_branch_contents, TestCaseWithFactory
 
+from lp.translations.scripts.translations_to_branch import (
+    ExportTranslationsToBranch)
+
 
 class TestExportTranslationsToBranch(TestCaseWithFactory):
 
@@ -120,5 +123,46 @@
             None, re.search("INFO\s+Committed [0-9]+ file", stderr))
 
 
+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', hosted=True)
+        self._setUpBranch(base_branch, base_tree, "Base branch.")
+
+        stacked_branch, stacked_tree = self.create_branch_and_tree(
+            'stacked', name='stacked', hosted=True)
+        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()
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2009-09-24 08:26:10 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2009-10-19 10:17:17 +0000
@@ -79,6 +79,39 @@
         """
         return DirectBranchCommit(db_branch)
 
+    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.
+        committer = self._makeDirectBranchCommit(db_branch)
+        if not db_branch.stacked_on:
+            # The normal case.
+            return committer
+
+        self.logger.info("Unstacking branch to work around bug 375013.")
+        try:
+            committer.bzrbranch.set_stacked_on_url(None)
+        finally:
+            committer.unlock()
+        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)
@@ -121,7 +154,7 @@
         self.logger.info("Exporting %s." % source.title)
         self._checkForObjections(source)
 
-        committer = self._makeDirectBranchCommit(source.translations_branch)
+        committer = self._prepareBranchCommit(source.translations_branch)
 
         bzr_branch = committer.bzrbranch