Merge lp:~jameinel/bzr/2.4-update-basis-by-delta-781168 into lp:bzr

[John A Meinel]

Fix bug #781168, and allow WT.update_basis_by_delta to not require the delta makes the basis match the current state.

This changes the WT.update_basis_by_delta code so that it no longer requires that the delta causes the basis tree to match the current state. That was relevant for commit, but it isn't relevant for uncommit, etc.

Thanks to Jonathan Riddell for pairing on this with me.

While working on this, we also discovered that there were edge cases that were *not* handled by the update_basis_by_delta code, it looked like they were accidentally triggered in update_by_delta code because of how the test state was set up. I have more confidence now that things are correct, though there could still be edge cases that we are missing.

[Martin Pool (mbp)]

> raise AssertionError('entry was missing, but now was found')

should that be an InconsistentDelta instead?

Could you log perhaps the entry key or other information so we have a bit of a clue if somebody cites this in a bug report?

I don't understand why the code is being deleted from test_inv.py.

[Martin Pool (mbp)]

review: Needs Information

[John A Meinel (jameinel)]

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

On 05/17/2011 02:41 PM, Martin Pool wrote:
>> raise AssertionError('entry was missing, but now was found')
>
> should that be an InconsistentDelta instead?
>
> Could you log perhaps the entry key or other information so we have a bit of a clue if somebody cites this in a bug report?
>
> I don't understand why the code is being deleted from test_inv.py.

That specific one is AssertionError. Specifically, we just checked that
the entry wasn't present with:
entry[0] is None

so if it now shows up as present, something is really wrong with our
data, and it has nothing to do with the delta that was supplied.
I can change it to any other error if you want, but it is "something is
corrupted". I can even take it out. I just was thinking to check
"present" but realized I shouldn't have to.

The code in test_inv.py is part of the "set up trees so that I can call
update_basis_by_delta them". It existed because the old version required
that the WT state matched the new basis state. So it went around
modifying active state to match the delta target. Since we no longer
want that synchronization to apply, I removed it.

This also revealed a bunch of tests that were accidentally passing.
Specifically, I'm guessing that "mutate state to match" was triggering
extra checks that weren't being done in update_basis_by_delta.

For example "test_unicode_file_ids". That tests that you can't add an
Entry that uses a Unicode file_id (rather than a utf-8 str id).
In bzr.dev today, the exception is being raised in "update_by_delta"
*not* "update_basis_by_delta".

The former is called when you mutate the working inventory state, the
latter when you are mutating the basis inventory state.
So the test was accidentally passing in the 'setup' phase of the code,
rather in the code-under-test part of the code.

Does that help you understand?
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3ScLYACgkQJdeBCYSNAAO29ACfZHDn06e9A/YTrqDUA0mz72Za
6jcAoKurliZDQ9FyO62cq4ei8lzCjWxf
=QL9d
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

thanks, that's great

review: Approve

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/dirstate.py'
--- bzrlib/dirstate.py	2011-05-11 01:58:47 +0000
+++ bzrlib/dirstate.py	2011-05-17 13:32:21 +0000
@@ -1326,6 +1326,14 @@
             raise
         return result
 
