Rev 4544: (robertc) Fix renames to lexographically lower paths in newly inited in file:///home/pqm/archives/thelove/bzr/%2Btrunk/
Canonical.com Patch Queue Manager
pqm at pqm.ubuntu.com
Fri Jul 17 01:21:21 BST 2009
At file:///home/pqm/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 4544 [merge]
revision-id: pqm at pqm.ubuntu.com-20090717002119-7zxzdnednsm818wf
parent: pqm at pqm.ubuntu.com-20090716093725-tb2a3b4hin8uia6j
parent: robertc at robertcollins.net-20090716232023-pyyca8djr07dge1v
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Fri 2009-07-17 01:21:19 +0100
message:
(robertc) Fix renames to lexographically lower paths in newly inited
trees. (Robert Collins, bug 395556)
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/dirstate.py dirstate.py-20060728012006-d6mvoihjb3je9peu-1
bzrlib/help_topics/en/debug-flags.txt debugflags.txt-20090312050229-rdspqbqq4fzbjtpe-1
bzrlib/tests/blackbox/test_commit.py test_commit.py-20060212094538-ae88fc861d969db0
bzrlib/tests/per_workingtree/test_commit.py test_commit.py-20060421013633-1610ec2331c8190f
bzrlib/tests/test_dirstate.py test_dirstate.py-20060728012006-d6mvoihjb3je9peu-2
=== modified file 'NEWS'
--- a/NEWS 2009-07-16 08:00:03 +0000
+++ b/NEWS 2009-07-16 23:20:23 +0000
@@ -36,6 +36,10 @@
* Fixed spurious "Source branch does not support stacking" warning when
pushing. (Andrew Bennetts, #388908)
+* Renames to lexographically lower basenames in trees that have never been
+ committed to will no longer corrupt the dirstate. This was caused by an
+ bug in the dirstate update_minimal method. (Robert Collins, #395556)
+
* ``WorkingTree4.unversion`` will no longer fail to unversion ids which
were present in a parent tree but renamed in the working tree.
(Robert Collins, #187207)
=== modified file 'bzrlib/dirstate.py'
--- a/bzrlib/dirstate.py 2009-07-10 05:33:07 +0000
+++ b/bzrlib/dirstate.py 2009-07-16 23:12:54 +0000
@@ -2422,6 +2422,9 @@
if 'evil' in debug.debug_flags:
trace.mutter_callsite(1,
"set_state_from_inventory called; please mutate the tree instead")
+ tracing = 'dirstate' in debug.debug_flags
+ if tracing:
+ trace.mutter("set_state_from_inventory trace:")
self._read_dirblocks_if_needed()
# sketch:
# Two iterators: current data and new data, both in dirblock order.
@@ -2436,7 +2439,9 @@
new_iterator = new_inv.iter_entries_by_dir()
# we will be modifying the dirstate, so we need a stable iterator. In
# future we might write one, for now we just clone the state into a
- # list - which is a shallow copy.
+ # list using a copy so that we see every original item and don't have
+ # to adjust the position when items are inserted or deleted in the
+ # underlying dirstate.
old_iterator = iter(list(self._iter_entries()))
# both must have roots so this is safe:
current_new = new_iterator.next()
@@ -2476,12 +2481,19 @@
# we make both end conditions explicit
if not current_old:
# old is finished: insert current_new into the state.
+ if tracing:
+ trace.mutter("Appending from new '%s'.",
+ new_path_utf8.decode('utf8'))
self.update_minimal(new_entry_key, current_new_minikind,
executable=current_new[1].executable,
path_utf8=new_path_utf8, fingerprint=fingerprint)
current_new = advance(new_iterator)
elif not current_new:
# new is finished
+ if tracing:
+ trace.mutter("Truncating from old '%s/%s'.",
+ current_old[0][0].decode('utf8'),
+ current_old[0][1].decode('utf8'))
self._make_absent(current_old)
current_old = advance(old_iterator)
elif new_entry_key == current_old[0]:
@@ -2494,6 +2506,9 @@
# kind has changed.
if (current_old[1][0][3] != current_new[1].executable or
current_old[1][0][0] != current_new_minikind):
+ if tracing:
+ trace.mutter("Updating in-place change '%s'.",
+ new_path_utf8.decode('utf8'))
self.update_minimal(current_old[0], current_new_minikind,
executable=current_new[1].executable,
path_utf8=new_path_utf8, fingerprint=fingerprint)
@@ -2505,6 +2520,9 @@
and new_entry_key[1:] < current_old[0][1:])):
# new comes before:
# add a entry for this and advance new
+ if tracing:
+ trace.mutter("Inserting from new '%s'.",
+ new_path_utf8.decode('utf8'))
self.update_minimal(new_entry_key, current_new_minikind,
executable=current_new[1].executable,
path_utf8=new_path_utf8, fingerprint=fingerprint)
@@ -2512,11 +2530,17 @@
else:
# we've advanced past the place where the old key would be,
# without seeing it in the new list. so it must be gone.
+ if tracing:
+ trace.mutter("Deleting from old '%s/%s'.",
+ current_old[0][0].decode('utf8'),
+ current_old[0][1].decode('utf8'))
self._make_absent(current_old)
current_old = advance(old_iterator)
self._dirblock_state = DirState.IN_MEMORY_MODIFIED
self._id_index = None
self._packed_stat_index = None
+ if tracing:
+ trace.mutter("set_state_from_inventory complete.")
def _make_absent(self, current_old):
"""Mark current_old - an entry - as absent for tree 0.
@@ -2612,25 +2636,44 @@
# grab one of them and use it to generate parent
# relocation/absent entries.
new_entry = key, [new_details]
- for other_key in existing_keys:
+ # existing_keys can be changed as we iterate.
+ for other_key in tuple(existing_keys):
# change the record at other to be a pointer to this new
# record. The loop looks similar to the change to
# relocations when updating an existing record but its not:
# the test for existing kinds is different: this can be
# factored out to a helper though.
- other_block_index, present = self._find_block_index_from_key(other_key)
- if not present:
- raise AssertionError('could not find block for %s' % (other_key,))
- other_entry_index, present = self._find_entry_index(other_key,
- self._dirblocks[other_block_index][1])
- if not present:
- raise AssertionError('could not find entry for %s' % (other_key,))
+ other_block_index, present = self._find_block_index_from_key(
+ other_key)
+ if not present:
+ raise AssertionError('could not find block for %s' % (
+ other_key,))
+ other_block = self._dirblocks[other_block_index][1]
+ other_entry_index, present = self._find_entry_index(
+ other_key, other_block)
+ if not present:
+ raise AssertionError(
+ 'update_minimal: could not find other entry for %s'
+ % (other_key,))
if path_utf8 is None:
raise AssertionError('no path')
- self._dirblocks[other_block_index][1][other_entry_index][1][0] = \
- ('r', path_utf8, 0, False, '')
+ # Turn this other location into a reference to the new
+ # location. This also updates the aliased iterator
+ # (current_old in set_state_from_inventory) so that the old
+ # entry, if not already examined, is skipped over by that
+ # loop.
+ other_entry = other_block[other_entry_index]
+ other_entry[1][0] = ('r', path_utf8, 0, False, '')
+ self._maybe_remove_row(other_block, other_entry_index,
+ id_index)
+ # This loop:
+ # adds a tuple to the new details for each column
+ # - either by copying an existing relocation pointer inside that column
+ # - or by creating a new pointer to the right row inside that column
num_present_parents = self._num_present_parents()
+ if num_present_parents:
+ other_key = list(existing_keys)[0]
for lookup_index in xrange(1, num_present_parents + 1):
# grab any one entry, use it to find the right path.
# TODO: optimise this to reduce memory use in highly
@@ -2643,7 +2686,7 @@
update_entry_index, present = \
self._find_entry_index(other_key, self._dirblocks[update_block_index][1])
if not present:
- raise AssertionError('could not find entry for %s' % (other_key,))
+ raise AssertionError('update_minimal: could not find entry for %s' % (other_key,))
update_details = self._dirblocks[update_block_index][1][update_entry_index][1][lookup_index]
if update_details[0] in 'ar': # relocated, absent
# its a pointer or absent in lookup_index's tree, use
@@ -2695,6 +2738,21 @@
self._dirblock_state = DirState.IN_MEMORY_MODIFIED
+ def _maybe_remove_row(self, block, index, id_index):
+ """Remove index if it is absent or relocated across the row.
+
+ id_index is updated accordingly.
+ """
+ present_in_row = False
+ entry = block[index]
+ for column in entry[1]:
+ if column[0] not in 'ar':
+ present_in_row = True
+ break
+ if not present_in_row:
+ block.pop(index)
+ id_index[entry[0][2]].remove(entry[0])
+
def _validate(self):
"""Check that invariants on the dirblock are correct.
=== modified file 'bzrlib/help_topics/en/debug-flags.txt'
--- a/bzrlib/help_topics/en/debug-flags.txt 2009-07-06 09:47:35 +0000
+++ b/bzrlib/help_topics/en/debug-flags.txt 2009-07-16 00:01:17 +0000
@@ -5,6 +5,7 @@
prefix) put in the ``debug_flags`` variable in ``bazaar.conf``.
-Dauth Trace authentication sections used.
+-Ddirstate Trace dirstate activity (verbose!)
-Derror Instead of normal error handling, always print a traceback
on error.
-Devil Capture call sites that do expensive or badly-scaling
=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- a/bzrlib/tests/blackbox/test_commit.py 2009-07-08 05:27:06 +0000
+++ b/bzrlib/tests/blackbox/test_commit.py 2009-07-15 01:13:20 +0000
@@ -639,25 +639,3 @@
out, err = self.run_bzr("commit tree/hello.txt")
last_rev = tree.branch.repository.get_revision(tree.last_revision())
self.assertEqual('save me some typing\n', last_rev.message)
-
- def test_commit_and_mv_dance_a(self):
- # see https://bugs.launchpad.net/bzr/+bug/395556
- tree = self.make_branch_and_tree(".")
- self.build_tree(["a"])
- tree.add("a")
- self.check_output("a => b\n", ["mv", "a", "b"])
- self.check_output("", ["commit", "-q", "-m", "Actually no, b"])
- self.check_output("b => a\n", ["mv", "b", "a"])
- self.check_output("", ["commit", "-q", "-m", "No, really, a"])
-
- def test_commit_and_mv_dance_b(self):
- # see https://bugs.launchpad.net/bzr/+bug/395556
- tree = self.make_branch_and_tree(".")
- self.build_tree(["b"])
- tree.add("b")
- self.check_output("b => a\n", ["mv", "b", "a"])
- self.check_output("", ["commit", "-q", "-m", "Actually no, a"])
- self.check_output("a => b\n", ["mv", "a", "b"])
- self.expectFailure("bug 395556: gives DuplicateFileId "
- "committing renames",
- self.check_output, "", ["commit", "-q", "-m", "No, really, b"])
=== modified file 'bzrlib/tests/per_workingtree/test_commit.py'
--- a/bzrlib/tests/per_workingtree/test_commit.py 2009-07-15 05:54:37 +0000
+++ b/bzrlib/tests/per_workingtree/test_commit.py 2009-07-16 00:52:51 +0000
@@ -611,29 +611,3 @@
revid = tree.commit('first post')
committed_tree = tree.basis_tree()
self.assertTrue(committed_tree.has_filename("newfile"))
-
- def test_commit_and_mv_dance_a(self):
- # should fail because of
- # <https://bugs.launchpad.net/bzr/+bug/395556> but apparently does
- # not, while the blackbox.test_commit equivalent does - maybe because
- # of different format combinations
- tree = self.make_branch_and_tree(".")
- self.build_tree(["a"])
- tree.add("a")
- tree.rename_one("a", "b")
- tree.commit("Actually no, b")
- tree.rename_one("b", "a")
- tree.commit("No, really, a")
-
- def test_commit_and_mv_dance_b(self):
- # should fail because of
- # <https://bugs.launchpad.net/bzr/+bug/395556> but apparently does
- # not, while the blackbox.test_commit equivalent does - maybe because
- # of different format combinations
- tree = self.make_branch_and_tree(".")
- self.build_tree(["b"])
- tree.add("b")
- tree.rename_one("b", "a")
- tree.commit("Actually no, a")
- tree.rename_one("a", "b")
- tree.commit("No, really, b")
=== modified file 'bzrlib/tests/test_dirstate.py'
--- a/bzrlib/tests/test_dirstate.py 2009-05-06 05:36:28 +0000
+++ b/bzrlib/tests/test_dirstate.py 2009-07-15 01:13:20 +0000
@@ -827,7 +827,6 @@
finally:
tree.unlock()
-
def test_set_state_from_inventory_mixed_paths(self):
tree1 = self.make_branch_and_tree('tree1')
self.build_tree(['tree1/a/', 'tree1/a/b/', 'tree1/a-b/',
@@ -942,7 +941,6 @@
finally:
state.unlock()
-
def test_set_parent_trees_no_content(self):
# set_parent_trees is a slow but important api to support.
tree1 = self.make_branch_and_memory_tree('tree1')
@@ -1238,6 +1236,38 @@
self.assertRaises(errors.BzrError,
state.add, '..', 'ass-id', 'directory', None, None)
+ def test_set_state_with_rename_b_a_bug_395556(self):
+ # bug 395556 uncovered a bug where the dirstate ends up with a false
+ # relocation record - in a tree with no parents there should be no
+ # absent or relocated records. This then leads to further corruption
+ # when a commit occurs, as the incorrect relocation gathers an
+ # incorrect absent in tree 1, and future changes go to pot.
+ tree1 = self.make_branch_and_tree('tree1')
+ self.build_tree(['tree1/b'])
+ tree1.lock_write()
+ try:
+ tree1.add(['b'], ['b-id'])
+ root_id = tree1.get_root_id()
+ inv = tree1.inventory
+ state = dirstate.DirState.initialize('dirstate')
+ try:
+ # Set the initial state with 'b'
+ state.set_state_from_inventory(inv)
+ inv.rename('b-id', root_id, 'a')
+ # Set the new state with 'a', which currently corrupts.
+ state.set_state_from_inventory(inv)
+ expected_result1 = [('', '', root_id, 'd'),
+ ('', 'a', 'b-id', 'f'),
+ ]
+ values = []
+ for entry in state._iter_entries():
+ values.append(entry[0] + entry[1][0][:1])
+ self.assertEqual(expected_result1, values)
+ finally:
+ state.unlock()
+ finally:
+ tree1.unlock()
+
class TestGetLines(TestCaseWithDirState):
More information about the bazaar-commits
mailing list