Code review comment for lp:~jtv/launchpad/bug-427278

= 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