Merge lp:~abentley/launchpad/incremental-diff-job into lp:launchpad/db-devel

[Aaron Bentley]

Implement jobs for generating incremental diffs.

= Summary =
Implement jobs for generating incremental diffs for merge proposals, and create
these jobs when the merge proposal source banch tip changes.

== Proposed fix ==
See above

== Pre-implementation notes ==
Discussed with Thumper

== Implementation details ==
As a driveby, I extracted commit_file and create_example_merge so that they
could be reused more easily, without necessarily deriving from DiffTestCase.

I also added a job_type parameter to BranchMergeProposalJobSource.iterReady, to
make testing easier.

There were also a few lint fixes, like removing a semicolon.

== Tests ==
bin/test -t TestGenerateIncrementalDiffJob -t test_script_runs

== Demo and Q/A ==
Create a merge proposal. After the initial diff has been generated, push new
changes to the source branch. As well as updating the diff, this should show a
diff from the old branch tip to the new branch tip.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/codehosting/scanner/tests/test_bzrsync.py
  lib/lp/code/browser/configure.zcml
  lib/lp/code/configure.zcml
  lib/lp/code/model/tests/test_branchmergeproposaljobs.py
  lib/lp/code/interfaces/branchmergeproposal.py
  lib/lp/code/model/branchmergeproposal.py
  lib/lp/code/model/tests/test_branchmergeproposal.py
  lib/lp/code/browser/tests/test_branchmergeproposal.py
  lib/lp/code/scripts/tests/test_merge_proposal_jobs.py
  lib/lp/code/model/diff.py
  lib/lp/testing/factory.py
  lib/lp/code/model/tests/test_diff.py
  lib/lp/code/templates/incrementaldiffcomment-body.pt
  lib/lp/code/model/branch.py
  lib/lp/code/model/branchmergeproposaljob.py
  lib/lp/code/browser/branchmergeproposal.py
  utilities/sourcedeps.conf
  lib/lp/code/templates/incrementaldiffcomment-header.pt
  lib/lp/code/interfaces/diff.py

./lib/lp/codehosting/scanner/tests/test_bzrsync.py
     618: E202 whitespace before ')'
     635: E231 missing whitespace after ','
./lib/lp/code/model/tests/test_branchmergeproposal.py
     192: E231 missing whitespace after ','
     194: E231 missing whitespace after ','
./lib/lp/code/scripts/tests/test_merge_proposal_jobs.py
      15: E202 whitespace before ')'
./lib/lp/code/model/diff.py
     166: E301 expected 1 blank line, found 0
./lib/lp/code/model/branchmergeproposaljob.py
       5: E303 too many blank lines (3)
     292: E202 whitespace before ')'
./lib/lp/code/browser/branchmergeproposal.py
     168: E301 expected 1 blank line, found 0
     178: E301 expected 1 blank line, found 0
     201: E202 whitespace before '}'
    1471: E301 expected 1 blank line, found 0

[Graham Binns (gmb)]

Hi Aaron,

Nice branch, just a few things that need fixing before it lands:

 * On line 94 of the diff, you need to indent the closing parenthesis by 4 more spaces.
 * On line 302 you need to add a \n after the opening bracket of the clauses list. You also need to fix the indentation of the list items so that they're four-spaces in from clauses = [ line.
 * You need to add docstrings / comments for the last four tests in TestGenerateIncrementalDiffJob.

review: Approve (code)

[Brad Crittenden (bac)]

Is this approved branch ready to land or are you blocked? If you have abandoned the work please change the MP status to "Rejected."

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/04/2010 11:09 PM, Brad Crittenden wrote:
> Is this approved branch ready to land or are you blocked? If you have abandoned the work please change the MP status to "Rejected."

