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

Proposed by John A Meinel on 2013-05-28
Status: Merged
Approved by: John A Meinel on 2013-05-29
Approved revision: 6058
Merge reported by: John A Meinel
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/2.4-dirstate-invalid-delta-855155
Merge into: lp:bzr/2.4
Diff against target: 134 lines (+61/-8)
4 files modified
bzrlib/ (+12/-3)
bzrlib/tests/blackbox/ (+17/-0)
bzrlib/tests/per_workingtree/ (+18/-3)
bzrlib/tests/ (+14/-2)
To merge this branch: bzr merge lp:~jameinel/bzr/2.4-dirstate-invalid-delta-855155
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve on 2013-05-29
Richard Wilbur 2013-05-28 Approve on 2013-05-28
Review via email:

Commit message

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

Description of the change

This is proposing Brian's fix for bug #855155 backported to bzr 2.4

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.

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

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.

review: Approve
Robert Collins (lifeless) wrote :

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

It is a valid delta and an invalid update logic.

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)

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)

6059. By John A Meinel on 2013-05-29

Add a direct Dirstate.update_basis_by_delta test.

Requires a slight tweak to the create_tree_from_shape code so that we
can set the exact revision_id for an entry (so a dir doesn't look changed
when it shouldn't).

John A Meinel (jameinel) wrote :

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

in TestUpdateBasisByDelta:

    def test_add_file_in_empty_dir_not_matching_active_state(self):
        state = self.assertUpdate(
                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.)

6060. By John A Meinel on 2013-05-29

Clean things up a bit.

Robert Collins (lifeless) wrote :

You have a

2480 #for path, file_id in shape:

in there.


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

Ah, fied in 6060.

review: Approve
John A Meinel (jameinel) wrote :

sent to pqm by email

6061. By John A Meinel on 2013-05-29

Remember to update release notes when submitting these things.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/'
2--- bzrlib/ 2013-05-19 11:45:42 +0000
3+++ bzrlib/ 2013-05-29 06:05:30 +0000
4@@ -1690,9 +1690,18 @@
5 entry_key = st(dirname, basename, file_id)
6 block_index, present = self._find_block_index_from_key(entry_key)
7 if not present:
8- self._raise_invalid(new_path, file_id,
9- "Unable to find block for this record."
10- " Was the parent added?")
11+ # The block where we want to put the file is not present.
12+ # However, it might have just been an empty directory. Look for
13+ # the parent in the basis-so-far before throwing an error.
14+ parent_dir, parent_base = osutils.split(dirname)
15+ parent_block_idx, parent_entry_idx, _, parent_present = \
16+ self._get_block_entry_index(parent_dir, parent_base, 1)
17+ if not parent_present:
18+ self._raise_invalid(new_path, file_id,
19+ "Unable to find block for this record."
20+ " Was the parent added?")
21+ self._ensure_block(parent_block_idx, parent_entry_idx, dirname)
23 block = self._dirblocks[block_index][1]
24 entry_index, present = self._find_entry_index(entry_key, block)
25 if real_add:
27=== modified file 'bzrlib/tests/blackbox/'
28--- bzrlib/tests/blackbox/ 2010-09-15 09:35:42 +0000
29+++ bzrlib/tests/blackbox/ 2013-05-29 06:05:30 +0000
30@@ -280,3 +280,20 @@
31 tree.commit(u'\u1234 message')
32 out, err = self.run_bzr('uncommit --force tree', encoding='ascii')
33 self.assertContainsRe(out, r'\? message')
36+class TestInconsistentDelta(TestCaseWithTransport):
37+ # See
38+ # See
39+ # bzr uncommit may result in error
40+ # 'An inconsistent delta was supplied involving'
42+ def test_inconsistent_delta(self):
43+ # Script taken from
44+ wt = self.make_branch_and_tree('test')
45+ self.build_tree(['test/a/', 'test/a/b', 'test/a/c'])
46+ wt.add(['a', 'a/b', 'a/c'])
47+ wt.commit('initial commit', rev_id='a1')
48+ wt.remove(['a/b', 'a/c'])
49+ wt.commit('remove b and c', rev_id='a2')
50+ self.run_bzr("uncommit --force test")
52=== modified file 'bzrlib/tests/per_workingtree/'
53--- bzrlib/tests/per_workingtree/ 2011-05-08 16:02:52 +0000
54+++ bzrlib/tests/per_workingtree/ 2013-05-29 06:05:30 +0000
55@@ -449,7 +449,7 @@
56 self.add_dir(new_shape, new_revid, 'root-id', None, '')
58 def assertTransitionFromBasisToShape(self, basis_shape, basis_revid,
59- new_shape, new_revid, extra_parent=None):
60+ new_shape, new_revid, extra_parent=None, set_current_inventory=True):
61 # set the inventory revision ids.
62 basis_shape.revision_id = basis_revid
63 new_shape.revision_id = new_revid
64@@ -464,8 +464,9 @@
65 parents.append(extra_parent)
66 tree.set_parent_ids(parents)
67 self.fake_up_revision(tree, new_revid, new_shape)
68- # give tree an inventory of new_shape
69- tree._write_inventory(new_shape)
70+ if set_current_inventory:
71+ # give tree an inventory of new_shape
72+ tree._write_inventory(new_shape)
73 self.assertDeltaApplicationResultsInExpectedBasis(tree, new_revid,
74 delta, new_shape)
75 # The tree should be internally consistent; while this is a moderately
76@@ -756,3 +757,17 @@
77 self.add_link(new_shape, old_revid, 'link-id-C', 'dir-id-B', 'C', 'D')
78 self.assertTransitionFromBasisToShape(basis_shape, old_revid,
79 new_shape, new_revid)
81+ def test_add_files_to_empty_directory(self):
82+ old_revid = 'old-parent'
83+ basis_shape = Inventory(root_id=None)
84+ self.add_dir(basis_shape, old_revid, 'root-id', None, '')
85+ self.add_dir(basis_shape, old_revid, 'dir-id-A', 'root-id', 'A')
86+ new_revid = 'new-parent'
87+ new_shape = Inventory(root_id=None)
88+ self.add_new_root(new_shape, old_revid, new_revid)
89+ self.add_dir(new_shape, old_revid, 'dir-id-A', 'root-id', 'A')
90+ self.add_file(new_shape, new_revid, 'file-id-B', 'dir-id-A', 'B',
91+ '1' * 32, 24)
92+ self.assertTransitionFromBasisToShape(basis_shape, old_revid,
93+ new_shape, new_revid, set_current_inventory=False)
95=== modified file 'bzrlib/tests/'
96--- bzrlib/tests/ 2011-05-25 13:44:28 +0000
97+++ bzrlib/tests/ 2013-05-29 06:05:30 +0000
98@@ -2477,7 +2477,12 @@
99 def create_tree_from_shape(self, rev_id, shape):
100 dir_ids = {'': 'root-id'}
101 inv = inventory.Inventory('root-id', rev_id)
102- for path, file_id in shape:
103+ for info in shape:
104+ if len(info) == 2:
105+ path, file_id = info
106+ ie_rev_id = rev_id
107+ else:
108+ path, file_id, ie_rev_id = info
109 if path == '':
110 # Replace the root entry
111 del inv._byid[inv.root.file_id]
112@@ -2485,7 +2490,7 @@
113 inv._byid[file_id] = inv.root
114 dir_ids[''] = file_id
115 continue
116- inv.add(self.path_to_ie(path, file_id, rev_id, dir_ids))
117+ inv.add(self.path_to_ie(path, file_id, ie_rev_id, dir_ids))
118 return revisiontree.InventoryRevisionTree(_Repo(), inv, rev_id)
120 def create_empty_dirstate(self):
121@@ -2613,6 +2618,13 @@
122 target=[('file', 'file-id')],
123 )
125+ def test_add_file_in_empty_dir_not_matching_active_state(self):
126+ state = self.assertUpdate(
127+ active=[],
128+ basis=[('dir/', 'dir-id')],
129+ target=[('dir/', 'dir-id', 'basis'), ('dir/file', 'file-id')],
130+ )
132 def test_add_file_missing_in_active_state(self):
133 state = self.assertUpdate(
134 active=[],


People subscribed via source and target branches