Rev 2931: Review feedback on dirstate update_basis_via_delta logic. in http://people.ubuntu.com/~robertc/baz2.0/dirstate.update_basis
Robert Collins
robertc at robertcollins.net
Wed Oct 24 06:47:46 BST 2007
At http://people.ubuntu.com/~robertc/baz2.0/dirstate.update_basis
------------------------------------------------------------
revno: 2931
revision-id:robertc at robertcollins.net-20071024054739-r4gjnintzwhnvg82
parent: robertc at robertcollins.net-20071023221432-j8zndh1oiegql3cu
committer: Robert Collins <robertc at robertcollins.net>
branch nick: dirstate.update_basis
timestamp: Wed 2007-10-24 15:47:39 +1000
message:
Review feedback on dirstate update_basis_via_delta logic.
added:
bzrlib/tests/blackbox/test_check.py test_check.py-20071024054728-mn44rt3z5hnqcbke-1
modified:
bzrlib/builtins.py builtins.py-20050830033751-fc01482b9ca23183
bzrlib/dirstate.py dirstate.py-20060728012006-d6mvoihjb3je9peu-1
bzrlib/tests/blackbox/__init__.py __init__.py-20051128053524-eba30d8255e08dc3
bzrlib/tests/test_dirstate.py test_dirstate.py-20060728012006-d6mvoihjb3je9peu-2
bzrlib/tests/workingtree_implementations/test_parents.py test_set_parents.py-20060807231740-yicmnlci1mj8smu1-1
=== added file 'bzrlib/tests/blackbox/test_check.py'
--- a/bzrlib/tests/blackbox/test_check.py 1970-01-01 00:00:00 +0000
+++ b/bzrlib/tests/blackbox/test_check.py 2007-10-24 05:47:39 +0000
@@ -0,0 +1,26 @@
+# Copyright (C) 2007 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+
+"""Tests for the 'check' CLI command."""
+
+from bzrlib.tests.blackbox import ExternalBase
+
+
+class TestCheck(ExternalBase):
+
+ def test_check_no_tree(self):
+ branch = self.make_branch('.')
+ self.run_bzr('check')
=== modified file 'bzrlib/builtins.py'
--- a/bzrlib/builtins.py 2007-10-17 07:52:47 +0000
+++ b/bzrlib/builtins.py 2007-10-24 05:47:39 +0000
@@ -2362,10 +2362,36 @@
def run(self, branch=None, verbose=False):
from bzrlib.check import check
if branch is None:
- branch = Branch.open_containing('.')[0]
- else:
- branch = Branch.open(branch)
- check(branch, verbose)
+ branch_obj = Branch.open_containing('.')[0]
+ else:
+ branch_obj = Branch.open(branch)
+ check(branch_obj, verbose)
+ # bit hacky, check the tree parent is accurate
+ try:
+ if branch is None:
+ tree = WorkingTree.open_containing('.')[0]
+ else:
+ tree = WorkingTree.open(branch)
+ except (errors.NoWorkingTree, errors.NotLocalUrl):
+ pass
+ else:
+ # This is a primitive 'check' for tree state. Currently this is not
+ # integrated into the main check logic as yet.
+ tree.lock_read()
+ try:
+ tree_basis = tree.basis_tree()
+ tree_basis.lock_read()
+ try:
+ repo_basis = tree.branch.repository.revision_tree(
+ tree.last_revision())
+ if len(list(tree_basis._iter_changes(repo_basis))):
+ raise errors.BzrCheckError(
+ "Mismatched basis inventory content.")
+ tree._validate()
+ finally:
+ tree_basis.unlock()
+ finally:
+ tree.unlock()
class cmd_upgrade(Command):
=== modified file 'bzrlib/dirstate.py'
--- a/bzrlib/dirstate.py 2007-10-23 22:14:32 +0000
+++ b/bzrlib/dirstate.py 2007-10-24 05:47:39 +0000
@@ -1208,17 +1208,17 @@
# expanding them recursively as needed.
# At the same time, to reduce interface friction we convert the input
# inventory entries to dirstate.
-
+ root_only = ('', '')
for old_path, new_path, file_id, inv_entry in delta:
if old_path is None:
adds.append((None, encode(new_path), file_id,
- inv_to_entry(inv_entry)))
+ inv_to_entry(inv_entry), True))
elif new_path is None:
- deletes.append((encode(old_path), None, file_id, None))
- elif old_path != new_path:
+ deletes.append((encode(old_path), None, file_id, None, True))
+ elif (old_path, new_path) != root_only:
# Renames:
# Because renames must preserve their children we must have
- # processed all reloations and removes before hand. The sort
+ # processed all relocations and removes before hand. The sort
# order ensures we've examined the child paths, but we also
# have to execute the removals, or the split to an add/delete
# pair will result in the deleted item being reinserted, or
@@ -1233,28 +1233,40 @@
new_path_utf8 = encode(new_path)
# Split into an add/delete pair recursively.
adds.append((None, new_path_utf8, file_id,
- inv_to_entry(inv_entry)))
+ inv_to_entry(inv_entry), False))
+ # Expunge deletes that we've seen so that deleted/renamed
+ # children of a rename directory are handled correctly.
+ new_deletes = reversed(list(self._iter_child_entries(1,
+ encode(old_path))))
# Remove the current contents of the tree at orig_path, and
# reinsert at the correct new path.
- new_deletes = list(reversed(list(self._iter_child_entries(1,
- encode(old_path)))))
for entry in new_deletes:
- source_path = '/'.join(entry[0][0:2])
+ if entry[0][0]:
+ source_path = entry[0][0] + '/' + entry[0][1]
+ else:
+ source_path = entry[0][1]
target_path = new_path_utf8 + source_path[len(old_path):]
- adds.append((None, target_path, entry[0][2], entry[1][1]))
- deletes.append((source_path, None, entry[0][2], None))
- deletes.append((encode(old_path), None, file_id, None))
+ adds.append((None, target_path, entry[0][2], entry[1][1], False))
+ deletes.append(
+ (source_path, target_path, entry[0][2], None, False))
+ deletes.append(
+ (encode(old_path), new_path, file_id, None, False))
else:
+ # changes to just the root should not require remove/insertion
+ # of everything.
changes.append((encode(old_path), encode(new_path), file_id,
inv_to_entry(inv_entry)))
+ # Finish expunging deletes/first half of renames.
self._update_basis_apply_deletes(deletes)
+ # Reinstate second half of renames and new paths.
+ self._update_basis_apply_adds(adds)
+ # Apply in-situ changes.
self._update_basis_apply_changes(changes)
- self._update_basis_apply_adds(adds)
- # remove all deletes
self._dirblock_state = DirState.IN_MEMORY_MODIFIED
self._header_state = DirState.IN_MEMORY_MODIFIED
+ self._id_index = None
return
def _update_basis_apply_adds(self, adds):
@@ -1271,8 +1283,8 @@
adds.sort()
# adds is now in lexographic order, which places all parents before
# their children, so we can process it linearly.
- absent = ('r', 'a')
- for old_path, new_path, file_id, new_details in adds:
+ absent = 'ar'
+ for old_path, new_path, file_id, new_details, real_add in adds:
assert old_path is None
# the entry for this file_id must be in tree 0.
entry = self._get_entry(0, file_id, new_path)
@@ -1280,7 +1292,7 @@
raise errors.BzrError('dirstate: cannot apply delta, working'
' tree does not contain new entry %r %r' %
(new_path, file_id))
- if entry[1][1][0] not in absent:
+ if real_add and entry[1][1][0] not in absent:
raise errors.BzrError('dirstate: inconsistent delta, with '
'tree 0. %r %r' % (new_path, file_id))
# We don't need to update the target of an 'r' because the handling
@@ -1294,7 +1306,7 @@
:param adds: A sequence of changes. Each change is a tuple:
(path_utf8, path_utf8, file_id, (entry_details))
"""
- absent = ('a', 'r')
+ absent = 'ar'
for old_path, new_path, file_id, new_details in changes:
assert old_path == new_path
# the entry for this file_id must be in tree 0.
@@ -1316,12 +1328,17 @@
pairs.
:param adds: A sequence of deletes. Each delete is a tuple:
- (old_path_utf8, None, file_id, None)
+ (old_path_utf8, new_path_utf8, file_id, None, real_delete).
+ real_delete is True when the desired outcome is an actual deletion
+ rather than the rename handling logic temporarily deleting a path
+ during the replacement of a parent.
"""
- absent = ('r', 'a')
- # Deletes are accumulated in lexographical order.
- for old_path, new_path, file_id, _ in deletes:
- assert new_path is None
+ null = DirState.NULL_PARENT_DETAILS
+ for old_path, new_path, file_id, _, real_delete in deletes:
+ if real_delete:
+ assert new_path is None
+ else:
+ assert new_path is not None
# the entry for this file_id must be in tree 1.
dirname, basename = osutils.split(old_path)
block_index, entry_index, dir_present, file_present = \
@@ -1334,10 +1351,21 @@
if entry[0][2] != file_id:
raise errors.BzrError('mismatched file_id in tree 1 %r %r' %
(old_path, file_id))
- if entry[1][0][0] not in absent:
- raise errors.BzrError('dirstate: inconsistent delta, with '
- 'tree 0. %r %r' % (old_path, file_id))
- del self._dirblocks[block_index][1][entry_index]
+ if real_delete:
+ if entry[1][0][0] != 'a':
+ raise errors.BzrError('dirstate: inconsistent delta, with '
+ 'tree 0. %r %r' % (old_path, file_id))
+ del self._dirblocks[block_index][1][entry_index]
+ else:
+ if entry[1][0][0] == 'a':
+ raise errors.BzrError('dirstate: inconsistent delta, with '
+ 'tree 0. %r %r' % (old_path, file_id))
+ elif entry[1][0][0] == 'r':
+ # implement the rename
+ del self._dirblocks[block_index][1][entry_index]
+ else:
+ # it is being resurrected here, so blank it out temporarily.
+ self._dirblocks[block_index][1][entry_index][1][1] = null
def update_entry(self, entry, abspath, stat_value,
_stat_to_minikind=_stat_to_minikind,
@@ -1598,8 +1626,8 @@
# linear search through entries at this path to find the one
# requested.
while entry_index < len(block) and block[entry_index][0][1] == basename:
- if block[entry_index][1][tree_index][0] not in \
- ('a', 'r'): # absent, relocated
+ if block[entry_index][1][tree_index][0] not in 'ar':
+ # neither absent or relocated
return block_index, entry_index, True, True
entry_index += 1
return block_index, entry_index, True, False
@@ -1751,13 +1779,19 @@
"""
pending_dirs = []
next_pending_dirs = [path_utf8]
- absent = ('r', 'a')
+ absent = 'ar'
while next_pending_dirs:
pending_dirs = next_pending_dirs
next_pending_dirs = []
for path in pending_dirs:
block_index, present = self._find_block_index_from_key(
(path, '', ''))
+ if block_index == 0:
+ block_index = 1
+ if len(self._dirblocks) == 1:
+ # asked for the children of the root with no other
+ # contents.
+ return
if not present:
# children of a non-directory asked for.
continue
@@ -1767,7 +1801,11 @@
if kind not in absent:
yield entry
if kind == 'd':
- next_pending_dirs.append('/'.join(entry[0][0:2]))
+ if entry[0][0]:
+ path = entry[0][0] + '/' + entry[0][1]
+ else:
+ path = entry[0][1]
+ next_pending_dirs.append(path)
def _iter_entries(self):
"""Iterate over all the entries in the dirstate.
@@ -2026,7 +2064,7 @@
# one: the current tree
for entry in self._iter_entries():
# skip entries not in the current tree
- if entry[1][0][0] in ('a', 'r'): # absent, relocated
+ if entry[1][0][0] in 'ar': # absent, relocated
continue
by_path[entry[0]] = [entry[1][0]] + \
[DirState.NULL_PARENT_DETAILS] * parent_count
@@ -2162,7 +2200,7 @@
return None
while current_new or current_old:
# skip entries in old that are not really there
- if current_old and current_old[1][0][0] in ('r', 'a'):
+ if current_old and current_old[1][0][0] in 'ar':
# relocated or absent
current_old = advance(old_iterator)
continue
@@ -2243,7 +2281,7 @@
all_remaining_keys = set()
# Dont check the working tree, because it's going.
for details in current_old[1][1:]:
- if details[0] not in ('a', 'r'): # absent, relocated
+ if details[0] not in 'ar': # absent, relocated
all_remaining_keys.add(current_old[0])
elif details[0] == 'r': # relocated
# record the key for the real path.
@@ -2349,7 +2387,7 @@
self._find_entry_index(other_key, self._dirblocks[update_block_index][1])
assert present, '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 ('r', 'a'): # relocated, absent
+ if update_details[0] in 'ar': # relocated, absent
# its a pointer or absent in lookup_index's tree, use
# it as is.
new_entry[1].append(update_details)
@@ -2450,7 +2488,6 @@
"dirblock for %r is not sorted:\n%s" % \
(dirblock[0], pformat(dirblock)))
-
def check_valid_parent():
"""Check that the current entry has a valid parent.
@@ -2491,12 +2528,15 @@
"wrong number of entry details for row\n%s" \
",\nexpected %d" % \
(pformat(entry), tree_count))
+ absent_positions = 0
for tree_index, tree_state in enumerate(entry[1]):
this_tree_map = id_path_maps[tree_index]
minikind = tree_state[0]
+ if minikind in 'ar':
+ absent_positions += 1
# have we seen this id before in this column?
if file_id in this_tree_map:
- previous_path = this_tree_map[file_id]
+ previous_path, previous_loc = this_tree_map[file_id]
# any later mention of this file must be consistent with
# what was said before
if minikind == 'a':
@@ -2516,19 +2556,23 @@
# pointed to by a relocation, which must point here
if previous_path != this_path:
raise AssertionError(
- "entry %r inconsistent with previous path %r" % \
- (entry, previous_path))
+ "entry %r inconsistent with previous path %r "
+ "seen at %r" %
+ (entry, previous_path, previous_loc))
check_valid_parent()
else:
if minikind == 'a':
# absent; should not occur anywhere else
- this_tree_map[file_id] = None
+ this_tree_map[file_id] = None, this_path
elif minikind == 'r':
# relocation, must occur at expected location
- this_tree_map[file_id] = tree_state[1]
+ this_tree_map[file_id] = tree_state[1], this_path
else:
- this_tree_map[file_id] = this_path
+ this_tree_map[file_id] = this_path, this_path
check_valid_parent()
+ if absent_positions == tree_count:
+ raise AssertionError(
+ "entry %r has no data for any tree." % (entry,))
def _wipe_state(self):
"""Forget all state information about the dirstate."""
=== modified file 'bzrlib/tests/blackbox/__init__.py'
--- a/bzrlib/tests/blackbox/__init__.py 2007-09-17 12:46:56 +0000
+++ b/bzrlib/tests/blackbox/__init__.py 2007-10-24 05:47:39 +0000
@@ -53,6 +53,7 @@
'bzrlib.tests.blackbox.test_bundle_info',
'bzrlib.tests.blackbox.test_cat',
'bzrlib.tests.blackbox.test_cat_revision',
+ 'bzrlib.tests.blackbox.test_check',
'bzrlib.tests.blackbox.test_checkout',
'bzrlib.tests.blackbox.test_command_encoding',
'bzrlib.tests.blackbox.test_commit',
=== modified file 'bzrlib/tests/test_dirstate.py'
--- a/bzrlib/tests/test_dirstate.py 2007-10-23 22:14:32 +0000
+++ b/bzrlib/tests/test_dirstate.py 2007-10-24 05:47:39 +0000
@@ -1494,6 +1494,21 @@
self.assertEqual(expected_result,
list(state._iter_child_entries(1, 'b')))
+ def test_iter_child_root(self):
+ state, dirblocks = self.create_dirstate_with_two_trees()
+ self.addCleanup(state.unlock)
+ expected_result = []
+ expected_result.append(dirblocks[1][1][0]) # a
+ expected_result.append(dirblocks[1][1][1]) # b
+ expected_result.append(dirblocks[1][1][3]) # d
+ expected_result.append(dirblocks[2][1][0]) # e
+ expected_result.append(dirblocks[2][1][1]) # f
+ expected_result.append(dirblocks[3][1][2]) # h2
+ expected_result.append(dirblocks[3][1][3]) # i
+ expected_result.append(dirblocks[3][1][4]) # j
+ self.assertEqual(expected_result,
+ list(state._iter_child_entries(1, '')))
+
class TestDirstateSortOrder(TestCaseWithTransport):
"""Test that DirState adds entries in the right order."""
=== modified file 'bzrlib/tests/workingtree_implementations/test_parents.py'
--- a/bzrlib/tests/workingtree_implementations/test_parents.py 2007-10-23 22:14:32 +0000
+++ b/bzrlib/tests/workingtree_implementations/test_parents.py 2007-10-24 05:47:39 +0000
@@ -362,6 +362,10 @@
tree._write_inventory(new_shape)
self.assertDeltaApplicationResultsInExpectedBasis(tree, new_revid,
delta, new_shape)
+ # The tree should be internally consistent; while this is a moderately
+ # large hammer, this is a particularly sensitive area of code, so the
+ # extra assurance is well worth it.
+ tree._validate()
osutils.rmtree('tree')
def test_no_parents_just_root(self):
@@ -493,6 +497,23 @@
self.assertTransitionFromBasisToShape(basis_shape, old_revid,
new_shape, new_revid)
+ def test_parent_child_swap(self):
+ # test a A->A/B and A/B->A path swap.
+ 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')
+ self.add_dir(basis_shape, old_revid, 'dir-id-B', 'dir-id-A', 'B')
+ self.add_link(basis_shape, old_revid, 'link-id-C', 'dir-id-B', 'C', 'C')
+ 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, new_revid, 'dir-id-B', 'root-id', 'A')
+ self.add_dir(new_shape, new_revid, 'dir-id-A', 'dir-id-B', 'B')
+ self.add_link(new_shape, new_revid, 'link-id-C', 'dir-id-A', 'C', 'C')
+ self.assertTransitionFromBasisToShape(basis_shape, old_revid,
+ new_shape, new_revid)
+
def test_path_swap(self):
# test a A->B and B->A path swap.
old_revid = 'old-parent'
More information about the bazaar-commits
mailing list