Merge lp:~vila/bzr/494221-unversion-children into lp:bzr/2.0

[Vincent Ladeuil]

Ensure we don't try to remove entries twice from the in-memory inventory.

This fixes bug #494221, with a test reproducing it.

While working on it with John, we encounter several bugs that made it harder to implement a better fix.

So this fix is minimal and targeted at 2.0 but better fixes will be targeted at trunk.

The issue at hand is that the in-memory inventory needs to be updated if it has already been loaded which occurs more often on windows than on linux.

We probably fixed the original bug on linux long ago so this fix should mostly concern windows.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/494221-unversion-children into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #494221 KeyError in remove_recursive_id during commit
> https://bugs.launchpad.net/bugs/494221
>
>
> This fixes bug #494221, with a test reproducing it.
>
> While working on it with John, we encounter several bugs that made it harder to implement a better fix.
>
> So this fix is minimal and targeted at 2.0 but better fixes will be targeted at trunk.
>
> The issue at hand is that the in-memory inventory needs to be updated if it has already been loaded which occurs more often on windows than on linux.
>
> We probably fixed the original bug on linux long ago so this fix should mostly concern windows.
>

 review: needsfixing

Small tweak. The test should check that the file-ids are actually
removed from the inventory. Otherwise good to merge.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkw8P9wACgkQJdeBCYSNAAP7wACfahJWOqloMEIsQUo+CwwKWcgR
fHkAniiNRT/eUPWxmvoteiWdle0DxMpS
=wWQQ
-----END PGP SIGNATURE-----

review: Needs Fixing

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-05-28 17:59:55 +0000
+++ NEWS	2010-07-13 10:31:38 +0000
@@ -26,6 +26,9 @@
   permissions as ``.bzr`` directory on a POSIX OS.
   (Parth Malwankar, #262450)
 
+* Don't traceback trying to unversion children files of an already
+  unversioned directory.  (Vincent Ladeuil, #494221)
+
 * Raise ValueError instead of a string exception.
   (John Arbash Meinel, #586926)
 

=== modified file 'bzrlib/tests/test_workingtree_4.py'
--- bzrlib/tests/test_workingtree_4.py	2009-08-26 03:20:32 +0000
+++ bzrlib/tests/test_workingtree_4.py	2010-07-13 10:31:38 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006 Canonical Ltd
+# Copyright (C) 2007-2010 Canonical Ltd
 # Authors:  Robert Collins <[e-mail]>
 #
 # This program is free software; you can redistribute it and/or modify
@@ -761,3 +761,23 @@
             ('', [(('', 'dir', 'dir-id'), ['d', 'd'])]),
             ('dir', [(('dir', 'file', 'file-id'), ['a', 'f'])]),
         ],  self.get_simple_dirblocks(state))
+
+
+class TestInventoryCoherency(TestCaseWithTransport):
+
+    def test_inventory_is_synced_when_unversioning_a_dir(self):
+        """Unversioning the root of a subtree unversions the entire subtree."""
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b', 'c/'])
+        tree.add(['a', 'a/b', 'c'], ['a-id', 'b-id', 'c-id'])
+        # within a lock unversion should take effect
+        tree.lock_write()
+        self.addCleanup(tree.unlock)
+        # Force access to the in memory inventory to trigger bug #494221: try
+        # maintaining the in-memory inventory
+        inv = tree.inventory
+        self.assertTrue(inv.has_id('a-id'))
+        self.assertTrue(inv.has_id('b-id'))
+        tree.unversion(['a-id', 'b-id'])
+        self.assertFalse(inv.has_id('a-id'))
+        self.assertFalse(inv.has_id('b-id'))

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2009-09-30 21:38:49 +0000
+++ bzrlib/workingtree_4.py	2010-07-13 10:31:38 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009 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
@@ -1236,7 +1236,8 @@
         # have to change the legacy inventory too.
         if self._inventory is not None:
             for file_id in file_ids:
-                self._inventory.remove_recursive_id(file_id)
+                if self._inventory.has_id(file_id):
+                    self._inventory.remove_recursive_id(file_id)
 
     @needs_tree_write_lock
     def rename_one(self, from_rel, to_rel, after=False):