This branch was blocked. See my "Mysterious storm bug" post. Then Jamu
and I found a solution. Then there was another bug relating to test
isolation with feature flags, which Tim was able to solve. I merged
Tim's changes from devel, but those brought with them actual windmill
failures introduced by Ian Booth. When those were resolved, I merged
again and attempted to land, only to discover we were in testfix mode
due to spurious windmill failures on db-devel. I forced a build and
when it passed, I attempted to land the branch, but the landing failed
due to conflicts. That was after EOD yesterday. Today I plan to
resolve the conflicts and land it.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzUBC4ACgkQ0F+nu1YWqI3a/ACcCcBLY4NCzM5jkqtJsYsxNGI4
6LQAn2JpFS1GQaqzDOaMlOdfU8d1f3Ll
=KAU9
-----END PGP SIGNATURE-----

[Aaron Bentley (abentley)]

I tried to land once again, and once again we are in testfix mode due to the same spurious windmill failure.

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-11-05 14:03:28 +0000
+++ lib/lp/code/configure.zcml	2010-11-05 14:03:38 +0000
@@ -307,6 +307,11 @@
     <allow interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalJobSource"/>
   </securedutility>
 
+  <class
+  class="lp.code.model.branchmergeproposaljob.GenerateIncrementalDiffJob">
+    <allow interface="lp.code.interfaces.branchmergeproposal.IGenerateIncrementalDiffJob"/>
+    <allow interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalJob" />
+  </class>
   <class class="lp.code.model.branchmergeproposaljob.UpdatePreviewDiffJob">
     <allow interface="lp.code.interfaces.branchmergeproposal.IUpdatePreviewDiffJob" />
     <allow interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalJob" />

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2010-10-25 05:33:20 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2010-11-05 14:03:38 +0000
@@ -17,6 +17,8 @@
     'ICodeReviewCommentEmailJobSource',
     'ICreateMergeProposalJob',
     'ICreateMergeProposalJobSource',
+    'IGenerateIncrementalDiffJob',
+    'IGenerateIncrementalDiffJobSource',
     'IMergeProposalNeedsReviewEmailJob',
     'IMergeProposalNeedsReviewEmailJobSource',
     'IMergeProposalUpdatedEmailJob',
@@ -556,6 +558,10 @@
     """A job source that will get all supported merge proposal jobs."""
 
 
+class IBranchMergeProposalJobSource(IJobSource):
+    """A job source that will get all supported merge proposal jobs."""
+
+
 class IBranchMergeProposalListingBatchNavigator(ITableBatchNavigator):
     """A marker interface for registering the appropriate listings."""
 
@@ -661,6 +667,20 @@
         """Return the UpdatePreviewDiffJob with this id."""
 
 
+class IGenerateIncrementalDiffJob(IRunnableJob):
+    """Interface for the job to update the diff for a merge proposal."""
+
+
+class IGenerateIncrementalDiffJobSource(Interface):
+    """Create or retrieve jobs that update preview diffs."""
+
+    def create(bmp, old_revision_id, new_revision_id):
+        """Create job to generate incremental diff for this merge proposal."""
+
+    def get(id):
+        """Return the GenerateIncrementalDiffJob with this id."""
+
+
 class ICodeReviewCommentEmailJob(IRunnableJob):
     """Interface for the job to send code review comment email."""
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-11-04 19:59:02 +0000
+++ lib/lp/code/model/branch.py	2010-11-05 14:03:38 +0000
@@ -471,10 +471,18 @@
 
     def scheduleDiffUpdates(self):
         """See `IBranch`."""
-        from lp.code.model.branchmergeproposaljob import UpdatePreviewDiffJob
-        jobs = [UpdatePreviewDiffJob.create(target)
-                for target in self.active_landing_targets
-                if target.target_branch.last_scanned_id is not None]
+        from lp.code.model.branchmergeproposaljob import (
+                GenerateIncrementalDiffJob,
+                UpdatePreviewDiffJob,
+            )
+        jobs = []
+        for merge_proposal in self.active_landing_targets:
+            if merge_proposal.target_branch.last_scanned_id is None:
+                continue
+            jobs.append(UpdatePreviewDiffJob.create(merge_proposal))
+            for old, new in merge_proposal.getMissingIncrementalDiffs():
+                GenerateIncrementalDiffJob.create(
+                    merge_proposal, old.revision_id, new.revision_id)
         return jobs
 
     def addToLaunchBag(self, launchbag):

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2010-11-05 14:03:28 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2010-11-05 14:03:38 +0000
@@ -785,6 +785,12 @@
         Store.of(self).flush()
         return self.preview_diff
 