+    def _check_delta_is_valid(self, delta):
+        return list(inventory._check_delta_unique_ids(
+                    inventory._check_delta_unique_old_paths(
+                    inventory._check_delta_unique_new_paths(
+                    inventory._check_delta_ids_match_entry(
+                    inventory._check_delta_ids_are_valid(
+                    inventory._check_delta_new_path_entry_both_or_None(delta)))))))
+
     def update_by_delta(self, delta):
         """Apply an inventory delta to the dirstate for tree 0
 
@@ -1349,13 +1357,8 @@
         new_ids = set()
         # This loop transforms the delta to single atomic operations that can
         # be executed and validated.
-        for old_path, new_path, file_id, inv_entry in sorted(
-            inventory._check_delta_unique_old_paths(
-            inventory._check_delta_unique_new_paths(
-            inventory._check_delta_ids_match_entry(
-            inventory._check_delta_ids_are_valid(
-            inventory._check_delta_new_path_entry_both_or_None(delta))))),
-            reverse=True):
+        delta = sorted(self._check_delta_is_valid(delta), reverse=True)
+        for old_path, new_path, file_id, inv_entry in delta:
             if (file_id in insertions) or (file_id in removals):
                 raise errors.InconsistentDelta(old_path or new_path, file_id,
                     "repeated file_id")
@@ -1475,9 +1478,6 @@
         Note that an exception during the operation of this method will leave
         the dirstate in a corrupt state where it should not be saved.
 
-        Finally, we expect all changes to be synchronising the basis tree with
-        the working tree.
-
         :param new_revid: The new revision id for the trees parent.
         :param delta: An inventory delta (see apply_inventory_delta) describing
             the changes from the current left most parent revision to new_revid.
@@ -1495,7 +1495,7 @@
 
         self._parents[0] = new_revid
 
-        delta = sorted(delta, reverse=True)
+        delta = sorted(self._check_delta_is_valid(delta), reverse=True)
         adds = []
         changes = []
         deletes = []
@@ -1653,7 +1653,17 @@
         for old_path, new_path, file_id, new_details, real_add in adds:
             # the entry for this file_id must be in tree 0.
             entry = self._get_entry(0, file_id, new_path)
-            if entry[0] is None or entry[0][2] != file_id:
+            if entry[0] is None:
+                # new_path is not versioned in the active WT state,
+                # but we are adding it to the basis tree state, we
+                # need to create a new entry record for it.
+                dirname, basename = osutils.split(new_path)
+                entry_key = (dirname, basename, file_id)
+                _, block = self._find_block(entry_key, add_if_missing=True)
+                index, _ = self._find_entry_index(entry_key, block)
+                entry = (entry_key, [DirState.NULL_PARENT_DETAILS]*2)
+                block.insert(index, entry)
+            elif entry[0][2] != file_id:
                 self._changes_aborted = True
                 raise errors.InconsistentDelta(new_path, file_id,
                     'working tree does not contain new entry')
@@ -1719,12 +1729,27 @@
                 raise errors.InconsistentDelta(old_path, file_id,
                     'mismatched file_id in tree 1')
             if real_delete:
-                if entry[1][0][0] != 'a':
-                    self._changes_aborted = True
-                    raise errors.InconsistentDelta(old_path, file_id,
-                            'This was marked as a real delete, but the WT state'
-                            ' claims that it still exists and is versioned.')
-                del self._dirblocks[block_index][1][entry_index]
+                if entry[1][0][0] == 'a':
+                    # The file was marked as deleted in the active
+                    # state, and it is now deleted in the basis state,
+                    # so just remove the record entirely
+                    del self._dirblocks[block_index][1][entry_index]
+                else:
+                    # The basis entry needs to be marked deleted
+                    entry[1][1] = null
+                # If we are deleting a directory, we need to make sure
+                # that all of its children are already deleted
+                block_i, entry_i, d_present, f_present = \
+                    self._get_block_entry_index(old_path, '', 0)
+                if d_present:
+                    # The dir block is still present in the dirstate; this could
+                    # be due to it being in a parent tree, or a corrupt delta.
+                    for child_entry in self._dirblocks[block_i][1]:
+                        if child_entry[1][1][0] not in ('r', 'a'):
+                            self._changes_aborted = True
+                            raise errors.InconsistentDelta(old_path, entry[0][2],
+                                "The file id was deleted but its children were "
+                                "not deleted.")
             else:
                 if entry[1][0][0] == 'a':
                     self._changes_aborted = True
@@ -1740,6 +1765,7 @@
                     del self._dirblocks[block_index][1][entry_index]
                 else:
                     # it is being resurrected here, so blank it out temporarily.
+                    # should be equivalent to entry[1][1] = null
                     self._dirblocks[block_index][1][entry_index][1][1] = null
 
     def _after_delta_check_parents(self, parents, index):

=== modified file 'bzrlib/tests/test_inv.py'
--- bzrlib/tests/test_inv.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_inv.py	2011-05-17 13:32:21 +0000
@@ -176,32 +176,6 @@
         # This reads basis from the repo and puts it into the tree's local
         # cache, if it has one.
         tree.set_parent_ids(['basis'])
-        paths = {}
-        parents = set()
-        for old, new, id, entry in delta:
-            if None in (new, entry):
-                continue
-            paths[new] = (entry.file_id, entry.kind)
-            parents.add(osutils.dirname(new))
-        parents = osutils.minimum_path_selection(parents)
-        parents.discard('')
-        # Put place holders in the tree to permit adding the other entries.
-        for pos, parent in enumerate(parents):
-            if not tree.path2id(parent):
-                # add a synthetic directory in the tree so we can can put the
-                # tree0 entries in place for dirstate.
-                tree.add([parent], ["id%d" % pos], ["directory"])
-        if paths:
-            # Many deltas may cause this mini-apply to fail, but we want to see what
-            # the delta application code says, not the prep that we do to deal with 
-            # limitations of dirstate's update_basis code.
-            for path, (file_id, kind) in sorted(paths.items()):
-                try:
-                    tree.add([path], [file_id], [kind])
-                except (KeyboardInterrupt, SystemExit):
-                    raise
-                except:
-                    pass
     finally:
         tree.unlock()
     # Fresh lock, reads disk again.
@@ -586,8 +560,41 @@
         self.assertRaises(errors.InconsistentDelta, self.apply_delta, self,
             inv, delta)
 
-
-class TestInventory(TestCase):
+    def test_add_file(self):
+        inv = self.get_empty_inventory()
+        file1 = inventory.InventoryFile('file-id', 'path', inv.root.file_id)
+        file1.revision = 'result'
+        file1.text_size = 0
+        file1.text_sha1 = ''
+        delta = [(None, u'path', 'file-id', file1)]
+        res_inv = self.apply_delta(self, inv, delta)
+        self.assertEqual('file-id', res_inv['file-id'].file_id)
+
+    def test_remove_file(self):
+        inv = self.get_empty_inventory()
+        file1 = inventory.InventoryFile('file-id', 'path', inv.root.file_id)
+        file1.revision = 'result'
+        file1.text_size = 0
+        file1.text_sha1 = ''
+        inv.add(file1)
+        delta = [(u'path', None, 'file-id', None)]
+        res_inv = self.apply_delta(self, inv, delta)
+        self.assertEqual(None, res_inv.path2id('path'))
+        self.assertRaises(errors.NoSuchId, res_inv.id2path, 'file-id')
+
+    def test_rename_file(self):
+        inv = self.get_empty_inventory()
+        file1 = inventory.InventoryFile('file-id', 'path', inv.root.file_id)
+        file1.revision = 'result'
+        file1.text_size = 0
+        file1.text_sha1 = ''
+        inv.add(file1)
+        file2 = file1.copy()
+        file2.name = 'path2'
+        delta = [(u'path', 'path2', 'file-id', file2)]
+        res_inv = self.apply_delta(self, inv, delta)
+        self.assertEqual(None, res_inv.path2id('path'))
+        self.assertEqual('file-id', res_inv.path2id('path2'))
 
     def test_is_root(self):
         """Ensure our root-checking code is accurate."""

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-17 10:30:37 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-17 13:32:21 +0000
@@ -83,6 +83,12 @@
   reporting subdirectories that were tree references (in formats that
   supported them). (John Arbash Meinel, #764677)
 
+* ``WT.update_basis_by_delta`` no longer requires that the deltas match
+  the current WT state. This allows ``update_basis_by_delta`` to be used
+  by more commands than just commit. Updating with a delta allows us to
+  not load the whole inventory, which can take 10+s with large trees.
+  (Jonathan Riddell, John Arbash Meinel, #781168)
+
 
 Documentation
 *************