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

Proposed by Brian de Alwis on 2013-05-27
Status: Rejected
Rejected by: Richard Wilbur on 2015-05-06
Proposed branch: lp:~slyguy/bzr/bug-855155
Merge into: lp:bzr
Diff against target: 50 lines (+29/-3)
2 files modified
bzrlib/dirstate.py (+12/-3)
bzrlib/tests/blackbox/test_uncommit.py (+17/-0)
To merge this branch: bzr merge lp:~slyguy/bzr/bug-855155
Reviewer Review Type Date Requested Status
Richard Wilbur superceded 2013-05-27 Disapprove on 2015-05-06
Review via email: mp+165929@code.launchpad.net

Description of the change

Bug fix with test for bug 855155.

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote :

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) wrote :

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) wrote :

(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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1648 entry_key = st(dirname, basename, file_id)1648 entry_key = st(dirname, basename, file_id)
1649 block_index, present = self._find_block_index_from_key(entry_key)1649 block_index, present = self._find_block_index_from_key(entry_key)
1650 if not present:1650 if not present:
1651 self._raise_invalid(new_path, file_id,1651 # The block where we want to put the file is not present. But it
1652 "Unable to find block for this record."1652 # might be because the directory was empty, or not loaded yet. Look
1653 " Was the parent added?")1653 # for a parent entry, if not found, raise invalid error
1654 parent_dir, parent_base = osutils.split(dirname)
1655 parent_block_idx, parent_entry_idx, _, parent_present = \
1656 self._get_block_entry_index(parent_dir, parent_base, 0)
1657 if not parent_present:
1658 self._raise_invalid(new_path, file_id,
1659 "Unable to find block for this record."
1660 " Was the parent added?")
1661 self._ensure_block(parent_block_idx, parent_entry_idx, dirname)
1662
1654 block = self._dirblocks[block_index][1]1663 block = self._dirblocks[block_index][1]
1655 entry_index, present = self._find_entry_index(entry_key, block)1664 entry_index, present = self._find_entry_index(entry_key, block)
1656 if real_add:1665 if real_add:
16571666
=== 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 @@
314 self.assertLength(14, self.hpss_calls)314 self.assertLength(14, self.hpss_calls)
315 self.assertLength(1, self.hpss_connections)315 self.assertLength(1, self.hpss_connections)
316 self.assertThat(self.hpss_calls, ContainsNoVfsCalls)316 self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
317
318
319class TestInconsistentDelta(TestCaseWithTransport):
320 # See https://bugs.launchpad.net/bzr/+bug/855155
321 # See https://bugs.launchpad.net/bzr/+bug/1100385
322 # bzr uncommit may result in error
323 # 'An inconsistent delta was supplied involving'
324
325 def test_inconsistent_delta(self):
326 # Script taken from https://bugs.launchpad.net/bzr/+bug/855155/comments/26
327 wt = self.make_branch_and_tree('test')
328 self.build_tree(['test/a/', 'test/a/b', 'test/a/c'])
329 wt.add(['a', 'a/b', 'a/c'])
330 wt.commit('initial commit', rev_id='a1')
331 wt.remove(['a/b', 'a/c'])
332 wt.commit('remove b and c', rev_id='a2')
333 self.run_bzr("uncommit --force test")