Merge lp:~slyguy/bzr/bug-855155 into lp:bzr

[Brian de Alwis]

Bug fix with test for bug 855155.

[Richard Wilbur (richard-wilbur)]

Nice to get a fix for one of the corner cases that turned out to plague a number of people. Thank you for fixing the design oversight that brought us bug 855155. That does look like an interesting one to write a test for!
+1

review: Approve

[John A Meinel (jameinel)]

I think the test is very good. I'm concerned that this is fixing where the exception was, when there might be a different logic error (why aren't we creating the entry for 'a' before this happens, even if it is an empty directory.)

I'll look into that side of the logic, if I can't find something better we should certainly land this.

[John A Meinel (jameinel)]

(Specifically, there is a regression from at least bzr 2.1 that manifests itself in 2.4+. bzr-2.1 does *not* fail this test. I'm looking to bisect to see if I can narrow down the change.)

[John A Meinel (jameinel)]

2.3 passes, 2.4 fails. Most likely because of this change:
* Speed up ``bzr uncommit``. Instead of resetting the dirstate from
  scratch, use ``update_basis_by_delta``, computing the delta from the
  repository. (John Arbash Meinel, #780544)

So that would indicate uncommit wasn't using update_basis_by_delta prior to 2.4. So it is possible this was just a latent bug in update_basis_by_delta. In 2.3 the only caller to update_basis_by_delta was commit.

In 2.4, it is anything calling 'set_parent_trees', which is called by things like update and merge. (Which made those things a lot faster on large trees because you don't have to think about the state of all the files which haven't changed.)

Certainly it could still just be a flaw in update_basis_by_delta that we didn't trigger in testing. Since it does happen in 2.4, it might be worth backporting this fix to 2.4 as well.

[John A Meinel (jameinel)]

I think I sorted out how this slipped through. If you look at "bzrlib/tests/per_workingtree.py" there are a bunch of tests in UpdateToOneParentViaDeltaTests.

I can add this test, which should trigger the bug:
    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.add_file(new_shape, new_revid, 'file-id-C', 'dir-id-A', 'C',
            '2' * 32, 24)
        self.assertTransitionFromBasisToShape(basis_shape, old_revid,
                new_shape, new_revid)

However, the test passes. The key is line 469:
 tree._write_inventory(new_shape)

update_basis_by_delta was originally written for the 'commit' case. So the working tree has been updated to the new state, and you are writing that into the repository, and then updating the cached basis tree to match (with selective commit, etc being possible).

With commit, you won't be missing the dirblock, because either the current tree has the record (you are adding the files), or the parent tree has the files (you are removing them).

uncommit is different, because it doesn't actually touch the 'current' tree (intentionally). So you are making the basis (parent) tree look like an older version, but not touching the current tree.

However, adding that test actually exposes a bug in your patch. Specifically, we are updating the *basis* tree, not the current tree, so we need to be looking up the parent block in tree '1' not in tree '0'. I'm working out a fix for that now.

[John A Meinel (jameinel)]

I backported the patch and proposed it with the extra test and tweak and proposed it here:
https://code.launchpad.net/~jameinel/bzr/2.4-dirstate-invalid-delta-855155/+merge/165984

[John A Meinel (jameinel)]

This tweak to your test case also shows why you need to lookup the parent in tree '1' rather than tree '0':
=== modified file 'bzrlib/tests/blackbox/test_uncommit.py'
--- bzrlib/tests/blackbox/test_uncommit.py 2013-05-27 19:08:27 +0000
+++ bzrlib/tests/blackbox/test_uncommit.py 2013-05-28 08:33:31 +0000
@@ -330,4 +330,5 @@
         wt.commit('initial commit', rev_id='a1')
         wt.remove(['a/b', 'a/c'])
         wt.commit('remove b and c', rev_id='a2')
+ wt.remove(['a'])
         self.run_bzr("uncommit --force test")

Specifically, we shouldn't care at all what is in the current working tree when we are updating the *basis* tree.
I don't know if we want to add that to the test or not, as it should be covered by the direct test.

[Richard Wilbur (richard-wilbur)]

Thanks Brian for the good work. I'm just going through active reviews and trying to clean things up. It seems that launchpad didn't notice your changes got merged in with John Meinel's tweaks in https://code.launchpad.net/~jameinel/bzr/2.4-dirstate-invalid-delta-855155/+merge/165984
so it didn't show this as being merged. I'm marking the merge request "Disapprove: Superceded" in order to de-activate the merge request as the changes have been incorporated and appear in trunk but the merge request is still shown as active.

Thanks again for your contributions, looking forward to the next time you have a bzr itch to scratch.

review: Disapprove (superceded)

Unmerged revisions

6580. By Brian de Alwis on 2013-05-27

Add test for bug 855155 (comment 26)

6579. By Brian de Alwis on 2013-01-16

Fix DirState._update_basis_apply_adds() to handle adds to empty directories

=== modified file 'bzrlib/dirstate.py'
--- bzrlib/dirstate.py	2013-05-23 10:04:17 +0000
+++ bzrlib/dirstate.py	2013-05-27 19:40:33 +0000
@@ -1648,9 +1648,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. But it
+                # might be because the directory was empty, or not loaded yet. Look
+                # for a parent entry, if not found, raise invalid 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, 0)
+                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	2011-12-14 20:21:52 +0000
+++ bzrlib/tests/blackbox/test_uncommit.py	2013-05-27 19:40:33 +0000
@@ -314,3 +314,20 @@
         self.assertLength(14, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
+
+
+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")