+    def getIncrementalDiffRanges(self):
+        groups = self.getRevisionsSinceReviewStart()
+        return [
+            (group[0].revision.getLefthandParent(), group[-1].revision)
+            for group in groups]
+
     def generateIncrementalDiff(self, old_revision, new_revision, diff=None):
         """Generate an incremental diff for the merge proposal.
 
@@ -870,6 +876,11 @@
         if current_group != []:
             yield current_group
 
+    def getMissingIncrementalDiffs(self):
+        ranges = self.getIncrementalDiffRanges()
+        diffs = self.getIncrementalDiffs(ranges)
+        return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
+
 
 class BranchMergeProposalGetter:
     """See `IBranchMergeProposalGetter`."""

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2010-10-26 15:47:24 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2010-11-05 14:03:38 +0000
@@ -19,6 +19,8 @@
     'BranchMergeProposalJobType',
     'CodeReviewCommentEmailJob',
     'CreateMergeProposalJob',
+    'GenerateIncrementalDiffJob',
+    'MergeProposalCreatedJob',
     'MergeProposalNeedsReviewEmailJob',
     'MergeProposalUpdatedEmailJob',
     'ReviewRequestedEmailJob',
@@ -83,6 +85,8 @@
     ICodeReviewCommentEmailJobSource,
     ICreateMergeProposalJob,
     ICreateMergeProposalJobSource,
+    IGenerateIncrementalDiffJob,
+    IGenerateIncrementalDiffJobSource,
     IMergeProposalNeedsReviewEmailJob,
     IMergeProposalNeedsReviewEmailJobSource,
     IMergeProposalUpdatedEmailJob,
@@ -92,6 +96,7 @@
     IUpdatePreviewDiffJob,
     IUpdatePreviewDiffJobSource,
     )
+from lp.code.interfaces.revision import IRevisionSet
 from lp.code.mail.branch import RecipientReason
 from lp.code.mail.branchmergeproposal import BMPMailer
 from lp.code.mail.codereviewcomment import CodeReviewCommentMailer
@@ -144,6 +149,11 @@
         that have been changed on the merge proposal itself.
         """)
 
+    GENERATE_INCREMENTAL_DIFF = DBItem(5, """
+        Generate incremental diff
+
+        This job generates an incremental diff for a merge proposal.""")
+
 
 class BranchMergeProposalJob(Storm):
     """Base class for jobs related to branch merge proposals."""
