Rev 4727: (jam) Fix bug #507566, concurrent autopacking correctness. in http://bazaar.launchpad.net/~jameinel/bzr/2.0.4-autopack-507566

[John Arbash Meinel]

At http://bazaar.launchpad.net/~jameinel/bzr/2.0.4-autopack-507566

------------------------------------------------------------
revno: 4727
revision-id: john at arbash-meinel.com-20100116204603-556rlu1v6blzf2vk
parent: pqm at pqm.ubuntu.com-20100113174036-jmh8u7l9tie2s3zf
fixes bug(s): https://launchpad.net/bugs/507566
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: 2.0.4-autopack-507566
timestamp: Sat 2010-01-16 14:46:03 -0600
message:
  (jam) Fix bug #507566, concurrent autopacking correctness.
  
  There was a bug where if two processes started autopacking synchronously,
  when one was told to reload, it would then consider its 'new' pack to be
  old, and wouldn't add it to the disk index. This would then look like a
  commit / push succeeded, but the repository would not be referencing the
  new data, leading to NoSuchRevision errors.
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS	2010-01-13 16:31:34 +0000
+++ b/NEWS	2010-01-16 20:46:03 +0000
@@ -38,6 +38,11 @@
   returns ``EINTR`` by calling ``PyErr_CheckSignals``.  This affected the
   optional ``_readdir_pyx`` extension.  (Andrew Bennetts, #495023)
 
+* Concurrent autopacks will no longer lose a newly created pack file.
+  There was a race condition, where if the reload happened at the right
+  time, the second packer would forget the name of the newly added pack
+  file. (John Arbash Meinel, Gareth White, #507566)
+
 * Give a clearer message if the lockdir disappears after being apparently
   successfully taken.  (Martin Pool, #498378)
 

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- a/bzrlib/repofmt/pack_repo.py	2009-09-09 18:52:56 +0000
+++ b/bzrlib/repofmt/pack_repo.py	2010-01-16 20:46:03 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008 Canonical Ltd
+# Copyright (C) 2007-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1987,8 +1987,13 @@
         if first_read:
             return True
         # out the new value.
-        disk_nodes, _, _ = self._diff_pack_names()
-        self._packs_at_load = disk_nodes
+        disk_nodes, deleted_nodes, new_nodes = self._diff_pack_names()
+        # _packs_at_load is meant to be the explicit list of names in
+        # 'pack-names' at then start. As such, it should not contain any
+        # pending names that haven't been written out yet.
+        pack_names_nodes = disk_nodes.difference(new_nodes)
+        pack_names_nodes.update(deleted_nodes)
+        self._packs_at_load = pack_names_nodes
         (removed, added,
          modified) = self._syncronize_pack_names_from_disk_nodes(disk_nodes)
         if removed or added or modified:

=== modified file 'bzrlib/tests/test_repository.py'
--- a/bzrlib/tests/test_repository.py	2009-09-09 01:58:47 +0000
+++ b/bzrlib/tests/test_repository.py	2010-01-16 20:46:03 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2006-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -62,6 +62,7 @@
     revision as _mod_revision,
     symbol_versioning,
     upgrade,
+    versionedfile,
     workingtree,
     )
 from bzrlib.repofmt import (
@@ -1279,6 +1280,45 @@
         self.assertEqual({revs[-1]:(revs[-2],)}, r.get_parent_map([revs[-1]]))
         self.assertFalse(packs.reload_pack_names())
 
+    def test_reload_pack_names_preserves_pending(self):
+        # TODO: Update this to also test for pending-deleted names
+        tree, r, packs, revs = self.make_packs_and_alt_repo(write_lock=True)
+        # We will add one pack (via start_write_group + insert_record_stream),
+        # and remove another pack (via _remove_pack_from_memory)
+        orig_names = packs.names()
+        orig_at_load = packs._packs_at_load
+        to_remove_name = iter(orig_names).next()
+        r.start_write_group()
+        self.addCleanup(r.abort_write_group)
+        r.texts.insert_record_stream([versionedfile.FulltextContentFactory(
+            ('text', 'rev'), (), None, 'content\n')])
+        new_pack = packs._new_pack
+        self.assertTrue(new_pack.data_inserted())
+        new_pack.finish()
+        packs.allocate(new_pack)
+        packs._new_pack = None
+        removed_pack = packs.get_pack_by_name(to_remove_name)
+        packs._remove_pack_from_memory(removed_pack)
+        names = packs.names()
+        all_nodes, deleted_nodes, new_nodes = packs._diff_pack_names()
+        new_names = set([x[0][0] for x in new_nodes])
+        self.assertEqual(names, sorted([x[0][0] for x in all_nodes]))
+        self.assertEqual(set(names) - set(orig_names), new_names)
+        self.assertEqual(set([new_pack.name]), new_names)
+        self.assertEqual([to_remove_name],
+                         sorted([x[0][0] for x in deleted_nodes]))
+        packs.reload_pack_names()
+        reloaded_names = packs.names()
+        self.assertEqual(orig_at_load, packs._packs_at_load)
+        self.assertEqual(names, reloaded_names)
+        all_nodes, deleted_nodes, new_nodes = packs._diff_pack_names()
+        new_names = set([x[0][0] for x in new_nodes])
+        self.assertEqual(names, sorted([x[0][0] for x in all_nodes]))
+        self.assertEqual(set(names) - set(orig_names), new_names)
+        self.assertEqual(set([new_pack.name]), new_names)
+        self.assertEqual([to_remove_name],
+                         sorted([x[0][0] for x in deleted_nodes]))
+
     def test_autopack_reloads_and_stops(self):
         tree, r, packs, revs = self.make_packs_and_alt_repo(write_lock=True)
         # After we have determined what needs to be autopacked, trigger a