Merge lp:~jtv/launchpad/bug-427278 into lp:launchpad/db-devel

[Jeroen T. Vermeulen]

[Jeroen T. Vermeulen (jtv)]

= Bug 427278 =

Translations exports to bzr branches can sometimes keep transactions open for a while. There's no particular reason to go that far, and it's Not Nice. We've had the script idle-killed a few times.

We've done several things that prevented a repetition:

1. We no longer export all translations for a template; now we only do ones that have changed since the last export. Makes the exports a lot faster.

2. The deadline for long transactions was extended, and obviously that's pretty effective at stopping the alarm bells from ringing. But ideal it is not.

3. The branch you see here makes the script commit more often around the likely pain points.

There's really only one catch: DirectBranchCommit checks the branch for conflicting changes. For that it needs to compare the latest revision in the branch to the last_scanned_id as stored in the database when the DirectBranchCommit was constructed. So instead of getting the last_scanned_id at the point where it's referenced, DirectBranchCommit will now retrieve and store it at construction time. This should also close off a race condition that may or may not exist in the case where database transactions are committed while a DirectBranchCommit is active.

Passing a transaction into DirectBranchCommit.commit, as I do here, is a bit weird. There are reasons for it:

 * Operations that access branch storage can take a long time, but don't need database access. That makes them excellent points to close off any ongoing transaction. The framework starts new transactions on demand, and that's going to happen only after the bzr operation completes. Until that time, the committer avoids being "idle in transaction" on the database.

 * Nowadays txn is global, so we could just "import transaction" and "transaction.commit()". But passing it as an object makes it easy to mock transactions in tests. Also, passing a boolean is slightly more error-prone. Named parameters are a big improvement--"dbc.commit(message, commit=True)"--but still relatively sensitive to mistakes.

 * Passing the transaction object to commit rather than the constructor makes it clear that you're not giving out a blanket license to commit. The DirectBranchCommit can commit only inside that one particular method call. Don't hide transaction boundaries in the program flow.

A lot of the actual diff went into nicer logging.

== Test, demo & Q/A ==

Test:
{{{
./bin/test -vv -t 'translations.*to.*branch' -t 'direct.*branch.*commit'
}}}

Create a branch on staging, and set it as the productseries branch and the translations export branch for some project. Enable translations and two-way bzr synchronization. Push a template to the branch.

Watch the project's import queue; wait for the template to be imported. Then translate a string in the template. Watch the revision log for the branch; an automatic translations commit should appear there within the hour.

No lint.

Jeroen

[Barry Warsaw (barry)]

Jeroen,

It looks like there's a lot more stuff in here than I'd expect from reading the cover letter. For example, why all the style and JavaScript changes? What's with the PreviewDiff changes? Something tells me that you've got a lot more changes in this merge proposal than you intend. Since I can't separate out what should be reviewed, I'm abstaining on this for now.

review: Abstain

[Jeroen T. Vermeulen (jtv)]

Oops; forgot to MP for devel instead of the default db-devel. Filing new MP.

=== modified file 'database/replication/Makefile'
--- database/replication/Makefile	2009-11-13 10:41:58 +0000
+++ database/replication/Makefile	2009-11-18 15:05:39 +0000
@@ -36,6 +36,8 @@
 STAGING_CONFIG=staging # For swapping fresh db into place.
 STAGING_DUMP=launchpad.dump # Dumpfile to build new staging from.
 STAGING_TABLESPACE=pg_default # 'pg_default' for default
+DOGFOOD_DBNAME=launchpad_dogfood
+DOGFOOD_DUMP=launchpad.dump
 
 _CONFIG=overridden-on-command-line
 _SLAVE_TABLESPACE=pg_default
@@ -98,12 +100,13 @@
 	psql -q -d lpmain_staging_new -f authdb_drop.sql
 	psql -q -d lpmain_staging_new -f authdb_create.sql \
 	    2>&1 | grep -v _sl || true
+	# Restore the data
+	pg_restore --dbname=lpmain_staging_new \
+	    --no-acl --no-owner --disable-triggers --data-only \
+	    --exit-on-error ${STAGING_DUMP}
 	# Uninstall Slony-I if it is installed - a pg_dump of a DB with
 	# Slony-I installed isn't usable without this step.
 	LPCONFIG=${NEW_STAGING_CONFIG} ./repair-restored-db.py
-	pg_restore --dbname=lpmain_staging_new \
-	    --no-acl --no-owner --disable-triggers --data-only \
-	    --exit-on-error ${STAGING_DUMP}
 	# Setup replication
 	make _replicate LPCONFIG=${NEW_STAGING_CONFIG} LAG="0 seconds" \
 	    _MASTER=lpmain_staging_new _SLAVE=lpmain_staging_slave_new \
@@ -138,10 +141,10 @@
 	psql -q -d ${DOGFOOD_DBNAME} -f authdb_drop.sql
 	psql -q -d ${DOGFOOD_DBNAME} -f authdb_create.sql \
 	    2>&1 | grep -v _sl || true
-	./repair-restored-db.py -d ${DOGFOOD_DBNAME}
 	pg_restore --dbname=${DOGFOOD_DBNAME} --no-acl --no-owner \
 	    --disable-triggers --data-only \
 	    --exit-on-error ${DOGFOOD_DUMP}
+	./repair-restored-db.py -d ${DOGFOOD_DBNAME}
 	../schema/upgrade.py -d ${DOGFOOD_DBNAME}
 	../schema/fti.py -d ${DOGFOOD_DBNAME}
 	../schema/security.py -d ${DOGFOOD_DBNAME}

=== modified file 'database/replication/repair-restored-db.py'
--- database/replication/repair-restored-db.py	2009-10-17 14:06:03 +0000
+++ database/replication/repair-restored-db.py	2009-11-18 15:05:38 +0000
@@ -27,7 +27,8 @@
 
 from canonical.config import config
 from canonical.database.postgresql import ConnectionString
-from canonical.database.sqlbase import connect, quote
+from canonical.database.sqlbase import (
+    connect, quote, ISOLATION_LEVEL_AUTOCOMMIT)
 from canonical.launchpad.scripts import db_options, logger_options, logger
 
 import replication.helpers
@@ -44,12 +45,23 @@
 
     log = logger(options)
 
-    con = connect(options.dbuser)
+    con = connect(options.dbuser, isolation=ISOLATION_LEVEL_AUTOCOMMIT)
 
     if not replication.helpers.slony_installed(con):
         log.info("Slony-I not installed. Nothing to do.")
         return 0
 
+    if not repair_with_slonik(log, options, con):
+        repair_with_drop_schema(log, con)
+
+    return 0
+
+
+def repair_with_slonik(log, options, con):
+    """Attempt to uninstall Slony-I via 'UNINSTALL NODE' per best practice.
+
+    Returns True on success, False if unable to do so for any reason.
+    """
     cur = con.cursor()
 
     # Determine the node id the database thinks it is.
@@ -60,27 +72,19 @@
         cur.execute(cmd)
         node_id = cur.fetchone()[0]
         log.debug("Node Id is %d" % node_id)
+
+        # Get a list of set ids in the database.
+        cur.execute(
+            "SELECT DISTINCT set_id FROM %s.sl_set"
+            % replication.helpers.CLUSTER_NAMESPACE)
+        set_ids = set(row[0] for row in cur.fetchall())
+        log.debug("Set Ids are %s" % repr(set_ids))
+
     except psycopg2.InternalError:
         # Not enough information to determine node id. Possibly
-        # this is an empty database. Just drop the _sl schema as
-        # it is 'good enough' with Slony-I 1.2 - this mechanism
-        # fails with Slony added primary keys, but we don't do that.
-        con.rollback()
-        cur = con.cursor()
-        cur.execute("DROP SCHEMA _sl CASCADE")
-        con.commit()
-        return 0
-
-    # Get a list of set ids in the database.
-    cur.execute(
-        "SELECT DISTINCT set_id FROM %s.sl_set"
-        % replication.helpers.CLUSTER_NAMESPACE)
-    set_ids = set(row[0] for row in cur.fetchall())
-    log.debug("Set Ids are %s" % repr(set_ids))
-
-    # Close so we don't block slonik(1)
-    del cur
-    con.close()
+        # this is an empty database.
+        log.debug('Broken or no Slony-I install.')
+        return False
 
     connection_string = ConnectionString(config.database.main_master)
     if options.dbname:
@@ -103,7 +107,22 @@
         log.debug(line)
     script = '\n'.join(script)
 
-    replication.helpers.execute_slonik(script, auto_preamble=False)
+    return replication.helpers.execute_slonik(
+        script, auto_preamble=False, exit_on_fail=False)
+
+
+def repair_with_drop_schema(log, con):
+    """
+    Just drop the _sl schema as it is 'good enough' with Slony-I 1.2.
+
+    This mechanism fails with Slony added primary keys, but we don't
+    do that.
+    """
+    log.info('Fallback mode - dropping _sl schema.')
+    cur = con.cursor()
+    cur.execute("DROP SCHEMA _sl CASCADE")
+    return True
+
 
 if __name__ == '__main__':
     sys.exit(main())

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2009-11-10 22:09:08 +0000
+++ lib/canonical/launchpad/icing/style.css	2009-11-18 15:05:38 +0000
@@ -1753,10 +1753,13 @@
   text-align: right;
 }
 .codereviewcomment {font: 120% monospace;}
-.codereviewcomment .attachment, #review-diff .attachment {
+.codereviewcomment .attachment, #review-diff .attachment,
+div.diff .attachmentBody {
   background-color: #f6f6f6;
 }
-
+div.diff .horizontal {
+  margin: 0;
+}
 body.tab-branches table.listing .section-heading {
   color: #d18b39;
 }
@@ -1765,12 +1768,16 @@
   color: #666666;
 }
 