@@ -282,7 +292,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BaseRunnableJob.getOopsVars(self)
+        vars = BaseRunnableJob.getOopsVars(self)
         bmp = self.context.branch_merge_proposal
         vars.extend([
             ('branchmergeproposal_job_id', self.context.id),
@@ -490,7 +500,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BranchMergeProposalJobDerived.getOopsVars(self)
+        vars = BranchMergeProposalJobDerived.getOopsVars(self)
         vars.extend([
             ('code_review_comment', self.metadata['code_review_comment']),
             ])
@@ -552,7 +562,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BranchMergeProposalJobDerived.getOopsVars(self)
+        vars = BranchMergeProposalJobDerived.getOopsVars(self)
         vars.extend([
             ('reviewer', self.metadata['reviewer']),
             ('requester', self.metadata['requester']),
@@ -601,7 +611,7 @@
     def getMetadata(delta_text, editor):
         metadata = {'delta_text': delta_text}
         if editor is not None:
-            metadata['editor'] = editor.name;
+            metadata['editor'] = editor.name
         return metadata
 
     @property
@@ -620,7 +630,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BranchMergeProposalJobDerived.getOopsVars(self)
+        vars = BranchMergeProposalJobDerived.getOopsVars(self)
         vars.extend([
             ('editor', self.metadata.get('editor', '(not set)')),
             ('delta_text', self.metadata['delta_text']),
@@ -638,6 +648,70 @@
         return 'emailing subscribers about merge proposal changes'
 
 
+class GenerateIncrementalDiffJob(BranchMergeProposalJobDerived):
+    """A job to generate an incremental diff for a branch merge proposal.
+
+    Provides class methods to create and retrieve such jobs.
+    """
+
+    implements(IGenerateIncrementalDiffJob)
+
+    classProvides(IGenerateIncrementalDiffJobSource)
+
+    class_job_type = BranchMergeProposalJobType.GENERATE_INCREMENTAL_DIFF
+
+    def acquireLease(self, duration=600):
+        return self.job.acquireLease(duration)
+
+    def run(self):
+        revision_set = getUtility(IRevisionSet)
+        old_revision = revision_set.getByRevisionId(self.old_revision_id)
+        new_revision = revision_set.getByRevisionId(self.new_revision_id)
+        diff = self.branch_merge_proposal.generateIncrementalDiff(
+            old_revision, new_revision)
+
+    @classmethod
+    def create(cls, merge_proposal, old_revision_id, new_revision_id):
+        metadata = cls.getMetadata(old_revision_id, new_revision_id)
+        job = BranchMergeProposalJob(
+            merge_proposal, cls.class_job_type, metadata)
+        return cls(job)
+
+    @staticmethod
+    def getMetadata(old_revision_id, new_revision_id):
+        return {
+            'old_revision_id': old_revision_id,
+            'new_revision_id': new_revision_id,
+        }
+
+    @property
+    def old_revision_id(self):
+        """The old revision id for the diff."""
+        return self.metadata['old_revision_id']
+
+    @property
+    def new_revision_id(self):
+        """The new revision id for the diff."""
+        return self.metadata['new_revision_id']
+
+    def getOopsVars(self):
+        """See `IRunnableJob`."""
+        vars = BranchMergeProposalJobDerived.getOopsVars(self)
+        vars.extend([
+            ('old_revision_id', self.metadata['old_revision_id']),
+            ('new_revision_id', self.metadata['new_revision_id']),
+            ])
+        return vars
+
+    def getOperationDescription(self):
+        return ('generating an incremental diff for a merge proposal')
+
+    def getErrorRecipients(self):
+        """Return a list of email-ids to notify about user errors."""
+        registrant = self.branch_merge_proposal.registrant
+        return format_address_for_person(registrant)
+
+
 class BranchMergeProposalJobFactory:
     """Construct a derived merge proposal job for a BranchMergeProposalJob."""
 
@@ -652,6 +726,8 @@
             ReviewRequestedEmailJob,
         BranchMergeProposalJobType.MERGE_PROPOSAL_UPDATED:
             MergeProposalUpdatedEmailJob,
+        BranchMergeProposalJobType.GENERATE_INCREMENTAL_DIFF:
+            GenerateIncrementalDiffJob,
         }
 
     @classmethod
@@ -692,21 +768,24 @@
         return BranchMergeProposalJobFactory.create(job)
 
     @staticmethod
-    def iterReady():
+    def iterReady(job_type=None):
         from lp.code.model.branch import Branch
         store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
         SourceBranch = ClassAlias(Branch)
         TargetBranch = ClassAlias(Branch)
+        clauses = [
+            BranchMergeProposalJob.job == Job.id,
+            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
+            BranchMergeProposalJob.branch_merge_proposal ==
+            BranchMergeProposal.id, BranchMergeProposal.source_branch ==
+            SourceBranch.id, BranchMergeProposal.target_branch ==
+            TargetBranch.id,
+            ]
+        if job_type is not None:
+            clauses.append(BranchMergeProposalJob.job_type == job_type)
         jobs = store.find(
             (BranchMergeProposalJob, Job, BranchMergeProposal,
-             SourceBranch, TargetBranch),
-            And(BranchMergeProposalJob.job == Job.id,
-                Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
-                BranchMergeProposalJob.branch_merge_proposal
-                    == BranchMergeProposal.id,
-                BranchMergeProposal.source_branch == SourceBranch.id,
-                BranchMergeProposal.target_branch == TargetBranch.id,
-                ))
+             SourceBranch, TargetBranch), And(*clauses))
         # Order by the job status first (to get running before waiting), then
         # the date_created, then job type.  This should give us all creation
         # jobs before comment jobs.

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2010-10-28 22:31:48 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2010-11-05 14:03:38 +0000
@@ -32,6 +32,8 @@
     IBranchMergeProposalJobSource,
     ICodeReviewCommentEmailJob,
     ICodeReviewCommentEmailJobSource,
+    IGenerateIncrementalDiffJob,
+    IGenerateIncrementalDiffJobSource,
     IMergeProposalNeedsReviewEmailJob,
     IMergeProposalUpdatedEmailJob,
     IMergeProposalUpdatedEmailJobSource,
@@ -45,12 +47,13 @@
     BranchMergeProposalJobDerived,
     BranchMergeProposalJobType,
     CodeReviewCommentEmailJob,
+    GenerateIncrementalDiffJob,
     MergeProposalNeedsReviewEmailJob,
     MergeProposalUpdatedEmailJob,
     ReviewRequestedEmailJob,
     UpdatePreviewDiffJob,
     )
-from lp.code.model.tests.test_diff import DiffTestCase
+from lp.code.model.tests.test_diff import DiffTestCase, create_example_merge
 from lp.code.subscribers.branchmergeproposal import merge_proposal_modified
 from lp.services.job.model.job import Job
 from lp.services.job.runner import JobRunner
@@ -192,7 +195,7 @@
 
     def test_run(self):
         self.useBzrBranches(direct_database=True)
-        bmp = self.createExampleMerge()[0]
+        bmp = create_example_merge(self)[0]
         job = UpdatePreviewDiffJob.create(bmp)
         self.factory.makeRevisionsForBranch(bmp.source_branch, count=1)
         bmp.source_branch.next_mirror_time = None
@@ -226,13 +229,76 @@
 
     def test_10_minute_lease(self):
         self.useBzrBranches(direct_database=True)
-        bmp = self.createExampleMerge()[0]
+        bmp = create_example_merge(self)[0]
         job = UpdatePreviewDiffJob.create(bmp)
         job.acquireLease()
         expiry_delta = job.lease_expires - datetime.now(pytz.UTC)
         self.assertTrue(500 <= expiry_delta.seconds, expiry_delta)
 
 
+def make_runnable_incremental_diff_job(test_case):
+    test_case.useBzrBranches(direct_database=True)
+    bmp, source_rev_id, target_rev_id = create_example_merge(test_case)
+    repository = bmp.source_branch.getBzrBranch().repository
+    parent_id = repository.get_revision(source_rev_id).parent_ids[0]
+    test_case.factory.makeRevision(rev_id=source_rev_id)
+    test_case.factory.makeRevision(rev_id=parent_id)
+    return GenerateIncrementalDiffJob.create(bmp, parent_id, source_rev_id)
+
+
+class TestGenerateIncrementalDiffJob(DiffTestCase):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_implement_interface(self):
+        """GenerateIncrementalDiffJob implements its interface."""
+        verifyObject(
+            IGenerateIncrementalDiffJobSource, GenerateIncrementalDiffJob)
+
+    def test_providesInterface(self):
+        """MergeProposalCreatedJob provides the expected interfaces."""
+        bmp = self.factory.makeBranchMergeProposal()
+        job = GenerateIncrementalDiffJob.create(bmp, 'old', 'new')
+        verifyObject(IGenerateIncrementalDiffJob, job)
+        verifyObject(IBranchMergeProposalJob, job)
+
+    def test_getOperationDescription(self):
+        """The description of the job is sane."""
+        bmp = self.factory.makeBranchMergeProposal()
+        job = GenerateIncrementalDiffJob.create(bmp, 'old', 'new')
+        self.assertEqual(
+            'generating an incremental diff for a merge proposal',
+            job.getOperationDescription())
+
+    def test_run(self):
+        """The job runs successfully, and its results can be committed."""
+        job = make_runnable_incremental_diff_job(self)
+        transaction.commit()
+        self.layer.switchDbUser(config.merge_proposal_jobs.dbuser)
+        job.run()
+        transaction.commit()
+
+    def test_run_all(self):
+        """The job can be run under the JobRunner successfully."""
+        job = make_runnable_incremental_diff_job(self)
+        transaction.commit()
+        self.layer.switchDbUser(config.merge_proposal_jobs.dbuser)
+        runner = JobRunner([job])
+        runner.runAll()
+        self.assertEqual([job], runner.completed_jobs)
+
+    def test_10_minute_lease(self):
+        """Newly-created jobs have a ten-minute lease."""
+        self.useBzrBranches(direct_database=True)
+        bmp = create_example_merge(self)[0]
+        job = GenerateIncrementalDiffJob.create(bmp, 'old', 'new')
+        transaction.commit()
+        self.layer.switchDbUser(config.merge_proposal_jobs.dbuser)
+        job.acquireLease()
+        expiry_delta = job.lease_expires - datetime.now(pytz.UTC)
+        self.assertTrue(500 <= expiry_delta.seconds, expiry_delta)
+
+
 class TestBranchMergeProposalJobSource(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2010-11-05 14:03:28 +0000
+++ lib/lp/code/model/tests/test_diff.py	2010-11-05 14:03:38 +0000
@@ -54,37 +54,41 @@
         self.records.append(record)
 
 
+def commit_file(branch, path, contents, merge_parents=None):
+    """Create a commit that updates a file to specified contents.
+
+    This will create or modify the file, as needed.
+    """
+    committer = DirectBranchCommit(
+        removeSecurityProxy(branch), no_race_check=True,
+        merge_parents=merge_parents)
+    committer.writeFile(path, contents)
+    try:
+        return committer.commit('committing')
+    finally:
+        committer.unlock()
+
+
+def create_example_merge(test_case):
+    """Create a merge proposal with conflicts and updates."""
+    bmp = test_case.factory.makeBranchMergeProposal()
+    # Make the branches of the merge proposal look good as far as the
+    # model is concerned.
+    test_case.factory.makeRevisionsForBranch(bmp.source_branch)
+    test_case.factory.makeRevisionsForBranch(bmp.target_branch)
+    bzr_target = test_case.createBzrBranch(bmp.target_branch)
+    commit_file(bmp.target_branch, 'foo', 'a\n')
+    test_case.createBzrBranch(bmp.source_branch, bzr_target)
+    source_rev_id = commit_file(bmp.source_branch, 'foo', 'd\na\nb\n')
+    target_rev_id = commit_file(bmp.target_branch, 'foo', 'c\na\n')
+    return bmp, source_rev_id, target_rev_id
+
+
 class DiffTestCase(TestCaseWithFactory):
 
-    @staticmethod
-    def commitFile(branch, path, contents, merge_parents=None):
-        """Create a commit that updates a file to specified contents.
-
-        This will create or modify the file, as needed.
-        """
-        committer = DirectBranchCommit(
-            removeSecurityProxy(branch), no_race_check=True,
-            merge_parents=merge_parents)
-        committer.writeFile(path, contents)
-        try:
-            return committer.commit('committing')
-        finally:
-            committer.unlock()
-
     def createExampleMerge(self):
-        """Create a merge proposal with conflicts and updates."""
         self.useBzrBranches(direct_database=True)
-        bmp = self.factory.makeBranchMergeProposal()
-        # Make the branches of the merge proposal look good as far as the
-        # model is concerned.
-        self.factory.makeRevisionsForBranch(bmp.source_branch)
-        self.factory.makeRevisionsForBranch(bmp.target_branch)
-        bzr_target = self.createBzrBranch(bmp.target_branch)
-        self.commitFile(bmp.target_branch, 'foo', 'a\n')
-        self.createBzrBranch(bmp.source_branch, bzr_target)
-        source_rev_id = self.commitFile(bmp.source_branch, 'foo', 'd\na\nb\n')
-        target_rev_id = self.commitFile(bmp.target_branch, 'foo', 'c\na\n')
-        return bmp, source_rev_id, target_rev_id
+        return create_example_merge(self)
 
     def checkExampleMerge(self, diff_text):
         """Ensure the diff text matches the values for ExampleMerge."""
@@ -111,12 +115,12 @@
             source = bmp.source_branch
             prerequisite = bmp.prerequisite_branch
         target_bzr = self.createBzrBranch(target)
-        self.commitFile(target, 'file', 'target text\n')
+        commit_file(target, 'file', 'target text\n')
         prerequisite_bzr = self.createBzrBranch(prerequisite, target_bzr)
-        self.commitFile(
+        commit_file(
             prerequisite, 'file', 'target text\nprerequisite text\n')
         source_bzr = self.createBzrBranch(source, prerequisite_bzr)
-        source_rev_id = self.commitFile(
+        source_rev_id = commit_file(
             source, 'file',
             'target text\nprerequisite text\nsource text\n')
         return (source_bzr, source_rev_id, target_bzr, prerequisite_bzr,
@@ -243,7 +247,7 @@
         # affect the diff.
         (source_bzr, source_rev_id, target_bzr, prerequisite_bzr,
          prerequisite) = self.preparePrerequisiteMerge()
-        self.commitFile(
+        commit_file(
             prerequisite, 'file', 'prerequisite text2\n')
         diff, conflicts = Diff.mergePreviewFromBranches(
             source_bzr, source_rev_id, target_bzr, prerequisite_bzr)
@@ -459,10 +463,10 @@
         self.useBzrBranches(direct_database=True)
         bmp = self.factory.makeBranchMergeProposal()
         bzr_target = self.createBzrBranch(bmp.target_branch)
-        self.commitFile(bmp.target_branch, 'foo', 'a\n')
+        commit_file(bmp.target_branch, 'foo', 'a\n')
         self.createBzrBranch(bmp.source_branch, bzr_target)
-        source_rev_id = self.commitFile(bmp.source_branch, 'foo', 'a\nb\n')
-        target_rev_id = self.commitFile(bmp.target_branch, 'foo', 'c\na\n')
+        source_rev_id = commit_file(bmp.source_branch, 'foo', 'a\nb\n')
+        target_rev_id = commit_file(bmp.target_branch, 'foo', 'c\na\n')
         diff = PreviewDiff.fromBranchMergeProposal(bmp)
         self.assertEqual('', diff.conflicts)
         self.assertFalse(diff.has_conflicts)
@@ -575,25 +579,22 @@
         bmp = self.factory.makeBranchMergeProposal(
             prerequisite_branch=prerequisite_branch)
         target_branch = self.createBzrBranch(bmp.target_branch)
-        old_revision_id = self.commitFile(
-            bmp.target_branch, 'foo', 'a\nb\ne\n')
+        old_revision_id = commit_file(bmp.target_branch, 'foo', 'a\nb\ne\n')
         old_revision = self.factory.makeRevision(rev_id=old_revision_id)
         source_branch = self.createBzrBranch(
             bmp.source_branch, target_branch)
-        self.commitFile(bmp.source_branch, 'foo', 'a\nc\ne\n')
+        commit_file(bmp.source_branch, 'foo', 'a\nc\ne\n')
         prerequisite = self.createBzrBranch(
             bmp.prerequisite_branch, target_branch)
-        prerequisite_revision = self.commitFile(bmp.prerequisite_branch,
-            'foo', 'd\na\nb\ne\n')
-        merge_parent = self.commitFile(bmp.target_branch, 'foo',
-            'a\nb\ne\nf\n')
+        prerequisite_revision = commit_file(
+            bmp.prerequisite_branch, 'foo', 'd\na\nb\ne\n')
+        merge_parent = commit_file(bmp.target_branch, 'foo', 'a\nb\ne\nf\n')
         source_branch.repository.fetch(target_branch.repository,
             revision_id=merge_parent)
-        self.commitFile(
-            bmp.source_branch, 'foo', 'a\nc\ne\nf\n', [merge_parent])
+        commit_file(bmp.source_branch, 'foo', 'a\nc\ne\nf\n', [merge_parent])
         source_branch.repository.fetch(prerequisite.repository,
             revision_id=prerequisite_revision)
-        new_revision_id = self.commitFile(
+        new_revision_id = commit_file(
             bmp.source_branch, 'foo', 'd\na\nc\ne\nf\n',
             [prerequisite_revision])
         new_revision = self.factory.makeRevision(rev_id=new_revision_id)

=== modified file 'lib/lp/code/scripts/tests/test_merge_proposal_jobs.py'
--- lib/lp/code/scripts/tests/test_merge_proposal_jobs.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/scripts/tests/test_merge_proposal_jobs.py	2010-11-05 14:03:38 +0000
@@ -7,8 +7,14 @@
 
 import unittest
 
+import transaction
+
 from canonical.launchpad.scripts.tests import run_script
 from canonical.testing.layers import ZopelessAppServerLayer
+from lp.code.model.tests.test_branchmergeproposaljobs import (
+    make_runnable_incremental_diff_job
+)
+from lp.services.job.interfaces.job import JobStatus
 from lp.testing import TestCaseWithFactory
 
 
@@ -18,6 +24,8 @@
 
     def test_script_runs(self):
         """Ensure merge-proposal-jobs script runs."""
+        job = make_runnable_incremental_diff_job(self)
+        transaction.commit()
         retcode, stdout, stderr = run_script(
             'cronscripts/merge-proposal-jobs.py', [])
         self.assertEqual(0, retcode)
@@ -25,7 +33,10 @@
         self.assertEqual(
             'INFO    Creating lockfile:'
             ' /var/lock/launchpad-merge-proposal-jobs.lock\n'
-            'INFO    Running through Twisted.\n', stderr)
+            'INFO    Running through Twisted.\n'
+            'INFO    Ran 1 GenerateIncrementalDiffJob jobs.\n', stderr)
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
--- lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-10-26 15:47:24 +0000
+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-11-05 14:03:38 +0000
@@ -31,11 +31,14 @@
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.revision import IRevisionSet
 from lp.code.model.branchrevision import BranchRevision
+from lp.code.model.branchmergeproposaljob import (
+    BranchMergeProposalJobSource, BranchMergeProposalJobType)
 from lp.code.model.revision import (
     Revision,
     RevisionAuthor,
     RevisionParent,
     )
+from lp.code.model.tests.test_diff import commit_file
 from lp.codehosting.bzrutils import write_locked
 from lp.codehosting.scanner.bzrsync import BzrSync
 from lp.services.osutils import override_environ
@@ -654,6 +657,37 @@
         self.assertIsNot(None, bmp.next_preview_diff_job)
 
 
+class TestGenerateIncrementalDiffJob(BzrSyncTestCase):
+    """Test the scheduling of GenerateIncrementalDiffJobs."""
+
+    def getPending(self):
+        return list(
+            BranchMergeProposalJobSource.iterReady(
+                BranchMergeProposalJobType.GENERATE_INCREMENTAL_DIFF
+                )
+            )
+
+    @run_as_db_user(config.launchpad.dbuser)
+    def test_create_on_new_revision(self):
+        """When branch tip changes, a job is created."""
+        parent_id = commit_file(self.db_branch, 'foo', 'bar')
+        self.factory.makeBranchRevision(self.db_branch, parent_id,
+                revision_date=self.factory.getUniqueDate())
+        self.db_branch.last_scanned_id = parent_id
+        bmp = self.factory.makeBranchMergeProposal(
+            source_branch=self.db_branch,
+            date_created=self.factory.getUniqueDate())
+        revision_id = commit_file(self.db_branch, 'foo', 'baz')
+        removeSecurityProxy(bmp).target_branch.last_scanned_id = 'rev'
+        self.assertEqual([], self.getPending())
+        transaction.commit()
+        LaunchpadZopelessLayer.switchDbUser(config.branchscanner.dbuser)
+        self.makeBzrSync(self.db_branch).syncBranchAndClose()
+        (job,) = self.getPending()
+        self.assertEqual(revision_id, job.new_revision_id)
+        self.assertEqual(parent_id, job.old_revision_id)
+
+
 class TestSetRecipeStale(BzrSyncTestCase):
     """Test recipes associated with the branch are marked stale."""