Bazaar

Merge lp:~lifeless/bzr/bug-395556 into lp:~bzr/bzr/trunk-old

  1. bug-395556
  2. Merge into trunk-old
Proposed by Robert Collins
Status: Superseded
Proposed branch: lp:~lifeless/bzr/bug-395556
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 163 lines
To merge this branch: bzr merge lp:~lifeless/bzr/bug-395556
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+8791@code.launchpad.net

This proposal has been superseded by a proposal from 2009-07-16.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Fix dirstate.set_state_from_inventory to have a more stable
old_iterator, fixing rename problems in new trees. (Robert Collins, bug
395556)

I've removed some tests that helped to diagnose the problem but had
nothing to do with where or how it was occuring; I don't think they add
value to the test environment - the particular way they tickled the bug,
though interesting, is not demonstrating an interface problem, and is a
particularly expensive way of showing that lower levels aren't broken.

-Rob

Revision history for this message
John A Meinel (jameinel) wrote :

This certainly seems to be the simple and straightforward fix. I believe a "better" fix (faster, but more complex) would be to copy the objects when we want to mutate them, rather than trying to mutate-in-place.

Considering we are trying to get away from mutate-in-place anyway (CHKInventory doesn't support apply_delta anymore, etc.)

Anyway, we might want a comment about performance if we see a regression from this. ISTR that Inventory.copy() time can easily became a dominant factor in a lot of operations. But we can certainly start here.

review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

> Anyway, we might want a comment about performance if we see a regression from
> this. ISTR that Inventory.copy() time can easily became a dominant factor in a
> lot of operations. But we can certainly start here.

Yah, copy() is bad ;). That said, we're copying tuples lists and strings not complex objects, so there is some reason to believe it will be lighter weight.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-07-15 19:06:39 +0000
3+++ NEWS 2009-07-15 21:35:39 +0000
4@@ -25,6 +25,11 @@
5 * BranchBuilder now accepts timezone to avoid test failures in countries far
6 from GMT. (Vincent Ladeuil, #397716)
7
8+* Renames to lexographically lower basenames in trees that have never been
9+ committed to will no longer corrupt the dirstate. This was caused by an
10+ aliasing bug in the dirstate set_state_from_inventory method.
11+ (Robert Collins, #395556)
12+
13 Improvements
14 ************
15
16
17=== modified file 'bzrlib/dirstate.py'
18--- bzrlib/dirstate.py 2009-07-10 05:33:07 +0000
19+++ bzrlib/dirstate.py 2009-07-15 21:35:39 +0000
20@@ -203,6 +203,7 @@
21
22 import bisect
23 import binascii
24+from copy import deepcopy
25 import errno
26 import os
27 from stat import S_IEXEC
28@@ -2436,8 +2437,11 @@
29 new_iterator = new_inv.iter_entries_by_dir()
30 # we will be modifying the dirstate, so we need a stable iterator. In
31 # future we might write one, for now we just clone the state into a
32- # list - which is a shallow copy.
33- old_iterator = iter(list(self._iter_entries()))
34+ # list using a deep copy so that forward changes don't make the logic
35+ # more complex. Using a shallow copy results in all entries being seen
36+ # but the state of the entries being wrong, and that leads to stale
37+ # entries being left behind.
38+ old_iterator = iter(deepcopy(list(self._iter_entries())))
39 # both must have roots so this is safe:
40 current_new = new_iterator.next()
41 current_old = old_iterator.next()
42
43=== modified file 'bzrlib/tests/blackbox/test_commit.py'
44--- bzrlib/tests/blackbox/test_commit.py 2009-07-08 05:27:06 +0000
45+++ bzrlib/tests/blackbox/test_commit.py 2009-07-15 21:35:39 +0000
46@@ -639,25 +639,3 @@
47 out, err = self.run_bzr("commit tree/hello.txt")
48 last_rev = tree.branch.repository.get_revision(tree.last_revision())
49 self.assertEqual('save me some typing\n', last_rev.message)
50-
51- def test_commit_and_mv_dance_a(self):
52- # see https://bugs.launchpad.net/bzr/+bug/395556
53- tree = self.make_branch_and_tree(".")
54- self.build_tree(["a"])
55- tree.add("a")
56- self.check_output("a => b\n", ["mv", "a", "b"])
57- self.check_output("", ["commit", "-q", "-m", "Actually no, b"])
58- self.check_output("b => a\n", ["mv", "b", "a"])
59- self.check_output("", ["commit", "-q", "-m", "No, really, a"])
60-
61- def test_commit_and_mv_dance_b(self):
62- # see https://bugs.launchpad.net/bzr/+bug/395556
63- tree = self.make_branch_and_tree(".")
64- self.build_tree(["b"])
65- tree.add("b")
66- self.check_output("b => a\n", ["mv", "b", "a"])
67- self.check_output("", ["commit", "-q", "-m", "Actually no, a"])
68- self.check_output("a => b\n", ["mv", "a", "b"])
69- self.expectFailure("bug 395556: gives DuplicateFileId "
70- "committing renames",
71- self.check_output, "", ["commit", "-q", "-m", "No, really, b"])
72
73=== modified file 'bzrlib/tests/per_workingtree/test_commit.py'
74--- bzrlib/tests/per_workingtree/test_commit.py 2009-07-10 07:14:02 +0000
75+++ bzrlib/tests/per_workingtree/test_commit.py 2009-07-15 21:35:39 +0000
76@@ -602,29 +602,3 @@
77 revid = tree.commit('first post')
78 committed_tree = tree.basis_tree()
79 self.assertTrue(committed_tree.has_filename("newfile"))
80-
81- def test_commit_and_mv_dance_a(self):
82- # should fail because of
83- # <https://bugs.launchpad.net/bzr/+bug/395556> but apparently does
84- # not, while the blackbox.test_commit equivalent does - maybe because
85- # of different format combinations
86- tree = self.make_branch_and_tree(".")
87- self.build_tree(["a"])
88- tree.add("a")
89- tree.rename_one("a", "b")
90- tree.commit("Actually no, b")
91- tree.rename_one("b", "a")
92- tree.commit("No, really, a")
93-
94- def test_commit_and_mv_dance_b(self):
95- # should fail because of
96- # <https://bugs.launchpad.net/bzr/+bug/395556> but apparently does
97- # not, while the blackbox.test_commit equivalent does - maybe because
98- # of different format combinations
99- tree = self.make_branch_and_tree(".")
100- self.build_tree(["b"])
101- tree.add("b")
102- tree.rename_one("b", "a")
103- tree.commit("Actually no, a")
104- tree.rename_one("a", "b")
105- tree.commit("No, really, b")
106
107=== modified file 'bzrlib/tests/test_dirstate.py'
108--- bzrlib/tests/test_dirstate.py 2009-05-06 05:36:28 +0000
109+++ bzrlib/tests/test_dirstate.py 2009-07-15 21:35:39 +0000
110@@ -827,7 +827,6 @@
111 finally:
112 tree.unlock()
113
114-
115 def test_set_state_from_inventory_mixed_paths(self):
116 tree1 = self.make_branch_and_tree('tree1')
117 self.build_tree(['tree1/a/', 'tree1/a/b/', 'tree1/a-b/',
118@@ -942,7 +941,6 @@
119 finally:
120 state.unlock()
121
122-
123 def test_set_parent_trees_no_content(self):
124 # set_parent_trees is a slow but important api to support.
125 tree1 = self.make_branch_and_memory_tree('tree1')
126@@ -1238,6 +1236,38 @@
127 self.assertRaises(errors.BzrError,
128 state.add, '..', 'ass-id', 'directory', None, None)
129
130+ def test_set_state_with_rename_b_a_bug_395556(self):
131+ # bug 395556 uncovered a bug where the dirstate ends up with a false
132+ # relocation record - in a tree with no parents there should be no
133+ # absent or relocated records. This then leads to further corruption
134+ # when a commit occurs, as the incorrect relocation gathers an
135+ # incorrect absent in tree 1, and future changes go to pot.
136+ tree1 = self.make_branch_and_tree('tree1')
137+ self.build_tree(['tree1/b'])
138+ tree1.lock_write()
139+ try:
140+ tree1.add(['b'], ['b-id'])
141+ root_id = tree1.get_root_id()
142+ inv = tree1.inventory
143+ state = dirstate.DirState.initialize('dirstate')
144+ try:
145+ # Set the initial state with 'b'
146+ state.set_state_from_inventory(inv)
147+ inv.rename('b-id', root_id, 'a')
148+ # Set the new state with 'a', which currently corrupts.
149+ state.set_state_from_inventory(inv)
150+ expected_result1 = [('', '', root_id, 'd'),
151+ ('', 'a', 'b-id', 'f'),
152+ ]
153+ values = []
154+ for entry in state._iter_entries():
155+ values.append(entry[0] + entry[1][0][:1])
156+ self.assertEqual(expected_result1, values)
157+ finally:
158+ state.unlock()
159+ finally:
160+ tree1.unlock()
161+
162
163 class TestGetLines(TestCaseWithDirState):
164