launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-revision-sync into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #718511 in Launchpad itself: "Failed initial scans require manual cleanup"
  https://bugs.launchpad.net/launchpad/+bug/718511

For more details, see:
https://code.launchpad.net/~abentley/launchpad/fix-revision-sync/+merge/64596

= Summary =
Fix bug #718511: Failed initial scans require manual cleanup

== Proposed fix ==
Ensure we do not attempt to add BranchRevisions that are already present in the database by removing from the database any BranchRevisions that are present and scheduled to be added.

== Pre-implementation notes ==
None

== Implementation details ==
The branch scanner uses last_scanned_id to determine which revisions to remove from the ancestry of a branch, but this is not necessarily in sync with the BranchRevision table.  If some revisions are present in the BranchRevision table that are not ancestors of last_scanned_id, then the scanner did not realize they were already there, and attemped to add them again.  This changes it to explicitly remove such BranchRevisions.

== Tests ==
bin/test -t test_ancestry_already_present

== Demo and Q/A ==
Untestable.  The instances we know of appear to be the result of the scanner
dying at just the wrong moment.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/codehosting/scanner/tests/test_bzrsync.py
  lib/lp/codehosting/scanner/bzrsync.py
-- 
https://code.launchpad.net/~abentley/launchpad/fix-revision-sync/+merge/64596
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/fix-revision-sync into lp:launchpad.

=== modified file 'lib/lp/codehosting/scanner/bzrsync.py'
--- lib/lp/codehosting/scanner/bzrsync.py	2011-03-29 13:57:20 +0000
+++ lib/lp/codehosting/scanner/bzrsync.py	2011-06-14 19:19:28 +0000
@@ -220,6 +220,13 @@
         revids_to_insert = dict(
             self.revisionsToInsert(
                 added_history, last_revno, added_ancestry))
+        # We must remove any stray BranchRevisions that happen to already be
+        # present.
+        existing_branchrevisions = Store.of(self.db_branch).find(
+            Revision.revision_id, BranchRevision.branch == self.db_branch,
+            BranchRevision.revision_id == Revision.id,
+            Revision.revision_id.is_in(revids_to_insert))
+        branchrevisions_to_delete.update(existing_branchrevisions)
 
         return (added_ancestry, list(branchrevisions_to_delete),
                 revids_to_insert)

=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
--- lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-12-02 16:13:51 +0000
+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py	2011-06-14 19:19:28 +0000
@@ -41,7 +41,10 @@
     RevisionParent,
     )
 from lp.code.model.tests.test_diff import commit_file
-from lp.codehosting.bzrutils import write_locked
+from lp.codehosting.bzrutils import (
+    read_locked,
+    write_locked,
+    )
 from lp.codehosting.scanner.bzrsync import BzrSync
 from lp.services.osutils import override_environ
 from lp.testing import (
@@ -274,7 +277,7 @@
         """
         file = open(os.path.join(self.bzr_tree.basedir, filename), "w")
         if contents is None:
-            file.write(str(time.time()+random.random()))
+            file.write(str(time.time() + random.random()))
         else:
             file.write(contents)
         file.close()
@@ -559,6 +562,28 @@
         self.assertEqual(expected_history, list(db_history))
 
 
+class TestPlanDatabaseChanges(BzrSyncTestCase):
+
+    def test_ancestry_already_present(self):
+        # If a BranchRevision is being added, and it's already in the DB, but
+        # not found through the graph operations, we should schedule it for
+        # deletion anyway.
+        rev1_id = self.bzr_tree.commit('initial commit')
+        merge_tree = self.bzr_tree.bzrdir.sprout('merge').open_workingtree()
+        merge_id = merge_tree.commit('mergeable commit')
+        self.bzr_tree.merge_from_branch(merge_tree.branch)
+        rev2_id = self.bzr_tree.commit('merge')
+        self.useContext(read_locked(self.bzr_tree))
+        syncer = BzrSync(self.db_branch)
+        syncer.syncBranchAndClose(self.bzr_tree.branch)
+        self.assertEqual(rev2_id, self.db_branch.last_scanned_id)
+        self.db_branch.last_scanned_id = rev1_id
+        db_ancestry, db_history = self.db_branch.getScannerData()
+        branchrevisions_to_delete = syncer.planDatabaseChanges(
+            self.bzr_branch, [rev1_id, rev2_id], db_ancestry, db_history)[1]
+        self.assertIn(merge_id, branchrevisions_to_delete)
+
+
 class TestBzrSyncOneRevision(BzrSyncTestCase):
     """Tests for `BzrSync.syncOneRevision`."""
 
@@ -595,7 +620,7 @@
 class TestBzrTranslationsUploadJob(BzrSyncTestCase):
     """Tests BzrSync support for generating TranslationsUploadJobs."""
 
-    def _makeProductSeries(self, mode = None):
+    def _makeProductSeries(self, mode=None):
         """Switch to the Launchpad db user to create and configure a
         product series that is linked to the the branch.
         """