Code review comment for lp:~slyguy/bzr/bug-855155

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.

« Back to merge proposal