+.yui-diff-overlay table.diff {
+  font-size: 98%;
+}
 #proposal-summary td, #proposal-summary th { padding-bottom: 2px; }
 table.diff {
    white-space: pre-wrap;
    font: 120% monospace;
    width: 100%;
    background-color: white;
+   margin-bottom: 0.5em;
 }
 table.diff .text { width: 99%; padding-left: 0.5em; }
 table.diff .line-no {

=== modified file 'lib/canonical/launchpad/javascript/code/branchlinks.js'
--- lib/canonical/launchpad/javascript/code/branchlinks.js	2009-08-13 13:16:04 +0000
+++ lib/canonical/launchpad/javascript/code/branchlinks.js	2009-11-18 15:05:39 +0000
@@ -1,4 +1,5 @@
-/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
+/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Code for handling links to branches from bugs and specs.
  *
@@ -24,11 +25,11 @@
     error_handler = new LP.client.ErrorHandler();
     error_handler.clearProgressUI = function() {
         destroy_temporary_spinner();
-    }
+    };
     error_handler.showError = function(error_message) {
         alert('An unexpected error has occurred.');
         Y.log(error_message);
-    }
+    };
 
     link_bug_overlay = new Y.lazr.FormOverlay({
         headerContent: '<h2>Link to a bug</h2>',

=== modified file 'lib/canonical/launchpad/javascript/code/branchstatus.js'
--- lib/canonical/launchpad/javascript/code/branchstatus.js	2009-11-10 05:12:59 +0000
+++ lib/canonical/launchpad/javascript/code/branchstatus.js	2009-11-18 15:05:38 +0000
@@ -1,4 +1,5 @@
-/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
+/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Code for handling the update of the branch status.
  *

=== modified file 'lib/canonical/launchpad/javascript/code/branchsubscription.js'
--- lib/canonical/launchpad/javascript/code/branchsubscription.js	2009-08-13 13:47:24 +0000
+++ lib/canonical/launchpad/javascript/code/branchsubscription.js	2009-11-18 15:05:39 +0000
@@ -1,4 +1,5 @@
-/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
+/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Subscription handling for branches.
  *

=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js	2009-10-28 16:49:59 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js	2009-11-18 15:05:39 +0000
@@ -1,4 +1,5 @@
-/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
+/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Library for code review javascript.
  *

=== added file 'lib/canonical/launchpad/javascript/code/popupdiff.js'
--- lib/canonical/launchpad/javascript/code/popupdiff.js	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/javascript/code/popupdiff.js	2009-11-18 15:05:38 +0000
@@ -0,0 +1,123 @@
+/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Code for handling the popup diffs in the pretty overlays.
+ *
+ * @module popupdiff
+ * @requires node
+ */
+
+YUI.add('code.branchmergeproposal.popupdiff', function(Y) {
+
+// The launchpad js client used.
+var lp_client;
+
+/*
+ * The DiffOverlay object inherits from the lazr-js PerttyOverlay.
+ *
+ * By sub-classing the DiffOverlay gets its own CSS class that is applied to
+ * the various HTML objeccts that are created.  This allows styling of the
+ * overlay in a different way to any other PrettyOverlays.
+ */
+var DiffOverlay = function() {
+    DiffOverlay.superclass.constructor.apply(this, arguments);
+};
+
+Y.extend(DiffOverlay, Y.lazr.PrettyOverlay, {
+        bindUI: function() {
+            // call PrettyOverlay's bindUI
+            this.constructor.superclass.bindUI.call(this);
+        }
+    });
+
+// The NAME gets appended to 'yui-' to give the class name 'yui-diff-overlay'.
+DiffOverlay.NAME = 'diff-overlay';
+
+// A local page cache of the diff overlays that have been rendered.
+// This makes subsequent views of an already loaded diff instantaneous.
+var rendered_overlays = {};
+
+/*
+ * Display the diff for the specified api_url.
+ *
+ * If the specified api_url has already been rendered in an overlay, show it
+ * again.  If it hasn't been loaded, show the spinner, and load the diff using
+ * the LP API.
+ *
+ * If the diff fails to load, the user is taken to the librarian url just as
+ * if Javascript was not enabled.
+ */
+function display_diff(node, api_url, librarian_url) {
+
+    // Look to see if we have rendered one already.
+    if (rendered_overlays[api_url] !== undefined) {
+        rendered_overlays[api_url].show();
+        return;
+    }
+
+    // Show a spinner.
+    var html = [
+        '<img src="/@@/spinner" alt="loading..." ',
+        '     style="padding-left: 0.5em"/>'].join('');
+    var spinner = Y.Node.create(html);
+    node.appendChild(spinner);
+
+    // Load the diff.
+    var config = {
+        on: {
+            success: function(formatted_diff) {
+                node.removeChild(spinner);
+                var diff_overlay = new DiffOverlay({
+                        bodyContent: Y.Node.create(formatted_diff),
+                        align: {
+                            points: [Y.WidgetPositionExt.CC,
+                                     Y.WidgetPositionExt.CC]
+                        },
+                        progressbar: false
+                    });
+                rendered_overlays[api_url] = diff_overlay;
+                diff_overlay.render();
+            },
+            failure: function() {
+                node.removeChild(spinner);
+                // Fail over to loading the librarian link.
+                document.location = librarian_url;
+            }
+        },
+        accept: LP.client.XHTML
+    };
+    lp_client.get(api_url, config);
+}
+
+
+// Grab the namespace in order to be able to expose the connect method.
+var popupdiff = Y.namespace('code.branchmergeproposal.popupdiff');
+
+/*
+ * Connect the diff links to their pretty overlay function.
+ */
+popupdiff.connect_diff_links = function() {
+    Y.log('connect_diff_links called');
+    // IE doesn't like pretty overlays.
+    if (Y.UA.ie) {
+        return;
+    }
+
+    // Setup the LP client.
+    lp_client = new LP.client.Launchpad();
+
+    // var status_content = Y.get('#branch-details-status-value');
+    var nl = Y.all('.popup-diff');
+    nl.each(function(node, index, nodelist){
+            var a = node.query('a');
+            a.addClass('js-action');
+            var librarian_url = a.getAttribute('href');
+            var api_url = node.query('a.api-ref').getAttribute('href');
+            a.on('click', function(e) {
+                    e.preventDefault();
+                    display_diff(a, api_url, librarian_url);
+                });
+        });
+};
+
+    }, '0.1', {requires: ['io', 'node', 'lazr.overlay', 'lp.client']});

=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml	2009-10-20 22:13:21 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml	2009-11-18 15:05:39 +0000
@@ -419,13 +419,6 @@
         />
 
     <adapter
-        for="lp.code.interfaces.diff.IPreviewDiff"
-        provides="zope.traversing.interfaces.IPathAdapter"
-        factory="canonical.launchpad.webapp.tales.PreviewDiffFormatterAPI"
-        name="fmt"
-        />
-
-    <adapter
         for="lp.code.interfaces.codeimport.ICodeImport"
         provides="zope.traversing.interfaces.IPathAdapter"
         factory="canonical.launchpad.webapp.tales.CodeImportFormatterAPI"

=== modified file 'lib/canonical/launchpad/webapp/tales.py'
--- lib/canonical/launchpad/webapp/tales.py	2009-11-14 22:40:15 +0000
+++ lib/canonical/launchpad/webapp/tales.py	2009-11-18 15:05:39 +0000
@@ -1407,68 +1407,6 @@
             '%(name)s</a>: %(title)s' % self._args(view_name))
 
 
-class PreviewDiffFormatterAPI(ObjectFormatterAPI):
-    """Formatter for preview diffs."""
-
-    def url(self, view_name=None, rootsite=None):
-        """Use the url of the librarian file containing the diff.
-        """
-        librarian_alias = self._context.diff_text
-        if librarian_alias is None:
-            return None
-        else:
-            return librarian_alias.getURL()
-
-    def link(self, view_name):
-        """The link to the diff should show the line count.
-
-        Stale diffs will have a stale-diff css class.
-        Diffs with conflicts will have a conflict-diff css class.
-        Diffs with neither will have clean-diff css class.
-
-        The title of the diff will show the number of lines added or removed
-        if available.
-
-        :param view_name: If not None, the link will point to the page with
-            that name on this object.
-        """
-        title_words = []
-        if self._context.conflicts is not None:
-            style = 'conflicts-diff'
-            title_words.append(_('CONFLICTS'))
-        else:
-            style = 'clean-diff'
-        # Stale style overrides conflicts or clean.
-        if self._context.stale:
-            style = 'stale-diff'
-            title_words.append(_('Stale'))
-
-        if self._context.added_lines_count:
-            title_words.append(
-                _("%s added") % self._context.added_lines_count)
-
-        if self._context.removed_lines_count:
-            title_words.append(
-                _("%s removed") % self._context.removed_lines_count)
-
-        args = {
-            'line_count': _('%s lines') % self._context.diff_lines_count,
-            'style': style,
-            'title': ', '.join(title_words),
-            'url': self.url(view_name),
-            }
-        # Under normal circumstances, there will be an associated file,
-        # however if the diff is empty, then there is no alias to link to.
-        if args['url'] is None:
-            return (
-                '<span title="%(title)s" class="%(style)s">'
-                '%(line_count)s</span>' % args)
-        else:
-            return (
-                '<a href="%(url)s" title="%(title)s" class="%(style)s">'
-                '<img src="/@@/download"/>&nbsp;%(line_count)s</a>' % args)
-
-
 class BranchSubscriptionFormatterAPI(CustomizableFormatter):
     """Adapter for IBranchSubscription objects to a formatted string."""
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_tales.py'
--- lib/canonical/launchpad/webapp/tests/test_tales.py	2009-10-21 17:26:37 +0000
+++ lib/canonical/launchpad/webapp/tests/test_tales.py	2009-11-18 15:05:39 +0000
@@ -3,20 +3,15 @@
 
 """tales.py doctests."""
 
-from difflib import unified_diff
 from textwrap import dedent
 import unittest
 
-from storm.store import Store
-from zope.security.proxy import removeSecurityProxy
 from zope.testing.doctestunit import DocTestSuite
 
-from canonical.launchpad.ftests import test_tales
 from canonical.launchpad.testing.pages import find_tags_by_class
 from canonical.launchpad.webapp.tales import FormattersAPI
-from canonical.testing import (
-    DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
-from lp.testing import login, TestCase, TestCaseWithFactory
+from canonical.testing import DatabaseFunctionalLayer
+from lp.testing import TestCase
 
 
 def test_requestapi():
@@ -268,134 +263,6 @@
         self.assertEqual(3, line_count)
 
 
-class TestPreviewDiffFormatter(TestCaseWithFactory):
-    """Test the PreviewDiffFormatterAPI class."""
-
-    layer = LaunchpadFunctionalLayer
-
-    def _createPreviewDiff(self, line_count=0, added=None, removed=None,
-                           conflicts=None):
-        # Login an admin to avoid the launchpad.Edit requirements.
-        login('[e-mail]')
-        # Create a dummy preview diff, and make sure the branches have the
-        # correct last scanned ids to ensure that the new diff is not stale.
-        bmp = self.factory.makeBranchMergeProposal()
-        if line_count:
-            content = ''.join(unified_diff('', 'random content'))
-        else:
-            content = ''
-        preview = bmp.updatePreviewDiff(
-            content, u'rev-a', u'rev-b', conflicts=conflicts)
-        bmp.source_branch.last_scanned_id = preview.source_revision_id
-        bmp.target_branch.last_scanned_id = preview.target_revision_id
-        # Update the values directly sidestepping the security.
-        naked_diff = removeSecurityProxy(preview)
-        naked_diff.diff_lines_count = line_count
-        naked_diff.added_lines_count = added
-        naked_diff.removed_lines_count = removed
-        # In order to get the canonical url of the librarian file, we need to
-        # commit.
-        # transaction.commit()
-        # Make sure that the preview diff is in the db for the test.
-        # Storm bug: 324724
-        Store.of(bmp).flush()
-        return preview
-
-    def _createStalePreviewDiff(self, line_count=0, added=None, removed=None,
-                                conflicts=None):
-        preview = self._createPreviewDiff(
-            line_count, added, removed, conflicts)
-        preview.branch_merge_proposal.source_branch.last_scanned_id = 'other'
-        return preview
-
-    def test_creation_method(self):
-        # Just confirm that our helpers do what they say.
-        preview = self._createPreviewDiff(12, 45, 23)
-        self.assertEqual(12, preview.diff_lines_count)
-        self.assertEqual(45, preview.added_lines_count)
-        self.assertEqual(23, preview.removed_lines_count)
-        self.assertEqual(False, preview.stale)
-        self.assertEqual(True, self._createStalePreviewDiff().stale)
-
-    def test_fmt_no_diff(self):
-        # If there is no diff, there is no link.
-        preview = self._createPreviewDiff(0)
-        self.assertEqual(
-            '<span title="" class="clean-diff">0 lines</span>',
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_lines_no_add_or_remove(self):
-        # If there is no diff, there is no link.
-        preview = self._createPreviewDiff(10, 0, 0)
-        self.assertEqual(
-            '<a href="%s" title="" class="clean-diff">'
-            '<img src="/@@/download"/>&nbsp;10 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_lines_some_added_no_removed(self):
-        # If there is no diff, there is no link.
-        preview = self._createPreviewDiff(10, 4, 0)
-        self.assertEqual(
-            '<a href="%s" title="4 added" class="clean-diff">'
-            '<img src="/@@/download"/>&nbsp;10 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_lines_no_added_some_removed(self):
-        # If there is no diff, there is no link.
-        preview = self._createPreviewDiff(10, 0, 4)
-        self.assertEqual(
-            '<a href="%s" title="4 removed" class="clean-diff">'
-            '<img src="/@@/download"/>&nbsp;10 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_lines_added_and_removed(self):
-        # If there is no diff, there is no link.
-        preview = self._createPreviewDiff(10, 6, 4)
-        self.assertEqual(
-            '<a href="%s" title="6 added, 4 removed" class="clean-diff">'
-            '<img src="/@@/download"/>&nbsp;10 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_simple_conflicts(self):
-        # If there is no diff, there is no link.
-        preview = self._createPreviewDiff(10, 2, 3, u'conflicts')
-        self.assertEqual(
-            '<a href="%s" title="CONFLICTS, 2 added, 3 removed" '
-            'class="conflicts-diff">'
-            '<img src="/@@/download"/>&nbsp;10 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_stale_empty_diff(self):
-        # If there is no diff, there is no link.
-        preview = self._createStalePreviewDiff(0)
-        self.assertEqual(
-            '<span title="Stale" class="stale-diff">0 lines</span>',
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_stale_non_empty_diff(self):
-        # If there is no diff, there is no link.
-        preview = self._createStalePreviewDiff(500, 89, 340)
-        self.assertEqual(
-            '<a href="%s" title="Stale, 89 added, 340 removed" '
-            'class="stale-diff"><img src="/@@/download"/>&nbsp;500 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-    def test_fmt_stale_non_empty_diff_with_conflicts(self):
-        # If there is no diff, there is no link.
-        preview = self._createStalePreviewDiff(500, 89, 340, u'conflicts')
-        self.assertEqual(
-            '<a href="%s" title="CONFLICTS, Stale, 89 added, 340 removed" '
-            'class="stale-diff"><img src="/@@/download"/>&nbsp;500 lines</a>'
-            % preview.diff_text.getURL(),
-            test_tales('preview/fmt:link', preview=preview))
-
-
 def test_suite():
     """Return this module's doctest Suite. Unit tests are also run."""
     suite = unittest.TestSuite()

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2009-10-16 11:07:37 +0000
+++ lib/devscripts/ec2test/builtins.py	2009-11-18 15:05:39 +0000
@@ -557,7 +557,7 @@
             'bzr pull -d /var/launchpad/download-cache '
             'lp:lp-source-dependencies')
         if public:
-            update_sourcecode_options = '--public-only'
+            update_sourcecode_options = ' --public-only'
         else:
             update_sourcecode_options = ''
         user_connection.run_with_ssh_agent(

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2009-10-26 18:40:04 +0000
+++ lib/lp/code/browser/branchlisting.py	2009-11-18 15:05:39 +0000
@@ -91,14 +91,7 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.sourcepackage import ISourcePackageFactory
 from lp.registry.model.sourcepackage import SourcePackage
-
-
-def get_plural_text(count, singular, plural):
-    """Return 'singular' if 'count' is 1, 'plural' otherwise."""
-    if count == 1:
-        return singular
-    else:
-        return plural
+from lp.services.browser_helpers import get_plural_text
 
 
 class CodeVHostBreadcrumb(Breadcrumb):

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2009-11-10 22:57:42 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2009-11-18 15:05:39 +0000
@@ -34,7 +34,7 @@
 
 import simplejson
 from zope.app.form.browser import TextAreaWidget
-from zope.component import adapter, getUtility
+from zope.component import adapter, adapts, getMultiAdapter, getUtility
 from zope.event import notify as zope_notify
 from zope.formlib import form
 from zope.interface import Interface, implementer, implements
@@ -60,7 +60,8 @@
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.interfaces import IPrimaryContext
 from canonical.launchpad.webapp.menu import NavigationMenu
-from canonical.launchpad.webapp.tales import FormattersAPI
+from canonical.launchpad.webapp.tales import (
+    DateTimeFormatterAPI, FormattersAPI)
 from canonical.widgets.lazrjs import (
     TextAreaEditorWidget, vocabulary_to_choice_edit_items)
 
@@ -74,6 +75,7 @@
 from lp.code.interfaces.codereviewcomment import ICodeReviewComment
 from lp.code.interfaces.codereviewvote import (
     ICodeReviewVoteReference)
+from lp.code.interfaces.diff import IPreviewDiff
 from lp.registry.interfaces.person import IPersonSet
 from lp.services.comments.interfaces.conversation import IConversation
 
@@ -138,6 +140,23 @@
             }
         return friendly_texts[self.context.queue_status]
 
+    @property
+    def status_title(self):
+        """The title for the status text.
+
+        Only set if the status is approved or rejected.
+        """
+        result = ''
+        if self.context.queue_status in (
+            BranchMergeProposalStatus.CODE_APPROVED,
+            BranchMergeProposalStatus.REJECTED
+            ):
+            formatter = DateTimeFormatterAPI(self.context.date_reviewed)
+            result = '%s %s' % (
+                self.context.reviewer.displayname,
+                formatter.displaydate())
+        return result
+
 
 class BranchMergeProposalMenuMixin:
     """Mixin class for merge proposal menus."""
@@ -427,10 +446,44 @@
         return SimpleVocabulary(terms)
 
 
+class DiffRenderingMixin:
+    """A mixin class for handling diff text."""
+
+    @cachedproperty
+    def preview_diff_text(self):
+        """Return a (hopefully) intelligently encoded review diff."""
+        preview_diff = self.preview_diff
+        if preview_diff is None:
+            return None
+        try:
+            diff = preview_diff.text.decode('utf-8')
+        except UnicodeDecodeError:
+            diff = preview_diff.text.decode('windows-1252', 'replace')
+        # Strip off the trailing carriage returns.
+        return diff.rstrip('\n')
+
+    @cachedproperty
+    def diff_oversized(self):
+        """Return True if the review_diff is over the configured size limit.
+
+        The diff can be over the limit in two ways.  If the diff is oversized
+        in bytes it will be cut off at the Diff.text method.  If the number of
+        lines is over the max_format_lines, then it is cut off at the fmt:diff
+        processing.
+        """
+        review_diff = self.preview_diff
+        if review_diff is None:
+            return False
+        if review_diff.oversized:
+            return True
+        diff_text = self.preview_diff_text
+        return diff_text.count('\n') >= config.diff.max_format_lines
+
 
 class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
                               BranchMergeProposalRevisionIdMixin,
-                              BranchMergeProposalStatusMixin):
+                              BranchMergeProposalStatusMixin,
+                              DiffRenderingMixin):
     """A basic view used for the index page."""
 
     implements(IBranchMergeProposalActionMenu)
@@ -496,36 +549,6 @@
             return self.context.review_diff.diff
         return None
 
-    @cachedproperty
-    def preview_diff_text(self):
-        """Return a (hopefully) intelligently encoded review diff."""
-        preview_diff = self.preview_diff
-        if preview_diff is None:
-            return None
-        try:
-            diff = preview_diff.text.decode('utf-8')
-        except UnicodeDecodeError:
-            diff = preview_diff.text.decode('windows-1252', 'replace')
-        # Strip off the trailing carriage returns.
-        return diff.rstrip('\n')
-
-    @cachedproperty
-    def review_diff_oversized(self):
-        """Return True if the review_diff is over the configured size limit.
-
-        The diff can be over the limit in two ways.  If the diff is oversized
-        in bytes it will be cut off at the Diff.text method.  If the number of
-        lines is over the max_format_lines, then it is cut off at the fmt:diff
-        processing.
-        """
-        review_diff = self.preview_diff
-        if review_diff is None:
-            return False
-        if review_diff.oversized:
-            return True
-        diff_text = self.preview_diff_text
-        return diff_text.count('\n') >= config.diff.max_format_lines
-
     @property
     def has_bug_or_spec(self):
         """Return whether or not the merge proposal has a linked bug or spec.
@@ -1290,3 +1313,28 @@
         html    = formatter(nomail).text_to_html()
         return html.encode('utf-8')
     return renderer
+
+
+class FormatPreviewDiffView(LaunchpadView, DiffRenderingMixin):
+    """A simple view to render a diff formatted nicely."""
+
+    __used_for__ = IPreviewDiff
+
+    @property
+    def preview_diff(self):
+        return self.context
+
+
+class PreviewDiffHTMLRepresentation:
+    adapts(IPreviewDiff, IWebServiceClientRequest)
+    implements(Interface)
+
+    def __init__(self, diff, request):
+        self.diff = diff
+        self.request = request
+
+    def __call__(self):
+        """Render `BugBranch` as XHTML using the webservice."""
+        diff_view = getMultiAdapter(
+            (self.diff, self.request), name="+diff")
+        return diff_view()

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2009-11-13 20:12:17 +0000
+++ lib/lp/code/browser/configure.zcml	2009-11-18 15:05:38 +0000
@@ -181,16 +181,24 @@
             name="+index"
             template="../templates/branchmergeproposal-index.pt"/>
         <browser:page
+            name="++diff"
+            template="../templates/branchmergeproposal-diff.pt"/>
+        <browser:page
             name="+pagelet-summary"
             template="../templates/branchmergeproposal-pagelet-summary.pt"/>
     </browser:pages>
-    <browser:page
-        name="+votes"
+    <browser:pages
         for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal"
         class="lp.code.browser.branchmergeproposal.BranchMergeProposalVoteView"
         facet="branches"
-        permission="launchpad.View"
-        template="../templates/branchmergeproposal-votes.pt"/>
+        permission="launchpad.View">
+        <browser:page
+            name="+votes"
+            template="../templates/branchmergeproposal-votes.pt"/>
+        <browser:page
+            name="+vote-summary"
+            template="../templates/branchmergeproposal-vote-summary.pt"/>
+      </browser:pages>
     <browser:pages
         for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal"
         class="lp.code.browser.branchmergeproposal.BranchMergeProposalEditView"
@@ -276,7 +284,7 @@
         class="lp.code.browser.branchmergeproposal.BranchMergeCandidateView"
         facet="branches"
         permission="zope.Public"
-        template="../templates/branch-merge-link-summary.pt"/>
+        template="../templates/branchmergeproposal-link-summary.pt"/>
     <browser:page
         name="+pagelet-subscribers"
         for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal"
@@ -898,6 +906,13 @@
         attribute_to_parent="branch_merge_proposal"
         rootsite="code" />
 
+    <browser:page
+        name="+diff"
+        for="lp.code.interfaces.diff.IPreviewDiff"
+        class="lp.code.browser.branchmergeproposal.FormatPreviewDiffView"
+        permission="launchpad.AnyPerson"
+        template="../templates/branchmergeproposal-diff.pt"/>
+
     <browser:menus
         module="lp.code.browser.branchlisting"
         classes="PersonProductBranchesMenu"/>
@@ -991,5 +1006,15 @@
         for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal"
         factory="lp.code.browser.branchmergeproposal.BranchMergeProposalBreadcrumb"
         permission="zope.Public"/>
+    <adapter
+         factory="lp.code.browser.branchmergeproposal.PreviewDiffHTMLRepresentation"
+         name="lazr.restful.EntryResource"/>
+
+    <adapter
+        for="lp.code.interfaces.diff.IPreviewDiff"
+        provides="zope.traversing.interfaces.IPathAdapter"
+        factory="lp.code.browser.diff.PreviewDiffFormatterAPI"
+        name="fmt"
+        />
 
 </configure>

=== added file 'lib/lp/code/browser/diff.py'
--- lib/lp/code/browser/diff.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/diff.py	2009-11-18 15:05:38 +0000
@@ -0,0 +1,77 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Display classes relating to diff objects of one sort or another."""
+
+__metaclass__ = type
+__all__ = [
+    'PreviewDiffFormatterAPI',
+    ]
+
+
+from canonical.launchpad import _
+from canonical.launchpad.webapp.tales import ObjectFormatterAPI
+from lp.services.browser_helpers import get_plural_text
+
+
+class PreviewDiffFormatterAPI(ObjectFormatterAPI):
+    """Formatter for preview diffs."""
+
+    def url(self, view_name=None, rootsite=None):
+        """Use the url of the librarian file containing the diff.
+        """
+        librarian_alias = self._context.diff_text
+        if librarian_alias is None:
+            return None
+        else:
+            return librarian_alias.getURL()
+
+    def link(self, view_name):
+        """The link to the diff should show the line count.
+
+        Stale diffs will have a stale-diff css class.
+        Diffs with conflicts will have a conflict-diff css class.
+        Diffs with neither will have clean-diff css class.
+
+        The title of the diff will show the number of lines added or removed
+        if available.
+
+        :param view_name: If not None, the link will point to the page with
+            that name on this object.
+        """
+        diff = self._context
+        conflict_text = ''
+        if diff.conflicts is not None:
+            conflict_text = _(' (has conflicts)')
+
+        count_text = ''
+        added = diff.added_lines_count
+        removed = diff.removed_lines_count
+        if (added is not None and removed is not None):
+            count_text = ' (+%d/-%d)' % (added, removed)
+
+        file_text = ''
+        diffstat = diff.diffstat
+        if diffstat is not None:
+            file_count = len(diffstat)
+            file_text = get_plural_text(
+                file_count, _(' %d file modified'), _(' %d files modified'))
+            file_text = file_text % file_count
+
+        args = {
+            'line_count': _('%s lines') % diff.diff_lines_count,
+            'file_text': file_text,
+            'conflict_text': conflict_text,
+            'count_text': count_text,
+            'url': self.url(view_name),
+            }
+        # Under normal circumstances, there will be an associated file,
+        # however if the diff is empty, then there is no alias to link to.
+        if args['url'] is None:
+            return (
+                '<span class="empty-diff">'
+                '%(line_count)s</span>' % args)
+        else:
+            return (
+                '<a href="%(url)s" class="diff-link">'
+                '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s</a>' % args)

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2009-11-01 23:13:09 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2009-11-18 15:05:38 +0000
@@ -7,10 +7,11 @@
 
 __metaclass__ = type
 
-from datetime import timedelta
+from datetime import datetime, timedelta
 from difflib import unified_diff
 import unittest
 
+import pytz
 import transaction
 from zope.component import getMultiAdapter
 from zope.security.interfaces import Unauthorized
@@ -66,6 +67,7 @@
         link = menu.add_comment()
         self.assertTrue(menu.add_comment().enabled)
 
+
 class TestDecoratedCodeReviewVoteReference(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -459,13 +461,6 @@
         self.bmp = self.factory.makeBranchMergeProposal(registrant=self.user)
         login_person(self.user)
 
-    def _createView(self):
-        # Construct the view and initialize it.
-        view = BranchMergeProposalView(
-            self.bmp, LaunchpadTestRequest())
-        view.initialize()
-        return view
-
     def makeTeamReview(self):
         owner = self.bmp.source_branch.owner
         review_team = self.factory.makeTeam()
@@ -477,7 +472,7 @@
         albert = self.factory.makePerson()
         albert.join(review.reviewer)
         login_person(albert)
-        view = self._createView()
+        view = create_initialized_view(self.bmp, '+index')
         view.claim_action.success({'review_id': review.id})
         self.assertEqual(albert, review.reviewer)
 
@@ -486,13 +481,13 @@
         review = self.makeTeamReview()
         albert = self.factory.makePerson()
         login_person(albert)
-        view = self._createView()
+        view = create_initialized_view(self.bmp, '+index')
         self.assertRaises(Unauthorized, view.claim_action.success,
                           {'review_id': review.id})
 
     def test_preview_diff_text_with_no_diff(self):
         """review_diff should be None when there is no context.review_diff."""
-        view = self._createView()
+        view = create_initialized_view(self.bmp, '+index')
         self.assertIs(None, view.preview_diff_text)
 
     def test_review_diff_utf8(self):
@@ -502,8 +497,9 @@
         diff = StaticDiff.acquireFromText('x', 'y', diff_bytes)
         transaction.commit()
         self.bmp.review_diff = diff
+        view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(diff_bytes.decode('utf-8'),
-                         self._createView().preview_diff_text)
+                         view.preview_diff_text)
 
     def test_review_diff_all_chars(self):
         """review_diff should work on diffs containing all possible bytes."""
@@ -512,8 +508,9 @@
         diff = StaticDiff.acquireFromText('x', 'y', diff_bytes)
         transaction.commit()
         self.bmp.review_diff = diff
+        view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(diff_bytes.decode('windows-1252', 'replace'),
-                         self._createView().preview_diff_text)
+                         view.preview_diff_text)
 
     def addReviewDiff(self):
         review_diff_bytes = ''.join(unified_diff('', 'review'))
@@ -532,27 +529,31 @@
     def test_preview_diff_prefers_preview_diff(self):
         """The preview will be used for BMP with both a review and preview."""
         preview_diff = self.addBothDiffs()
-        self.assertEqual(preview_diff, self._createView().preview_diff)
+        view = create_initialized_view(self.bmp, '+index')
+        self.assertEqual(preview_diff, view.preview_diff)
 
     def test_preview_diff_uses_review_diff(self):
         """The review diff will be used if there is no preview."""
         review_diff = self.addReviewDiff()
+        view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(review_diff.diff,
-                         self._createView().preview_diff)
+                         view.preview_diff)
 
     def test_review_diff_text_prefers_preview_diff(self):
         """The preview will be used for BMP with both a review and preview."""
         preview_diff = self.addBothDiffs()
         transaction.commit()
+        view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(
-            preview_diff.text, self._createView().preview_diff_text)
+            preview_diff.text, view.preview_diff_text)
 
     def test_linked_bugs_excludes_mutual_bugs(self):
         """List bugs that are linked to the source only."""
         bug = self.factory.makeBug()
         self.bmp.source_branch.linkBug(bug, self.bmp.registrant)
         self.bmp.target_branch.linkBug(bug, self.bmp.registrant)
-        self.assertEqual([], self._createView().linked_bugs)
+        view = create_initialized_view(self.bmp, '+index')
+        self.assertEqual([], view.linked_bugs)
 
 
 class TestBranchMergeProposalChangeStatusOptions(TestCaseWithFactory):
@@ -692,5 +693,42 @@
         self.assertEqual(u'\u2615', diff_attachment.diff_text)
 
 
+class TestBranchMergeCandidateView(TestCaseWithFactory):
+    """Test the status title for the view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_needs_review_title(self):
+        # No title is set for a proposal needing review.
+        bmp = self.factory.makeBranchMergeProposal(
+            set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+        view = create_initialized_view(bmp, '+link-summary')
+        self.assertEqual('', view.status_title)
+
+    def test_approved_shows_reviewer(self):
+        # If the proposal is approved, the approver is shown in the title
+        # along with when they approved it.
+        bmp = self.factory.makeBranchMergeProposal()
+        owner = bmp.target_branch.owner
+        login_person(bmp.target_branch.owner)
+        owner.displayname = 'Eric'
+        bmp.approveBranch(owner, 'some-rev', datetime(
+                year=2008, month=9, day=10, tzinfo=pytz.UTC))
+        view = create_initialized_view(bmp, '+link-summary')
+        self.assertEqual('Eric on 2008-09-10', view.status_title)
+
+    def test_rejected_shows_reviewer(self):
+        # If the proposal is rejected, the approver is shown in the title
+        # along with when they approved it.
+        bmp = self.factory.makeBranchMergeProposal()
+        owner = bmp.target_branch.owner
+        login_person(bmp.target_branch.owner)
+        owner.displayname = 'Eric'
+        bmp.rejectBranch(owner, 'some-rev', datetime(
+                year=2008, month=9, day=10, tzinfo=pytz.UTC))
+        view = create_initialized_view(bmp, '+link-summary')
+        self.assertEqual('Eric on 2008-09-10', view.status_title)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== added file 'lib/lp/code/browser/tests/test_tales.py'
--- lib/lp/code/browser/tests/test_tales.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/tests/test_tales.py	2009-11-18 15:05:39 +0000
@@ -0,0 +1,163 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the tales formatters."""
+
+__metaclass__ = type
+
+from difflib import unified_diff
+import unittest
+
+from storm.store import Store
+from zope.security.proxy import removeSecurityProxy
+
+from lp.testing import login, TestCaseWithFactory, test_tales
+from canonical.testing import LaunchpadFunctionalLayer
+
+
+class TestPreviewDiffFormatter(TestCaseWithFactory):
+    """Test the PreviewDiffFormatterAPI class."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def _createPreviewDiff(self, line_count=0, added=None, removed=None,
+                           conflicts=None, diffstat=None):
+        # Login an admin to avoid the launchpad.Edit requirements.
+        login('[e-mail]')
+        # Create a dummy preview diff, and make sure the branches have the
+        # correct last scanned ids to ensure that the new diff is not stale.
+        bmp = self.factory.makeBranchMergeProposal()
+        if line_count:
+            content = ''.join(unified_diff('', 'random content'))
+        else:
+            content = ''
+        preview = bmp.updatePreviewDiff(
+            content, u'rev-a', u'rev-b', conflicts=conflicts)
+        bmp.source_branch.last_scanned_id = preview.source_revision_id
+        bmp.target_branch.last_scanned_id = preview.target_revision_id
+        # Update the values directly sidestepping the security.
+        naked_diff = removeSecurityProxy(preview)
+        naked_diff.diff_lines_count = line_count
+        naked_diff.added_lines_count = added
+        naked_diff.removed_lines_count = removed
+        naked_diff.diffstat = diffstat
+        # In order to get the canonical url of the librarian file, we need to
+        # commit.
+        # transaction.commit()
+        # Make sure that the preview diff is in the db for the test.
+        # Storm bug: 324724
+        Store.of(bmp).flush()
+        return preview
+
+    def _createStalePreviewDiff(self, line_count=0, added=None, removed=None,
+                                conflicts=None, diffstat=None):
+        preview = self._createPreviewDiff(
+            line_count, added, removed, conflicts, diffstat)
+        preview.branch_merge_proposal.source_branch.last_scanned_id = 'other'
+        return preview
+
+    def test_creation_method(self):
+        # Just confirm that our helpers do what they say.
+        preview = self._createPreviewDiff(
+            12, 45, 23, u'conflicts', {'filename': (3,2)})
+        self.assertEqual(12, preview.diff_lines_count)
+        self.assertEqual(45, preview.added_lines_count)
+        self.assertEqual(23, preview.removed_lines_count)
+        self.assertEqual(False, preview.stale)
+        self.assertEqual(True, self._createStalePreviewDiff().stale)
+        self.assertEqual(u'conflicts', preview.conflicts)
+        self.assertEqual({'filename': (3,2)}, preview.diffstat)
+
+    def test_fmt_no_diff(self):
+        # If there is no diff, there is no link.
+        preview = self._createPreviewDiff(0)
+        self.assertEqual(
+            '<span class="empty-diff">0 lines</span>',
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_lines_no_add_or_remove(self):
+        # If the lines added and removed are None, they are now shown.
+        preview = self._createPreviewDiff(10, added=None, removed=None)
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '10 lines</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_lines_some_added_no_removed(self):
+        # If the added and removed values are not None, they are shown.
+        preview = self._createPreviewDiff(10, added=4, removed=0)
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '10 lines (+4/-0)</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_lines_files_modified(self):
+        # If the diffstat has been set, the number of entries in the dict
+        # defines the number of files modified.
+        preview = self._createPreviewDiff(
+            10, added=4, removed=0, diffstat={
+                'file1': (1, 0),
+                'file2': (3, 0)})
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '10 lines (+4/-0) 2 files modified</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_lines_one_file_modified(self):
+        # If only one file has been modified, a singular value is used.
+        preview = self._createPreviewDiff(
+            10, added=4, removed=0, diffstat={
+                'file': (3, 0)})
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '10 lines (+4/-0) 1 file modified</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_simple_conflicts(self):
+        # Conflicts are indicated using text in the link.
+        preview = self._createPreviewDiff(10, 2, 3, u'conflicts')
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '10 lines (+2/-3) (has conflicts)</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_stale_empty_diff(self):
+        # If there is no diff, there is no link.
+        preview = self._createStalePreviewDiff(0)
+        self.assertEqual(
+            '<span class="empty-diff">0 lines</span>',
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_stale_non_empty_diff(self):
+        # If there is no diff, there is no link.
+        diffstat = dict(
+            (self.factory.getUniqueString(), (2,3)) for x in range(23))
+        preview = self._createStalePreviewDiff(
+            500, 89, 340, diffstat=diffstat)
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '500 lines (+89/-340) 23 files modified</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_stale_non_empty_diff_with_conflicts(self):
+        # If there is no diff, there is no link.
+        diffstat = dict(
+            (self.factory.getUniqueString(), (2,3)) for x in range(23))
+        preview = self._createStalePreviewDiff(
+            500, 89, 340, u'conflicts', diffstat=diffstat)
+        self.assertEqual(
+            '<a href="%s" class="diff-link">'
+            '500 lines (+89/-340) 23 files modified (has conflicts)</a>'
+            % preview.diff_text.getURL(),
+            test_tales('preview/fmt:link', preview=preview))
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
+

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2009-10-05 14:17:48 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2009-11-18 15:05:38 +0000
@@ -361,7 +361,8 @@
         # or superseded, then it is valid to be merged.
         return (self.queue_status not in FINAL_STATES)
 
-    def _reviewProposal(self, reviewer, next_state, revision_id):
+    def _reviewProposal(self, reviewer, next_state, revision_id,
+                        _date_reviewed=None):
         """Set the proposal to one of the two review statuses."""
         # Check the reviewer can review the code for the target branch.
         old_state = self.queue_status
@@ -371,21 +372,25 @@
         self._transitionToState(next_state, reviewer)
         # Record the reviewer
         self.reviewer = reviewer
-        self.date_reviewed = UTC_NOW
+        if _date_reviewed is None:
+            _date_reviewed = UTC_NOW
+        self.date_reviewed = _date_reviewed
         # Record the reviewed revision id
         self.reviewed_revision_id = revision_id
         notify(BranchMergeProposalStatusChangeEvent(
                 self, reviewer, old_state, next_state))
 
-    def approveBranch(self, reviewer, revision_id):
+    def approveBranch(self, reviewer, revision_id, _date_reviewed=None):
         """See `IBranchMergeProposal`."""
         self._reviewProposal(
-            reviewer, BranchMergeProposalStatus.CODE_APPROVED, revision_id)
+            reviewer, BranchMergeProposalStatus.CODE_APPROVED, revision_id,
+            _date_reviewed)
 
-    def rejectBranch(self, reviewer, revision_id):
+    def rejectBranch(self, reviewer, revision_id, _date_reviewed=None):
         """See `IBranchMergeProposal`."""
         self._reviewProposal(
-            reviewer, BranchMergeProposalStatus.REJECTED, revision_id)
+            reviewer, BranchMergeProposalStatus.REJECTED, revision_id,
+            _date_reviewed)
 
     def enqueue(self, queuer, revision_id):
         """See `IBranchMergeProposal`."""

=== modified file 'lib/lp/code/model/directbranchcommit.py'
--- lib/lp/code/model/directbranchcommit.py	2009-09-09 20:03:19 +0000
+++ lib/lp/code/model/directbranchcommit.py	2009-11-18 15:05:38 +0000
@@ -74,6 +74,8 @@
         self.db_branch = db_branch
         self.to_mirror = to_mirror
 
+        self.last_scanned_id = self.db_branch.last_scanned_id
+
         if committer is None:
             committer = db_branch.owner
         self.committer = committer
@@ -172,12 +174,18 @@
         assert self.is_locked, "Getting revision on un-locked branch."
         last_revision = None
         last_revision = self.bzrbranch.last_revision()
-        if last_revision != self.db_branch.last_scanned_id:
+        if last_revision != self.last_scanned_id:
             raise ConcurrentUpdateError(
                 "Branch has been changed.  Not committing.")
 
-    def commit(self, commit_message):
-        """Commit to branch."""
+    def commit(self, commit_message, txn=None):
+        """Commit to branch.
+
+        :param commit_message: Message for branch's commit log.
+        :param txn: Transaction to commit.  Can be helpful in avoiding
+            long idle times in database transactions.  May be committed
+            more than once.
+        """
         assert self.is_open, "Committing closed DirectBranchCommit."
         assert self.is_locked, "Not locked at commit time."
 
@@ -185,6 +193,9 @@
         try:
             self._checkForRace()
 
+            if txn:
+                txn.commit()
+
             rev_id = self.revision_tree.get_revision_id()
             if rev_id == NULL_REVISION:
                 if list(self.transform_preview.iter_changes()) == []:
@@ -193,6 +204,9 @@
                 self.bzrbranch, commit_message)
             IMasterObject(self.db_branch).requestMirror()
 
+            if txn:
+                txn.commit()
+
         finally:
             self.unlock()
             self.is_open = False

=== modified file 'lib/lp/code/stories/branches/xx-branch-merge-proposals.txt'
--- lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-10-28 19:31:47 +0000
+++ lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-11-18 15:05:39 +0000
@@ -29,6 +29,8 @@
     >>> _unused = branch.subscribe(subscriber,
     ...     BranchSubscriptionNotificationLevel.NOEMAIL, None,
     ...     CodeReviewNotificationLevel.FULL)
+    >>> from lp.code.tests.helpers import make_erics_fooix_project
+    >>> locals().update(make_erics_fooix_project(factory))
     >>> logout()
 
 Any logged in user can register a merge proposal.  Registering
@@ -68,7 +70,8 @@
 
 Registering the merge proposal takes the user to the new merge proposal.
 
-    >>> print nopriv_browser.url
+    >>> klingon_proposal = nopriv_browser.url
+    >>> print klingon_proposal
     http://code.launchpad.dev/~name12/gnome-terminal/klingon/+merge/...
 
 The summary reflects the selected target and prerequisite.
@@ -97,9 +100,6 @@
     ...     'Add more <b>mojo</b>')
     >>> nopriv_browser.getControl('Update').click()
 
-    >>> print nopriv_browser.url
-    http://code.launchpad.dev/~name12/gnome-terminal/klingon/+merge/1
-
     >>> print_tag_with_id(nopriv_browser.contents, 'edit-description')
     Commit Message
     Add more &lt;b&gt;mojo&lt;/b&gt;
@@ -113,7 +113,6 @@
 for the source_branch.
 
     >>> login('[e-mail]')
-    >>> eric = factory.makePerson(email="[e-mail]", password="test")
     >>> bmp = factory.makeBranchMergeProposal(registrant=eric)
     >>> bmp_url = canonical_url(bmp)
     >>> branch_url = canonical_url(bmp.source_branch)
@@ -128,14 +127,13 @@
 
     >>> sample_browser = setupBrowser(auth="Basic [e-mail]:test")
 
+
 Requesting reviews
 ------------------
 
 You can request a review of a merge proposal.
 
-    >>> sample_browser.open(
-    ...    'http://code.launchpad.dev/~name12/gnome-terminal/klingon/+merge/1'
-    ...    )
+    >>> sample_browser.open(klingon_proposal)
     >>> sample_browser.getLink('Request a review').click()
     >>> sample_browser.getControl('Reviewer').value = 'mark'
     >>> sample_browser.getControl('Review type').value = 'first'
@@ -169,7 +167,7 @@
     Reviewer          Review Type    Date Requested    Status
     Sample Person                                      Pending [Review]
     Mark Shuttleworth second         ... ago           Pending
-    Review via email: [e-mail]
+    Review via email: [e-mail]
                                                  Request another review
 
 
@@ -178,16 +176,14 @@
 
 People not logged in cannot perform reviews.
 
-    >>> anon_browser.open('http://code.launchpad.dev/~name12/gnome-terminal'
-    ...     '/klingon/+merge/1')
+    >>> anon_browser.open(klingon_proposal)
     >>> link = anon_browser.getLink('[Review]')
     Traceback (most recent call last):
     LinkNotFoundError
 
 People who are logged in can perform reviews.
 
-    >>> nopriv_browser.open('http://code.launchpad.dev/~name12/gnome-terminal'
-    ...     '/klingon/+merge/1')
+    >>> nopriv_browser.open(klingon_proposal)
     >>> nopriv_browser.getLink('Add a review or comment').click()
     >>> nopriv_browser.getControl(name='field.comment').value = "Don't like it"
     >>> nopriv_browser.getControl(name='field.vote').getControl(
@@ -201,8 +197,7 @@
 
 People can claim reviews for teams of which they are a member.
 
-    >>> sample_browser.open('http://code.launchpad.dev/~name12/gnome-terminal'
-    ...     '/klingon/+merge/1')
+    >>> sample_browser.open(klingon_proposal)
     >>> sample_browser.getLink('Request another review').click()
     >>> sample_browser.getControl('Reviewer').value = 'hwdb-team'
     >>> sample_browser.getControl('Review type').value = 'claimable'
@@ -213,8 +208,7 @@
     Reviewer                         Review Type  Date Requested Status...
     HWDB Team                        claimable    ... ago        Pending ...
     >>> foobar_browser = setupBrowser(auth="Basic [e-mail]:test")
-    >>> foobar_browser.open('http://code.launchpad.dev/~name12/gnome-terminal'
-    ...     '/klingon/+merge/1')
+    >>> foobar_browser.open(klingon_proposal)
     >>> foobar_browser.getControl('Claim review').click()
     >>> pending = find_tag_by_id(
     ...     foobar_browser.contents, 'code-review-votes')
@@ -230,8 +224,7 @@
     >>> foobar_browser.getLink('Reassign').click()
     >>> foobar_browser.getControl('Reviewer').value = 'hwdb-team'
     >>> foobar_browser.getControl('Reassign').click()
-    >>> foobar_browser.open('http://code.launchpad.dev/~name12/gnome-terminal'
-    ...     '/klingon/+merge/1')
+    >>> foobar_browser.open(klingon_proposal)
     >>> pending = find_tag_by_id(
     ...     foobar_browser.contents, 'code-review-votes')
 
@@ -248,8 +241,7 @@
 When a branch has been merged into the target branch, the proposal should
 be marked as merged.
 
-    >>> nopriv_browser.open(
-    ...     'http://code.launchpad.dev/~name12/gnome-terminal/klingon/+merge/1')
+    >>> nopriv_browser.open(klingon_proposal)
 
 The edit icon at the end of the status allows the user to edit the status.
 
@@ -264,8 +256,6 @@
     >>> nopriv_browser.getControl(name='field.queue_status').displayValue = (
     ...     ['Merged'])
     >>> nopriv_browser.getControl('Change Status').click()
-    >>> print nopriv_browser.url
-    http://code.launchpad.dev/~name12/gnome-terminal/klingon/+merge/1
 
 The proposal is now shown as have being merged.
 
@@ -282,6 +272,7 @@
     >>> print extract_text(find_tag_by_id(
     ...     sample_browser.contents, 'landing-targets'))
     Merged into lp://dev/~name12/gnome-terminal/main
+    ...
     >>> sample_browser.getLink('Merged').click()
     >>> print_summary(sample_browser)
     Status: Merged
@@ -303,6 +294,7 @@
     >>> print extract_text(find_tag_by_id(
     ...     sample_browser.contents, 'landing-targets'))
     Merged into lp://dev/~name12/gnome-terminal/main at revision 42
+    ...
 
 
 Resubmitting proposals
@@ -315,9 +307,7 @@
 branches but with the state set to work-in-progress.
 
     >>> login('[e-mail]')
-    >>> fooix = factory.makeProduct(name='fooix')
-    >>> target = factory.makeProductBranch(product=fooix, owner=eric)
-    >>> bmp = factory.makeBranchMergeProposal(target_branch=target)
+    >>> bmp = factory.makeBranchMergeProposal(target_branch=trunk)
     >>> bmp_url = canonical_url(bmp)
     >>> logout()
     >>> eric_browser.open(bmp_url)
@@ -538,7 +528,6 @@
     Bug #...: Bug for linking (Undecided &ndash; New)
 
 
-
 Target branch edge cases
 ------------------------
 
@@ -626,3 +615,30 @@
     >>> print extract_text(get_review_diff())
     Preview Diff
     Empty
+
+
+Merge proposal details shown on the branch page
+-----------------------------------------------
+
+A branch that has a merge proposal, but no requested reviews shows this on the
+branch page.
+
+    >>> nopriv_browser.open('http://code.launchpad.dev/~fred/fooix/feature')
+    >>> nopriv_browser.getLink('Propose for merging').click()
+    >>> nopriv_browser.getControl('Reviewer').value = ''
+    >>> nopriv_browser.getControl('Propose Merge').click()
+    >>> nopriv_browser.getLink('lp://dev/~fred/fooix/feature').click()
+    >>> print_tag_with_id(nopriv_browser.contents, 'landing-targets')
+    Ready for review for merging into lp://dev/fooix
+      No reviews requested
+
+If there are reviews either pending or completed these are also shown.
+
+The api tag is a hidden anchor that holds the URL for the merge proposal api
+access.
+
+    >>> nopriv_browser.open('http://code.launchpad.dev/~fred/fooix/proposed')
+    >>> print_tag_with_id(nopriv_browser.contents, 'landing-targets')
+    Ready for review for merging into lp://dev/fooix
+      Eric the Viking: Pending (code) requested ... ago
+      Diff: 47 lines (+7/-0) 2 files modified                api

=== modified file 'lib/lp/code/templates/branch-index.pt'
--- lib/lp/code/templates/branch-index.pt	2009-10-11 02:33:30 +0000
+++ lib/lp/code/templates/branch-index.pt	2009-11-18 15:05:39 +0000
@@ -25,6 +25,20 @@
     #download-url dd span.branch-url, #upload-url dd span.branch-url {
       font-weight: normal;
     }
+    dl.reviews dd {
+      padding-left: 2em;
+      margin-bottom: 0;
+    }
+    .yui-diff-overlay-hidden {
+      display: none;
+    }
+    .yui-diff-overlay {
+      width: 80%;
+    }
+    .yui-diff-overlay #yui-pretty-overlay-modal {
+      width: auto;
+      color: black;
+    }
 
   </style>
   <script type="text/javascript"
@@ -43,12 +57,20 @@
       tal:attributes="src string:${lp_js}/code/branchstatus.js">
   </script>
   <script type="text/javascript"
+      tal:condition="devmode"
+      tal:define="lp_js string:${icingroot}/build"
+      tal:attributes="src string:${lp_js}/code/popupdiff.js">
+  </script>
+  <script type="text/javascript"
           tal:content="string:
     YUI().use('node', 'event', 'widget', 'plugin', 'overlay',
-              'lazr.choiceedit', 'code.branchstatus', function(Y) {
+              'lazr.choiceedit', 'code.branchstatus',
+              'code.branchmergeproposal.popupdiff',
+              function(Y) {
         Y.on('load',
              function(e) {
                  Y.branchstatus.connect_status(${view/status_config});
+                 Y.code.branchmergeproposal.popupdiff.connect_diff_links();
              },
              window);
     });

=== modified file 'lib/lp/code/templates/branch-pending-merges.pt'
--- lib/lp/code/templates/branch-pending-merges.pt	2009-09-18 21:20:42 +0000
+++ lib/lp/code/templates/branch-pending-merges.pt	2009-11-18 15:05:38 +0000
@@ -36,6 +36,7 @@
           <div>
             <tal:merge-fragment tal:replace="structure mergeproposal/@@+link-summary" />
           </div>
+          <tal:merge-fragment tal:replace="structure mergeproposal/@@+vote-summary" />
         </tal:landing-candidates>
       </div>
 

=== added file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
--- lib/lp/code/templates/branchmergeproposal-diff.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff.pt	2009-11-18 15:05:39 +0000
@@ -0,0 +1,33 @@
+<tal:root
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+>
+
+  <div class="boardComment attachment diff"
+       tal:define="attachment view/preview_diff/diff_text">
+    <tal:real-diff condition="attachment">
+      <div class="boardCommentDetails filename">
+        <ul class="horizontal">
+          <li>
+            <a tal:attributes="href attachment/getURL">
+              <img src="/@@/download"/>
+              Download diff
+            </a>
+          </li>
+        </ul>
+      </div>
+      <div class="boardCommentBody attachmentBody">
+        <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
+      </div>
+      <div class="boardCommentFooter"
+           tal:condition="view/diff_oversized">
+        The diff has been truncated for viewing.
+      </div>
+    </tal:real-diff>
+    <tal:empty-diff condition="not: attachment">
+      <div class="boardCommentBody attachmentBody">
+        <em>Empty</em>
+      </div>
+    </tal:empty-diff>
+  </div>
+
+</tal:root>
\ No newline at end of file

=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2009-11-10 21:49:38 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2009-11-18 15:05:38 +0000
@@ -128,34 +128,8 @@
   </div>
 
   <div id="review-diff" tal:condition="view/preview_diff">
-  <h2>Preview Diff</h2>
-  <div class="boardComment attachment"
-       tal:define="attachment view/preview_diff/diff_text">
-    <tal:real-diff condition="attachment">
-      <div class="boardCommentDetails filename">
-        <ul class="horizontal">
-          <li>
-            <a tal:attributes="href attachment/getURL">
-              <img src="/@@/download"/>
-              Download diff
-            </a>
-          </li>
-        </ul>
-      </div>
-      <div class="boardCommentBody attachmentBody">
-        <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
-      </div>
-      <div class="boardCommentFooter"
-           tal:condition="view/review_diff_oversized">
-        The diff has been truncated for viewing.
-      </div>
-    </tal:real-diff>
-    <tal:empty-diff condition="not: attachment">
-      <div class="boardCommentBody attachmentBody">
-        <em>Empty</em>
-      </div>
-    </tal:empty-diff>
-  </div>
+    <h2>Preview Diff</h2>
+    <div tal:replace="structure context/@@++diff"/>
   </div>
 
 <tal:script

=== renamed file 'lib/lp/code/templates/branch-merge-link-summary.pt' => 'lib/lp/code/templates/branchmergeproposal-link-summary.pt'
--- lib/lp/code/templates/branch-merge-link-summary.pt	2009-09-22 16:31:42 +0000
+++ lib/lp/code/templates/branchmergeproposal-link-summary.pt	2009-11-18 15:05:39 +0000
@@ -4,7 +4,8 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n">
 
   <img src="/@@/merge-proposal-icon" />
-  <a tal:attributes="href context/fmt:url"
+  <a tal:attributes="href context/fmt:url;
+                     title view/status_title"
      tal:content="view/friendly_text">Approved</a>
   <tal:for-merging condition="not: context/queue_status/enumvalue:MERGED">
     for merging

=== added file 'lib/lp/code/templates/branchmergeproposal-vote-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-vote-summary.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/branchmergeproposal-vote-summary.pt	2009-11-18 15:05:38 +0000
@@ -0,0 +1,52 @@
+<tal:root
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n">
+
+  <tal:comment condition="nothing">
+    <!--
+        Yet again we are bitten by white space issues, so some tags in this
+        template have closing brackets on following lines, and not breaks
+        between some tags.
+    -->
+  </tal:comment>
+
+<dl class="reviews">
+  <dd tal:condition="not: view/reviews">
+    No reviews requested
+  </dd>
+  <dd tal:repeat="review view/current_reviews">
+    <tal:reviewer replace="structure review/reviewer/fmt:link:mainsite">
+    Eric the Reviewer</tal:reviewer
+    ><tal:community condition="not: review/trusted">
+    (community)</tal:community>:
+    <span tal:attributes="class string:vote${review/comment/vote/name}"
+          tal:content="review/status_text">
+      Approved
+    </span>
+    <tal:vote-tags condition="review/review_type_str">
+      (<tal:tag replace="review/review_type_str"/>)
+    </tal:vote-tags>
+    <tal:date replace="review/date_of_comment/fmt:displaydate" />
+  </dd>
+  <dd tal:repeat="review view/requested_reviews"
+      tal:attributes="id string:review-${review/reviewer/name}">
+    <tal:reviewer
+        tal:replace="structure review/reviewer/fmt:link:mainsite" />:
+
+    <span class="votePENDING">Pending</span>
+    <tal:vote-tags condition="review/review_type_str">
+      (<tal:tag replace="review/review_type_str"/>)
+    </tal:vote-tags>
+    requested
+    <tal:date replace="review/date_requested/fmt:approximatedate"/>
+  </dd>
+  <dd tal:condition="context/preview_diff"
+      tal:attributes="class string:popup-diff mp-${context/id}">
+    Diff: <tal:diff replace="structure context/preview_diff/fmt:link"/>
+    <a class="api-ref" style="display:none"
+       tal:attributes="href context/preview_diff/fmt:api_url">api</a>
+  </dd>
+
+</dl>
+</tal:root>

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2009-10-26 18:40:04 +0000
+++ lib/lp/code/tests/helpers.py	2009-11-18 15:05:38 +0000
@@ -6,10 +6,12 @@
 __metaclass__ = type
 __all__ = [
     'make_linked_package_branch',
+    'make_erics_fooix_project',
     ]
 
 
 from datetime import timedelta
+from difflib import unified_diff
 from itertools import count
 
 from zope.component import getUtility
@@ -23,6 +25,46 @@
 from lp.testing import time_counter
 
 
+def make_erics_fooix_project(factory):
+    """Make Eric, the Fooix project, and some branches.
+
+    :return: a dict of objects to put into local scope.
+    """
+    result = {}
+    eric = factory.makePerson(
+        name='eric', displayname='Eric the Viking',
+        email='[e-mail]', password='test')
+    fooix = factory.makeProduct(
+        name='fooix', displayname='Fooix', owner=eric)
+    trunk = factory.makeProductBranch(
+        owner=eric, product=fooix, name='trunk')
+    removeSecurityProxy(fooix.development_focus).branch = trunk
+    # Development is done by Fred.
+    fred = factory.makePerson(
+        name='fred', displayname='Fred Flintstone',
+        email='[e-mail]', password='test')
+    feature = factory.makeProductBranch(
+        owner=fred, product=fooix, name='feature')
+    proposed = factory.makeProductBranch(
+        owner=fred, product=fooix, name='proposed')
+    bmp = proposed.addLandingTarget(
+        registrant=fred, target_branch=trunk, needs_review=True,
+        review_requests=[(eric, 'code')])
+    # And fake a diff.
+    naked_bmp = removeSecurityProxy(bmp)
+    preview = removeSecurityProxy(naked_bmp.updatePreviewDiff(
+        ''.join(unified_diff('', 'random content')), u'rev-a', u'rev-b'))
+    naked_bmp.source_branch.last_scanned_id = preview.source_revision_id
+    naked_bmp.target_branch.last_scanned_id = preview.target_revision_id
+    preview.diff_lines_count = 47
+    preview.added_lines_count = 7
+    preview.remvoed_lines_count = 13
+    preview.diffstat = {'file1': (3, 8), 'file2': (4, 5)}
+    return {
+        'eric': eric, 'fooix': fooix, 'trunk':trunk, 'feature': feature,
+        'proposed': proposed, 'fred': fred}
+
+
 def make_linked_package_branch(factory, distribution=None,
                                sourcepackagename=None):
     """Make a new package branch and make it official."""

=== modified file 'lib/lp/code/tests/test_directbranchcommit.py'
--- lib/lp/code/tests/test_directbranchcommit.py	2009-10-02 09:48:30 +0000
+++ lib/lp/code/tests/test_directbranchcommit.py	2009-11-18 15:05:39 +0000
@@ -37,7 +37,7 @@
 
         self.committer = DirectBranchCommit(self.db_branch)
         if update_last_scanned_id:
-            self.db_branch.last_scanned_id = (
+            self.committer.last_scanned_id = (
                 self.committer.bzrbranch.last_revision())
 
     def _tearDownCommitter(self):

=== added file 'lib/lp/code/windmill/tests/test_popup_diff.py'
--- lib/lp/code/windmill/tests/test_popup_diff.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/windmill/tests/test_popup_diff.py	2009-11-18 15:05:39 +0000
@@ -0,0 +1,64 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test for the popup diff."""
+
+__metaclass__ = type
+__all__ = []
+
+import transaction
+import unittest
+
+import windmill
+from windmill.authoring import WindmillTestClient
+
+from canonical.launchpad.windmill.testing.constants import PAGE_LOAD
+from lp.code.tests.helpers import make_erics_fooix_project
+from lp.code.windmill.testing import CodeWindmillLayer
+from lp.testing import TestCaseWithFactory
+
+
+POPUP_DIFF = (
+    u'//dd[contains(@class, "popup-diff")]'
+    '/a[contains(@class, "js-action")]')
+VISIBLE_DIFF = (
+    u'//table[contains(@class, "yui-diff-overlay") and '
+     'not(contains(@class, "yui-diff-overlay-hidden"))]')
+CLOSE_VISIBLE_DIFF = (
+    u'//table[contains(@class, "yui-diff-overlay")]'
+     '//a[@class="close-button"]')
+JS_ONLOAD_EXECUTE_DELAY = 1000
+
+
+class TestPopupOnBranchPage(TestCaseWithFactory):
+    """Test the popup diff."""
+
+    layer = CodeWindmillLayer
+
+    def test_branch_popup_diff(self):
+        """Test branch diff popups."""
+        client = WindmillTestClient("Branch popup diffs")
+        make_erics_fooix_project(self.factory)
+        transaction.commit()
+
+        start_url = (
+            windmill.settings['TEST_URL'] + '~fred/fooix/proposed')
+        client.open(url=start_url)
+        client.waits.forPageLoad(timeout=PAGE_LOAD)
+        # Sleep for a bit to make sure that the JS onload has had time to execute.
+        client.waits.sleep(milliseconds=JS_ONLOAD_EXECUTE_DELAY)
+
+        # Make sure that the link anchor has the js-action class.
+        client.asserts.assertNode(xpath=POPUP_DIFF)
+        client.click(xpath=POPUP_DIFF)
+
+        # Wait for the diff to show.
+        client.waits.forElement(xpath=VISIBLE_DIFF)
+        # Click on the close button.
+        client.click(xpath=CLOSE_VISIBLE_DIFF)
+        # Make sure that the diff has gone.
+        client.asserts.assertNotNode(xpath=VISIBLE_DIFF)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/services/browser_helpers.py'
--- lib/lp/services/browser_helpers.py	2009-09-07 14:54:34 +0000
+++ lib/lp/services/browser_helpers.py	2009-11-18 15:05:39 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'get_user_agent_distroseries',
+    'get_plural_text',
     ]
 
 import re
@@ -28,3 +29,10 @@
     else:
         return None
 
+
+def get_plural_text(count, singular, plural):
+    """Return 'singular' if 'count' is 1, 'plural' otherwise."""
+    if count == 1:
+        return singular
+    else:
+        return plural

=== modified file 'lib/lp/translations/doc/translations-export-to-branch.txt'
--- lib/lp/translations/doc/translations-export-to-branch.txt	2009-10-20 06:03:46 +0000
+++ lib/lp/translations/doc/translations-export-to-branch.txt	2009-11-18 15:05:39 +0000
@@ -45,7 +45,7 @@
     ...         if self.simulate_race:
     ...             raise ConcurrentUpdateError("Simulated race condition.")
     ...
-    ...     def commit(self, message=None):
+    ...     def commit(self, message=None, txn=None):
     ...         self._checkForRace()
     ...         self.logger.info("Committed %d file(s)." % self.written_files)
     ...
@@ -129,11 +129,11 @@
 
     >>> transaction.commit()
     >>> script = MockExportTranslationsToBranch(
-    ...     'export-to-branch', test_args=['-vv'])
+    ...     'export-to-branch', test_args=[])
     >>> script.main()
     INFO Exporting to translations branches.
     INFO Exporting Gazblachko trunk series.
-    DEBUG No previous translations commit found.
+    DEBUG ...
     INFO Writing file 'po/main/nl.po':
     INFO # ...
     msgid ""
@@ -143,6 +143,7 @@
     msgid "maingazpot msgid"
     msgstr "maingazpot msgstr"
     <BLANKLINE>
+    DEBUG ...
     INFO Writing file 'po/module/nl.po':
     INFO # ...
     msgid ""
@@ -152,6 +153,8 @@
     <BLANKLINE>
     msgid "gazmod msgid"
     msgstr "gazmod msgstr"
+    <BLANKLINE>
+    DEBUG ...
     INFO Committed 2 file(s).
     INFO Unlock.
     INFO Processed 1 item(s); 0 failure(s).
@@ -186,6 +189,7 @@
     >>> script.main()
     INFO Exporting to translations branches.
     INFO Exporting Gazblachko trunk series.
+    DEBUG ....
     DEBUG Last commit was at ....
     INFO Unlock.
     INFO Processed 1 item(s); 0 failure(s).
@@ -197,6 +201,7 @@
     >>> script.main()
     INFO Exporting to translations branches.
     INFO Exporting Gazblachko trunk series.
+    DEBUG ....
     DEBUG Last commit was at ...
     INFO Writing file 'po/main/nl.po':
     INFO ...
@@ -228,11 +233,14 @@
     >>> script.main()
     INFO Exporting to translations branches.
     INFO Exporting Gazblachko trunk series.
+    DEBUG ....
     DEBUG No previous translations commit found.
+    DEBUG ....
     INFO Writing file 'po/main/nl.po':
     ...
     msgstr "gazmod msgstr"
     <BLANKLINE>
+    DEBUG ...
     INFO Unlock.
     ERROR Failure: Simulated race condition.
     INFO Processed 1 item(s); 1 failure(s).

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2009-10-20 06:03:46 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2009-11-18 15:05:39 +0000
@@ -119,7 +119,7 @@
     def _commit(self, source, committer):
         """Commit changes to branch.  Check for race conditions."""
         self._checkForObjections(source)
-        committer.commit(self.commit_message)
+        committer.commit(self.commit_message, txn=self.txn)
 
     def _isTranslationsCommit(self, revision):
         """Is `revision` an automatic translations commit?"""
@@ -159,6 +159,9 @@
         self._checkForObjections(source)
 
         committer = self._prepareBranchCommit(source.translations_branch)
+        self.logger.debug("Created DirectBranchCommit.")
+        if self.txn:
+            self.txn.commit()
 
         bzr_branch = committer.bzrbranch
 
@@ -185,15 +188,17 @@
                 base_path = os.path.dirname(template.path)
 
                 for pofile in template.pofiles:
-
                     has_changed = (
                         changed_since is None or
                         pofile.date_changed > changed_since)
                     if not has_changed:
                         continue
 
+                    language_code = pofile.getFullLanguageCode()
+                    self.logger.debug("Exporting %s." % language_code)
+
                     pofile_path = os.path.join(
-                        base_path, pofile.getFullLanguageCode() + '.po')
+                        base_path, language_code + '.po')
                     pofile_contents = pofile.export()
 
                     committer.writeFile(pofile_path, pofile_contents)
@@ -210,6 +215,7 @@
                     template.clearPOFileCache()
 
             if change_count > 0:
+                self.logger.debug("Writing to branch.")
                 self._commit(source, committer)
         finally:
             committer.unlock()
@@ -231,13 +237,14 @@
                 raise
             except Exception, e:
                 items_failed += 1
-                self.logger.error("Failure: %s" % e)
+                message = unicode(e)
+                if message == u'':
+                    message = e.__class__.__name__
+                self.logger.error("Failure: %s" % message)
                 if self.txn:
                     self.txn.abort()
 
             items_done += 1
-            if self.txn:
-                self.txn.begin()
 
         self.logger.info("Processed %d item(s); %d failure(s)." % (
             items_done, items_failed))

=== modified file 'lib/lp/translations/utilities/gettext_po_exporter.py'
--- lib/lp/translations/utilities/gettext_po_exporter.py	2009-11-02 00:10:05 +0000
+++ lib/lp/translations/utilities/gettext_po_exporter.py	2009-11-18 15:05:39 +0000
@@ -14,6 +14,7 @@
     'GettextPOExporter',
     ]
 
+import logging
 import os
 
 from zope.interface import implements
@@ -344,8 +345,20 @@
                         raise UnicodeEncodeError(
                             '%s:\n%s' % (file_path, str(error)))
 
-                    # This message cannot be represented in current encoding,
-                    # change to UTF-8 and try again.
+                    # This message cannot be represented in the current
+                    # encoding.
+                    if translation_file.path:
+                        file_description = translation_file.path
+                    elif translation_file.language_code:
+                        file_description = (
+                            "%s translation" % translation_file.language_code)
+                    else:
+                        file_description = "template"
+                    logging.info(
+                        "Can't represent %s as %s; using UTF-8 instead." % (
+                            file_description,
+                            translation_file.header.charset.upper()))
+
                     old_charset = translation_file.header.charset
                     translation_file.header.charset = 'UTF-8'
                     # We need to update the header too.