Merge lp:~jameinel/bzr/2.4-dirstate-invalid-delta-855155 into lp:bzr/2.4

[John A Meinel]

Fix bug #855155. Dirstate.update_basis_by_delta needs to handle adding entries to directories that were otherwise empty (in all trees).

This is proposing Brian's fix for bug #855155 backported to bzr 2.4
https://code.launchpad.net/~slyguy/bzr/bug-855155/+merge/165929

It has 2 tweaks, one is to do a direct update_basis_by_delta test in per_workingtree, which also showed that we needed to be looking up the parent block in the basis tree, not in the 'current' tree.

[Richard Wilbur (richard-wilbur)]

Thanks Brian and John for the patch with the tweaks, and for backporting it to 2.4! Now we just need to land something similar on 2.5 and trunk.
+1

review: Approve

[Robert Collins (lifeless)]

I'd just like to doule-check here: this is a valid delta, and incorrect logic in the update routine? It would be easier to be sure of that if there was a test that uses the language of the layer directly: a delta supplied by the test.

review: Needs Information

[John A Meinel (jameinel)]

It is a valid delta and an invalid update logic.
assertTransitionFromBasisToShape

Is doing this. The delta is simply add a file to a directory that was otherwise empty but must be empty and preferably non-existant in the "current" tree.
Would you be happier with a direct test at the Dirstate level?

The setup would be:

Create a Dirstate with just a tree root in current.
Set the parent tree 0 to a tree with just one directory (a/)
Update the parent tree to a new parent tree with the directory and a file in that directory (a/b)
boom.

This doesn't go boom if the current tree has any records in a (it could be a/c, but likely it would be a/b).
The first patch wouldn't go boom if (a/) exists in the current tree (which is why the setup creates nothing in the current tree)

[John A Meinel (jameinel)]

I tried to add this test, but it did not explode:

in test_dirstate.py TestUpdateBasisByDelta:

    def test_add_file_in_empty_dir_not_matching_active_state(self):
        state = self.assertUpdate(
                active=[],
                basis=[('dir/', 'dir-id')],
                target=[('dir/', 'dir-id'), ('dir/file', 'file-id')],
                )

The reason it does not explode is because the inventory entry for dir/ in basis has a last-modified revision of 'basis', while the inventory entry in target has a last-modified revision of 'target'.
So the delta produced still includes 'dir/' even though it isn't supposed to change in this test. And we have this bit of code in _update_basis_apply_adds:
            new_kind = new_details[0]
            if new_kind == 'd':
                self._ensure_block(block_index, entry_index, new_path)

So if we see the parent directory in the delta, we will call _ensure_block.

The case we are seeing here is:

1) The directory already exists and is unchanged
2) The file is just being added into that directory
3) The directory is empty, so it does not already have a dirblock existing for it

I'm pretty sure the delta of:
[(None, 'A/B', 'file-id-B',
  InventoryFile('file-id-B', 'B', parent_id='dir-id-A',
  sha1='11111111111111111111111111111111', len=24, revision=new-parent))]

Is accurate for the case of just adding a file to a directory that is otherwise unchanged.

I did push up a tweaked version of a direct Dirstate.update_basis_by_delta test, which requires tweaking the test infrastructure to allow you to specify the exact last-modified-revision for path entries. I think it is reasonably tasteful and does give you a test at the layer the bug exists. (It really is a bug in Dirstate itself, and wouldn't inherently apply to all other tree formats that might exist, it only triggers because we don't force blocks to exist for empty directories.)

[Robert Collins (lifeless)]

You have a

2480 #for path, file_id in shape:

in there.

and

2536 import pdb; pdb.set_trace()

other than that - great. Thanks for isolating it; this is complex code and I feel happier knowing we have a layer specific test!

[Robert Collins (lifeless)]

Ah, fied in 6060.

review: Approve

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/dirstate.py'
--- bzrlib/dirstate.py	2013-05-19 11:45:42 +0000
+++ bzrlib/dirstate.py	2013-05-29 06:05:30 +0000
@@ -1690,9 +1690,18 @@
             entry_key = st(dirname, basename, file_id)
             block_index, present = self._find_block_index_from_key(entry_key)
             if not present:
-                self._raise_invalid(new_path, file_id,
-                    "Unable to find block for this record."
-                    " Was the parent added?")
+                # The block where we want to put the file is not present.
+                # However, it might have just been an empty directory. Look for
+                # the parent in the basis-so-far before throwing an error.
+                parent_dir, parent_base = osutils.split(dirname)
+                parent_block_idx, parent_entry_idx, _, parent_present = \
+                    self._get_block_entry_index(parent_dir, parent_base, 1)
+                if not parent_present:
+                    self._raise_invalid(new_path, file_id,
+                        "Unable to find block for this record."
+                        " Was the parent added?")
+                self._ensure_block(parent_block_idx, parent_entry_idx, dirname)
+
             block = self._dirblocks[block_index][1]
             entry_index, present = self._find_entry_index(entry_key, block)
             if real_add:

=== modified file 'bzrlib/tests/blackbox/test_uncommit.py'
--- bzrlib/tests/blackbox/test_uncommit.py	2010-09-15 09:35:42 +0000
+++ bzrlib/tests/blackbox/test_uncommit.py	2013-05-29 06:05:30 +0000
@@ -280,3 +280,20 @@
         tree.commit(u'\u1234 message')
         out, err = self.run_bzr('uncommit --force tree', encoding='ascii')
         self.assertContainsRe(out, r'\? message')
+
+
+class TestInconsistentDelta(TestCaseWithTransport):
+    # See https://bugs.launchpad.net/bzr/+bug/855155
+    # See https://bugs.launchpad.net/bzr/+bug/1100385
+    # bzr uncommit may result in error
+    # 'An inconsistent delta was supplied involving'
+
+    def test_inconsistent_delta(self):
+        # Script taken from https://bugs.launchpad.net/bzr/+bug/855155/comments/26
+        wt = self.make_branch_and_tree('test')
+        self.build_tree(['test/a/', 'test/a/b', 'test/a/c'])
+        wt.add(['a', 'a/b', 'a/c'])
+        wt.commit('initial commit', rev_id='a1')
+        wt.remove(['a/b', 'a/c'])
+        wt.commit('remove b and c', rev_id='a2')
+        self.run_bzr("uncommit --force test")

=== modified file 'bzrlib/tests/per_workingtree/test_parents.py'
--- bzrlib/tests/per_workingtree/test_parents.py	2011-05-08 16:02:52 +0000
+++ bzrlib/tests/per_workingtree/test_parents.py	2013-05-29 06:05:30 +0000
@@ -449,7 +449,7 @@
             self.add_dir(new_shape, new_revid, 'root-id', None, '')
 
     def assertTransitionFromBasisToShape(self, basis_shape, basis_revid,
-        new_shape, new_revid, extra_parent=None):
+        new_shape, new_revid, extra_parent=None, set_current_inventory=True):
         # set the inventory revision ids.
         basis_shape.revision_id = basis_revid
         new_shape.revision_id = new_revid
@@ -464,8 +464,9 @@
                 parents.append(extra_parent)
             tree.set_parent_ids(parents)
         self.fake_up_revision(tree, new_revid, new_shape)
-        # give tree an inventory of new_shape
-        tree._write_inventory(new_shape)
+        if set_current_inventory:
+            # give tree an inventory of new_shape
+            tree._write_inventory(new_shape)
         self.assertDeltaApplicationResultsInExpectedBasis(tree, new_revid,
             delta, new_shape)
         # The tree should be internally consistent; while this is a moderately
@@ -756,3 +757,17 @@
         self.add_link(new_shape, old_revid, 'link-id-C', 'dir-id-B', 'C', 'D')
         self.assertTransitionFromBasisToShape(basis_shape, old_revid,
             new_shape, new_revid)
+
+    def test_add_files_to_empty_directory(self):
+        old_revid = 'old-parent'
+        basis_shape = Inventory(root_id=None)
+        self.add_dir(basis_shape, old_revid, 'root-id', None, '')
+        self.add_dir(basis_shape, old_revid, 'dir-id-A', 'root-id', 'A')
+        new_revid = 'new-parent'
+        new_shape = Inventory(root_id=None)
+        self.add_new_root(new_shape, old_revid, new_revid)
+        self.add_dir(new_shape, old_revid, 'dir-id-A', 'root-id', 'A')
+        self.add_file(new_shape, new_revid, 'file-id-B', 'dir-id-A', 'B',
+            '1' * 32, 24)
+        self.assertTransitionFromBasisToShape(basis_shape, old_revid,
+                new_shape, new_revid, set_current_inventory=False)

=== modified file 'bzrlib/tests/test_dirstate.py'
--- bzrlib/tests/test_dirstate.py	2011-05-25 13:44:28 +0000
+++ bzrlib/tests/test_dirstate.py	2013-05-29 06:05:30 +0000
@@ -2477,7 +2477,12 @@
     def create_tree_from_shape(self, rev_id, shape):
         dir_ids = {'': 'root-id'}
         inv = inventory.Inventory('root-id', rev_id)
-        for path, file_id in shape:
+        for info in shape:
+            if len(info) == 2:
+                path, file_id = info
+                ie_rev_id = rev_id
+            else:
+                path, file_id, ie_rev_id = info
             if path == '':
                 # Replace the root entry
                 del inv._byid[inv.root.file_id]
@@ -2485,7 +2490,7 @@
                 inv._byid[file_id] = inv.root
                 dir_ids[''] = file_id
                 continue
-            inv.add(self.path_to_ie(path, file_id, rev_id, dir_ids))
+            inv.add(self.path_to_ie(path, file_id, ie_rev_id, dir_ids))
         return revisiontree.InventoryRevisionTree(_Repo(), inv, rev_id)
 
     def create_empty_dirstate(self):
@@ -2613,6 +2618,13 @@
             target=[('file', 'file-id')],
             )
 
+    def test_add_file_in_empty_dir_not_matching_active_state(self):
+        state = self.assertUpdate(
+                active=[],
+                basis=[('dir/', 'dir-id')],
+                target=[('dir/', 'dir-id', 'basis'), ('dir/file', 'file-id')],
+                )
+
     def test_add_file_missing_in_active_state(self):
         state = self.assertUpdate(
             active=[],