Rev 3957: Solve things in a different way. in lp:///~jameinel/bzr/win32-shelve
John Arbash Meinel
john at arbash-meinel.com
Fri Jan 23 17:59:20 GMT 2009
At lp:///~jameinel/bzr/win32-shelve
------------------------------------------------------------
revno: 3957
revision-id: john at arbash-meinel.com-20090123175911-ymf5xvskhkk9yh5o
parent: john at arbash-meinel.com-20090123174855-65tgyycwsc54uis9
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: win32-shelve
timestamp: Fri 2009-01-23 11:59:11 -0600
message:
Solve things in a different way.
Instead of having the Shelf (et al) code try to properly manage the lock lifetimes,
punt and make DirStateRevisionTree grab its dirstate object through
the workingtree that created it, rather than managing its own object.
This allows sloppy code to be written, but as we already *have* sloppy code
it at least lets things pass on Win32, where otherwise the sloppy code
was causing OS lock problems.
-------------- next part --------------
=== modified file 'bzrlib/shelf.py'
--- a/bzrlib/shelf.py 2009-01-23 17:48:55 +0000
+++ b/bzrlib/shelf.py 2009-01-23 17:59:11 +0000
@@ -37,8 +37,7 @@
def __init__(self, work_tree, target_tree, file_list=None):
"""Constructor.
- :param work_tree: The working tree to apply changes to,
- this tree should already be write locked.
+ :param work_tree: The working tree to apply changes to
:param target_tree: The tree to make the working tree more similar to.
:param file_list: The files to make more similar to the target.
"""
@@ -187,7 +186,6 @@
"""Release all resources used by this ShelfCreator."""
self.work_transform.finalize()
self.shelf_transform.finalize()
- self.work_tree.unlock()
def transform(self):
"""Shelve changes from working tree."""
=== modified file 'bzrlib/shelf_ui.py'
--- a/bzrlib/shelf_ui.py 2009-01-23 17:48:55 +0000
+++ b/bzrlib/shelf_ui.py 2009-01-23 17:59:11 +0000
@@ -73,7 +73,6 @@
:param directory: The directory containing the working tree.
"""
tree, path = workingtree.WorkingTree.open_containing(directory)
- tree.lock_write()
target_tree = builtins._get_one_revision_tree('shelf2', revision,
tree.branch, tree)
files = builtins.safe_relpath_files(tree, file_list)
=== modified file 'bzrlib/tests/test_shelf.py'
--- a/bzrlib/tests/test_shelf.py 2009-01-23 17:48:55 +0000
+++ b/bzrlib/tests/test_shelf.py 2009-01-23 17:59:11 +0000
@@ -38,7 +38,6 @@
tree.add(['foo'], ['foo-id'])
tree.commit('foo')
tree.rename_one('foo', 'bar')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('rename', 'foo-id', 'foo', 'bar')],
@@ -57,7 +56,6 @@
tree.add(['foo', 'bar', 'foo/baz'], ['foo-id', 'bar-id', 'baz-id'])
tree.commit('foo')
tree.rename_one('foo/baz', 'bar/baz')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('rename', 'baz-id', 'foo/baz', 'bar/baz')],
@@ -87,7 +85,6 @@
tree.add('foo', 'foo-id')
tree.commit('Committed foo')
self.build_tree_contents([('foo', 'b\na\nc\n')])
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('modify text', 'foo-id')],
@@ -104,7 +101,6 @@
tree.commit('Empty tree')
self.build_tree_contents([('foo', 'a\n'), ('bar/',)])
tree.add(['foo', 'bar'], ['foo-id', 'bar-id'])
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('add file', 'bar-id', 'directory', 'bar'),
@@ -133,7 +129,6 @@
tree.commit('Empty tree')
os.symlink('bar', 'foo')
tree.add('foo', 'foo-id')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('add file', 'foo-id', 'symlink', 'foo')],
@@ -153,7 +148,6 @@
self.build_tree(['foo'])
tree.add('foo', 'foo-id')
os.unlink('foo')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('add file', 'foo-id', None, 'foo')],
@@ -178,7 +172,6 @@
tree.unversion(['foo-id', 'bar-id'])
os.unlink('tree/foo/bar')
os.rmdir('tree/foo')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('delete file', 'bar-id', 'file', 'foo/bar'),
@@ -197,7 +190,6 @@
tree.add('foo', 'foo-id')
tree.commit('Added file and directory')
os.unlink('tree/foo')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('delete file', 'foo-id', 'file', 'foo')],
@@ -213,7 +205,8 @@
tree.commit('Added file and directory')
os.unlink('tree/foo')
os.mkdir('tree/foo')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
+ tree.lock_write()
+ self.addCleanup(tree.unlock)
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('change kind', 'foo-id', 'file', 'directory',
@@ -231,7 +224,6 @@
tree.add('foo', 'foo-id')
tree.commit('Added file and directory')
tree.unversion(['foo-id'])
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
self.assertEqual([('delete file', 'foo-id', 'file', 'foo')],
@@ -242,7 +234,6 @@
def test_shelve_serialization(self):
tree = self.make_branch_and_tree('.')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
shelf_file = open('shelf', 'wb')
@@ -257,7 +248,6 @@
tree = self.make_branch_and_tree('tree')
self.build_tree(['tree/foo'])
tree.add('foo', 'foo-id')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
list(creator.iter_shelvable())
@@ -290,7 +280,6 @@
tree.lock_write()
self.addCleanup(tree.unlock)
tree.add('foo', 'foo-id')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
list(creator.iter_shelvable())
@@ -314,7 +303,6 @@
tree.add('foo', 'foo-id')
tree.commit('first commit')
self.build_tree_contents([('tree/foo', 'a\nb\nd\n')])
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
list(creator.iter_shelvable())
@@ -334,7 +322,6 @@
tree.lock_write()
self.addCleanup(tree.unlock)
tree.commit('rev1', rev_id='rev1')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
manager = tree.get_shelf_manager()
@@ -462,7 +449,6 @@
self.build_tree_contents([('tree/foo', 'bar')])
self.assertFileEqual('bar', 'tree/foo')
tree.add('foo', 'foo-id')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
self.addCleanup(creator.finalize)
list(creator.iter_shelvable())
@@ -476,9 +462,7 @@
def test_get_metadata(self):
tree = self.make_branch_and_tree('.')
- tree.lock_write() # ShelfCreator.finalize will unlock for us
creator = shelf.ShelfCreator(tree, tree.basis_tree())
- self.addCleanup(creator.finalize)
shelf_manager = tree.get_shelf_manager()
shelf_id = shelf_manager.shelve_changes(creator, 'foo')
metadata = shelf_manager.get_metadata(shelf_id)
=== modified file 'bzrlib/workingtree_4.py'
--- a/bzrlib/workingtree_4.py 2008-12-22 08:44:20 +0000
+++ b/bzrlib/workingtree_4.py 2009-01-23 17:59:11 +0000
@@ -1024,13 +1024,16 @@
WorkingTree4 supplies revision_trees for any basis tree.
"""
+ # XXX: It would be better to call self._must_be_locked at this point,
+ # rather than using @needs_read_lock for revision_tree()
+ # self._must_be_locked()
dirstate = self.current_dirstate()
parent_ids = dirstate.get_parent_ids()
if revision_id not in parent_ids:
raise errors.NoSuchRevisionInTree(self, revision_id)
if revision_id in dirstate.get_ghosts():
raise errors.NoSuchRevisionInTree(self, revision_id)
- return DirStateRevisionTree(dirstate, revision_id,
+ return DirStateRevisionTree(self, revision_id,
self.branch.repository)
@needs_tree_write_lock
@@ -1478,8 +1481,9 @@
class DirStateRevisionTree(Tree):
"""A revision tree pulling the inventory from a dirstate."""
- def __init__(self, dirstate, revision_id, repository):
- self._dirstate = dirstate
+ def __init__(self, wt, revision_id, repository):
+ self._wt = wt
+ # self._dirstate = dirstate
self._revision_id = revision_id
self._repository = repository
self._inventory = None
@@ -1491,7 +1495,8 @@
def __repr__(self):
return "<%s of %s in %s>" % \
- (self.__class__.__name__, self._revision_id, self._dirstate)
+ (self.__class__.__name__, self._revision_id,
+ self.wt._current_dirstate())
def annotate_iter(self, file_id,
default_revision=_mod_revision.CURRENT_REVISION):
@@ -1543,7 +1548,7 @@
def _get_parent_index(self):
"""Return the index in the dirstate referenced by this tree."""
- return self._dirstate.get_parent_ids().index(self._revision_id) + 1
+ return self._wt._current_dirstate().get_parent_ids().index(self._revision_id) + 1
def _get_entry(self, file_id=None, path=None):
"""Get the dirstate row for file_id or path.
@@ -1561,7 +1566,7 @@
if path is not None:
path = path.encode('utf8')
parent_index = self._get_parent_index()
- return self._dirstate._get_entry(parent_index, fileid_utf8=file_id, path_utf8=path)
+ return self._wt._current_dirstate()._get_entry(parent_index, fileid_utf8=file_id, path_utf8=path)
def _generate_inventory(self):
"""Create and set self.inventory from the dirstate object.
@@ -1576,15 +1581,16 @@
'cannot generate inventory of an unlocked '
'dirstate revision tree')
# separate call for profiling - makes it clear where the costs are.
- self._dirstate._read_dirblocks_if_needed()
- if self._revision_id not in self._dirstate.get_parent_ids():
+ state = self._wt._current_dirstate()
+ state._read_dirblocks_if_needed()
+ if self._revision_id not in state.get_parent_ids():
raise AssertionError(
'parent %s has disappeared from %s' % (
- self._revision_id, self._dirstate.get_parent_ids()))
- parent_index = self._dirstate.get_parent_ids().index(self._revision_id) + 1
+ self._revision_id, state.get_parent_ids()))
+ parent_index = state.get_parent_ids().index(self._revision_id) + 1
# This is identical now to the WorkingTree _generate_inventory except
# for the tree index use.
- root_key, current_entry = self._dirstate._get_entry(parent_index, path_utf8='')
+ root_key, current_entry = state._get_entry(parent_index, path_utf8='')
current_id = root_key[2]
if current_entry[parent_index][0] != 'd':
raise AssertionError()
@@ -1598,7 +1604,7 @@
# we could do this straight out of the dirstate; it might be fast
# and should be profiled - RBC 20070216
parent_ies = {'' : inv.root}
- for block in self._dirstate._dirblocks[1:]: #skip root
+ for block in state._dirblocks[1:]: #skip root
dirname = block[0]
try:
parent_ie = parent_ies[dirname]
@@ -1767,8 +1773,10 @@
"""Lock the tree for a set of operations."""
if not self._locked:
self._repository.lock_read()
- if self._dirstate._lock_token is None:
- self._dirstate.lock_read()
+ # Shouldn't this lock self._wt as well?
+ state = self._wt._current_dirstate()
+ if state._lock_token is None:
+ state.lock_read()
self._dirstate_locked = True
self._locked += 1
@@ -1793,7 +1801,8 @@
self._inventory = None
self._locked = 0
if self._dirstate_locked:
- self._dirstate.unlock()
+ state = self._wt._current_dirstate()
+ state.unlock()
self._dirstate_locked = False
self._repository.unlock()
More information about the bazaar-commits
mailing list